Support reading saves from older game version

[ivan-mogilko]

Overview

This is a rough draft made as a proof of concept. The purpose of this feature is to let engine load saves from older versions of same game into the newer version, and correctly apply only data that existed in the old version.

It's important to underline that this current feature is intentionally done as a minimal possible solution for the problem, which would also be suitable for the backwards compatible branch (ags3). It is meant to work only if user's changes to the game satisfy certain requirements (see below). It's not meant as a "final" or "perfect" solution: that remains a matter for another time (in ags4, I suppose).

Saves are a huge problem, so here's only for a quick reference: historically AGS saves do not identify objects by a unique id, but rather by their index in certain array. There are multiple reasons why is it so, including lack of planning, perhaps, but also technical ones. For instance, AGS editor does not enforce unique ids (script names) at all, and it's possible to have unnamed objects. Another, more complicated issue is that script variables are not distinguished by their names after script is compiled, for the outside viewer they are presented just as a single "array" of bytes. There are other less obvious things, but I won't go deep into this now.

What this current feature is intended for:

1. Let game load a save made in a previous version if:
   • The older version had LESS data than the new one, in any type.
   • The ORDER of old data did not change.
2. Let game developer react for the restoration of "mismatching" save, by writing a validation / data fixup in script.

In other words, this functionality is primarily meant for patching released games. It's less suitable during early stage of development, when the things change alot en mass.

---

Solution

How this goal is achieved here.

1. Engine reads the save, and compares the entries in a save with the game data, for consistency.
2. If an inconsistency is found (e.g. number of entries or size of data is different), 2.1. If save has less data then the engine, then a) engine marks and remembers this fact; b) engine aborts reading save, and reloads the game in order to clear all its data to the initial state. c) after reloading the game, engine restarts reading same save once again, but keeps in memory the fact that it is being read after clean reset. d) if there's a marker of a clean reset, then the above restart does not happen, and reading continues. 2.2 If save has more data, then currently this is not permitted, and engine will bail with error.
3. Save is read, and game data reinitialized as necessary.
4. Engine tries to run a script callback, named "validate_restored_save". 4.1. If such callback does not exist, and there's no indication of save being inconsistent, then the game is simply continued as normal. 4.2. If such callback does not exist, and there's a recorded indication about save being inconsistent, then the game is aborted with error message. 4.3. If such callback exists in script, then it is run.
5. The script callback receives a struct called "RestoredSaveInfo", containing information about the restored save:
   • a Result variable telling if save is inconsistent, and how (using a set of flags);
   • a group of variables that tell counts of entries per object type: how many characters, gui, etc was read from this save.
   • a "Cancel" field that user may write to in order to change the default engine's decision.
   • a "RetryWithoutComponents" field that user may write to in order to ignore certain save components (see SaveComponentSelection) This information, as well as a read script data, may be used by game developer to decide what to do with this save. Developer's decision may be passed to the engine by changing the "Cancel" field before returning from the callback. Besides letting the engine to quit with error, user may optionally keep the save, and handle this situation in their own way after the game scripts start running again (e.g. display the error in-game, and suggest to load another save, or fallback to a reserved "restart" slot).

From the scripting perspective, there's a new callback and a new struct:

enum RestoredSaveResult
{
  eRestoredSave_ClearData   = 0x01,
  eRestoredSave_MissingData = 0x08,
  eRestoredSave_ExtraData   = 0x10
};

managed struct RestoredSaveInfo
{
  import attribute bool Cancel;
  import attribute SaveComponentSelection RetryWithoutComponents;
  import readonly attribute RestoredSaveResult Result;
  import readonly attribute String EngineVersion;
  import readonly attribute int AudioClipTypeCount;
  import readonly attribute int CharacterCount;
  import readonly attribute int DialogCount;
  import readonly attribute int GUICount;
  import readonly attribute int GUIControlCount[];
  import readonly attribute int InventoryItemCount;
  import readonly attribute int CursorCount;
  import readonly attribute int ViewCount;
  import readonly attribute int ViewLoopCount[];
  import readonly attribute int ViewFrameCount[];
  import readonly attribute int GlobalScriptDataSize;
  import readonly attribute int ScriptModuleCount;
  import readonly attribute int ScriptModuleDataSize[];
  import readonly attribute int RoomScriptDataSize;
};

