Page MenuHomeWildfire Games

Delete Savegame version
ClosedPublic

Authored by elexis on Dec 9 2017, 3:49 PM.

Details

Summary

The savegame version check is already redundant with the engine and mod version check.
If we would not use one common version tracking system (engine + mod version), then we would have to add these component versions all over the place.

Test Plan

Notice the meaninglessness and the beauty of removing that from the code.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Dec 9 2017, 3:49 PM
Vulcan added a subscriber: Vulcan.Dec 9 2017, 4:40 PM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Vulcan added a comment.Dec 9 2017, 4:41 PM
Executing section Default...
Executing section Source...
Executing section JS...
Imarok added a subscriber: Imarok.Dec 10 2017, 1:41 AM

The savegame version is the version of the savegame format, so it's not identical to the engine or mod version.

In D1131#45460, @Imarok wrote:

The savegame version is the version of the savegame format, so it's not identical to the engine or mod version.

See the second sentence of the summary:

If we would not use one common version tracking system (engine + mod version), then we would have to add these component versions all over the place.

For instance we could add a replay format version, a simulation component version, a gamesetup version, a session verison, a selection panels version, a user config format version (that is bumped each time we add a hotkey).

Each time we do a change in the serialized part of the simulation, we change the savegame format but not bump the version number.

A file format (or network protocol) versioning system is only relevant if there can be more than one application. But pyrogenesis savegames are only loaded by pyrogenesis.

(It's common practice to add it to network protocols because one assumes that a protocol is used by different client implementations. Someone implementing a network client for 0AD that can work with many protocols of 0AD is highly unlikely too but still more likely than someone implementing a 0AD savegame loader application and keep compatibility with different savegame versions within the same release)

Like how likely is it that someone develops a new savegame version that can be used with an old 0ad version, or updates 0ad support of savegames of old versions?

Agreeing with the approach.

Yesterday 2017-12-28 on irc:

22:51 < elexis> Philip: we were wondering if we could remove the savegame version or if that had any use case we didn't discover
22:52 < elexis> it was added in https://code.wildfiregames.com/rP10454
22:53 < elexis> (it still saves the engine version (release number) and mods)
22:54 < elexis> (SAVED_GAME_VERSION_MAJOR in SavedGame.cpp)
22:58 < Philip> I think version numbers are always useful
22:58 < Philip> or, rather, lack of version numbers often becomes a huge pain in the future
22:58 < Philip> and including them has very little cost, so they should almost always be included
23:00 < elexis> but how is the savegame different from any other file format, maps, simulation components, gui files
23:01 < elexis> if we only consider releases, the savegame format is covered by the engine version
23:02 < elexis> two savegames of different svn revisions are incompatible despite the same savegame version number
23:03 < elexis> can you mention an example where it would be a pain to not have it?
23:04 < elexis> *construct

23:07 < Philip> Oh, I didn't know someone had added engine_version
23:07 < Philip> Maybe that's sufficient (except for really old saves that didn't have engine_version, but that's unlikely to matter)
23:08 < elexis> mostly because the savegame version format never changed since
23:08 < elexis> and we needed an engine version for replays and such too

23:08 < Philip> Saved games are different from game/mod content files (maps, scripts, etc) because we expect users to upgrade the game and still have all their old saved games and have the game respond sensibly when they try to open them
23:09 < Philip> (either by saying it's incompatible, or ideally by making it work somehow)

23:09 < elexis> backwards compatibility is something we can't really achieve without adding tons of ugly code
23:09 < Philip> whereas the game engine should never be incompatible with the game content files, unless the installer has really messed up
23:10 < elexis> yes
23:12 < elexis> thanks for your feedback!

23:13 < Philip> I think replays and saved games have similar compatibility requirements (since people will save them for a long time, share them, etc)
23:16 < elexis> the engine version was added in https://code.wildfiregames.com/rP16906 in preparation for the replay compatibility filter

wraitii accepted this revision.Dec 30 2017, 8:01 AM
wraitii added a subscriber: wraitii.

It seems to me we can perfectly handle incompatibilities by knowing that we broke save games in some given version of the engine, so I'd say this fits requirements.

I assume this doesn't break loading existing saved games.

This revision is now accepted and ready to land.Dec 30 2017, 8:01 AM

Thanks for your guys feedback.

source/ps/SavedGame.cpp
39 ↗(On Diff #4668)

I leave this TODO in here for now.

I guess it wants to tell us that the engine version check should be in C++ and the JS GUI should only display an error message based on that function, rather than having an own logic to check for the engine version in JS.

elexis added inline comments.Dec 30 2017, 3:54 PM
binaries/data/mods/public/gui/common/functions_utility_loadsave.js
8 ↗(On Diff #4668)

and this one goes too before the commit (testing ftw).

In fact this function now looks equal to isReplayCompatible and they should likely be merged in some mods directory.
Leaving random @Imarok ping.

This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Dec 30 2017, 3:54 PM