Skip to content
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

Merged
merged 1 commit into from Jun 6, 2021
Merged

Conversation

@glx22
Copy link
Contributor

@glx22 glx22 commented May 2, 2021

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 of ChunkHandler pointers, indexed by chunk id, and stored inside ChunkHandler struct itself.
_chunk_handlers is now a vector of ChunkHandler 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.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
@JGRennison
Copy link
Contributor

@JGRennison JGRennison commented May 2, 2021

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.
If this ordering is removed, loading of savegames should be tested carefully.

@glx22
Copy link
Contributor Author

@glx22 glx22 commented May 2, 2021

Oh I see, indeed loading fails (I should have tested it before opening the PR).

@glx22 glx22 marked this pull request as draft May 2, 2021
@James103
Copy link
Contributor

@James103 James103 commented May 2, 2021

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.

@glx22 glx22 marked this pull request as ready for review May 2, 2021
@glx22
Copy link
Contributor Author

@glx22 glx22 commented May 2, 2021

At least now it can load savegames it created. They can be loaded in master too.

@JGRennison
Copy link
Contributor

@JGRennison JGRennison commented May 2, 2021

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.
This may create the opportunity in future for some builds to have difficult to diagnose save/load issues, but not other builds of the same source.
Determinism is in general a good thing. Is it really necessary to have an undefined savegame chunk order to remove the FOR_ALL_CHUNK_HANDLERS macro?

@glx22 glx22 marked this pull request as draft May 3, 2021
@LordAro
Copy link
Member

@LordAro LordAro commented May 3, 2021

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

@LordAro
Copy link
Member

@LordAro LordAro commented May 3, 2021

Though maybe a sentinel value (can't have nullptr) within the actual chunks might be better than CH_LAST

@glx22
Copy link
Contributor Author

@glx22 glx22 commented May 3, 2021

Another try, but I may change everything again :)

@glx22
Copy link
Contributor Author

@glx22 glx22 commented May 31, 2021

I tried another solution in master...glx22:saveload2

The idea is still to get a flat chunk list.

@glx22
Copy link
Contributor Author

@glx22 glx22 commented Jun 1, 2021

There's another option in master...glx22:saveload3

@LordAro
Copy link
Member

@LordAro LordAro commented Jun 1, 2021

saveload3 is least bad, but __ variable names are very much undefined behaviour. Otherwise seems fine

IMO

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jun 1, 2021

I think it is a lot better than the other attempts indeed. I guess you can safely remove __ from the variable names, as they are now local static variables anyway.

What did felt a bit oddly complex to me, was the ChunkHandlers. Also it added the initializer_list again, and if there is one thing I learnt yesterday from the response from a few, that it is really unusual to use that in any explicit form :P
EDIT: I revisited this, and made less changes and kept your struct ChunkHandlers instead :)

So, I came up with some suggestions as addition to yours:
glx22/OpenTTD@saveload3...TrueBrain:saveload3

I also couldn't let go the const ChunkHandler *ch .. I replaced them with const ChunkHandler &ch instead :P Why act like you will accept a nullptr while it would crash and burn anyway :D

My variant does copy all the ChunkHandler entries again in the vector, but I doubt it makes any real difference memory-wise. It just makes other code slightly less awkward, imo :)

These are just suggestions, feel free to cherry-pick which-ever you like!

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jun 1, 2021

Fully a personal opinion, but I won't do the fancy ChunkHandlers-is-a-vector thing. From an academic point, it is really cool to fiddle with, but there is only 1 user of this. So it is a bit much, I think.

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>
@glx22
Copy link
Contributor Author

@glx22 glx22 commented Jun 1, 2021

Merged saveload3 and @TrueBrain's suggestions in the PR.

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jun 3, 2021

With that, this is ready for review?

@glx22 glx22 marked this pull request as ready for review Jun 3, 2021
@glx22
Copy link
Contributor Author

@glx22 glx22 commented Jun 3, 2021

It is, unless we switch to auto-register with deterministic order based on priority and id, as suggested by @rubidium42.

Copy link
Member

@TrueBrain TrueBrain left a comment

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 :)

@glx22 glx22 merged commit c27afdf into OpenTTD:master Jun 6, 2021
13 checks passed
@glx22 glx22 deleted the saveload branch Jun 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants