Anti-suggestions

[Eeyoocah5Moh]

Don't implement any new features until you fully reimplement original game or at least big part of it such as geoscape.

Create separate programs for geoscape and battlescape.  Use the same file format as original X-COM.  This will let you replace one of the programs with original program running inside DOSBox.  Share source files between these programs.  Don't create shared libraries, just compile them from overlapping sets of source files.

Use less abstractions.

For example, you have "Soldier" class.  Most of it's methods just get and set private fields.  There is only one place in code that modify soldier's names, it can handle everything that is related to renaming so you don't really need "setName" function.  And nothing is calculated when you get name so you don't need "getName" method.  Just make "name" field public.

Also you have XcomRuleset that is based on Ruleset.  You don't really need more than one ruleset in one game, just create one "Ruleset" class and everyone will be able to modify it if they want to change rules of the game.

If you want others to be able to modify game engine, don't try to create bulletproof abstractions and don't add scripting engines. Abstractions don't let you fully modify game until you understand code behind them.  Scripting engines are even more limited.

There is already nearly 8k lines of code (and 15k if you count blank lines and comments).  This number can be greatly reduced by removing most of such abstractions.

I am just afraid I would not be able to modify code when you finish implementing bigger part of it.

[pmprog]

For example, you have "Soldier" class.  Most of it's methods just get and set private fields.  There is only one place in code that modify soldier's names, it can handle everything that is related to renaming so you don't really need "setName" function.  And nothing is calculated when you get name so you don't need "getName" method.  Just make "name" field public.

Whilst on occasion I'll do this, it is looked upon as bad practice; it also means that should you want to enter any validation or cascading updates based upon the field, you'll have to reimplment the methods anyway. Example of the latter is if you want to set Arm Damage, you might want to cascade an update to Accuracy to decrease it, then you don't need to be constantly recalculating the value.

Also you have XcomRuleset that is based on Ruleset.  You don't really need more than one ruleset in one game, just create one "Ruleset" class and everyone will be able to modify it if they want to change rules of the game.

I think it was in a news post, SupSuper was referring to a screen to toggle fixes/options on or off (before the game starts). I'm assuming that they will be implemented as rulesets. Could be wrong on this point though.

If you want others to be able to modify game engine, don't try to create bulletproof abstractions and don't add scripting engines. Abstractions don't let you fully modify game until you understand code behind them.  Scripting engines are even more limited.

