Fix ID collisions on save and scenario loading #533
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Implement logic to track the highest ID count per ID type. Now, when loading a save file with units "worker-1" and "worker-2", the ID generator will start producing "worker" IDs from 3.
pcen
reviewed
Jan 30, 2025
| // "worker": 3, | ||
| // } | ||
| public Factory(SaveGame save) { | ||
| IEnumerable<IHasID> entities = new List<IEnumerable<IHasID>>() {save.Units, save.Cities} |
There was a problem hiding this comment.
I understand the motivation of only doing this for units and cities, but I suspect it may be simpler and less error prone to have a way to get the ID factory used to create the SaveGame, and copy the internal state from that factory instead of iterating over the SaveGame contents themselves
There was a problem hiding this comment.
Can you elaborate? I don't see how it would be viable for loading a JSON save that already has a set of predefined IDs.
TomWerner
approved these changes
Feb 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes ID collisions when loading existing game files by introducing a new constructor to the ID Factory class. The constructor accepts a SaveGame instance and tracks the highest ID count per ID type.
Currently, this is implemented only for units and cities, as they appear to be the only entity types created during gameplay. In contrast, civilizations and technologies are assigned IDs only at the start of the game, so tracking them when loading a save file is unnecessary.
The PR includes unit tests for the ID Factory.