[Scummvm-tracker] [ScummVM] #10756: QFG4: Grabbing the second bookshelf passage, msg doesn't reflect its closed/open state

[Vhati]

#10756: QFG4: Grabbing the second bookshelf passage, msg doesn't reflect its
closed/open state
--------------------------------+----------------------------
  Reporter:  Vhati              |      Owner:  (none)
      Type:  defect             |     Status:  new
  Priority:  low                |  Component:  Engine: SCI
Resolution:                     |   Keywords:  SCI32 original
      Game:  Quest for Glory 4  |
--------------------------------+----------------------------

Comment (by 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 founds some unnecessary bytes!
 \\
 \\
 disasm sSecret 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

--
Ticket URL: <https://bugs.scummvm.org/ticket/10756#comment:6>
ScummVM <https://bugs.scummvm.org>
ScummVM