Have you ever worked with Unreal Tournament (the original, I don't know a lot about the others)? Abstractions are good, because you don't need to understand the code behind them. You just inherit the original (with code), and add/replace the bits with your new version.
As for scripting engines, well, if you break open the Unreal Tournament code, you'll find that almost every aspect of the game is scripting. Okay, so the script gets compiled before running, but it's still scripted.

In all, designing it in the way it's being done should actually make your life easier should you want to start hacking away at parts.

Sorry if this seemed a little negative. I think the structure of the code looks good, and will be more beneficial to hackers and modders with this approach.

As for not implementing new features until the original stuff is complete, I can see where you're coming from, but if you don't plan for some features, you may have to rewrite segments of code to add it in later. I think features do need to be thought about, even if they are not implemented at the time.

[Eeyoocah5Moh]

Whilst on occasion I'll do this, it is looked upon as bad practice; it also means that should you want to enter any validation or cascading updates based upon the field, you'll have to reimplment the methods anyway. Example of the latter is if you want to set Arm Damage, you might want to cascade an update to Accuracy to decrease it, then you don't need to be constantly recalculating the value.

You should not recalculate accuracy. Instead, when soldier shoot, you should calculate some kind of "effective accuracy" based on damage, state of soldier (sitting, standing, flying) etc. Maybe some fields should be private, but don't just do everything private.

Also you have XcomRuleset that is based on Ruleset.  You don't really need more than one ruleset in one game, just create one "Ruleset" class and everyone will be able to modify it if they want to change rules of the game.

I think it was in a news post, SupSuper was referring to a screen to toggle fixes/options on or off (before the game starts). I'm assuming that they will be implemented as rulesets. Could be wrong on this point though.

Then just store these options inside "Ruleset". Currently it looks like we are going to have two or more rulesets at a time.

If you want others to be able to modify game engine, don't try to create bulletproof abstractions and don't add scripting engines. Abstractions don't let you fully modify game until you understand code behind them.  Scripting engines are even more limited.

Have you ever worked with Unreal Tournament (the original, I don't know a lot about the others)? Abstractions are good, because you don't need to understand the code behind them. You just inherit the original (with code), and add/replace the bits with your new version.
As for scripting engines, well, if you break open the Unreal Tournament code, you'll find that almost every aspect of the game is scripting. Okay, so the script gets compiled before running, but it's still scripted.

In all, designing it in the way it's being done should actually make your life easier should you want to start hacking away at parts.

Sorry if this seemed a little negative. I think the structure of the code looks good, and will be more beneficial to hackers and modders with this approach.

As for not implementing new features until the original stuff is complete, I can see where you're coming from, but if you don't plan for some features, you may have to rewrite segments of code to add it in later. I think features do need to be thought about, even if they are not implemented at the time.

If code is small, it is not hard to rewrite part of it the way you want. It is easy to use abstractions as long as they are suitable for your ideas. But when they don't, you will need to rewrite a lot of code. Abstractions are ok, just don't use them everywhere. Game is going to be X-COM remake, not some abstract turn-based strategy.

[SupSuper]

Don't implement any new features until you fully reimplement original game or at least big part of it such as geoscape.

That is the plan. Any improvements I introduce in the mean-time are purely because of how simple/handy they are.

Create separate programs for geoscape and battlescape.  Use the same file format as original X-COM.  This will let you replace one of the programs with original program running inside DOSBox.  Share source files between these programs.  Don't create shared libraries, just compile them from overlapping sets of source files.

That is a very bad idea:

- Having a program split into pieces like that is just asking for trouble. Sure back in the day you needed to save extra byte of memory you could, but you have to manage information between programs yourself which is bothersome and error-prone, there's no proper way to handle one of the parts failing (when the X-Com Battlescape died, the Geoscape wouldn't notice and just use the last available mission results), and it just restricts the parts needlessly (you couldn't quit or load saves from the Battlescape because the Geoscape had to do that). Who knows if I won't later extend the game to a point that the Geoscape and Battlescape should be tied together.

- The community still hasn't fully reverse-engineered the game's file formats, so a lot of them still remain a mystery and it'd force me to have to wait around until I could use them. Plus the old file formats also bring a lot of old bugs and limitations with them like the "proximity mine bug", the various engine limits, and in the end it'd just restrict me from extending them and taking full advantage of a new engine.

I get that in a perfect world this would mean that I could write a new Geoscape and you'd plug it in with the old Battlescape and pretty much have a full game in half the time wouldn't that be awesome!!!!!, but I don't even want to imagine the trouble of getting both to cooperate without issue, specially if you mixed platforms. In any case the point of OpenXcom isn't to reverse-engineer the original and replace/patch it (there's already plenty of hacks trying to do that), but to provide a new clean codebase based on the original but relying on modern standards and practices, to not be bogged down on all the old needless restrictions, file formats and so on.

Use less abstractions.

For example, you have "Soldier" class.  Most of it's methods just get and set private fields.  There is only one place in code that modify soldier's names, it can handle everything that is related to renaming so you don't really need "setName" function.  And nothing is calculated when you get name so you don't need "getName" method.  Just make "name" field public.

Well this is mostly me sticking to OOP best practices and keeping the code consistent. Never having classes with public fields, always use get/set methods, etc.

I know this can get kinda painful since inevitably a lot of classes in a game are just gonna be a bunch of numbers. Hell, looking at the Soldier class as is now, why don't I just replace it all with one line like:

Code: [Select]

struct Soldier { int _tu, _stamina, _health, _bravery, _reactions, _firing, _throwing, _strength, _psiStrength, _psiSkill, _melee; }
Now that's a whole lot simpler with virtually the same functionality! Man all the time and lines I could've saved.

But then the code starts getting kinda ugly. You get stuff mixed like "save.funds -= base.getTotalMaintenance()". Then you don't know if some class uses a getNameOrSomething() or do I access the field directly. What if I suddenly decide I wanna append something to the end of soldier's names? Or start validating user input?

I know this is all exaggerating and you're probably right that a name will always be just a name and so on, but I like sticking to these kind of standards as it teaches me to be consistent and not get carried away and be lazy. It's a slippery slope, I'm very prone to being lazy and next thing you know the code is all condensed in a few lines with no comments or anything and you'll have a real adventure trying to modify it.

Also you have XcomRuleset that is based on Ruleset.  You don't really need more than one ruleset in one game, just create one "Ruleset" class and everyone will be able to modify it if they want to change rules of the game.

Ruleset is probably a bit of a misnomer. In fact I still haven't set in stone what it really does, but there probably won't be many actual "rules" in there since an X-Com engine is an X-Com engine and there probably isn't a whole lot you wanna change without becoming a different game entirely. It probably won't contain gameplay features like whether

For now it's just a nice place to keep all the typically hardcoded numbers like craft stats, facility stats, item stats, weapon stats, research tree, etc. For example, as far as the engine is concerned, a craft is a craft and it has various properties and can contain weapons and soldiers and it can fly around intercepting stuff and going on missions. It's the ruleset that tells it there's a craft type X with acceleration Y and crew capacity Z and so on. Whether you call it a Skyranger or a Triton the engine will treat it just the same.

In any case, the reason there's support for "multiple rulesets" is people will probably wanna play around with these stats a lot, so I need to keep them fairly separate from the engine. The XcomRuleset has all the original game stats, but some people would probably prefer improved tanks and weapons like in XcomUtil. You might only have one ruleset in use by one game at a time, but the player might have a wide selection to choose from when they start a new game (kinda like mods). Hell, maybe they're a picky bunch and decide they want the "improved tanks" but not the "improved weapons", so you need to split the rulesets into bits so the player can pick multiple ones that get combined for them. That is the reason there's an abstract Ruleset, planning for the future.

If you want others to be able to modify game engine, don't try to create bulletproof abstractions and don't add scripting engines. Abstractions don't let you fully modify game until you understand code behind them.  Scripting engines are even more limited.

There is already nearly 8k lines of code (and 15k if you count blank lines and comments).  This number can be greatly reduced by removing most of such abstractions.

I am just afraid I would not be able to modify code when you finish implementing bigger part of it.

I don't really see what point you're trying to make. What do you mean by "bulletproof abstractions"? I'm not writing some generic game engine, as you pointed out, but an X-Com engine. There are Crafts, Soldiers, Bases, etc. Obviously if someone wants to use the engine for something else they'll have to rip that stuff apart and it'll be a lot of work, but that's their own fault for not using a proper generic game engine.

There is a whole bunch of stuff that could probably apply to any game, yes, like a state machine, interface, graphics, sounds, etc. And a lot of it could be clustered together, but there's really no need. That's not really trying to abstract away the whole system, but keeping things tidy and organized and in their place. I used to have all the resource loading in the Game class until I decided it was getting kinda bulky and moved it all out to its own class. As you code a lot you find more and more stuff that would fit better elsewhere and makes it easier to work. You can see this in the SVN history, the game grew from a little bunch of clustered files to a ton more cleaner files, and it's much easier to work with this way. You said it yourself, half the code is just commenting, spacing and other stuff to make it more pleasant to read and work on.

But it's still all designed to load X-Com formats, use 8bpp palette graphics, etc, and you'd probably have a hard time getting the engine to do something else.

[Eeyoocah5Moh]

That is a very bad idea:

- Having a program split into pieces like that is just asking for trouble. Sure back in the day you needed to save extra byte of memory you could, but you have to manage information between programs yourself which is bothersome and error-prone, there's no proper way to handle one of the parts failing (when the X-Com Battlescape died, the Geoscape wouldn't notice and just use the last available mission results), and it just restricts the parts needlessly (you couldn't quit or load saves from the Battlescape because the Geoscape had to do that). Who knows if I won't later extend the game to a point that the Geoscape and Battlescape should be tied together.

Engines are separated not for saving memory. Look at Worms, for example. They have separate GUI for options and "battlescape". It just simplifies program.

If Battlescape died it is a bug. Just handle signals properly so it would be impossible to quit by closing the window.

- The community still hasn't fully reverse-engineered the game's file formats, so a lot of them still remain a mystery and it'd force me to have to wait around until I could use them. Plus the old file formats also bring a lot of old bugs and limitations with them like the "proximity mine bug", the various engine limits, and in the end it'd just restrict me from extending them and taking full advantage of a new engine.

Yes, there are some limitations in existing formats, but most bugs can be fixed by checking for integer overflows. Just don't let stats grow over 255 instead of setting them to 0.

But right, this may not be the best solution. Need to think more about it.

Also you have XcomRuleset that is based on Ruleset.  You don't really need more than one ruleset in one game, just create one "Ruleset" class and everyone will be able to modify it if they want to change rules of the game.

Ruleset is probably a bit of a misnomer. In fact I still haven't set in stone what it really does, but there probably won't be many actual "rules" in there since an X-Com engine is an X-Com engine and there probably isn't a whole lot you wanna change without becoming a different game entirely. It probably won't contain gameplay features like whether

For now it's just a nice place to keep all the typically hardcoded numbers like craft stats, facility stats, item stats, weapon stats, research tree, etc. For example, as far as the engine is concerned, a craft is a craft and it has various properties and can contain weapons and soldiers and it can fly around intercepting stuff and going on missions. It's the ruleset that tells it there's a craft type X with acceleration Y and crew capacity Z and so on. Whether you call it a Skyranger or a Triton the engine will treat it just the same.

In any case, the reason there's support for "multiple rulesets" is people will probably wanna play around with these stats a lot, so I need to keep them fairly separate from the engine. The XcomRuleset has all the original game stats, but some people would probably prefer improved tanks and weapons like in XcomUtil. You might only have one ruleset in use by one game at a time, but the player might have a wide selection to choose from when they start a new game (kinda like mods). Hell, maybe they're a picky bunch and decide they want the "improved tanks" but not the "improved weapons", so you need to split the rulesets into bits so the player can pick multiple ones that get combined for them. That is the reason there's an abstract Ruleset, planning for the future.

Aren't these constants stored in X-COM data files? If not, it is possible to load them from your own file format.

If you want the game to be moddable, then these rules should be stored in files. If not, then there should be one hardcoded ruleset. But I don't understand why you want to put multiple hardcoded rulesets inside compiled game.

[michal]

First of all i would like to say that i like openxcom code, it looks clear and well designed.

Engines are separated not for saving memory. Look at Worms, for example. They have separate GUI for options and "battlescape". It just simplifies program.

But geoscape is far more than "GUI for options".

Personally i don't see any advantage for splitting geoscape and battlescape. I don't see how it could simplify anything. It makes thing even harder - you would need to design some data format for transmitting informations between geoscape and battlescape.

Aren't these constants stored in X-COM data files? If not, it is possible to load them from your own file format.

If you want the game to be moddable, then these rules should be stored in files. If not, then there should be one hardcoded ruleset. But I don't understand why you want to put multiple hardcoded rulesets inside compiled game.

But ruleset probaly won't only contain stats but also formulas / algorithms. For example calculating shot accuracy in battlescape. In original ruleset accuracy won't be dependent on distance, but maybe in "openxcom" ruleset it will be. There can be also another factors for that. How would you like to put this in file? Without scripting language it will be difficult.

[pmprog]

Engines are separated not for saving memory. Look at Worms, for example. They have separate GUI for options and "battlescape". It just simplifies program.

If Battlescape died it is a bug. Just handle signals properly so it would be impossible to quit by closing the window.

But it's just completely unnecessary, and adds complications. It also means duplicating certain code (General GUI widgets for menus and buttons etc.)

Keeping it all in one program lets you access *any* data related to the game from whichever screen you're in. If you seperated them, you'd have to change two programs if you needed to expose any new data.

But ruleset probaly won't only contain stats but also formulas / algorithms. For example calculating shot accuracy in battlescape. In original ruleset accuracy won't be dependent on distance, but maybe in "openxcom" ruleset it will be. There can be also another factors for that. How would you like to put this in file? Without scripting language it will be difficult.

I agree with Eeyoocah here, whilst your class which holds the stats in memory, and has the algorithms for the game can be placed in the class, I would load the stats from a file in the constructor. Your algos don't need to change, so that's fine, but it does give you tweaking power...

[Eeyoocah5Moh]

But it's just completely unnecessary, and adds complications. It also means duplicating certain code (General GUI widgets for menus and buttons etc.)

You can share source files between programs.

[SupSuper]

That is a very bad idea:

- Having a program split into pieces like that is just asking for trouble. Sure back in the day you needed to save extra byte of memory you could, but you have to manage information between programs yourself which is bothersome and error-prone, there's no proper way to handle one of the parts failing (when the X-Com Battlescape died, the Geoscape wouldn't notice and just use the last available mission results), and it just restricts the parts needlessly (you couldn't quit or load saves from the Battlescape because the Geoscape had to do that). Who knows if I won't later extend the game to a point that the Geoscape and Battlescape should be tied together.

Engines are separated not for saving memory. Look at Worms, for example. They have separate GUI for options and "battlescape". It just simplifies program.

If Battlescape died it is a bug. Just handle signals properly so it would be impossible to quit by closing the window.

I presume you mean Worms 2, since none of the other games do that, and it's still a minority.

In any case I prefer sticking to what I'm familiar with and avoid any techniques outside my skillset.

Also you have XcomRuleset that is based on Ruleset.  You don't really need more than one ruleset in one game, just create one "Ruleset" class and everyone will be able to modify it if they want to change rules of the game.

Ruleset is probably a bit of a misnomer. In fact I still haven't set in stone what it really does, but there probably won't be many actual "rules" in there since an X-Com engine is an X-Com engine and there probably isn't a whole lot you wanna change without becoming a different game entirely. It probably won't contain gameplay features like whether

For now it's just a nice place to keep all the typically hardcoded numbers like craft stats, facility stats, item stats, weapon stats, research tree, etc. For example, as far as the engine is concerned, a craft is a craft and it has various properties and can contain weapons and soldiers and it can fly around intercepting stuff and going on missions. It's the ruleset that tells it there's a craft type X with acceleration Y and crew capacity Z and so on. Whether you call it a Skyranger or a Triton the engine will treat it just the same.

In any case, the reason there's support for "multiple rulesets" is people will probably wanna play around with these stats a lot, so I need to keep them fairly separate from the engine. The XcomRuleset has all the original game stats, but some people would probably prefer improved tanks and weapons like in XcomUtil. You might only have one ruleset in use by one game at a time, but the player might have a wide selection to choose from when they start a new game (kinda like mods). Hell, maybe they're a picky bunch and decide they want the "improved tanks" but not the "improved weapons", so you need to split the rulesets into bits so the player can pick multiple ones that get combined for them. That is the reason there's an abstract Ruleset, planning for the future.

Aren't these constants stored in X-COM data files? If not, it is possible to load them from your own file format.

If you want the game to be moddable, then these rules should be stored in files. If not, then there should be one hardcoded ruleset. But I don't understand why you want to put multiple hardcoded rulesets inside compiled game.

The constants are hardcoded into the X-COM executables, but since there's various versions with different offsets, it's easier to just "copy-paste" a bunch of numbers than loading them straight from the source.

Plus you're right, eventually the rulesets will go on external files, but for now the focus is to "Don't implement any new features until you fully reimplement original game or at least big part of it such as geoscape." so don't take any of the design as final.

[battlesquid]

Funny, we recently had the getter/setter discussion brought up at work. Only one developer really think getters and setters are evil, however he has a valid point about boiler plate/cluttered/unreadable code. No matter the size, it's a lot easier to get an overview of the class fields/methods when there's no getters and setters. But I also know that as IDEs get better and better (already actually), overview can be gained more easily by using integrated tools/functions of the IDE than by scrolling through the whole file.

And when you think you don't need to plan for the future (when suddenly you field cannot simply be accessed directly any more) you find yourself with your hands full of other things (don't act  surprised ). Then you wished you didn't need to refactor these things as well as handling every other issue/feature/bug. Lots of things to do seems to cause more errors imho, and too much to do can cause small projects to die (in general). Do correct me if I'm taking this too far, but my point is that it's best to avoid later refactorings when possible. And I think encapsulation is one such thing.

[SupSuper]

Funny, we recently had the getter/setter discussion brought up at work. Only one developer really think getters and setters are evil, however he has a valid point about boiler plate/cluttered/unreadable code. No matter the size, it's a lot easier to get an overview of the class fields/methods when there's no getters and setters. But I also know that as IDEs get better and better (already actually), overview can be gained more easily by using integrated tools/functions of the IDE than by scrolling through the whole file.

And when you think you don't need to plan for the future (when suddenly you field cannot simply be accessed directly any more) you find yourself with your hands full of other things (don't act  surprised ). Then you wished you didn't need to refactor these things as well as handling every other issue/feature/bug. Lots of things to do seems to cause more errors imho, and too much to do can cause small projects to die (in general). Do correct me if I'm taking this too far, but my point is that it's best to avoid later refactorings when possible. And I think encapsulation is one such thing.

I do hope that with the documentation people don't need to deeply dig around the files to figure out what the code does.

[battlesquid]

I do hope that with the documentation people don't need to deeply dig around the files to figure out what the code does.

Pretty docs, but to be frank I for one never read really docs unless the code is hard to read/follow or is un-/poorly commented. Slows down the development process to have to sit and read something other than the code you're working on I think.

[djemon_lda]

There is already nearly 8k lines of code (and 15k if you count blank lines and comments).  This number can be greatly reduced by removing most of such abstractions.

I am just afraid I would not be able to modify code when you finish implementing bigger part of it.

well 8k lines in a project is basically nothing. I have no picture on how many lines of code have appeared since the last update of this topic, but having proper abstractions - which in turn do not require full knowledge of the whole product right in the begining - reduce the amount of lines of code instead of bloating the code.
Scripting is just another way to reduce the amount of code and boost the speed of production. It is going to be harsh, but I'm afraid that you still need to learn A LOT before you can participate in making some software...