Engine: register static objects missing from a restored save's pool

[craigharman]

Restoring a save replaces the managed object pool with the one stored in the save. That pool only contains the audio clips, characters, dialogs, GUIs and GUI controls that existed when the save was made. If the developer adds any of these in a later build, those objects are missing from the pool once an old save is restored.

Using them directly still works, e.g. aNewMusic.Play(). The first script that stores a pointer to one fails, because storing a pointer looks the address up in the pool:

Error: Pointer cast failure: the object being pointed to is not in the managed object pool

Passing one as a function argument counts as storing, so an ordinary helper is enough to trigger it:

function SwapMusic(AudioClip* clip) { ... }   // fails on entry
...
SwapMusic(aNewMusic);

Our players hit this after an update that appended five music clips. Saves from the previous build crashed as soon as a room passed one of the new clips to a helper. We saw the same thing with a GUI added in a later update, when it was passed through a GUI* parameter.

Fix

After ReadManagedPool restores the pool, register any audio clip, character, dialog, GUI or GUI control that the pool doesn't already contain. Objects that are already there keep their saved handles. Missing ones get new handles, which is safe because no saved data can refer to them. Characters are included because a game can accept a save with a different character count through validate_restored_save. Inventory items, audio channels, hotspots, objects and regions are left alone. The engine registers a fixed maximum number of each, so they're always in the pool.

This adds a small helper, ccRegisterManagedObjectIfMissing(), to dynobj_manager.

To reproduce

1. Make a game with one audio clip, and a function that takes an AudioClip* parameter.
2. Run the game and save.
3. Add a second audio clip and rebuild.
4. Restore the save and call the function with the new clip. Without this change, the call fails with the error above. With it, the call works.

[ivan-mogilko]

Interesting, this problem means that the support for restoring old saves in a game with more objects (added in 3.6.2) was not complete. Perhaps, not many people tried this system yet.

I see that in your commit you register the objects right in the ReadManagedPool(). I'd rather move this code to DoAfterRestore(), which is run after all save data was loaded. This function already is registering "missing" audiochannels, and re-exports gui controls, as you can see here:

ags/Engine/game/savegame.cpp

Lines 645 to 650 in 0badfb5

	// Re-export any missing audio channel script objects, e.g. if restoring old save
	export_missing_audiochans();
	// CHECKME: find out why are we doing this here? why only to gui controls?
	for (int i = 0; i < game.numgui; ++i)
	export_gui_controls(i);

[craigharman]

Thanks, that makes sense. I've moved the registration into DoAfterRestore(), right after export_missing_audiochans() and reverted ReadManagedPool(). The loop now covers audio clips, characters, dialogs and GUIs. I left GUI controls out because DoAfterRestore() already re-registers them with export_gui_controls().

I kept the new ccRegisterManagedObjectIfMissing() helper instead of following export_missing_audiochans(). That function checks with ccGetObjectHandleFromAddress(), which raises "Pointer cast failure" through cc_error for every object that isn't in the pool. On an old save I believe that's the normal case (?). The helper looks the address up in the pool directly. If you'd rather use the same pattern as the audio channels, I can change it.

About the // CHECKME above export_gui_controls(): I think it causes a separate problem. export_gui_controls() calls ccRegisterManagedObject() for every control without checking whether it's already in the pool. After a restore, most controls are already there, loaded from the save. ManagedObjectPool::AddObject() still gives each one a new handle. handleByAddress.insert() doesn't overwrite the existing entry, so lookups keep returning the original handle, but the new entry stays in objects. Static objects ignore Dispose(), so the garbage collection in WriteToDisk() never removes these entries, and they're written into the next save. As a result, every restore adds one unused handle per GUI control. They build up across save/restore cycles, and the pool and save files grow a little each time. Nothing crashes, but I think it could use the same "register only if missing" check. I haven't included that in this PR, because I wasn't 100% sure and it seemed better as a separate change. I'm happy to add it here if you'd prefer.