aliens

Author Topic: Questions about code / design decisions  (Read 22883 times)

Offline karvanit

  • Captain
  • ***
  • Posts: 94
    • View Profile
Questions about code / design decisions
« on: November 09, 2012, 01:24:35 am »
I have the following questions about some of the decisions in OpenXcom code:
  • Why are we not using YAML anchors and aliases instead of thesaveId() functions? The file structure will be cleaner, and so will the code.
  • Instead of creating the objects and then loading them, why don't we have a "loading" constructor? Again, the code will be cleaner, since we can remove some functions that only get used once.
  • Why don't we have the Game object as a global object (or Singleton)? It is required in LOTS of places and we just store and pass it around (or it's Ruleset and SavedGame members, which amounts to the same thing). Making it visible everywhere would simplify LOTS of code and remove redundant arguments from lots of functions. It's not like we will ever have more than one.
  • Since we gave get/set for every member in some classes, why not make them plain structs?
  • Why do we return pointers to collections (eg Game::getUfos()) instead of references? The code has a lot of places where references are more suited to what we mean instead of pointers (eg Everywhere we store Game, Rule* and SavedGame pointers in classes as fields).
  • Why do we return non-const rules? They should not have writable fields after they are initialized.

If the answer is "too much work to change", I am more than willing to make a patch for either of the above.
« Last Edit: November 09, 2012, 02:33:16 am by karvanit »

Offline Fenyő

  • Colonel
  • ****
  • Posts: 423
    • View Profile
Re: Questions about code / design decisions
« Reply #1 on: November 09, 2012, 07:44:07 am »
Why don't we have the Game object as a global object (or Singleton)? It is required in LOTS of places and we just store and pass it around (or it's Ruleset and SavedGame members, which amounts to the same thing). Making it visible everywhere would simplify LOTS of code and remove redundant arguments from lots of functions. It's not like we will ever have more than one.[/li][/list]
Having global variables is pretty much defying to OOP's principles. Even if it would be simpler. :)

Volutar

  • Guest
Re: Questions about code / design decisions
« Reply #2 on: November 09, 2012, 08:06:52 am »
So OOP principles work on complication? Well, pragmatic approach defies any principle that implies any complication. OOP is not some holy cow to idolize it.
« Last Edit: November 09, 2012, 08:08:47 am by Volutar »

Offline Fenyő

  • Colonel
  • ****
  • Posts: 423
    • View Profile
Re: Questions about code / design decisions
« Reply #3 on: November 09, 2012, 08:28:58 am »
Actually it's not me who adheres to that principle, but the OpenXcom coding style does.

Offline karvanit

  • Captain
  • ***
  • Posts: 94
    • View Profile
Re: Questions about code / design decisions
« Reply #4 on: November 09, 2012, 10:22:54 am »
Classes having their data blindly manipulated from others directly is not OOP.
OOP means having objects represent ideas and keeping interactions to a minimum, each object containing all the data it needs and preneting the actions that make sense. So, for example, the Ufo should present methods for moving, damaging it or checking for interesting conditions (destroyed, landed, moving). Not give free access to everyone to directly set member variables. Moving all moving things on the map should be the responsibility of the class holding the list of all movables on the map, etc for many things.

Our Game object now has 3 distinct roles. It's the main loop, the game state database and the rules database.
The main loop is a service and should be a global object.
Many things require the ruleset, and we only ever care for one, so a global ruleset is not unreasonable.
Since our objects don't have  a clean state separation (data and behaviour are split across many objects), we need to pass the savegame around. But that is because it too has two roles. It's the 'persistent data' and also the 'current game state of all objects'. The 'current game state' is also a candidate for a global object, to be used everywhere except load/save routines (which actually care for the persistent data).

The above are my personal opinion and are not gospel. But I am obviously biased to think they are good ideas, unless proven otherwise.

Offline Warboy1982

  • Administrator
  • Commander
  • *****
  • Posts: 2333
  • Developer
    • View Profile
Re: Questions about code / design decisions
« Reply #5 on: November 09, 2012, 10:41:48 am »
oh, so THAT'S what you were doing!

sorry, i didn't mean it in a BAD way, i just wasn't sure exactly what you were doing it or why you were doing it that way (did i mention OOP is a relatively new concept to me?)
« Last Edit: November 09, 2012, 10:43:38 am by Warboy1982 »

Volutar

  • Guest
Re: Questions about code / design decisions
« Reply #6 on: November 09, 2012, 11:36:31 am »
Original XCom was programmed in procedural style with giant array of different global variables and arrays of structures.
So what's wrong with singleton? Is it out of oop paradigm? Or it's make mess in code?

Offline karvanit

  • Captain
  • ***
  • Posts: 94
    • View Profile
Re: Questions about code / design decisions
« Reply #7 on: November 09, 2012, 05:12:35 pm »
Original XCom was programmed in procedural style with giant array of different global variables and arrays of structures.
So what's wrong with singleton? Is it out of oop paradigm? Or it's make mess in code?

A singleton is (not just IMO) a glorified global pointer. Which IS what I am proposing, and I could live with
Code: [Select]
Game::instance()->foo() instead of
Code: [Select]
theGame->foo().

Offline SupSuper

  • Lazy Developer
  • Administrator
  • Commander
  • *****
  • Posts: 2159
    • View Profile
Re: Questions about code / design decisions
« Reply #8 on: November 13, 2012, 05:35:08 pm »
Oh boy programming debates, these are always fun! ::) Eight posts in and there's already various opposing justifications and viewpoints without any lead programmers even posting.

To stay on topic I'm just gonna address the points about the project, also note this is all just my opinion and I can't speak for Daiky and if I had done it today I probably would've done it completely differently and etc etc.

  • Why are we not using YAML anchors and aliases instead of thesaveId() functions? The file structure will be cleaner, and so will the code.
Don't YAML nodes just point to others? You'd still have to lookup the equivalent data structure in SavedGame, so I dunno about making the code clearer, although I guess the file format would be. But I don't know what yaml-cpp's support for them is like, there's no code examples.

  • Instead of creating the objects and then loading them, why don't we have a "loading" constructor? Again, the code will be cleaner, since we can remove some functions that only get used once.
Consistency I guess. Not every object is only loaded on construction (eg. modular rulesets), and it leaves the objects more open for alternate loading later.

  • Why don't we have the Game object as a global object (or Singleton)? It is required in LOTS of places and we just store and pass it around (or it's Ruleset and SavedGame members, which amounts to the same thing). Making it visible everywhere would simplify LOTS of code and remove redundant arguments from lots of functions. It's not like we will ever have more than one.
Singletons are... debatable. I don't really like them and I've always been taught not to like them. I don't see the need to arbitrarily restrict a class.

In this case you just want it as a global so you don't have to pass around pointers, but I don't think that really fixes the problem. Most game entities shouldn't need or even care about the entire game state, they just need particular bits. Game just started off as an abstraction for the core engine stuff but kinda grew into this big encompassing class because well, stuff has to go somewhere, and as particular elements needed particular stuff it just went all over the place. I guess optimally Game should be broken up so elements only need to reach what they need or something? I don't know how to properly redesign it so this wouldn't happen, OOP is great in theory until you find yourself not knowing how to actually put it into practice outside isolated examples, and you can end up going mad debating such design into infinity if you don't move on.

  • Since we gave get/set for every member in some classes, why not make them plain structs?
Whether it's better to have public fields or getter/setters is arguable. Personally I just do it for the consistency, if I do something once I do it all the time, and it makes fields easier to control and document (specially since OpenXcom is ever-changing). Yeah it becomes a lot of boilerplate code in some classes, but there's plenty of tools to generate it for you.

  • Why do we return pointers to collections (eg Game::getUfos()) instead of references? The code has a lot of places where references are more suited to what we mean instead of pointers (eg Everywhere we store Game, Rule* and SavedGame pointers in classes as fields).
Again, consistency. Probably has to do with old habits, I've always worked with pointers, so when references popped up I never got into them. What is their advantage over pointers? Aside from hiding the * which I don't really mind.

  • Why do we return non-const rules? They should not have writable fields after they are initialized.
Aren't rules read-only already? Anyways const-correctness is one of those things that takes forever to implement and forever to master. I don't know everywhere where it should go, though I've done my best to apply it across OpenXcom without breaking anything, so feel free to add more if you want.

