Questions about code / design decisions

[Warboy1982]

you make a good point. i think the best compromise here would be some sort of "savegame updater" tool, and i have a friend making one as i type this.

[Fenyő]

The default-valued save/load handling is a better version, you already agreed that.

Now. What would happen if i change the whole save/load handling on my own, and make it in a PR?
Will you - and everyone - then follow that default-value convention for the newer features?

If yes, then i'll make this change.

[Warboy1982]

i can't speak for everyone, but if it's something as simple as adding an if statement then yeah, i see no issue.
i don't really understand the YAMLCPP library at all, or how the save code really even works. i just know that it does, and that's enough for me

[Fenyő]

Ok then:
@SupSuper: If i make this, will you prescribe the default-valued save/load convention as a coding guideline?

[SupSuper]

Ain't hindsight a wonderful thing?

You gotta realize, OpenXcom is kinda old. We've been at this for over 3 years now, it starts to show. Much like any coder will look at anyone else's code and go "pfffff this is terrible, it should be done this way", they will also have the same reaction just looking at their own code after a few months. I was still starting university when I began OpenXcom and now that I'm almost finishing I see code completely differently. Different code was written at different times with different knowledge and different priorities (and since it's OSS, often by different people), so eventually it all starts to look a bit schizophrenic.

So about the load/save code. It wasn't implemented from the start, it was added in later in 0.2. And as anyone knows, serializing in C++ is a colossal effort, specially if you don't do it from the start, because there's no convenient reflection or whatever, you have to go through every single class and add in the appropriate code manually, and debug throughly all the mistakes you're bound to make. It took forever, it was a huge pain, but in the end it worked and that was all we cared about. No real concern for performance or maintainability or whatever, that would all just be even more extra effort and code, and our priorities weren't in it.

So of course there's a bunch of things wrong with it. I bet I know more of them than you. Yes, it's not size-efficient, it just dumps all the data straight into files, not accounting for default values and always breaking when something is added. It's also slow, although this is mostly yaml-cpp's fault, I've discussed it with their developer. For example, did you know accessing a keyvalue in a map node (eg. node["value"]) is actually a O(N) operation and not a O(1) operation as you'd expect? yaml-cpp manually looks up the value in the map everytime instead of keeping an index or whatever. The workaround is to iterate through the keyvalues yourself, instead of doing lookup. Even then, yaml-cpp is still slow (libyaml has it completely beat, it loads even our massive saves in a few secs, but it's C-based). Not only that, the current API we're using for yaml-cpp is deprecated, so the whole thing probably needs to be rewritten anyways. Yeah. That sounds like a fun time, deciding all that stuff. So it's been in the backburner for a while.

For comparison, the ruleset loading code, which came later, accounts for default values and the O(N) problem, so if you want a better example of yaml-cpp code to follow, that would be it. Of course this makes it much more verbose, basically consisting of huge else-ifs. And given all the underlying issues anyways, you really gotta consider if it's worth just keep on "patching" the existing load/save code.

@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.

OpenXcom pre-release is not supposed to be backwards-compatible. At all. I want the freedom to make and break the save structure whenever I want. When the game is nice and stable and pretty, sure, backwards-compatible for everyone, but not now. How are you gonna deal when the alien AI gets added and people already have saves in 3000 with everything unlocked and none of the required alien infrastructure?

Not only that, you're not supposed to keep carrying saves from old pre-release versions. You're not supposed to be trying to play for completion here, but testing every version brand new. If it's too heart-breaking to always start fresh whenever something changes, tough, that's testing for you, it's mindlessly repetitive. Old versions might carry old bugs that bleed into the saves, and constantly carrying and patching them around might just end up producing non-bugs later down the line. Testing is useless if you can't be sure the origin is actually the thing you're testing.

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)

This is an extremely optimistic view. If you really expect OpenXcom to just freeze on 1.0 and don't see how much stuff keeps getting added because people keep wanting more, you've got another thing coming. The version number in the save isn't just for show, it's a precaution.

[grrussel]

re the version number in the save file; its probably wise to bump it immediately after a release - as now we will have 0.4.5 tagged save files with various features ;-)

This looks currently set from Options.cpp
std::string _version = "0.4.5";

If you think the yaml-cpp is slow, you should see the load time of the save game files in python!

