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
out << YAML::Anchor(toAnchorText(object));
object->save(out);
. . .
out << YAML::Alias(toAnchorText(other->ref_object));
instead of
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.
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:
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.