Note that I made this a struct with plain fields, rather than properties, thinking that maybe it's going to be simpler. Unfortunately I forgot that ags3 cannot have dynamic arrays in a managed struct, so I had to still add functions that return dynamic arrays with gui control count (per gui), view loop and frames count (per view), etc. This struct may be reviewed and changed.

The callback is optional and is defined as:

function validate_restored_save(RestoredSaveInfo* saveInfo)
{
  // check saveInfo and decide what to do
}

Here user may script a "compatible" or "incompatible" save detection, and set the saveInfo.Cancel field.

The Cancel field will default to false if no mismatches were found, and to true if there are mismatches. This is done in case this callback is not present in script: in that case the engine will always fail.

I shall repeat that a user does not have to "Cancel" the save with default error message. It's possible to handle this in a custom way, let save to restore, and then display error in game in a more pretty way.

Specifics

The GUI controls appeared to be an interesting problem. In AGS data they are saved not as nested entries of their parent GUIs, a tree structure, but as linear array housing all controls of particular type.

For them to load correctly from the "old" save, I had to code an algorithm that would calculate the ranges of indexes occupied by controls belonging to each GUI in the old and new version of a game, and then apply the "old" controls into correct slots of the new arrays.

TODO

[AlanDrake]

I wonder if there should be something to expose DoOnceOnly's data.

[ivan-mogilko]

I wonder if there should be something to expose DoOnceOnly's data.

Please elaborate what do you imply here, expose how, and for which purpose?

If you are referring to script, in the past I've been wondering if DoOnceOnly may be replaced by a Set object, but never actually did that, because Set's api is too different, and I thought that users may be confused by such change.

In 3.6.1 I added ResetDoOnceOnly by user's request. This function completely resets the entries. It does not have an argument, but I thought that if a need arises, a "name" arg could be added, in which case this function will work in two ways: if a name is valid then a single entry removed, and if a name is a null ptr (default value) then it will reset all.

For more control over its data we'd need something that only tests the entry, without adding. But then, there will be no way to iterate over entries, you will only be able to check for exact ones, if you know what they are.

The Set allows to do all of this and more. Now when I think about this, I could have actually exposed a "DoOnceOnly"'s Set object as a separate property, without removing existing command. This would've let users use DoOnceOnly command as before, but also have precise control over the data in uncommon situations. EDIT: although if done so, there's an issue of changing how this data saves, because DoOnceOnly tokens are saved on their own, and Sets are dynamic objects that are saved as a part of a managed pool data.

[AlanDrake]

Please elaborate what do you imply here, expose how, and for which purpose?

I was trying to think if there may be a valid reason to inspect DoOnces, but probably not. Even if a user renamed/added a do once, he could simply trigger it when he detects he's upgrading an older save. If it was removed, then it's likely a non-problem.

I guess it's not necessary.

[ivan-mogilko]

As I stated in the ticket description, it's not quite possible to support auto loading old saves when the order of items changed, because unique ids are not enforced in AGS (and some objects dont have any).

While thinking about what's possible in game patching, I came to a suggestion that there are 2 things that may be required in a patch, but may change order of things:

• script modules
• plugins.

sometimes a dev might want to add a extra feature to the game, and this feature is implemented by a script module or a plugin. Script Modules must be ordered in certain way in game because of their dependencies, and the order of plugins seems undefined. Also, dev may want to remove or replace a plugin. Unlike other things in game, plugins is not something you like to keep when it's unused, as it's an extra dll coming along with the distributive.

Therefore, it would be nice to be able to make these two things independent of the order in saves. In order to achieve that we'd need to ensure they are identified by a unique name. Plugins have names. Script modules also have name, but these are not saved explicitly into the game data. There's a chance that script's name may be learned by reading its "section" array, that usually holds original script's filename as the 0th element; at least that worked when building RTTI.

[ivan-mogilko]

