SCUMM: SAMNMAX: copyRectToSurface() assert triggered in 2.8.0git when loading a save

[dwatteau]

In current ScummVM 2.8.0git, loading the attached Sam & Max savegame from the GMM triggers the following assert(), when running from a debug build:

scummvm: graphics/surface.cpp:169: void Graphics::Surface::copyRectToSurface(const void*, int, int, int, int, int): Assertion `destX >= 0 && destX < w' failed.

(gdb) bt
#0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
#1  0x00007ffff75cad2f in __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
#2  0x00007ffff757bef2 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#3  0x00007ffff7566472 in __GI_abort () at ./stdlib/abort.c:79
#4  0x00007ffff7566395 in __assert_fail_base (fmt=0x7ffff76daa70 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
    assertion=assertion@entry=0x555555b41437 "destX >= 0 && destX < w", file=file@entry=0x555555b413a3 "graphics/surface.cpp", line=line@entry=169, 
    function=function@entry=0x555555b413e0 "void Graphics::Surface::copyRectToSurface(const void*, int, int, int, int, int)") at ./assert/assert.c:92
#5  0x00007ffff7574df2 in __GI___assert_fail (assertion=0x555555b41437 "destX >= 0 && destX < w", file=0x555555b413a3 "graphics/surface.cpp", 
    line=169, function=0x555555b413e0 "void Graphics::Surface::copyRectToSurface(const void*, int, int, int, int, int)") at ./assert/assert.c:101
#6  0x0000555555943aae in Graphics::Surface::copyRectToSurface (this=0x5555570ba070, buffer=0x555556cf887c, srcPitch=0, destX=0, destY=0, width=0, 
    height=0) at graphics/surface.cpp:169
#7  0x0000555555943c9e in Graphics::Surface::copyRectToSurface (this=0x5555570ba070, srcSurface=..., destX=0, destY=0, subRect=...)
    at graphics/surface.cpp:183
#8  0x00005555558fbd1a in Graphics::CursorManager::replaceCursor (this=0x555556b7a370, surf=..., hotspotX=0, hotspotY=0, keycolor=255, 
    dontScale=false, mask=0x0) at graphics/cursorman.cpp:169
#9  0x00005555558fbb1b in Graphics::CursorManager::replaceCursor (this=0x555556b7a370, buf=0x555556cf887c, w=0, h=0, hotspotX=0, hotspotY=0, 
    keycolor=255, dontScale=false, format=0x7fffffffbca3, mask=0x0) at graphics/cursorman.cpp:138
#10 0x00005555556baf89 in Scumm::ScummEngine::updateCursor (this=0x555556cefbe0) at engines/scumm/cursor.cpp:260
#11 0x00005555556679a9 in Scumm::ScummEngine::saveLoadWithSerializer (this=0x555556cefbe0, s=...) at engines/scumm/saveload.cpp:1559
#12 0x0000555555663ded in Scumm::ScummEngine::loadState (this=0x555556cefbe0, slot=2, compat=false, filename=...) at engines/scumm/saveload.cpp:730
#13 0x0000555555663802 in Scumm::ScummEngine::loadState (this=0x555556cefbe0, slot=2, compat=false) at engines/scumm/saveload.cpp:597
#14 0x00005555556775da in Scumm::ScummEngine::go (this=0x555556cefbe0) at engines/scumm/scumm.cpp:2325
#15 0x000055555567c638 in Scumm::ScummEngine::run (this=0x555556cefbe0) at ./engines/scumm/scumm.h:574
#16 0x0000555555638f47 in runGame (plugin=0x555555cf9bd0, enginePlugin=0x555555ce3d50, system=..., debugLevels=...) at base/main.cpp:318
#17 0x000055555563af1f in scummvm_main (argc=1, argv=0x7fffffffe138) at base/main.cpp:758
#18 0x00005555556364ce in main (argc=1, argv=0x7fffffffe138) at backends/platform/sdl/posix/posix-main.cpp:44

This is with the French release of Sam & Max (sorry, that's all I have on this computer, but it's available on GOG, and I can create a new save with the English release in a couple of days if required). I just played the game until meeting Doug for the first time, and I saved there (I'm using original_gui=true).

The issue doesn't happen in ScummVM 2.7.0. Git bisect tells me that the problem appeared with commit dd1232325d9b310a85244f78ca5d12c063cf047f ("GRAPHICS: Add support for cursor as Surface").

I can't say if the issue comes from the SCUMM engine, or from the new graphics code.

[dwatteau]

Sam & Max (French) save to load from the GMM

[AndywinXp]

Was the cursor, by any chance, off screen when the savegame was created? The assertion might indicate that something like that happened

[dwatteau]

I don't think it was, I can still trigger the assert with a new save where the cursor is in the middle of the screen.

I am playing in windowed mode, though, if that matters.

[dwatteau]

Same save for Sam & Max EN; made with ScummVM 2.7.0

[dwatteau]

Here's the same save made (created in ScummVM 2.7.0) for the English release for easier testing.

Here's how I've created it:

1. Started a new Sam & Max game with original_gui=true and playing in windowed mode (opengl vs. surfacesdl doesn't seem to matter)
2. Played until meeting Doug for the first time
3. Put the cursor in the corner
4. Hit F5 to open the original save menu, moved the cursor in order to click on the "Save" button
5. Then, launched a 2.8.0git debug build (so that assert() calls are triggered) and loaded that save from the GMM.

[tag2015]

I can reproduce this too on latest git on Win10.
Any save created using the Sam & Max original GUI triggers the assert on load (from the launcher, original gui, or scummvm GMM).
You don't need to proceed in the game, it happens everywhere.
A save created using scummvm's GMM instead (disabling the original gui in the game options or using ctrl-f5) works flawlessly (even when loaded from the original gui).

[athrxx]

The problem is that in gfx_gui.cpp, lines 1754-1756 it restores 4 variables that actually never get saved in the first place (for version 6). So these variables will be invalid, the cursor width and height will be zero. This triggers the assert.

The invalid cursor parameters are also in the save files. So it would make sense to patch these, too...

[AndywinXp]

Thanks athrxx! I have yet to fully reconstruct the strange course of events which consciously led me to do ​this, here's what I roughly tested as a fix between some real code and some debugger magic:

• Removing this section from gfx_gui.cpp (because, again, I can't figure out my rationale from 8 months ago):

        else if (_game.version == 6 && _game.id != GID_TENTACLE) {
		setCursorHotspot(_curCursorHotspotX, _curCursorHotspotY);
		_cursor.width = _curCursorWidth;
		_cursor.height = _curCursorHeight;
	}

• Patching savegame loading in saveload.cpp by inserting some valid values after line 1440:

	s.syncAsSint16LE(_cursor.width, VER(20));
	s.syncAsSint16LE(_cursor.height, VER(20));
	s.syncAsSint16LE(_cursor.hotspotX, VER(20));
	s.syncAsSint16LE(_cursor.hotspotY, VER(20));
        if (_game.version == 6 && (_cursor.width == 0 || _cursor.height == 0 || ...)) {
           /* Apply decent values, the value of which I frankly do not know... */
        }

I'm going to take some time to properly figure this out (for the third time, why did I put the code there in the first place? :-) ), and it's kind of late right now so I also need to rest.

[AndywinXp]

Okay, I'm biting the bullet; after spending time performing cyber-archeology on 8 months ago Discord chats and PR comments, I have concluded that at the time I committed code that didn't make sense at all. So I prepared a fix for this as described above and I'm going to commit it shortly.

[AndywinXp]

Fix pushed to master, could someone please confirm that it works as intended now?

[athrxx]

I can confirm that it now seems to load up normally. I am not exactly sure why, since it does not even trigger the post-load fix for me, even for savegames that would have triggered the assert before. Also, the cursor size of 15x15 seems not to be the real size. But even that seems to be fine (e. g., for a 24x24 cursor, I would have expected it to draw an incomplete/messed up cursor with a forced size of 15x15. But it does not happen, the cursor looks fine).

So, it "works as intended", but I am just not 100% sure how... :-)

[tag2015]

Works fine now :)

[AndywinXp]

Great, thanks! Yes, the reason why you are not seeing a messed up cursor is that the post-save/load script is always being executed before rendering anything, so a proper cursor image - along with the correct width, height and hotspot values - gets loaded anyway.

So I could have assigned a height of 2 and a width of 67 and it still would have worked, just as long as those values were not zero :-)

[AndywinXp]

As for why the post-load fix is not being executed for you... That's a mystery of its own! :-P

[dwatteau]

Incorrect cursor being displayed after restoring an impacted save

[dwatteau]

Thanks for the fixes, the various saves I had in this scene don't trigger the assert anymore.

As you've said above, the cursor image is sometimes wrong when restoring such a save. See the new screenshot as an example; the cross cursor is being displayed but that's not the proper image for the cursor action which was selected when saving. Maybe it can be restored from the current context (e.g. looking at some variable holding the current verb or something?) but I can't say if fixing this is worth it.