Enable save game - Githubissues

pcen commented 2 years ago

Works towards https://github.com/C7-Game/Prototype/issues/223

A couple of notably broken features when loading saves that I want to fix incrementally to keep PRs easy to review:

upon loading a save, units with a preset path will "forget" this and lose their path
AI data is messed up and will throw exceptions under some circumstances

A main implementation detail I'd like to decide on in this PR is how to serialize Json. I changed the current serialization settings to preserve references. As far as I can tell, this decreases the save size, and also makes it easier to load saves into a working state (as opposed to serializing all objects "by value", which requires a significant amount of post load processing to get into a valid state to run the game). The downside of references is that they make the save format slightly less human friendly, and the "format" itself is dependant on how the serialization assigns reference IDs. I personally think this is fine since, ideally, there won't be any need to mess with save files directly since C7 should ultimately have a good enough editor.

WildWeazel commented 2 years ago

bump

QuintillusCFC commented 2 years ago

Re-based to build upon the current development branch. There were about as many conflicts as would be expected, but they didn't take super long to resolve. Though it isn't quite working yet... I think because of subsequent data added to the save.

QuintillusCFC commented 2 years ago

I've got this so it's based on the current Develop branch, and works on turn 0. But it fails later, it seems to be because units move from their original tile and there are references to units that aren't there anymore.

c7-static-map-save.zip

This appears to be due to recursiveness in the structure. In the attached file, on line 51,424, you'll see tile 65, 33 (or 1352 in 1D). On line 51,445 it has its unitsOnTile object. On line 51,483, we see the first unit has its owner object. On line 51,509, that owner object has its units object. On line 51,521, that units object's first unit has its previousLocation object. On line 51,564, that previousLocation tile object has its unitsOnTile object.

Despite all the duplication, though, if you put this file as your c7-static JSON file, you'll see it fails on line 51,757, a reference to another of the owner's units, item 5709, which doesn't exist for some reason. Though we can see it on line 51,568, a Chariot which is listed as a unit on the tile 64, 34.

QuintillusCFC commented 2 years ago

I think we missed the forest for the trees with this PR the first time around, despite Paul calling out the forest - the main implementation detail of how we want to serialize our files. As it is now, it's using references to refer to everything (see the diff for SaveFormat.json). This has plusses and minuses:

In theory, makes it easier for us (less manual JSON serialization configuration
- Not as human-friendly for editing. In late 2021, human-friendly editing was specifically a goal of the project.

The human-friendly editing is an area where I feel like we haven't really settled on a good answer, though. It goes back in part to the GUID discussion - I'm still of the opinion that GUIDs are not human-friendly, and in fact we'd be better off with the references we see in the static-JSON generated by this changeset. If we want something human-friendly, it needs to be in a format that average humans (not developers) can understand. Carthage-Warrior-23, not 183ae-f28e-330bd.

QuintillusCFC commented 2 years ago

The other big issue I see is the recursive aspect I mentioned above. A tile has units, a unit has an owner, the owner has units, and things are getting duplicated. This also negatively impacts the human modifiability (if a unit is duplicated in 13 places, you have to update any property you update in 13 places). But as seen in my prior comment, it seems to also run a risk of gumming up the works for JSON deserialization.

At the present time, my thinking is that we should try to flatten out the structure as much as possible when saving. By that I mean that while having nested references is very convenient in-game, it rapidly becomes confusing in a save because you have to somehow draw the line at where the canonical copy of an object is stored.

Example: We let units reference their location, and locations (tiles) reference the units on them. Great in-game, you can look things up whichever way is convenient, which is quicker from a CPU standpoint and only costs a few bytes for a reference in memory. But when you save it, that can look like:

saveGame {
  units {
    unit1 {
      tile {
        42, 58
        unitsOnTile {
          unit1 {
             //another copy of unit1?
     ...
  tile {
   ...
   tile 42, 58 {
     unitsOnTile {
       unit1 {
         tile {
          //another copy of 42, 58?

If we only store the data in one location, preferably with a relatively flat structure, and re-inflate our references on load, this potentially-never-ending chain of references ceases to be a problem:

saveGame {
  units {
    unit1 {
      key: "Carthage-Warrior-23"
      //don't store location here

tile {
  ...
  tile 42, 58 {
    unitsOnTile {
      key: "Carthage-Warrior-23"

Or

saveGame {
  units {
    unit1 {
      locationKey: "42, 58"

tile {
  ...
  tile 42, 58 {
    //don't need to store units on tile

This probably is a bit more work up-front for serialization, but it means we don't have data duplication in the file, so you can update the data fairly safely as a human and not have things break because you didn't know there were five other places you had to update the same data. It also cuts down on file sizes (though that's less of a concern once we add compression).

QuintillusCFC commented 2 years ago

Another hazard that would soon appear once the line 51,757 one is resolved is on line 52,162. This is where the first AI's strategic priorities are stored, or more accurately, not stored. System.Text.Json doesn't support polymorphic deserialization, and it's storing the AI's priorities as references to objects that it doesn't serialize. Search for the reference IDs is stores, and they are only in one place.

Polymorphism is key to the pluggable (and thus extendable/moddable) AI, and while System.Text.Json does allow writing custom deserializers for polymorphic classes, that's a big ask for anyone wanting to mod the AI, beyond what would already be required; it's also another step that will slow down AI development.

Thus, IMO, it's worth switching to Newtonsoft JSON for its automatic polymorphic deserialization. Although Microsoft plans to add support for that in .NET 7 (https://learn.microsoft.com/en-us/dotnet/api/system.text.json.serialization.jsonderivedtypeattribute?view=net-7.0), so maybe that is an option too.

Overall, though, I think the "how do we want the save to be structured?" question mentioned above is the most important one.

pcen commented 1 year ago

closing because https://github.com/C7-Game/Prototype/pull/399 implements the equivalent based on Godot 4 / .NET 7; it was easier to create a new PR than to rebase this one

C7-Game / Prototype

Enable save game #235