Classes having their data blindly manipulated from others directly is not OOP.
OOP means having objects represent ideas and keeping interactions to a minimum, each object containing all the data it needs and preneting the actions that make sense. So, for example, the Ufo should present methods for moving, damaging it or checking for interesting conditions (destroyed, landed, moving). Not give free access to everyone to directly set member variables. Moving all moving things on the map should be the responsibility of the class holding the list of all movables on the map, etc for many things.
There's always a new evil going around in programming circles. Back in my day, getters and setters were all the rage. :P

Seriously though, I've seen this argument go around a lot with OpenXcom. "Oh no, your code is just getters/setters instead of proper abstractions, the horror!". I'll admit I'm not an amazing coder and OpenXcom isn't the pinnacle of design. But have you tried implementing game rules, especially for strategy stuff like X-Com? You'll be shocked to know most of it is just getting/setting values, there's no big deep simulation or behavior or whatever, and trying to fully abstract it all away results in design nightmares because game rules aren't all nice and well-behaved, they're tons of values interacting with each other.

For example, take a Craft. You think "oh a craft has to move to places". So you need to change its position, just add a "setPosition". Then you think "but but but the craft should move to places, not be directly controlled". Ok you instead put a  "setDestination" (maybe you'd prefer calling it "moveTo", it's just semantics), it does what it says, you give it a destination and the Craft slowly goes there by its rules. Woo, OOP, etc. But wait what about when you need to move the craft instantly, like when it's transferred between bases? Whoops turns out you needed that direct position control after all! And you'll run into this kind of stuff all the time, and your wonderful design goes to pot as you need to get stuff done and players just want to play things.

Our Game object now has 3 distinct roles. It's the main loop, the game state database and the rules database.
The main loop is a service and should be a global object.
Many things require the ruleset, and we only ever care for one, so a global ruleset is not unreasonable.
Since our objects don't have  a clean state separation (data and behaviour are split across many objects), we need to pass the savegame around. But that is because it too has two roles. It's the 'persistent data' and also the 'current game state of all objects'. The 'current game state' is also a candidate for a global object, to be used everywhere except load/save routines (which actually care for the persistent data).
I'm not sure I understand your definitions here, so this is the way I see the OpenXcom design:

- The Game class handles the core engine stuff, the SDL subsystems, the state and event management. It also contains the game contents (resource, ruleset, save) out of convenience.

- The states take care of all the game behavior and interaction with the player, and are what connect and interact with everything else forming a running game.

- The SavedGame and related classes are intended to contain the variable game state, everything that needs to be saved to disk to preserve the game. The 'current game state of all objects' is the 'persistent data', so serializing Savegame saves the game and preserves the game state. This is ignoring superficial stuff which you might consider "state" like UI etc. Originally it was just intended to be "dumb" variable storage that the states would manipulate, but some behavior ended up leaking into it, so now it's kinda mixed between states and savegames.

- The Ruleset and related classes are intended to contain the fixed game variables. This is all the fixed definitions in the game rules, all the same fixed values from game to game. No behavior.

Hope that clears things up.

Offline karvanit

  • Captain
  • ***
  • Posts: 94
    • View Profile
Re: Questions about code / design decisions
« Reply #9 on: November 14, 2012, 12:05:21 am »
First of all, thank you for answering. I'll try to justify my positions and / or provide clarifications.
Don't YAML nodes just point to others? You'd still have to lookup the equivalent data structure in SavedGame, so I dunno about making the code clearer, although I guess the file format would be. But I don't know what yaml-cpp's support for them is like, there's no code examples.
I agree that this part of yaml-cpp is not clear. However, what it would allow us to do is to avoid the whole 'saveId()' part. We'll just have
Code: [Select]
out << YAML::Anchor(toAnchorText(object));
object->save(out);
. . .
out << YAML::Alias(toAnchorText(other->ref_object));
instead of
Code: [Select]
object->save(out);
. . .
other->ref_object->saveId();

True, it's longer, but I believe it's cleaner as we don't replicate YAML, we specificaly state what we do (emit a reference to an existing object). And we don't have to create the 'saveId()' part, it's as if the whole node is duplicated. It will also make it easier to abstract I/O away for object hierarchies.

