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.

Sign up for GitHub

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

Jump to bottom

Fix #8987: Display 'modified version' error when failing to load JGRPP savegames #8996

Closed
wants to merge 0 commits into from
Closed

Fix #8987: Display 'modified version' error when failing to load JGRPP savegames #8996

wants to merge 0 commits into from
+0 −0

Conversation

FLHerne
Copy link
Contributor

@FLHerne FLHerne commented Apr 10, 2021 •
edited
Loading

Motivation / Problem

#8987.

Limitations

As noted in the issue, this isn't an ideal way to represent patchpack versions and a different solution is needed long-term.

This will still be useful after such a solution is implemented, because the many existing JGRPP saves won't go away.

EDIT: maybe using JGR's extended versioning is a long-term solution, if documented properly and supported upstream.

I wasn't sure whether to add this flag to the enum or just as a separate constant.

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 reviewed Apr 10, 2021
View reviewed changes
src/saveload/saveload.h Outdated
SL_MAX_VERSION, ///< Highest possible saveload version
SL_MAX_VERSION, ///< Highest possible saveload version [for upstream versions until this one]

SLV_JGR_EXT_BASE = (1 << 15) ///< 32768 JGRPP sets this bit to distinguish savegames using its extended versioning system.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is wrong to mention JGRPP here, as by what you pointed out yourself, this is the advise for all patchpack creators. Additionally, it is a bit-check, so maybe we should name it as such.

SL_PATCHPACK_FLAG = 1 << 15 ///< Patchpacks should set this bit to distinguish savegames from vanilla

or something?

Sorry, something went wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should recommend that all new patchpacks use JGR's extended versioning system specifically; not blindly set this as an "I'm a patchpack" flag.

I thought that was what you meant in the original comment, but apparently not...

Will rephrase the comments, and see if @JGRennison has any docs we can link to.

In principle, we could also add support for loading such savegames into upstream versions, if they use only extended features marked as discardable/ignorable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you are right. I mostly meant the "JGRPP sets this bit .." when we want all patchpacks to do this. "Patchpacks set this bit.." makes more sense.

We still refer to the way JGRPP extended the savegame format, with feature toggles etc. I just never bothered to properly refer to that, so yeah, anything another patchpack author can pick up from would be greatly appreciated!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only documentation is here: https://github.com/JGRennison/OpenTTD-patches/blob/jgrpp/src/saveload/extended_ver_sl.cpp#L8-L36, which is admittedly a bit sparse.

@FLHerne FLHerne changed the title Fix #8987: Display 'saved with patchpack' error when failing to load JGRPP savegames Fix #8987: Display 'modified version' error when failing to load JGRPP savegames Apr 10, 2021
glx22
glx22 reviewed Apr 10, 2021
View reviewed changes
src/saveload/saveload.cpp Outdated
@@ -2647,9 +2647,12 @@ static SaveOrLoadResult DoLoad(LoadFilter *reader, bool load_check)

DEBUG(sl, 1, "Loading savegame version %d", _sl_version);

if (_sl_version >= SLV_JGR_EXT_BASE ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should use HasBit()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. It's a uint16 so they should be equivalent, but would be clearer.

@FLHerne
Copy link
Contributor Author

FLHerne commented Apr 10, 2021

IRC suggestions for permanence:

<frosch123> FLHerne: TrueBrain: i think you can read the gamelog (even for otherwise unknown savegame versions) to give a versionstring for the openttd that saved a savegame

<frosch123> if the message would say "savegame was created with XYZ and can not be loaded with this version of OpentTD" you can avoid all that newer/older trouble
<TrueBrain> frosch123: sounds like a very solid solution
<TrueBrain> "savegame was created with OpenTTD 1.11 and cannot be loaded .." if you load the game in 1.10 would help a lot of people

@JGRennison
Copy link
Contributor

IRC suggestions for permanence:

<frosch123> FLHerne: TrueBrain: i think you can read the gamelog (even for otherwise unknown savegame versions) to give a versionstring for the openttd that saved a savegame

<frosch123> if the message would say "savegame was created with XYZ and can not be loaded with this version of OpentTD" you can avoid all that newer/older trouble
<TrueBrain> frosch123: sounds like a very solid solution
<TrueBrain> "savegame was created with OpenTTD 1.11 and cannot be loaded .." if you load the game in 1.10 would help a lot of people

Unfortunately the snag with this is that future versions are entitled to use a different gamelog chunk format for example to add more fields. In general you can't reliably read the gamelog chunk of a future version. My branch already uses a slightly different gamelog chunk format.

@LC-Zorg
Copy link

LC-Zorg commented Apr 11, 2021

From the player's point of view: I have encountered this problem a lot. I downloaded some save and the mentioned message appeared. Newer version? But which? Usually it turned out to be JGR versions, but sometimes Springpatch too. Maybe that's wrong, but I think it would be good if each save had information about the version of the game saved. Then the message would be: "Savegame requires version XXXX or later". In case the save is incompatible and doesn't have version information: _"Savegame requires a different, older / patched version of the game" or something similar.

@FLHerne FLHerne closed this May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TrueBrain TrueBrain

@glx22 glx22

@JGRennison JGRennison

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@FLHerne @JGRennison @LC-Zorg @TrueBrain @glx22