Enable save game

[pcen]

Works towards #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.

[maxpetul]

Does this need to be public? The project still compiles if it's left as private. I'd rather we don't have parameterless public constructors unless we need them.

[pcen]

Yes, in order to deserialize TilePaths, the parameterless constructor needs to be public

[maxpetul]

I see. I'm used to C++ where private is only enforced when compiling. It's inconvenient that we're forced to include these constructors since they prevent us from enforcing RAII.

[pcen]

Agreed. I can add a comment explaining why it's public at least, but it's unfortunate that it's enforced at runtime

[maxpetul]

It would be convenient to print out the save file location here too, since we don't yet give the player the option to choose it. I ran this branch to test it and it was not obvious to me that the save file would overwrite c7-static-map-save.json.

[maxpetul]

I got a null reference crash on this line when testing a game that I had saved out then loaded back in. That null check on unit.path is still needed. Preferably we could prevent MapUnit.path from ever containing null in the first place. There is a TilePath.NONE object but evidently it's not being used somewhere it should be. In general, we're doing a halfhearted take on the empty object pattern which leaves us with the worst of both worlds as references to either null or an empty object appear unpredictably.

Also, after commenting out this line, I received another crash: "AI data not recognized" from PlayerAI.getAIForUnitStrategy.

[pcen]

Yes, with regard to empty objects vs. null, it's pretty inconvenient in the code right now, since with the current lack of standard, in order to be sure you won't cause an exception, you basically need to check for null and the empty instance. And even when there is an empty instance, it's not consistent as to whether or not they can be treated as other instances, and in some cases they may contain null references. We should probably pick between null vs. empty objects and do some cleanup

[pcen]

I think the AI data not recognised stems from the Json serialiser not being able to handle instances of an interface type. I haven't really touched the AI code, but is there a clear advantage to using interfaces over ABCs with only a single level of inheritance? tagging @QuintillusCFC if you have any thoughts on this. It seems like our options are either to write a bunch of code to serialise and deserialise instances of each interface type we want to save, swap out the interface for a class type, or reconstruct AI data at load / runtime.

[QuintillusCFC]

On the AI data: The main benefit of using the interface is that MapUnit.cs can store the data easily using the interface type. With only concrete types, we'd either have to have unused storage spots for every type on MapUnit, or use whatever C#'s equivalent of Java's Object is... which would probably have the same serialization/deserialization challenges.

My inclination is to learn more about C# serialization, as we can't be the first project to want to serialize a concrete type and refer to it with an interface in our code.

As for null versus empty objects, I support the empty objects. Null objects were one of the biggest pain points when working with my editor, and empty objects provide better readability as well. It is true that we haven't done a good job of enforcing this (in part due to a hesitancy to enforce standards in the Wild West days), and that in some cases the empty objects weren't fully initialized. I recall seeing the null TilePath at some point and wondering why the code wasn't just using TilePath.NONE . This would be a good standardizing opportunity.

[WildWeazel]

bump

[QuintillusCFC]

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]

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]

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]

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]

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]

closing because #399 implements the equivalent based on Godot 4 / .NET 7; it was easier to create a new PR than to rebase this one