SCUMM: Recent Rect change triggers assertion in Maniac Mansion

[eriktorbjorn]

The recent change from int16 to int32 in Common::Rect() appears to have caused a regression in (at least) Maniac Mansion v2. I walked into the kitchen from the right when Ed was there, and the game crashed:

scummvm: ./common/rect.h:161: Common::Rect::Rect(int32, int32, int32, int32): Assertion `isValidRect()' failed.

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007ffff6949537 in __GI_abort () at abort.c:79
#2  0x00007ffff694940f in __assert_fail_base
    (fmt=0x7ffff6ab2128 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5555593a5c57 "isValidRect()", file=0x5555593a5c47 "./common/rect.h", line=161, function=<optimized out>) at assert.c:92
#3  0x00007ffff6958662 in __GI___assert_fail
    (assertion=0x5555593a5c57 "isValidRect()", file=0x5555593a5c47 "./common/rect.h", line=161, function=0x5555593a5c18 "Common::Rect::Rect(int32, int32, int32, int32)") at assert.c:101
#4  0x0000555555e27c8d in Common::Rect::Rect(int, int, int, int)
    (this=0x7fffffffc6a0, x1=-176, y1=2147483583, x2=16, y2=-2147483585)
    at ./common/rect.h:161
#5  0x0000555555eac9c5 in Common::Rect::center(int, int, int, int)
    (cx=-80, cy=2147483647, w=192, h=128) at ./common/rect.h:364
#6  0x0000555555ea9152 in Scumm::ScummEngine::setTalkingActor(int) (this=
    0x55555cf810e0, i=11) at engines/scumm/actor.cpp:2784
#7  0x0000555555ea97a6 in Scumm::ScummEngine::actorTalk(unsigned char const*)
    (this=0x55555cf810e0, msg=0x7fffffffc770 "Intruder, halt!")
    at engines/scumm/actor.cpp:2902
#8  0x0000555555f61e9a in Scumm::ScummEngine_v2::decodeParseString()
    (this=0x55555cf810e0) at engines/scumm/script_v2.cpp:415
#9  0x0000555555f72fff in Scumm::ScummEngine_v5::o5_print()
    (this=0x55555cf810e0) at engines/scumm/script_v5.cpp:1437
#10 0x0000555555f66bc2 in Common::Functor0Mem<void, Scumm::ScummEngine_v2>::operator()() const (this=0x55555cba3610) at ./common/func.h:398
#11 0x0000555555de078c in Scumm::ScummEngine::executeOpcode(unsigned char)
    (this=0x55555cf810e0, i=20 '\024') at engines/scumm/script.cpp:493
#12 0x0000555555de06b5 in Scumm::ScummEngine::executeScript()
    (this=0x55555cf810e0) at engines/scumm/script.cpp:486
#13 0x0000555555ddfd86 in Scumm::ScummEngine::runScriptNested(int)
    (this=0x55555cf810e0, script=3) at engines/scumm/script.cpp:338
#14 0x0000555555ddf1d3 in Scumm::ScummEngine::runScript(int, bool, bool, int*, int)
    (this=0x55555cf810e0, script=88, freezeResistant=false, recursive=false, lvarptr=0x0, cycle=1) at engines/scumm/script.cpp:90
#15 0x0000555555f663b2 in Scumm::ScummEngine_v2::o2_chainScript()
    (this=0x55555cf810e0) at engines/scumm/script_v2.cpp:1607
#16 0x0000555555f66bc2 in Common::Functor0Mem<void, Scumm::ScummEngine_v2>::operator()() const (this=0x55555c779b40) at ./common/func.h:398
#17 0x0000555555de078c in Scumm::ScummEngine::executeOpcode(unsigned char)
    (this=0x55555cf810e0, i=74 'J') at engines/scumm/script.cpp:493
#18 0x0000555555de06b5 in Scumm::ScummEngine::executeScript()
    (this=0x55555cf810e0) at engines/scumm/script.cpp:486
#19 0x0000555555de26a1 in Scumm::ScummEngine::runAllScripts()
    (this=0x55555cf810e0) at engines/scumm/script.cpp:920

Notably, the y coordinates of the rect have some pretty strange values. Before the change, apparently they were truncated down to something more sensible. Changing the data types of x and y from int to int16 in ScummEngine::setTalkingActor() seems to fix the problem, but I'm worried that if I do that I may simply be papering over some larger problem.

[eriktorbjorn]

I've attached a savegame for my English Maniac Mansion, v2. Just walk left through the door to trigger the assertion.

[athrxx]

AFAICT from disam the original intepreters (at least <=3) don't have a function like setTalkingActor(). They will just set VAR_TALK_ACTOR and that's it. The whole _system->setFocusRectangle() thing seems to be pure ScummVM magic. Maybe the data used here to make up the rect for _system->setFocusRectangle() (in particular the y coordinate) aren't valid in that situation. So this could be seen as an encouragement to just fix it inside setTalkingActor()...

[athrxx]

The problematic value is _actors[i]->_top at 0x7fffffff. Which is set for actor 11 via actor.cpp, lines 2292 and 2344, right before that actor talk.

So, it seems to me that the problematic line really is 2780. _actors[i]->_top can't be used there. The issue was apparently just silently (and unknowingly) covered up till now.

[eriktorbjorn]

The Rect change has been reverted (to be replaced with a different solution at a later date, apparently), so this bug no longer happens.