News:

SimuTranslator
Make Simutrans speak your language.

Cleanup of savegame version checks

Started by ceeac, May 13, 2019, 09:30:46 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

ceeac

This patch consists of three parts:

  • 01-version-check.patch: Cleans up checking of savegame versions by introducing three functions: loadsave_t::is_version_atleast, loadsave_t::is_version_less and loadsave_t::is_version_equal. The first function can be used when introducing new data to the save game:
if (file->is_version_atleast(120, 8)) { /* load/save new data */ }
The second function can be used for compatibility with old saves:

if (file->is_version_less(120, 8)) { /* code to load/save old saves */ }

    This is IMO more readable and less error-prone than comparing the output of file->get_version() to a raw integer.
    [/li]
  • 02-cleanup-int-version.patch: Cleans up loadsave_t::int_version
  • 03-fix-invalid-version-check.patch: Fixes a bug that caused invalid saves to be potentially recognized as valid saves

ACarlotti

Quote from: ceeac on May 13, 2019, 09:30:46 AM01-version-check.patch: Cleans up checking of savegame versions by introducing three functions: loadsave_t::is_version_atleast, loadsave_t::is_version_less and loadsave_t::is_version_equal.
I have been considering introducing something similar to Extended (where I was thinking of names 'is_extended_version_before' and 'is_extended_version_after' to correspond to your first two functions). In Extended the current situation is more complicated than Standard because we're having to check for two different version numbers in combination (requiring three numerical comparisions) rather than encoding this in a single integer.

prissi

The old binary mode was different from the zipped mode. But such old games will fail anyway.

Why the test for "if (pak_extension_str) {"?

Also, why renaming "get_version" to "get_version_raw"? There is no gain I see, but making more conflict on patches.

ceeac

Quote from: prissi on May 15, 2019, 12:14:14 AMWhy the test for "if (pak_extension_str) {"?
It may be NULL, see e.g. simmain.cc line 631. It is more of a precaution since currently for all calls to int_version where pak_extension_str is NULL, version_text contains a valid version string.

Quote from: prissi on May 15, 2019, 12:14:14 AMAlso, why renaming "get_version" to "get_version_raw"?
My intention was to make it clear that the function returns a raw integer as a version number and not a (hypothetical) version_t structure. But I can change it back if that's desired.

prissi

One could of course define version as an enum and generate warnings when compared to intergers.

But for your puspose, how about get_version_int? This makes much clearer that an integer is returned.

ceeac

#5
That is fine by me. I updated the patch files accordingly.

EDIT: Last patch file was broken, should be fixed now.

prissi