Consistency I guess. Not every object is only loaded on construction (eg. modular rulesets), and it leaves the objects more open for alternate loading later.
I can understand the _more open_ part. But right now every class with I/O is either constructed and filled (via setters), or constructed and effectivelly overwritten (with 'load()'), except the ruleset, which is only partially overwritten in 'load()'. Which, I guess, is the reason for this API.
Singletons are... debatable. I don't really like them and I've always been taught not to like them. I don't see the need to arbitrarily restrict a class.
Singletons are, IMO, pretty-OOPified globals. But the idea of having globals is not bad in itself. In OpenXcom, we already use globals / singletons (RNG and Ufopaedia). The idea of a single GameEngine at least is not extreme.

In this case you just want it as a global so you don't have to pass around pointers, but I don't think that really fixes the problem. Most game entities shouldn't need or even care about the entire game state, they just need particular bits.
Most stuff needs the pointers only during I/O and creation time, but since eg Game is not global we have to keep it around for when we just need to pass it down to another class. And this tends to proliferate.

Whether it's better to have public fields or getter/setters is arguable. Personally I just do it for the consistency, if I do something once I do it all the time, and it makes fields easier to control and document (specially since OpenXcom is ever-changing). Yeah it becomes a lot of boilerplate code in some classes, but there's plenty of tools to generate it for you.
I agree, getters / setters are the best practice, since they allow you to change this internal representation. But due to the ad-hoc class APIs we are not reallly hiding anything. Still, keeping them allows us to eventually move to a better design.

Again, consistency. Probably has to do with old habits, I've always worked with pointers, so when references popped up I never got into them. What is their advantage over pointers? Aside from hiding the * which I don't really mind.
A reference states (in code) that what you want is always going to be there and will never change.
Code: [Select]
class Ufo
{
private:
  const RuleUfo &_rules;
};
This means that a Ufo requires a rules object and it will be the same for the lifetime of the Ufo object.
They also allow the beautiful (IMO) getter/setter functions:
Code: [Select]
class Example
{
private:
  int _field;
public:
  int field() const { return _field; } https://Getter
  int &field() { return _field; } https://Setter
};
Aren't rules read-only already? Anyways const-correctness is one of those things that takes forever to implement and forever to master. I don't know everywhere where it should go, though I've done my best to apply it across OpenXcom without breaking anything, so feel free to add more if you want.
Will do so, gradually, thanks for  the ok.

For example, take a Craft. You think "oh a craft has to move to places". So you need to change its position, just add a "setPosition". Then you think "but but but the craft should move to places, not be directly controlled". Ok you instead put a  "setDestination" (maybe you'd prefer calling it "moveTo", it's just semantics), it does what it says, you give it a destination and the Craft slowly goes there by its rules. Woo, OOP, etc. But wait what about when you need to move the craft instantly, like when it's transferred between bases? Whoops turns out you needed that direct position control after all! And you'll run into this kind of stuff all the time, and your wonderful design goes to pot as you need to get stuff done and players just want to play things.
I understand what you mean, but the specific example IS  flawed. A Craft does not "move" (fly in Geoscape) between bases, and the owning base is a matter of game database. So the destination has nothing to do with it. You need a 'setBase()' (which, in an ideal world, only transfers would use).

I'm not sure I understand your definitions here, so this is the way I see the OpenXcom design:

- The Game class handles the core engine stuff, the SDL subsystems, the state and event management. It also contains the game contents (resource, ruleset, save) out of convenience.

- The states take care of all the game behavior and interaction with the player, and are what connect and interact with everything else forming a running game.

- The SavedGame and related classes are intended to contain the variable game state, everything that needs to be saved to disk to preserve the game. The 'current game state of all objects' is the 'persistent data', so serializing Savegame saves the game and preserves the game state. This is ignoring superficial stuff which you might consider "state" like UI etc. Originally it was just intended to be "dumb" variable storage that the states would manipulate, but some behavior ended up leaking into it, so now it's kinda mixed between states and savegames.

- The Ruleset and related classes are intended to contain the fixed game variables. This is all the fixed definitions in the game rules, all the same fixed values from game to game. No behavior.

Hope that clears things up.

Yes, things are cleaner now. What I meant was that the 'keeper of rules' and 'keeper of persistent data' should be outside Game, in top level objects of their own. And that would make these three classes good candidates for singleton / global data. Since what they do is provide  a service to all the rest of the code, so instead of having a pointer to them in each class, we just keep a global pointer of them. Like we (effectivelly) do with RNG and Ufopaedia.

Offline grrussel

  • Captain
  • ***
  • Posts: 72
    • View Profile
Re: Questions about code / design decisions
« Reply #10 on: November 18, 2012, 12:37:07 am »
The YAML format for save games is very explicit about default values. Its quite inefficient, space wise, and slow to load on large saved games (e.g. enlarged battles)

The bulk of an enlarged battle state is tile state. Most of the tiles are not on fire. They have no smoke. They are (not yet) discovered. These facts are all explicitly stored in YAML.
They can be defaulted in the load code, and the save code can emit YAML only if the content is not an implicit default.

Omitting fire: 0 and smoke: 0 in tile saves, and then discovered[false,false,false] reduced file sizes (in KB) as shown. (It may reduce save/load times)

george-russells-imac:OpenXcom grrussel$ ls -sk boom3.sav
4624 boom3.sav
george-russells-imac:OpenXcom grrussel$ ls -sk test_sav_change.sav
3928 test_sav_change.sav
george-russells-imac:OpenXcom grrussel$ ls -sk test_sav_change2.sav
3164 test_sav_change2.sav


The tile positions are also explicitly given, and could likewise be implicit due to ordering in the sequence.

Offline Fenyő

  • Colonel
  • ****
  • Posts: 423
    • View Profile
Re: Questions about code / design decisions
« Reply #11 on: November 18, 2012, 02:38:34 am »
@grrussel:
The man who wrote the save/load code; i also didn't understand why he made everything explicit.
Beyond what you have described (which you have totally right about) the backwards compatibility is also worse this way!
If there were default values, then problems like this could be avoided:
"hyperDetected:" is introduced, and now we have to convert all savegames manually by adding "hyperDetected: false" for every ufo.
And every single addition like this produces this problem, which could be avoided with default values.

So i also vote that the save/load code should be changed to work with default values.

Offline Warboy1982

  • Administrator
  • Commander
  • *****
  • Posts: 2333
  • Developer
    • View Profile
Re: Questions about code / design decisions
« Reply #12 on: November 18, 2012, 02:54:42 am »
while i agree with you i also feel it is worth stating:

backwards compatibility of saved games should not be a concern in what is essentially a development version.

Post 1.0, yes, this should DEFINITELY be taken into account, but at this stage of development, new versions breaking old saved games is par for the course.

i say just be thankful that it is saved in a plain-text format that allows you to make old saved games compatible with new revisions.

Offline karvanit

  • Captain
  • ***
  • Posts: 94
    • View Profile
Re: Questions about code / design decisions
« Reply #13 on: November 18, 2012, 03:55:12 am »
while i agree with you i also feel it is worth stating:

backwards compatibility of saved games should not be a concern in what is essentially a development version.

Post 1.0, yes, this should DEFINITELY be taken into account, but at this stage of development, new versions breaking old saved games is par for the course.

I'm 100% with warboy on this. Although saving only deviations from the default for the tiles (and even a little less verbose format for tile data) should be possible and desirable.

Offline Fenyő

  • Colonel
  • ****
  • Posts: 423
    • View Profile
Re: Questions about code / design decisions
« Reply #14 on: November 19, 2012, 09:35:00 am »
while i agree with you i also feel it is worth stating:

backwards compatibility of saved games should not be a concern in what is essentially a development version.

Post 1.0, yes, this should DEFINITELY be taken into account, but at this stage of development, new versions breaking old saved games is par for the course.

i say just be thankful that it is saved in a plain-text format that allows you to make old saved games compatible with new revisions.
I think that's the whole point of this problem. After 1.0 the savegame format will not change. (...much, or so frequently)
So after 1.0 the backward-compatibility doesn't needed anymore. (everyone convert his savegame when 1.0 comes out, and that's it)