Date: Mon, 9 Nov 2009 14:17:28 -0800
From: Stefan O'Rear <stefanor@cox.net>
To: crawl-ref-discuss@lists.sourceforge.net
Message-ID: <20091109221728.GA7668@localhost.localdomain>
Subject: [Crawl-ref-discuss] Savefile compatibility: how and why
Robert asked me to write something for the list on the subject of
savefile compatibility, so here goes.
1. WHY
Having a savefile compatibility procedure is crucial in the stable
branches, as it allows applying bug fix releases in mid-game. It's
also important in the development trees, although we currently give
it far less attention than it deserves. The reason for this is that
properly testing many features, such as the recent portal refactors
that happened to make leaving Zot impossible, require a long game.
If it is impossible to upgrade in the middle of a game, then all bugs
for late-game features will be reported against old versions, need-
lessly complicating the bug fixing process.
The situation is exacerbated by the public beta server (Marc Thoben's
crawl.develz.org), since it can only be updated site-wide; it cannot
be updated between games as there are almost always saved games on
the server, so it is simply updated (with loss of saves) rarely. And
rarely is bad.
2. HOW
[note added by galehar]
First, if you're adding a property to one of the actor, player or monster
class, consider storing it in the props hash (declared in actor.h). Especially
if the property is temporary or not applied to all monsters. Also, it won't
create any save compatibility issues.
If you are changing any of the code in tags.cc (which implements load
and save for savefiles), you probably need to be concerned about save
compatibility. Any code which uses the marshall* and unmarshall*
functions is, likewise, probably unsafe to simply change. Changing
the values of existing enums (for instance, adding a new option
before existing options) is not generally safe, as saves store the
numeric values of enum variables.
By way of example, suppose you wanted to add a new field to the player
structure, you.xyzzy. Now you want to save this field, and without
breaking saves. To do this, find the functions tag_read_you and
tag_construct_you in tags.cc. Unfortunately, you can't just add
marshall and unmarshall calls, since if you get an old save, the wrong
value will be unmarshalled, synchronization is lost and things
fall apart. So first, add a new option to the tag_minor_version enum
in tag-version.h. Make sure the new option is at the end, and that you
update TAG_MINOR_VERSION to correspond to the new option. Now, any
savefile created by the new code, or any later savefile, will have a
minor version >= your new version number. So, make the marshall and
unmarshall code conditional on minorVersion, like so:
if (minorVersion >= TAG_MINOR_JIYVA)
you.second_god_name = unmarshallString(th);
...
marshallString(th, you.second_god_name);
Is this clear? If you want to change the representation of an existing
field, or delete an old field, use a check in the other direction for
saves created by *older* versions of Crawl; for instance, take this
code, which reads in either a Subversion or a Git revision number,
depending on version:
if (minorVersion < TAG_MINOR_GITREV)
{
std::string rev_str = unmarshallString(th);
int rev_int = unmarshallLong(th);
UNUSED(rev_str);
UNUSED(rev_int);
}
if (minorVersion >= TAG_MINOR_GITREV)
{
std::string rev_str = unmarshallString(th);
UNUSED(rev_str);
}
If you want to change an enum without breaking savefile compatibility,
the cardinal rule is that the numeric values of all existing constants
must not change. If you are adding an option, add it at the end; if
you are removing a value, leave a placeholder of some kind. Be sure
to test your code well, as there are sometimes obscure requirements on
what needs to be done for a placeholder. Case in point: if a
MUT_UNUSED_n placeholder exists in the mutation_type enumeration, but
no mutation definition exists in mutation_defs, it will work fine until
somebody plays with a Vampire, as generating the list of fully active
mutations requires looking at mutation definitions for all mutations.
In extreme cases, it may not be possible to preserve load compatibility.
If this is the case, and please do this sparingly, you should increase
TAG_MAJOR_VERSION. This will ensure that old save files are correctly
rejected, instead of causing a crash.
Historically TAG_MAJOR_VERSION has always been equal to the submajor
version of Crawl itself; however, this buys us very little, as users
do not need to identify Crawl save versions by contents under any
normal circumstance, and has cost us dearly in time spent debugging
startup crashes. Therefore we should not do it; TAG_MAJOR_VERSION
should be incremented on all incompatible changes.
When TAG_MAJOR_VERSION is incremented, all existing TAG_MINOR_ checks
are no longer necessary and can be removed, along with deleting the
values of TAG_MINOR_ and restarting minor numbering from 1.
Thank you for listening, I hope this was useful.
Stefan
A few comments on Tiles
-----------------------
Whenever an old tile is removed without replacement, a new tile is added,
or the order of tiles changes, saved games can look wonky when loaded.
This happens because the tiles for floor and wall tiles get rolled once on
level creation and are subsequently stored in the level files, so they don't
change every time you reload the game. Any time a tile's number is changed,
the actually displayed tile gets shifted a bit which can lead to floor
looking like walls, staircases like shops and similar odd occurences.
Items and monsters that are cached out of sight may similarly be affected.
This is not a big problem and in the development version, we don't even
try to prevent this, but in the stable branches it should simply not happen.
This means that we should never add new tiles for a minor release, unless
said tiles replace the same amount of existing ones.
If we expect that we might have to add new tiles in a stable branch later
on, we should add placeholder tiles before the official release, that can
later be replaced without affecting saved games.
Johanna
A few comments on map caches and Lua markers
--------------------------------------------
Map and vault definitions are read from the relevant .des files and are stored
in a binary format to prevent slow-down every time crawl starts. If new
attributes or properties are added to vault definitions, save-compatibility
needs to be ensured in much the same way as for normal saves. Until recently,
the .des cache used a different version number to the main major/minor system.
In theory, all that needs to be done now is to bump the minor version, which
will cause the being-loaded .des cache file to be considered invalid, and thus
rebuilt.
When modifying a Lua marker, specifically when adding new options, etc, you'll
need to likewise ensure save compatibility. The minor version is currently
accessible by calling file.minor_version(reader). This will return a numeric
value that has to be manually compared to the relevant minor tag.
The current major version of the software is accessible by calling
file.major_version(). There's no need to pass the reader in, as if the major
version of a file mis-matches, the game won't attempt to load it, and you will
never get to a stage of being able to look for it.
All Lua markers will need to be checked for minor-version checks when updating
the major version.
due
Scheduling compatibility code for removal
-----------------------------------------
In order to increase long-term maintainability, it is customary to wrap
save compatibility code in #if blocks that check for the current value
of TAG_MAJOR_VERSION:
#if TAG_MAJOR_VERSION == 34
if (you.mutation[MUT_TELEPORT_CONTROL] == 1)
you.mutation[MUT_TELEPORT_CONTROL] = 0;
if (you.mutation[MUT_TRAMPLE_RESISTANCE] > 1)
you.mutation[MUT_TRAMPLE_RESISTANCE] = 1;
#endif
Then when the major version is incremented and old saves are no longer
supported, the compatibility code will not be compiled, and can be
easily identified and removed for readability.
For code that should run only for newer saves, the control structure
itself, as well as any alternative code to handle older saves, should be
protected by such #ifs; but the new code should be outside them.
#if TAG_MAJOR_VERSION == 34
if (th.getMinorVersion() >= TAG_MINOR_LORC_TEMPERATURE)
{
#endif
you.temperature = unmarshallFloat(th); // new save: keep this
you.temperature_last = unmarshallFloat(th);
#if TAG_MAJOR_VERSION == 34
}
else
{
you.temperature = 0.0;
you.temperature_last = 0.0;
}
#endif
Then when the major version is incremented, the code will preprocess to:
you.temperature = unmarshallFloat(th);
you.temperature_last = unmarshallFloat(th);
and the new behaviour will happen unconditionally.
Other targets for #if TAG_MAJOR_VERSION checks include enumerators that
are no longer used; and new enumerators that were added to the end of
their enum for compatibility reasons but make more sense somewhere else
in the list. In the latter case one would use a pair of #ifs, one for
the current version and one for future versions:
DNGN_ENTER_ABYSS,
DNGN_EXIT_ABYSS,
#if TAG_MAJOR_VERSION > 34 // new location
DNGN_ABYSSAL_STAIR,
#endif
DNGN_STONE_ARCH,
. . .
DNGN_UNKNOWN_PORTAL,
#if TAG_MAJOR_VERSION == 34 // old location
DNGN_ABYSSAL_STAIR,
DNGN_BADLY_SEALED_DOOR,
#endif
neil
Merges
------
Whenever you do a merge or rebase of a branch that introduces new monsters,
items, etc., it is very important to pay attention to enums. Unfortunately, git
will sometimes automatically resolve the merge in exactly the way you didn't
want, putting enumerators in their branch location rather than after the new
trunk enumerators, all without giving any kind of conflict warning. If such a
problem isn't caught before the servers are rebuilt, players will be left with
buggy saves and the problems can be very hard to fix, with nasty kludges to try
to guess the correct type of a monster. See for example the fixes in:
0.13-a0-2175-ga079a5c
0.14-a0-2325-gf687a29
To avoid situations like these, do not rely on conflict detection and
resolution to merge enums correctly! Manually review all enum changes to make
sure that nothing is inserted before an enumerator that already exists in
trunk. You can look at the overall changes, with intervening history squashed,
using a command like:
~$ git diff master..HEAD enum.h
neil
