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: add and use the ability to store (lists of) structs in savegames #9339

Merged
merged 9 commits into from Jun 14, 2021

Conversation

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jun 7, 2021

Motivation / Problem

In a few places (stations, vehicles, towns, companies, ..) the SaveLoad code needs to handle either structs in structs, or lists of structs in structs.

This is all done manually, and not only hard to read, also easy to mess up.

Description

Introduce SL_STRUCT and SL_STRUCTLIST to embed a struct in another struct.

For example, for Vehicles it used to have a special SL command to embed the common vehicle structure. This meant that SaveLoad needed to know about Vehicles.

Now, the common vehicle sub-struct is called SlVehicleCommon and to save it you can use:

SLEG_STRUCT(SlVehicleCommon)

This works with structs in structs in structs in structs in structs in ... just fine too.

NOTE: I made several commits to introduce SL_STRUCT in the rest of the code, as I am hoping that makes reviewing easier.
NOTE 2: I left a lot of odd constructs in there, just to make review easier. The code really could use a cleanup, but .. I am trying not to do everything at once, also for my own sanity :D
NOTE 3: there should be no difference before or after this PR in respect to what is written/loaded from disk.

Limitations

  • I tested this with ~500 savegames, ranging from TTDp till 1.11.2, validating that they load and run. Nothing that wasn't broken before this PR is broken now. So I think there are no regressions, but with this many changes in the SaveLoad code, that cannot be guaranteed.

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')
@TrueBrain TrueBrain marked this pull request as draft Jun 7, 2021
src/saveload/cheat_sl.cpp Outdated Show resolved Hide resolved
Loading
src/saveload/saveload.h Show resolved Hide resolved
Loading
src/saveload/town_sl.cpp Outdated Show resolved Hide resolved
Loading
src/saveload/saveload.h Outdated Show resolved Hide resolved
Loading
@TrueBrain TrueBrain force-pushed the saveload-sub-struct branch 11 times, most recently from 2a20061 to 2178bdf Jun 11, 2021
@TrueBrain TrueBrain force-pushed the saveload-sub-struct branch from 2178bdf to c4be800 Jun 14, 2021
@TrueBrain TrueBrain marked this pull request as ready for review Jun 14, 2021
@TrueBrain TrueBrain force-pushed the saveload-sub-struct branch from c4be800 to 02146c8 Jun 14, 2021
@TrueBrain TrueBrain force-pushed the saveload-sub-struct branch from 02146c8 to bb42b19 Jun 14, 2021
src/saveload/vehicle_sl.cpp Show resolved Hide resolved
Loading
src/saveload/saveload.h Outdated Show resolved Hide resolved
Loading
src/saveload/saveload.h Outdated Show resolved Hide resolved
Loading
The commits following this will use this new functionality.

Currently, a few places do this manually. This has as drawback that
the Save() and Load() code need to be in sync, and that any change
can result in (old) savegames no longer loading. In general, it is
annoying code to maintain.

By putting everything in a description table, and use that for
both Save() and Load(), it becomes easier to see what is going on,
and hopefully less likely for people to make mistakes.
@TrueBrain TrueBrain force-pushed the saveload-sub-struct branch from bb42b19 to 4229a0d Jun 14, 2021
src/saveload/station_sl.cpp Outdated Show resolved Hide resolved
Loading
src/saveload/station_sl.cpp Show resolved Hide resolved
Loading
TrueBrain added 7 commits Jun 14, 2021
With the new SLEG_STRUCT it is much easier to embed a struct
in a struct, where the sub-struct has limitations on when it is
being used.
This makes both the code easier to read (less magic) and avoids
the SaveLoad needing to know all these things about Stations
and Vehicles.
There was a lot of code duplication for no real reason. Now with
SLEG_STRUCT support, we can just re-use the code, hopefully making
it easier for future-us to make changes to this, without breaking
everything for old games.
@TrueBrain TrueBrain force-pushed the saveload-sub-struct branch from 4229a0d to f08773e Jun 14, 2021
@TrueBrain TrueBrain merged commit 7b135a8 into OpenTTD:master Jun 14, 2021
15 checks passed
Loading
@TrueBrain TrueBrain deleted the saveload-sub-struct branch Jun 14, 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