AGOS: Waxworks crashing at Egypt Level 3, corrupting save file

[Cobradabest]

I'm playing through Waxworks (GOG Version, which is English/DOS/Floppy), and when I reach the first glass on the 3rd floor in the pyramid level and beyond that point, the game crashes if I attempt to save the game and it corrupts the save file. The resulting save file is empty.

While it doesn't stop you from beating the game, it definitely impairs the gameplay since the game is very RNG heavy and full of dead ends and deaths.

I've recorded this bug, which can be seen ​here.

The version I'm using is Version 2.9.0git (Jan 20 2024) on Ubuntu 22.04.

[Cobradabest]

Save file I've been loading from

[digitall]

Cobradabest: Please don't adjust priority as it is used by developers for setting blockers on release etc.

[digitall]

The video linked shows that this is a engine abort at the save with "ERROR: itemPtrToId not found!". This error occurs when the engine tries to lookup an invalid item:
​https://github.com/scummvm/scummvm/blob/master/engines/agos/items.cpp#L456

[digitall]

Replicated with the attached savegame i.e. load this ("Cobra"), then walk along the corridor until you reach a glass sheet, then save with name i.e. Crash! ... the game will crash on save with the error indicated previous. This used Waxworks (Floppy/DOS/English) version. Will debug.

[digitall]

Hmm. After a crash, the original savegame file used has been corrupted and will not load again so will need to get from here or backup copy to retry replication.

The fault is coming from:
​https://github.com/scummvm/scummvm/blob/master/engines/agos/saveload.cpp#L1587

The issue seems to be the 4th item i.e. index 3 of the itemstore, but the use of pointers in this makes it hard to see what that item is ...

[digitall]

Tracked down why this is happening. The problematic item number is 1509. _itemArrayPtr[1509] is initialized fine when loading, but that is cleared to nullptr by the Waxworks specific loadRoomItems() function when called on Room 1169:
​https://github.com/scummvm/scummvm/blob/master/engines/agos/rooms.cpp#L362

The exact line clearing this is:
​https://github.com/scummvm/scummvm/blob/master/engines/agos/rooms.cpp#L387

Since this then fails to lookup the mapping between item pointer and reference, the fatal error is emitted.

[valsu]

Just encountered this bug and both my Egypt level saves got corrupted. I now need to restart the whole Egypt level. Not pretty. Setting priority to high as this bug affects basic game functionality and results in corrupted files.

[athrxx]

The diagnosis above seems to be correct. loadRoomItems() invalidates the current subset of items and loads a new subset. It does not update the items contained in the _itemStore array, though. That array may still hold items that have been invalidated. And when the savegame code tries to find the id of such an invalidated item it will trigger that fatal error.

Unfortunately, I haven't discovered any particular flaw in our implementation of loadRoomItems() or in our savegame code. Our code seems to be modelled very closely after the original code. The original will also not tolerate invalid items in the _itemStore array and will exit to DOS with a similar error if it encounters an invalid item.

Now, I took the savegame from here and made it loadable with the original interpreter in DOSBox. And the behavior there is exactly the same: when I walk around the corner to trigger the loadRoomItems() call and then try to save the game it will error out. So it seems that the game data already is in a broken state in that Cobra savegame. And the walk around the corner with the loadRoomItems() just finishs it off completely. Sadly, this makes it much harder to track down the exact cause of the bug...

[athrxx]

The culprit seems to be the item in _itemStore[3]. In DOSBox plays I have a value of 217 in there, in ScummVM I have a value of 1509 (savegame uploaded here) or 1438 (one of my own tries). Both of the latter values are high enough to clash with the items exchanged by loadRoomItems().
The item in question seems to be the dead body of the last enemy killed. When the corpse comes into sight the item is copied into _itemStore[3]. The original interpreter seems to replace that value as soon as you look somewhere else. But with ScummVM, the corpse item stays.
So, the good news is: it is likely that this can be fixed in a way that the existing savegames will still be useable (we just need to understand how the original interpreter updates that array and fix that). The bad news is that currently the pyramid is not really playable. The corruption takes place after killing the first enemy, so restarting the pyramid won't help at all. It might be possible to get through that 3rd floor area without saving, but I don't know if/when saving would ever become possible again. I really recommend waiting for a fix.

[athrxx]

I have now made a fix that seems to work fine with the savegame you provided here. The bug is in the savegame code and breaks the game timers. Unfortunately, each time you load a game, save again, etc. it gets broken more and more. While most of these timers don't seem to have a relevant impact on the gameplay, Waxworks has a timer that (among other things) cleans up the items chain, which prevents invalid item handles. Please test if the fix works for you.

[somaen]

Flagging this as a release-blocker for 2.9.0.

Did you have a chance to test the above fix?

[bluegr]

I've just tested this, and I can confirm that the fix done by athrxx works correctly

[athrxx]

When I made the fix I also played through the whole game.

The bug was easy to reproduce, the pyramid really wasn't playable, the data corruption was basically guaranteed. And with the fix it was fine...