Okay, somehow this slipped out of my mind, but thankfully, the plugins are already saving their data identified by their names since 3.6.1 (e84746a5049b2855cd0774ba0ff641753f640c27, done as a part of #2276).

It's a shame that this was not done from the start (of the new save format), but at least if the game saves are from 3.6.1 or later, the game may have added or removed plugins without a problem.

What's left is script modules then.

[ivan-mogilko]

Implemented a technique for reading saves with less script modules, this at the same time supports modules in different order.

This is achieved by writing their "names" and when reading back the script data is loaded into the module of matching name. As a consequence though, if you rename a script - that will be interpreted as having a new script module, and previously saved data will get dropped on save restore.

EDIT: I now need to adjust how the loaded scripts are reported by RestoredSaveInfo struct. I suppose, it needs to tell user which of the existing modules were loaded from saves, and how much data was loaded. Maybe it could just report 0 data restored for modules that were not found in a save, and that would be enough.

[NPatch]

Hi, Sorry in case this is not the most appropriate place to report this, but I was messing around with reading saves from a script and found that ReadStringBack was reading one byte less than it should.

  auto buf = ScriptString::CreateBuffer(data_sz - 1); // stored len + 1
  in->Read(buf.Get(), data_sz);
  return CreateNewScriptString(std::move(buf));

Looking at the save through ImHex, the string size stored before the string itself is the length of the string without the null termination. Also the strings themselves in the hex do not show any null termination character being stored. So either storing is not len+1 as it should, or you don't need data_sz - 1.

[ivan-mogilko]

@NPatch File.ReadStringBack() script command is purposed to read back a string written by File.WriteString(). If you check out File.WriteString implementation, you will notice that it writes a string with null terminator, and the prepended "length" includes null terminator count too: https://github.com/adventuregamestudio/ags/blob/6d2a866dfe2afc4e030a5c6e3ebfedd1d6e288f8/Engine/ac/file.cpp#L149-L151 https://github.com/adventuregamestudio/ags/blob/6d2a866dfe2afc4e030a5c6e3ebfedd1d6e288f8/Engine/ac/global_file.cpp#L64-L69

These are old functions, and their behavior is kept for backwards compatibility.

Saves, and game data in general, is not written or read using these script functions, these script functions not necessarily match common serialization format used there.

[NPatch]

So this API is only meant for custom serialization?

[ivan-mogilko]

So this API is only meant for custom serialization?

This particular function, and similar ones - yes it was only meant for custom files. There are few warnings about this in the related sections of the manual.

There are also few File script functions that allow to read "raw" data. But it's quite possible that this API is missing convenient functions for reading certain things. In which case it's a matter of adding more of them.

[NPatch]

I was trying to read back a save file's header to check if it would create a crash when loaded in a newer version, but I did manage to read the full header. Got up to the components.

Mostly ReadRawInt for numbers and for strings I rewrote ReadStringBack to account for that missing termination char and it was sufficient. Not sure if components have anything different.

Anyway, I took enough of your time for sth irrelevant it seems. Thanks for the responses though.

[ivan-mogilko]

Mostly ReadRawInt for numbers and for strings I rewrote ReadStringBack to account for that missing termination char and it was sufficient.

Note that you could do this in script, combining ReadRawInt for reading string's length, and then ReadRawChar in loop. That may be somewhat slower in case of long strings, but then you won't have to edit engine's code.

Other than that, what is your use case? Is it checking for save format change, or game contents change? I'm currently gathering potential issues with saves, trying to figure out a minimal possible builtin solution.

[NPatch]

Mostly ReadRawInt for numbers and for strings I rewrote ReadStringBack to account for that missing termination char and it was sufficient.

Note that you could do this in script, combining ReadRawInt and then ReadRawChar in loop. That may be somewhat slower in case of long strings, but then you won't have to edit engine's code.

Other than that, what is your use case? Is it checking for save format change, or game contents change? I'm currently gathering potential issues with saves, trying to figure out a minimal possible builtin solution.

Yeah, that's what I did (although used for, since the length was fixed and no null term char could be used).

The use case, although just for kicks, not contracted or anything, is to check if there was an easy way to figure out how to filter older saves from appearing in a build's save game dialog on launch. e.g. consider people playing a demo, where the project remains the same (so the Game's GUID does too) and then you also have betas during development. When releasing a game, it will have to somehow filter those without getting to the point of a user trying to load a savegame that was left there because they all show regardless of demo or beta. Getting a message after the fact seems a bit too late. So I was thinking of filtering them (e.g. renaming/changing extension or sth non invasive) on gamestart by going trough all 50 possible slots and using the File API to read the headers in case they had enough info to do that. I also considered just checking components serialized size since the demo would have considerably less content to serialize.

