GROOVIE: Disable loading saves during runtime

[dafioram]

There is currently a crash that will happen when loading another save
during some puzzles. So we just disable loading at runtime.

Saving during runtime works just fine so we keep that.

See Trac#10664.

[bluegr]

That looks to be too generic. Does the game crash in all puzzles, or just the crypt puzzle?

[dafioram]

When trying to load a saved game using the GMM while a puzzle is interactive will cause a crash in some puzzles

[bluegr]

But you just said the same thing: "some puzzles". Which ones? In the ticket, you mention the crypt puzzle. Is there any other puzzle that has this issue?

[dafioram]

I have updated the ticket to reflect the load behavior for various puzzles in the game.

[dafioram]

Any objections to merging this? The loading can always be enabled later once thats figured out.

[digitall]

@dafioram : We could merge this, but a better solution would be to expand the canLoadGameStateCurrently (and probably canSaveGameStateCurrently) functions to correctly mask off the cases when saving or loading game state would result in an invalid state:
https://github.com/scummvm/scummvm/blob/master/engines/groovie/groovie.cpp#L374

This might not be an either-or situation i.e. we could merge this as a stopgap and then you can look at doing testing for indicators of the problematic puzzles and test for them to prevent saving in this function... and then remove the stopgap once implemented.

[dafioram]

That sounds okay, we should probably talk about the gmm saving as well. Do you think #10663 is caused by the gmm saving I've added?

[digitall]

You mean https://bugs.scummvm.org/ticket/10663 ... As you said there it is certainly possible. If the game state is not such that it allows loading without leaving some odd "leftover" state, then it is likely that in those situations the converse is true.

Really, the Groovie engine and savestate contents should be examined to work out what is not covered by the savestate and thus what might need reset at load or to be added to the save/load format.... but again for now, GMM saving might want the same solution to avoid invalid gamestate files.

[scott-t]

At a hunch I would have said that loading games at any time should be safe (assuming you're loading games saved through the normal process, and when you 'load' them, the game picks up from as if it's just loaded straight out of the normal ouija board).

I could imagine an issue saving a game with the GMM though - the normal process of saving a game will return from the puzzle subscript, replace some of the variables that were pushed as a saved state (see o_loadscript and o_returnscript), then go through the save stages (opening ouija board, etc). Variables 0x00 through 0xFF get cleared a lot (there's a dedicated opcode for it), but 0x100-0x106 are a some special ones that may not being tidied up properly, which combined with the state restore of 0x107 onwards that usually happens may cause some odd behaviour.

(Additional edit, thinking more about the loading process, I had a re-check of the code and our engine doesn't deal with if the existing engine state is inside a puzzle, so the load operation occurs and then it jumps to the expected offset of script.grv where the loads begin. Inside a puzzle, the _code variable refers to the puzzle's script and whether you crash or not probably depends on sheer fluke if the load offset 0x287 is at the start of an opcode or not - either way you won't load cleanly.

BTW, you can tell if you're in a puzzle if _savedCode is non-null within the Script object).

[digitall]

@scott-t Thanks for the information... Any chance of working up a small patch for GROOVIE's canLoadGameStateCurrently and canSaveGameStateCurrently functions? or for fixing the Save / Load code to allow clean GMM loads?

[scott-t]

@digitall Not sure what my weekend holds yet but I might make a go at it. Happy to vet any patches if someone else throws one together as well.

[digitall]

@scott-t : That would be very good. Thanks! :)

[digitall]

Have merged #1376 which should fix this without needing to disable GMM load or save, so am going to close this.

@dafioram : Thanks for finding these bugs and proposing the initial fix... If you test and find any issues, please feel free to comment / update.