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

Change: make lists in savegames consistent #9374

Merged
merged 6 commits into from Jun 15, 2021

Conversation

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jun 14, 2021

Motivation / Problem

We have a bunch of different lists, which are all done slightly different.

  • We have a "fixed-length" list, where the code dictates the length. No validation is done if that same value was also used when writing to disk, which means we have plenty of savegame bumps for cases where it did.
  • We have lists like SL_REFLIST that have a 32bit length indicator.
  • We have all kind of custom stuff with SL_STRUCTLIST, where sometimes the length is stored in the parent blob, saved/loaded to a global. But sometimes totally different solutions are used.

This makes self-described savegames really hard (the ultimate goal of this set of PRs, see #9322), but also means savegame-readers need to be really aware of what they are reading, and support a lot of different savegame-versions to do the right thing.

Description

So .. as you might expect, I set out to fix that problem. Now the rules are simple:

  • Any list, no matter who/what you are, has a gamma before the list to indicate how many entries are in the list.

In most cases this means you know the length. For SL_STRUCT this is not the case, and as such you need to know what struct is stored to figure out the length. This will be addressed in a later PR.

As using SL_STRUCT is normally only useful if it is an optional field (why else bother making it in a new struct, and not just embed it in the parent?), internally it is a SL_STRUCTLIST of either 0 or 1 in length. So if it is empty (nothing saved), the length-field is 0. Otherwise, length-field is 1.

Limitations

  • A lot of testing is involved, see #9339 and #9322 for details. So no known issues or limitations.
  • Other solutions have been considered, instead of a length-field, like for example a "type", where the last entry is an empty one indicating "end". But given the current state of the code, that is vastly more work.

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')
src/saveload/company_sl.cpp Outdated Show resolved Hide resolved
Loading
src/saveload/station_sl.cpp Outdated Show resolved Hide resolved
Loading
src/saveload/town_sl.cpp Outdated Show resolved Hide resolved
Loading
src/saveload/saveload.cpp Show resolved Hide resolved
Loading
@TrueBrain TrueBrain force-pushed the saveload-array branch 3 times, most recently from a0adf99 to 1da944e Jun 15, 2021
src/saveload/station_sl.cpp Outdated Show resolved Hide resolved
Loading
src/saveload/town_sl.cpp Outdated Show resolved Hide resolved
Loading
src/saveload/company_sl.cpp Outdated Show resolved Hide resolved
Loading
TrueBrain added 6 commits Jun 15, 2021
This wasn't consistently done, and often variables were used that
were read by an earlier blob. By moving it next to the struct
itself, the code becomes a bit more self-contained and easier to
read.

Additionally, this allows for external tooling to know how many
structs to expect, instead of having to know where to find the
length-field or a hard-coded value that can change at any moment.
This helps external tooling to understand if a SL_STRUCT should
be skipped when reading. Basically, this transforms an SL_STRUCT
into a SL_STRUCTLIST with either 0 or 1 length.
The current SaveLoad is a bit inconsistent how long a length field
is. Sometimes it is a 32bit, sometimes a gamma. Make it consistent
across the board by making them all gammas.
…thing

Using SL_ARR for this gives us a bit of trouble later on, where we
add a length-field to SL_ARR. This of course is not the intention
of SLE_CONDNULL. So better seperate it.
In the end, the code was already doing the right thing, but a few
functions deep, and not really obvious. When validating what objects
can handle SLE_VAR_NULL, it is nicer to just have this obvious.
This means that during loading we can validate that what is saved
is also that what is expected. Additionally, this makes all list
types similar to how they are stored on disk:
First a gamma to indicate length, followed by the data.
The size still depends on the type.
@TrueBrain TrueBrain merged commit 97b94bd into OpenTTD:master Jun 15, 2021
15 checks passed
Loading
@TrueBrain TrueBrain deleted the saveload-array branch Jun 27, 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

2 participants