QFG4: Grabbing the second bookshelf passage, msg doesn't reflect its closed/open state

[Vhati]

ScummVM 2.1.0git3770-g15306581ab (Oct 18 2018 04:27:32)
Windows 7 64bit
QFG4 CD (English)

In the castle, past the bat-infested stairs. After you open the bookshelf passage... If you click HAND on the *already open* bookshelf, "The bookshelf shifts slightly as you push against it, but you haven't found the trigger yet."

There's no open+HAND issue with the the first bookshelf (the one the leads to the crypt) because it automatically makes the hero walk through when it's triggered to open.

File - 5kb MD5 - Full MD5
RESOURCE.000 - 263dce4aa34c49d3ad29bec889007b1c - 1364ba69e3c0abb68cc0170650a56692
RESOURCE.AUD - c39521bffb1d8b19a57394866184a0ca - 71098b9e97e20c8941c0e4812d5f906f
RESOURCE.MAP - aba367f2102e81782d961b14fbe3d630 - 801a04cc6aa5d437681a2dd0b6545248
RESOURCE.SFX - 3cf95e09dab8b11d675e0537e18b499a - 7c858d7253f86dab4cc6066013c5ecec

[Vhati]

SavedGame - SecondBookshelf

[digitall]

@Vhati: Thanks for the bug report. Could you try testing this with the original SCI interpreter to see if this occurs in the original as well?

[Vhati]

The original interpreter also does this.

[Vhati]

Occurs in the floppy edition under ScummVM, and its original interpreter, too.


ScummVM 2.1.0git3797-ge7d23d2cd9 (Oct 25 2018 04:17:12)
Windows 7 64bit
QFG4 Floppy 1.1a + note patch (English)

File - 5kb MD5 - Full MD5
RESOURCE.000 - f64fd6aa3977939a86ff30783dd677e1 - ff42260a665995a85aeb277ad80aac8a
RESOURCE.MAP - d10a4cc177d2091d744e2ad8c049b0ae - 3695b1b0a1d15f3d324ea9f0cc325245
RESOURCE.SFX - 3cf95e09dab8b11d675e0537e18b499a - 7c858d7253f86dab4cc6066013c5ecec

[Vhati]

SavedGame (Floppy) - SecondBookshelf

[Vhati]

tl;dr: I think I figured out a solution. Getting there was a ride.
---


This room is script 663, aka rm663, aka global2.

pSecretDoor2 is a Prop instance with only init() and dispose() methods. Which I think implies there's nowhere specific in that bookshelf amenable to patching with regard to intercepting HAND. The superclass handles it (I imagine to request a noun/verb/cond message).

pSecretDoor2's "cel" property equals 4 when fully open, so that could be used as a flag to check.

pCrest is a Prop that DOES override its doVerb() method, which, for HAND, toggles local1 (0=closed, 1=open) and does setScript with sSecret or sCloseSecretDoor. I thought local vars were at the script level, but I wasn't able to see it in the debugger unless I set a breakpoint on pCrest::doVerb.

send pCrest doVerb 4  # Toggle (fun to watch)
send rm663 setScript sSecret  # Open
send rm663 setScript sCloseSecretDoor  # Close

sComeOnIn - a script set when entering the room from either the door or secret passage - will directly call pSecretDoor2::setCycle to briefly open then close it. It would be unaffected if the scripts above were modified.

When the bookshelf is open, an exit can be programmatically triggered with a walk.

# class_table command finds PolyPath (Class 0x23, 001b:0270 on CD & floppy).
# I picked arbitrary coords around in the region that work.

send hero setMotion 001b:0270 40 150 rm663

If hero::setMotion could be inserted into sSecret just before the script disposes itself. That might be an elegant solution: automatically walk to the exit when the crest is turned. Technically, the player would still have control and be able to interrupt a walk then grab the bookshelf, but the bug would be less obvious.

pSecretDoor2::init sets up a doorMat. Upon arrival, sLeaveSecretly is triggered. It knows to abort if local1 != 1, which is nice. It does handsOff, setMotion, and newRoom. Unfortunately instead of PolyPath, it uses MoveTo (Class 0x1d, 0017:0658), which doesn't honor obstacles. If that were patched to PolyPath and if it could be started as sSecret ends, that'd be good.


Wild speculation

I have no idea how to squeeze a custom instruction in here. Well... I can think of an inelegant one. The end of sSecret's declaration is followed by sCloseSecretDoor.

• One patch against pCrest::doVerb to never close the passage.
• A second patch against sCloseSecretDoor to hollow it out until it's a stub that does nothing but return.
  • Use the rest of that space as a code cave for custom instructions.
  • Include the end of sSecret in that patch signature. Change its last bytes into a jump to the cave, add something to set up the walk-out, and conclude with the last instructions that the jump had clobbered.
• It sounds cool and all, but that'd involve adding a big block of assembly to "script_patches.cpp" over such a trivial bug.

Hm. In script 64994, the Room class setScript() method disposes any previously active script. That means a script's dispose() instruction CAN be swapped out to chain another script, because the dispose() would happen either way!

"rm663::setScript sLeaveSecretly" is just a few bytes longer than "sSecret::dispose". So close.


Wait, wait. I found some unnecessary bytes!


disasm sSecret changeState bc

0510:0882: 38 19 02       pushi	0219		; 537, handsOn
0510:0885: 76             push0
0510:0886: 81 01          lag  	01
0510:0888: 4a 04 00       send 	0004
0510:088b: 38 94 00       pushi	0094		; 148, dispose
0510:088e: 76             push0
0510:088f: 54 04 00       self 	0004
0510:0892: 3a             toss 
0510:0893: 48             ret

We don't need that handsOn() either. If we're splicing in sLeaveSecretly() the first thing it's gonna do is handsOff().

Throwing away both "Glory::handsOff" and "sSecret::dispose" should leave enough room for "rm663::setScript sLeaveSecretly". It'll still need a second patch to make it PolyPath motion, but I'm pleased right now. :)


Double checking doorMat's declaration in script 49... If I'm reading it right, it has a check to avoid triggering the doorMat script while the room has an active script already. So there's no worry about the spliced sLeaveSecretly's walk setting it off.

Clicking HAND on the crest will open the bookshelf and hero will automatically walk out.

Still pleased. :D

[digitall]

@Vhati: Excellent... Look forward to seeing this as a Github Pull Request to add a script patch to the SCI engine :)

[Vhati]

Well, I can confirm it behaves as anticipated.

There were complications, of course.

The CD/floppy editions each got a pair of patches because they disagreed on class numbers (Polygon/MoveTo/PolyPath) and the location of sLeaveSecretly. Hope it's not too verbose.

On the plus side, the class inconsistency was a distinct signature to tailor the address for each edition.

I didn't see a means in "script_patches.cpp" to predict the memory location of sLeaveSecretly by name.

Hopefully it doesn't wander around.

That would wreck my solution. I don't know of a selector to look it up live either; not enough free bytes to do such a query here anyway.

The lofsa relative offset was a pain. None of my landmarks/math fit. I had to use trial and error.

"view_object sLeaveSecretly" gave me a goal to aim for (CD=0516:0e93, floppy=0453:0e4c). At first I'd step to discover lofsa's opcode address and where it pointed. Then I'd adjust away the discrepancy in my code, set a breakpoint at the opcode, and check again.

"scummvm -d 3 --debugflags=scriptpatcher" helped confirm when signatures successfully meddled in the room.


​Pull Request: SCI32: Fix QFG4 crest bookshelf HAND interaction

[bluegr]

Thanks for your work! The pull request has been merged, so this can be closed now