[NPatch]

I do think that the engine already does most of this though(when it crashes there's always a reason and e.g. differing counts in case that's what differs) so it shouldn't be difficult to expose a validate_save that only tells you if it's valid or not with this build. But since there wasn't one, I was messing about in case it was easy to do by script and hoped there would be sth I could use in the header. Savegame API and anything involved are not exposed, so I just replicated the cpp for the header reading part and it should be doable for the components, but also too much work for sth much much easier to deal with engine side.

[ivan-mogilko]

Supporting a quick-scan of a savegame is definitely something that I am considering, besides what is already done here in this PR. Coincidentally, a user request elsewhere reminded me of this lately.

Loading whole game only to find a mismatch in the last component may take unnecessary long time if you do this while testing 50+ saves. And not just that, but currently engine has to unload parts of runtime data prior to loading a save, which means that even if loading failure was caught, then the only fallback is to restart the game (into the title screen).

So it might have perfect sense to have a script command that "tests a save". It's not clear what such command would do exactly, this is something to plan first. Either it could quickly read only essential parts of save, which contain object counts, skipping rest. Or support a "info component" with these values conveniently stored there. Or support "user component" where game developer could write their own custom data. Or all of these things combined.

[NPatch]

Afaik, custom save systems are already possible.

As for the runtime state reset, I agree. This functionality shouldn't actually try restoring game state. Only parsing parts of the save to determine whether current build breaks compatibility. e.g. counts would be the major check and mostly seek in-between to jump to the next relevant count. I'm not familiar with the details of the components but this would be a very good first step. Just reducing the use cases that can crash the engine which already forces a restart from 100 to 20 is huge.

That said, it would be very convenient if the save header contained a section with some kind of hash that can be compared to. Created when saving the game and another function from engine side that can give the current build's equivalent hash, so you can bypass most of this on runtime, but this would require a new format. Unless conveniently added in the last part of a save and provided we know how to jump the whole components section (not sure if currently the save serializes the components total size before the components section).

PS: I don't see the Game Version from General Settings stored anywhere in the save file. That could also be a very easy indicator. You'd need to have at least the same Major and perhaps minor version to even consider doing anything else to validate. Same as the Release Date. Iirc, the save header provides the UserText which contains a date, which unless I'm wrong, is the file creation date of the save. That could also be used as an early exit. And both version and release date could just be an early exit in the validate function for scripting. Not necessarily exposed in General Settings, but it would give some measure of control for someone filtering things to avoid crashes.

[ivan-mogilko]

@NPatch in regards to ReadStringBack, I revisited it, and I realized that it is possible to adjust it to work both with the strings that were saved with and without null terminator, by asking "data_sz" for the buffer, and then testing last - 1 byte after it's read. ...In fact, it might just do strlen there, in case there are sudden zeroes in the middle. I suppose extra safety check is a proper thing to do for deserialized data.

About everything else, I must take time and consider everything carefully.

[NPatch]

No problem. Take your time. I might look into it more out of curiosity mostly. I'll ping back if I figure out anything noteworthy, otherwise I'll just wait for your thoughts.

[ivan-mogilko]

Had to rebase after #2543

Added an extra option to RestoredSaveInfo, which was planned prior to #2543 , but I decided to do a #2543 feature separately first. This option is called "RetryWithoutComponents", and it lets user to instruct the engine to restore this save once again, but ignoring certain components. This is purposed for the case when some of the differences are too complicated to apply to a game, so it's easier to skip these parts completely and then adjust everything in script.

One of the real practical cases when this may be used is when reading much older saves, which did not have GUI control to parent links (I added these only recently in 3.6.2 branch).

I will cleanup and adjust this PR further, but I think that it's almost ready for the real use.

