-
Notifications
You must be signed in to change notification settings - Fork 27
Save without references for godot4 branch #443
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
583a347 to
f0936b8
Compare
WildWeazel
left a comment
There was a problem hiding this 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 { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- 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
- 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
- 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
- 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
There was a problem hiding this comment.
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.
|
@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 |
…esToKnown since neighbors shouldn't automatically be included
fa9e4db to
9e94222
Compare
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.