FWIW: the speed of save load/store is not noticeable so far for normal XCOM - only with my way larger battlescape sizes.

[Fenyő]

I want the freedom to make and break the save structure whenever I want.

How the default-valuing breaks this freedom? I think you could break the structure at any time with default-values too.

How are you gonna deal when the alien AI gets added and people already have saves in 3000 with everything unlocked and none of the required alien infrastructure?

I don't see the problem here. It would have its default value. I admit that those games might look a little schizophrenic because some parts look like 'the game just begun' after game-loading.

And actually i don't expect FULL backwards-compatibility from the default-valuing, the main purpose to handle the MINOR changes, like adding things like "hyperDetected". And the second main purpose is the size and speed efficiency, of course.

The version number in the save isn't just for show, it's a precaution.

Yes, i know that.
But still, if someone wants to migrate a savegame to a new version, he would have a better chance this way, by only setting the version number in the savegame.

[karvanit]

The proper fix for default values is to have ALL fields initialized in a class' constructor with values representing the default, and perform conditional overwrite in [oad(). For some fields it may not really work, but it's the best possible solution.

Still, savegame compatibility right now IS a useless goal. The game is still in flux, saving a game should only be considered part of bug hunting (keep a save to have a test case for reproducing a bugs).

[Fenyő]

The proper fix for default values is to have ALL fields initialized in a class' constructor with values representing the default, and perform conditional overwrite in [oad(). For some fields it may not really work, but it's the best possible solution.

That's what i thought.
And a conditional save() is also needed, checking if the value to be written is equal to the default or not. (and only write out if it's not the default)

[karvanit]

Regarding yaml-cpp deficiency:
Using the if-else and iterating in order is not significally faster, we still do O(N) comparisons for each node.
In addition, the code is harder to read and we'll get no benefit if/when yaml-cpp changes to keep values in maps instead of vectors. Using code like the following is my best solution so far:

Code: [Select]

const YAML::Node &node = doc["thingy"];
node["field1"] >> variable;
node["field2"] >> variable;
The code above only searches for "thingy" once, and the follonode["field1"] >> variable;wing searches are (hopefully) in the smaller contents.
And it is still easier to read and understand.

[karvanit]

That's what i thought.
And a conditional save() is also needed, checking if the value to be written is equal to the default or not. (and only write out if it's not the default)

For smaller save games yes. But not for compatibility. I don't think the savings will be enough to go to the trouble of doing it for every little field. If the size becomes a problem, it's much better to simply keep the files compressed.

[Fenyő]

Don't forget if we have smaller savegames, then the O(N) means much less.

And this is the point for having smaller savegames, compressing it is not reducing the O(N).

[karvanit]

Don't forget if we have smaller savegames, then the O(N) means much less.

But if you remove fields, you are always searching the entire list for nothing! 

[Fenyő]

But the entire list is a LOT smaller.

And BTW, if you read it without checking isn't that searching the entire list also?

[SupSuper]

My post was about the load/save code in general, not default-values. If you wanna implement default-values everywhere be my guest, I agree it's a good idea, just be aware that it might be a fruitless effort since the load/save code might just be rewritten later, and it will be a massive amount of work as the load/save code is huge and probably very bug-prone (not everything has immediately obvious default-values), so you better know what you're doing.

re the version number in the save file; its probably wise to bump it immediately after a release - as now we will have 0.4.5 tagged save files with various features ;-)

This looks currently set from Options.cpp
std::string _version = "0.4.5";

I use the version number more to tempt people when a new version is coming.

Using the if-else and iterating in order is not significally faster, we still do O(N) comparisons for each node.

Doing N iterations is a lot cheaper than N comparisons. But yes I clearly stated that this isn't a huge deal, since the N amount of elements in a savegame yaml map is in the dozens at most.

Using code like the following is my best solution so far:

Code: [Select]

const YAML::Node &node = doc["thingy"];
node["field1"] >> variable;
node["field2"] >> variable;
The code above only searches for "thingy" once, and the follonode["field1"] >> variable;wing searches are (hopefully) in the smaller contents.
And it is still easier to read and understand.

So leave it as is?

The proper fix for default values is to have ALL fields initialized in a class' constructor with values representing the default, and perform conditional overwrite in [oad(). For some fields it may not really work, but it's the best possible solution.

This is also how ruleset loading already works.