-
-
Notifications
You must be signed in to change notification settings - Fork 936
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
Codechange: Remove FOR_ALL_CHUNK_HANDLERS #9180
Conversation
The order in which chunks are saved does matter. Some chunks needed to be loaded before others in order for things to work properly. This is not actually documented anywhere but is implicit in the order in which chunk handlers are defined. |
Oh I see, indeed loading fails (I should have tested it before opening the PR). |
Would it be possible to assign a number for each savegame chunk to indicate where in the save file the chunk should be at? For example, the chunk assigned №1 is the first chunk to be saved, the chunk assigned №2 is the second chunk to be saved, and so on and so forth. That way, the order of the chunks can be preserved during saving, allowing the resulting savegame to be loaded properly. |
At least now it can load savegames it created. They can be loaded in master too. |
Currently, the gamelog chunk is saved first so that in the event of a crash during load, or if the savegame structure is lost part way through, the crashlog or debug logging still contains some useful information. With this applied the savegame chunk order may differ depending on the platform/compiler, what build flags are used, etc. |
I don't think any of the vector conversion is necessary here, a plain table works fine and you can still use ranged-for loops with them as std::begin/end is implemented for such types |
Though maybe a sentinel value (can't have nullptr) within the actual chunks might be better than CH_LAST |
Another try, but I may change everything again :) |
I tried another solution in master...glx22:saveload2 The idea is still to get a flat chunk list. |
There's another option in master...glx22:saveload3 |
saveload3 is least bad, but __ variable names are very much undefined behaviour. Otherwise seems fine IMO |
I think it is a lot better than the other attempts indeed. I guess you can safely remove What did felt a bit oddly complex to me, was the So, I came up with some suggestions as addition to yours: I also couldn't let go the My variant does copy all the These are just suggestions, feel free to cherry-pick which-ever you like! |
Fully a personal opinion, but I won't do the fancy glx22/OpenTTD@saveload3...TrueBrain:saveload3-1 Is an alternative, with just a very simple one-time-filling of the vector :) But, we are in the area of bikeshedding :D |
Co-Authored-By: Patric Stout <truebrain@openttd.org>
Merged saveload3 and @TrueBrain's suggestions in the PR. |
With that, this is ready for review? |
It is, unless we switch to auto-register with deterministic order based on priority and id, as suggested by @rubidium42. |
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 kinda need this for other changes I want to make to chunk handlers. Given nobody objected to this, lets go for this!
We can always change it later if we want to :)
Motivation / Problem
Chunk handlers are stored in an static array of pointers to static arrays, using special values as markers for the end of array.
FOR_ALL_CHUNK_HANDLERS()
parses the arrays to get each handler.Every new handler must be added manually to the list.
Description
I added a map ofChunkHandler
pointers, indexed by chunk id, and stored insideChunkHandler
struct itself._chunk_handlers
is now a vector ofChunkHandler
pointers.Now every handler is automatically registered, simplifying addition of new handler.
Then I could remove end markers, and enforce chunk type validation.
Limitations
Order of chunks in new savegames will be different, but it should not matter.
Groups of chunk handlers are reordered, but the order inside each group is kept.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.