There's still a feature request of pre-scanning a save (or a list of saves), as noted above by NPatch; I consider to address that afterwards.

[ivan-mogilko]

Marked this ready for review. Frankly speaking, i hoped to get at least few of AGS users to test this, but to no avail. Tested this myself, and this works in principle. I do not want to spend more time waiting, because this seems counter-productive at this point. These changes are a basis, but other things may be derived from this later (like prescanning a save without overwriting game data, etc).

To reiterate, this feature's idea is following:

1. Engine principally supports reading saves with less data of each kind (script data, object numbers).
2. Prior to reading a save, it looks up for the presence of script function named "validate_restored_save". If such function is present in any script module (except room script), then it toggles "allow to read save with less data" mode.
3. If less data is detected during save restoration, following is done:
   • Game data is fully reinitialized, reloaded from game file. This has to be done in order to bring everything to the default state, because otherwise parts of the game that were not read from save will keep their previous values. (atm this is done by making an internal call to RunAGSGame, but I suppose a separate function may be coded specifically for this purpose later, if this method turns to cause trouble)
   • Save is restored once again, now in "clean data" mode, and reads everything up to the end.
   • After restoration is complete, engine triggers a "validate_restored_save" callback.
4. In "validate_restored_save" user may do any checks necessary, and instruct engine to either proceed or cancel restoration process. Latter will unfortunately stop the game (as it's data is already overwritten). On another hand, user may code a fallback action for a bad save, where it will not cancel immediately, but proceed up to some event (such as eEventRestoreGame or other), display a message and restore another save, or restart the game. This is an experimental area, worth investigating to see what can be done there. After restoring an incompatible save, game may be stable (as in - not crashing), but some things look unexpected.
5. Note that user MUST add "validate_restored_save" and set RestoredSaveInfo.Cancel to false explicitly in there in order to let load saves with less data. This works as a proof that user at least understands what will happen in such case (i dont want to do anything secretly there).

[ericoporto]

The AGS Editor when it loads it shows a list of recently edited games. This list is aware of AGS Editor versions - if a recently edited game is in a different AGS Editor version, it doesn't show up.

I think the pre-scanning request enables achieving something similar, you can see which of the saves use which version of the game, and then you can filter it by such in your save interface - thus preventing a runtime crash.

What this PR does is after the fact, you have loaded the game and some stuff loaded alright and some didn't and you give the gamedev some interface meant for data migration - doing a parallel with DB versioning. So it feels like some way to deal with an exception.

I can't figure what does ScriptModuleDataSize and RoomScriptDataSize, what they do and how such thing can be used.

But anyway, in case of a GUI which AGS orders by ID, I guess if you delete one you may need to do some migration - since the GUI order would change. But realistically, you kinda just add GUIs, so what would happen is now you have some GUIs that didn't load new data, and maybe what your migration process would be in such case is figuring where you are in the adventure game progress and adjust the GUIs to match - in case your script doesn't adjust the GUI just before making it visible.

[ivan-mogilko]

I think the pre-scanning request enables achieving something similar, you can see which of the saves use which version of the game, and then you can filter it by such in your save interface - thus preventing a runtime crash.

It might, although I did not add prescanning right in this PR, but would such prescan be implemented it may end up using same "validation" script callback that I have here.

Indeed, this PR only adds a solution for "fixing" the game after a save was already loaded.

I can't figure what does ScriptModuleDataSize and RoomScriptDataSize, what they do and how such thing can be used.

These are total sizes of the script data in modules (meaning - variables). In theory, they may be checked if user knows which script variables were added since. In practice, this has same limitation as everything else (item counts), where user has to keep track of what they add if they want to be able to do these checks, and also only adding is reliable, removing and changing order is not.

As almost everything in AGS is ordered by ID (not just GUI that you mention). This solution here does not support changing order of these things, only adding new ones. In order to support change of order, the GUI and other items must be identified by a obligatory unique id (such as script name or guid). (and i am not certain of what to do with script variables in such case)

you kinda just add GUIs, so what would happen is now you have some GUIs that didn't load new data, and maybe what your migration process would be in such case is figuring where you are in the adventure game progress and adjust the GUIs to match

That's right, either that, or you ignore restored data completely and reconfigure all GUI from scratch after game is restored.

[NPatch]

This feature makes more sense if you do it from minor version to another, or from one major version to its previous one, not restoring super old ones where you have to do incredible changes to make them work. But there should be a way to patch things, even with removals.

Adding guids would make the most sense honestly. As for variables, I don't know how you store and reload scripts and their values so I can't offer a suggestion currently. I was thinking if order is an issue and additions can work, you can invalidate a single entry and add a new one which gets a new index as a copy of the invalidated one, but there needs to be a way to patch references to the new index that way. Not sure if possible.

4. In "validate_restored_save" user may do any checks necessary, and instruct engine to either proceed or cancel restoration process. Latter will unfortunately stop the game (as it's data is already overwritten). 

As I understand it, Restart Game works by making a save of the runtime as it enters the intro and stores it in .999. Can you do the same and store the runtime up to the point when the user tried to restore a game that is eventually cancelled in validate_restored_save and then either auto-revert the saved backup or allow the user to do what they want? From the user's perspective, doing anything other than failing and leaving them back where they were, in some loading menu, would be disruptive.

[ivan-mogilko]

This feature makes more sense if you do it from minor version to another, or from one major version to its previous one, not restoring super old ones where you have to do incredible changes to make them work. But there should be a way to patch things, even with removals.

Yes that's true, but adding unique ids for everything is a big change, as that might affect everything: source project, compiled game data, and save format (and likely require rewriting restoration process quite a bit), and I did not plan it for this moment. I noted that in this PR I'm doing a minimal possible solution for patching released games (and that's the most common situation that everyone may get in), which may be applied on 3.6. branch without any breaking changes, and letting load saves made by older versions of the engine*.

Any more serious change would have to be carefully designed first and applied to ags4 branch, which is meant for major rewrites and breaking changes.

As I understand it, Restart Game works by making a save of the runtime as it enters the intro and stores it in .999. Can you do the same and store the runtime up to the point when the user tried to restore a game that is eventually cancelled in validate_restored_save and then either auto-revert the saved backup or allow the user to do what they want?

That is indeed possible to save the game to a temporary slot before trying to load another. But I'd rather leave this for a user to script this if they like, not run automatically by the engine. OTOH I am still looking into the "prescanning" idea, and imo that might work better in such situation.

[NPatch]

I didn't imply you should add GUIDs in a single PR. This is sth that should be considered for 4.X. Just stating that it's worth doing if you're ever to add proper patching, without too much hassle. I get that it's done this way to have everything compact and simple but it requires a lot of gymnastics to patch properly.

As for the other topic, you can take a backup as a matter of course and allow the user to default to it. Iirc, scripting wise you get a load event only after it's been executed. Not sure if you can do it by creating a custom Load Menu gui, but it seems a bit unnecessary if you can create a backup yourself and ask them for what they want to do. Also it doesn't have to be for all cases, but at least for those where the runtime editor version doesn't match the save's, which is metadata you can read, without loading the save. Or it can be an opt-in situation. If they decide to cancel, you can still allow them to use that save, or do sth they want. At any rate, depending on the answer, you can either load it or scrap it. You still allow for dev agency on whether to use it.

At any rate, I'm all for the pre-scanning idea. It can save you from so many troubles. Much simpler to filter out impossible saves or the trouble in general if you don't care to patch old saves and doesn't impact the user at all since they can't see the saves or can get a meaninful message that they are incompatible with the new version (much like the tumbleweed game template which as i understand it uses its own save system, hence the ability to detect without hitch)

[ericoporto]

That is indeed possible to save the game to a temporary slot before trying to load another. But I'd rather leave this for a user to script this if they like, not run automatically by the engine.

Agree with this approach, specially because writing can be slow in some cases and this would add unnecessary hangs when you expect you are reading, if you are dealing with a device that has slow IO (like a console or games in a memory card)

[ivan-mogilko]

I went ahead and merged this.

This does not modify game data or save format in any way, the restoration process is only affected IF there's a script callback in user's script (engine checks its presence before starting to restore a save), so any pre-existing games, or game that do not use this feature explicitly should not be affected.

I will try to make an experimental proposal for save prescanning in the next several days.