Kitteh6660 / Corruption-of-Champions-Mod

CoC source from fenoxo, modded by Kitteh6660
232 stars 97 forks source link

Serialize player attributes #1388

Closed brrritssocold closed 5 years ago

brrritssocold commented 5 years ago

This PR is part of my ongoing efforts to clean up the save code. Will split the effort into multiple PRs due to the sheer size.

This particular PR:

TODO

Find a robust solution for serializing inheritance, as sub/super classes can have different versions, but they serialize to the same object. Currently the issue is side stepped by performing upgrade operations on every load, as it was done in Saves

Stadler76 commented 5 years ago

Since there were issues with old deprecated saveFile-props I'd suggest to dump old props after converting them.

Example (Did some backup->try&error&'check savefile in Minerva' testing):

        // Fix possible old save for Plot & Exploration
        flags[kFLAGS.TIMES_EXPLORED_LAKE]     = (flags[kFLAGS.TIMES_EXPLORED_LAKE] || saveFile.data.exploredLake || 0);
        flags[kFLAGS.TIMES_EXPLORED_MOUNTAIN] = (flags[kFLAGS.TIMES_EXPLORED_MOUNTAIN] || saveFile.data.exploredMountain || 0);
        flags[kFLAGS.TIMES_EXPLORED_FOREST]   = (flags[kFLAGS.TIMES_EXPLORED_FOREST] || saveFile.data.exploredForest || 0);
        flags[kFLAGS.TIMES_EXPLORED_DESERT]   = (flags[kFLAGS.TIMES_EXPLORED_DESERT] || saveFile.data.exploredDesert || 0);
        flags[kFLAGS.TIMES_EXPLORED]          = (flags[kFLAGS.TIMES_EXPLORED] || saveFile.data.explored || 0);

        flags[kFLAGS.JOJO_STATUS]        = (flags[kFLAGS.JOJO_STATUS] || saveFile.data.monk || 0);
        flags[kFLAGS.SANDWITCH_SERVICED] = (flags[kFLAGS.SANDWITCH_SERVICED] || saveFile.data.sand || 0);
        flags[kFLAGS.GIACOMO_MET]        = (flags[kFLAGS.GIACOMO_MET] || saveFile.data.giacomo || 0);

        // store them back into the saveFile
        var convertedFlags:Array = [
            kFLAGS.TIMES_EXPLORED_LAKE,
            kFLAGS.TIMES_EXPLORED_MOUNTAIN,
            kFLAGS.TIMES_EXPLORED_FOREST,
            kFLAGS.TIMES_EXPLORED_DESERT,
            kFLAGS.TIMES_EXPLORED,
            kFLAGS.JOJO_STATUS,
            kFLAGS.SANDWITCH_SERVICED,
            kFLAGS.GIACOMO_MET,
        ];
        for each (var flagID:int in convertedFlags) {
            if (flags[flagID] != 0) {
                saveFile.data.flags[flagID] = flags[flagID];
            }
        }

        // discard the values after conversion to avoid breaking later savefiles by double-converting them
        delete saveFile.data.exploredLake;
        delete saveFile.data.exploredMountain;
        delete saveFile.data.exploredForest;
        delete saveFile.data.exploredDesert;
        delete saveFile.data.explored;

        delete saveFile.data.monk;
        delete saveFile.data.sand;
        delete saveFile.data.giacomo;

PS: if (flags[flagID] != 0) { is intended to be non-strict because of the behavior of DefaultDict

brrritssocold commented 5 years ago

Well, data in the old save would be deleted because the next time the game is saved, the new structure would be written to the file.

As for the flags, I have not gotten round to cleaning them up. My current approach would be to make the class serializable, then move the flag to the class. e.g.: Make Jojo serializable, then move his state information into the Jojo class.

Currently the deserialization and upgrade code work like this:

  1. Does the object have a version? If yes, use that otherwise it is 0 (legacy)
  2. Upgrade function is called with the determined version, this function moves and modifies data in the loaded save game object so it will have the format the deserializer function expects
  3. Data is loaded by the deserialize function which restores the state of the class instance from the loaded save game object
Stadler76 commented 5 years ago

IMHO the saveFile itself has no 'real' version stored in it. Only the utterly outdated 'count props' code. Upon loading the saveFile it should be auto-updated to the newest version at some point.

My main reason behind this suggestion was, that leftover deprecated saveFile-props might be the cause of issues like issue #931.

For the version numbers: I did some brainstorming in issue #337. Might as well use this here as well.

CoC_Main.sol has one. Actually: permObjVersionID

PS: The flag MOD_SAVE_VERSION should be unaffected by this. Commit 953aafdae8875b690d4a16bb0cde96d775613213 was rejected, but it might give you some ideas.

Note, that this is a 'can do', not a 'must/should do'.

brrritssocold commented 5 years ago

Save file version could be used if the structure changes, e.g. it is decided that all serialized player properties should go to data.player instead of data. Or it can be used to initialize new properties, eliminating if / else code down the line when loading data.

Thank you for linking #931 , I thought I had resolved that, need to take another stab at it.

Version numbers per se is not the issue, but having more than one attribute for the serialization version on the same saved object, of the top of my head I would have gone with UUID (so the class can be moved renamed, etc. and correctly generated UUIDs should not collide). Then use an Array or Dictionary as a Map, to allow a lookup of the corresponding version (still need to look at the finer details of Array and Dictionary). This is of course if I don't run into any blocking issue during implementation, such as edge cases that would make it fail horribly.

The version numbers are sequential because this makes for simpler upgrade code: a switch/case with fall-through behavior, start at the given version and run through the rest of the sequence. Once the code reaches the end of the switch, it's up to date.

Whenever I extract something from Saves I check if there is any upgrade code scattered around and extract that to the upgradeSerializationVersion method which will then upgrade the 'legacy' save. If that is not possible for some reason (as currently with dealing with inheritance) I put it in the deserialization function with a TODO explaining why it is here and not in the upgrade method and what needs to be done.

Stadler76 commented 5 years ago

@brrritssocold wrote:

Thank you for linking #931 , I thought I had resolved that, need to take another stab at it.

I think this would only happen, if a player has corrupted Jojo 'before' CoC.monk was moved to the flags and then ascended. Thus its a very rare case, but it shows, that deprecated values should be deleted from the saves.

Version numbers per se is not the issue, but having more than one attribute for the serialization version on the same saved object, of the top of my head I would have gone with UUID (so the class can be moved renamed, etc. and correctly generated UUIDs should not collide). Then use an Array or Dictionary as a Map, to allow a lookup of the corresponding version (still need to look at the finer details of Array and Dictionary). This is of course if I don't run into any blocking issue during implementation, such as edge cases that would make it fail horribly.

The 'problems' with MOD_SAVE_VERSION is that the flag is being used in CoC Vanilla+ to avoid incompatibilities since CoC Vanilla+ is backwards compatible with old style CoC, but incompatible with Revamp saves. However: For that I'd rather suggest to rename the save files to for example CoCR_(XX|Main).sol to avoid mod to mod incompatibilities. About UUIDs: That might work, but the advantage of versionIDs is, that they are comparable. And if you do more than one update to the save file structure, then you could always do a simple versionID++ meaning: 1041500 → 1041501 → 1041502 → 1041503 → ... UUIDs would try to fix an extremely rare case, where multiple parallel updates in the same game version clash with each other, thus adding unneeded complexity. Anyway: Its just 'my' opinion. You don't have to share it ;-)

[Edit:] Oh, before I forget to mention: Here I'm talking about future updates to the save file structure. This PR looks good to go to me.