Skip to content

Conversation

@pcen
Copy link
Contributor

@pcen pcen commented Apr 18, 2024

cherry-picked #425 and added some fixes to make it work on the godot4 base branch

I will also cherry-pick the save game code path as a separate PR.

@pcen pcen changed the base branch from Development to godot4 April 18, 2024 22:07
@pcen pcen force-pushed the godot-4-base-save-without-references branch from 583a347 to f0936b8 Compare April 19, 2024 00:18
@pcen pcen marked this pull request as ready for review April 19, 2024 03:13
@pcen pcen changed the title Save without references --> godot4 Save without references for godot4 branch Apr 19, 2024
@pcen pcen requested a review from QuintillusCFC April 19, 2024 03:14
Copy link
Member

@WildWeazel WildWeazel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General comment: I won't insist you reformat everything unless it's trivial to do so, but we're following Godot's C# style guide which prescribes opening braces on a new line: https://docs.godotengine.org/en/stable/tutorials/scripting/c_sharp/c_sharp_style_guide.html#line-breaks-and-blank-lines
I want to put together some IDE settings that will automatically apply formatting.

@@ -0,0 +1,62 @@
using System.Collections.Generic;

namespace C7GameData.Save {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still trying to work out exactly how it should work, but in the future I would like for everything in this namespace to instead be associated with the actual domain objects. In other words things in the game state should be able to (de)serialize themselves, rather than have a separate namespace for handling saves. This is partly to keep tightly coupled code together so we aren't always having to update things in two places (as there are a bunch of explicit field copies in here that have to be maintained), but also because as the game engine grows the save format will evolve and eventually be more composable like if mods need to include their own data in the save. It takes some of the complexity out of the process if everything can handle its own save/load logic and just relies on the save manager to handle file IO and high level direction. But this isn't an action item for this merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should have game state objects serialize themselves. We tried this a couple of times before but couldn't make it work. There's a couple issues:

  1. None of the available JSON serialization frameworks could properly support reference types, it required a lot of hacks and after a couple of attempts we couldn't really get it to work
  2. The save format structure doesn't map to domain objects. We need everything in the save file to be defined once. If this serialization logic is done by game state objects, the location of any entity in the save is implicit and would have to be inferred from reading all of the game state code. With explicit save objects that are plain old values, you can understand the save structure just by reading the class definitions
  3. There's a non-trivial amount of logic to inflate a save and create a valid game state. If serialization logic was done by the game state objects, we'd have a bunch of implicit dependencies between game state objects (ie. Tile shouldn't serialize City information, because Player already serializes this into the save). Even though there's some duplicate code, I think it's much simpler to maintain these save objects where the rules are clear and everything is serialized "as is". Also, this save inflation/packing logic should all be in one place, if it's spread out on each game state class it would be harder to follow imo
  4. For custom mod data in the save, I think it's also better to have the logic to identify this and inject it into the game state accordingly be with the rest of the save loading logic. I think decoupling all of this from the game state object will result in easier to maintain code. There's also the versioning issue - if we have game state objects handle serialization, they need to be aware of the save version, and handle serialization/deserialization accordingly. If we have save objects instead that are plain old data, it's easy to load a save version, and then have methods to "upgrade" the save objects in memory to most recent representation, and then create the game state objects. This way, game state doesn't need to worry about backwards compatibility, as this will be handled by the classes/logic in the Save namespace

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I pretty much agree with what pcen said - the downside of having to have the load-and-save code solves the problems that pcen enumerated. Points 2 and 3 in particular were clear shortcomings with our "let the framework help every object (de)serialize itself" approach, and how to resolve that while sticking with that approach was not clear. Perhaps it could theoretically be done with enough annotations spread around those classes about what (not) to serialize, but I see the approach this PR has taken as a "define the interface once and in one place, and everything on both sides can communicate with it" approach, which has benefits.

Mods are still a "figuring out how it can work" issue, but it's been long enough that I think "move forward with what works for the base case" is the right thing to do. The "logic to identify this and inject it" makes me think that perhaps we allow mods that want to have separate structures in the save (not just a new building, but a new concept) to hook into the save loading/saving process - a sort of "we'll load everything the base games knows about by default, a mod can tell us how to load anything unique to it" approach. Though "figuring out how it can work" applies to that idea as well.

@WildWeazel
Copy link
Member

@pcen bump... if you can do something else with those saves and reformat if you want to now, then I think we're good to merge this

@pcen pcen force-pushed the godot-4-base-save-without-references branch from fa9e4db to 9e94222 Compare October 1, 2024 04:21
@pcen pcen merged commit caffcdd into godot4 Oct 1, 2024
@pcen pcen deleted the godot-4-base-save-without-references branch October 1, 2024 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants