OpenXcom > Suggestions
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:
--- Quote from: Eeyoocah5Moh on June 25, 2010, 12:29:25 PM ---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.
--- End quote ---
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.
--- Quote from: Eeyoocah5Moh on June 25, 2010, 12:29:25 PM ---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.
--- End quote ---
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.
--- Quote from: Eeyoocah5Moh on June 25, 2010, 12:29:25 PM ---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.
--- End quote ---
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:
--- Quote from: pmprog on June 25, 2010, 01:19:12 PM ---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.
--- End quote ---
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.
--- Quote from: pmprog on June 25, 2010, 01:19:12 PM ---
--- Quote from: Eeyoocah5Moh on June 25, 2010, 12:29:25 PM ---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.
--- End quote ---
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.
--- End quote ---
Then just store these options inside "Ruleset". Currently it looks like we are going to have two or more rulesets at a time.
--- Quote from: pmprog on June 25, 2010, 01:19:12 PM ---
--- Quote from: Eeyoocah5Moh on June 25, 2010, 12:29:25 PM ---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.
--- End quote ---
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.
--- End quote ---
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:
--- Quote from: Eeyoocah5Moh on June 25, 2010, 12:29:25 PM ---Don't implement any new features until you fully reimplement original game or at least big part of it such as geoscape.
--- End quote ---
That is the plan. Any improvements I introduce in the mean-time are purely because of how simple/handy they are.
--- Quote from: Eeyoocah5Moh on June 25, 2010, 12:29:25 PM ---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.
--- End quote ---
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.
--- Quote from: Eeyoocah5Moh on June 25, 2010, 12:29:25 PM ---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.
--- End quote ---
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: ---struct Soldier { int _tu, _stamina, _health, _bravery, _reactions, _firing, _throwing, _strength, _psiStrength, _psiSkill, _melee; }
--- End code ---
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.
--- Quote from: Eeyoocah5Moh on June 25, 2010, 12:29:25 PM ---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.
--- End quote ---
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.
--- Quote from: Eeyoocah5Moh on June 25, 2010, 12:29:25 PM ---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.
--- End quote ---
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:
--- Quote from: SupSuper on June 26, 2010, 07:28:30 PM ---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.
--- End quote ---
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.
--- Quote from: SupSuper on June 26, 2010, 07:28:30 PM ---- 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.
--- End quote ---
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.
--- Quote from: SupSuper on June 26, 2010, 07:28:30 PM ---
--- Quote from: Eeyoocah5Moh on June 25, 2010, 12:29:25 PM ---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.
--- End quote ---
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.
--- End quote ---
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.
Navigation
[0] Message Index
[#] Next page
Go to full version