QFG4: Lock up when casting Trigger after Summon Staff

[tomasz89]

Casting trigger on the gate never ceases.

[Vhati]

@tomasz89:
Can you elaborate?
I could not reproduce with my CD or floppy editions under ScummVM.

I don't think GOG ships any patches to the gate room (script 600) or Trigger (script 11)

Here's what I did.
Do you get the bug if you do the same?

• Create a new mage character.
• Bring up the debugger (ctrl-shift-d to show, escape to hide).
  • Acquire the Summon Staff spell.
    • send hero learn 32
  • Teleport to the gate.
    • room 600
• Cast Summon Staff
• Cast Trigger on the gate.

For me, the narrator says...

• "Your magical staff fails to appear; you are too far from where you created it. Perhaps you can find a new one here."
• "Your spell dissipates as if blocked by magic. It does not seem to have had any effect."

I tried casting both spells during the day and at night.


QFG4 CD (English)

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

QFG4 Floppy 1.1a + note patch (English)

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

[tomasz89]

I get the same as you without summon staff first. If you summon staff then trigger, cactus. Same at Erana's garden where you attempt to get protection.

[tomasz89]

I will retry as you have it though

[tomasz89]

OK the difference is the summon staff fails because you don't possess the staff, so it's the same as casting trigger without the staff. In other words, you need to succeed in summoning the staff before this will trigger the condition.

[tomasz89]

I took the sequence from #10835 and can reproduce it.

Create a new mage character.
Set flag 450

vv g 528 8192

Learn "Summon Staff" spell.

send hero learn 32 200

Teleport to the castle gate room.

room 600

Then cast summon staff, followed by trigger on the gate.

[Vhati]

I see.

I CAN reproduce this in both my CD and floppy editions under ScummVM and their original interpreters.

Didn't have a lategame mage to test with, so I used Sierra's ​debug scripts. (alt-g 450, alt-k 32 200)


castTriggerScript is getting stuck in state 2.

[Vhati]

This bug can be seen at the squid monolith (room 800), and presumably any other triggerable location, when the staff is successfully summoned first.

[Vhati]

SCI Companion got confused, so I decompiled by hand.
Raw disasm from ScummVM is attached.


script 11 - castTriggerScript::changeState()

(switch (= state param1)
	(0
		(= local4 (IntArray with: 0 0 0 1 0 0 2 3))
		(= local5 (IntArray with: 2 3 6 7))
		(= local6 (IntArray with: 2 3 0 3 0 1 2 3))
		(g1_Glory handsOff:)
		(g0_hero setHeading: (GetAngle (g0_hero x?) (g0_hero y?) g441_myX g442_myY) self)
	)
	(1
		(= local1 (go_hero loop?))
		(if (and (> (go_hero view?) 17) (< (go_hero view?) 21))
			# hero's holding the staff.
			(= local2 (go_hero cel?))
			(g0_hero
				view: 19
				loop: (local4 at: local1)
				setCel: 0
				setCycle: End self
			)
		else
			(g0_hero
				view: 14
				loop: (local6 at: local1)
				setCel: 0
				setCycle End self
			)
		)
	)
	(2
		(triggerEffect
			x: g441_myX
			y: g442_myY
			setScaler: g0_hero
			cycleSpeed: 0
			setPri: 180
			init:
			setCycle: Fwd
		)
		(if register
			(if (register onMe: g441_myX g442_myY)
				(= local0 register)
			)
			(= register 0)
		)
		(if (not (and (> (go_hero view?) 17) (< (go_hero view?) 21)))
			# hero's NOT holding the staff.
			(g0_hero setCycle: Beg self)
		)
# No else!?
		(soundFX number: 934 play:)
	)
	(3
		(triggerEffect dispose:)
		(= cycles 2)
	)
	(4
		(if (and (> (go_hero view?) 17) (< (go_hero view?) 21))
			# hero's holding the staff.
			(g0_hero
				view: 20
				loop: (local5 at: (g0_hero loop?))
				cel: (if (< (g0_hero loop?) 6) 4 else 5)
			)
		else
			(g0_hero normalize:)
		)

		(cond
			((not (proc64999_5 global11 270 290 340 440 460 520 580 593 600 641 643 650 750 800))
				# "Nothing seems to have been triggered."
				(g91_gloryMessager say: 0 0 1 0 self 11)
			)
			(local0
				(local0 doVerb: -82)  # 0xae
			)
			(else
				# "Nothing seems to have been triggered."
				(g91_gloryMessager say: 0 0 1 0 self 11)
			)
		)
	)
	(5
		(g1_Glory handsOn:)
		(self dispose:)
	)
)

It doesn't seem to have any means to advance if hero IS holding the staff.



EDIT: State 4's normalize is only when there's no staff.

[Vhati]

I tried some other spells and found that the Open spell has a similar effect without getting stuck.

I had to manually decompile that too, and it is indeed VERY similar.
It's attached. I'll quote the important difference.


script 13 - castOpenScript::changeState(2)

(2
# ...
	(if (not (and (> (go_hero view?) 17) (< (go_hero view?) 21)))
		# hero's NOT holding the staff.
		(g0_hero setCycle: Beg)

		# Unlike castTriggerScript, there's self arg to cue.
	)
	(soundFX number: 934 play:)

	# Use seconds to cue, with or without the staff.
	(= seconds 3)
)

Another interesting difference: After state 4's doVerb(), it disposes the three arrays in the same block.


I realized there is no cue there - in either Trigger or Open. The called doVerb() method is expected to do the handsOn() and replace the room script... otherwise the stalled script would block any further spellcasting.

Example: script 800 - pSquidStone::doVerb()

(82
	(g2_myCurrentRoom setScript: (ScriptID 11) 0 self)  # castTriggerScript
	(return 1)
)
(-82
	(if (== (pSquidStone cel?) 0)
		(pSquidStone setPri: 110)
		(g2_myCurrentRoom setScript: (ScriptID 806 0))  # sToppleSquid
	else
		(g2_myCurrentRoom setScript: 0)

		# "The standing stone has fallen to form a sort of
		#   stone bridge up to the ledge."
		(g91_gloryMessager say: 4 1 41 0)
		(g1_Glory handsOn:)
	)
)

[Vhati]

Okay... To make Trigger work like Open, I wrote a patch that inserts a "seconds" assignment and erases the self arg.

The staff disappears because doVerb() leads to a normalize.


script 600 - aGate::doVerb()

(82
	(if local7
		# "Something about being with Katrina makes all thought
		#   of magic swirl around in your head. You don't seem to
		#   be able to cast any of your spells."
		(g91_gloryMessager say: 12 6 28)
	else
		(= g441_myX ((User curEvent?) x?))
		(= g442_myY ((User curEvent?) y?))
		(= loc5_action 9)
		(g2_myCurrentRoom setScript: (ScriptID 11) 0 self)  # castTriggerScript
	)
)
(-82
	(g0_hero trySkill: 22)  # myTriggerSkill
	(g2_myCurrentRoom setScript: sThrowIt)
)

script 600 - sThrowIt::changeState()

(1
	(g0_hero normalize:)
	(= seconds 2)
)
(2
	(if (proc64999_5 loc5_action 1 2)
		# "Your missile vanishes as if by magic.
		#   (In fact, it probably IS by magic.)"
		(g91_gloryMessager say: 12 6 6 0 self)
	else
		# "Your spell dissipates as if blocked by magic.
		#   It does not seem to have had any effect."
		(g91_gloryMessager say: 12 6 7 0 self)
	)
)

[Vhati]

Another bug. On successive castings, the green projectile isn't visible.

After the room is reset by hero re-entering, it will be visible again only for the first casting.


This is weird.
triggerEffect ignores the first dispose() call.


script 11 - triggerEffect::dispose()

(method (dispose)
	(if local3 (super dispose:) else (= local3 1))
)

The Open spell has the same bug and dispose() weirdness.

[Vhati]

I patched them to super::dispose() always, and that made both spells' effects consistently visible.

Another bug.
The cursor lingers after the first casting.
Then it advances after the second casting.
I don't know why they're different.

Somehow it's not getting cached or the cache is erased.


handsOff() calls proc0_7() to cache the cursor and sets global438=1 to ensure that consecutive handsOff() calls don't re-cache.

script 0 - proc0_7()

(procedure (proc0_7)
	(if
		(not
			# Open, Trigger, Fetch are ALWAYS exempted!?
			(proc64999_5 ((User curEvent?) message?) 80 82 87)
		)
		(if (g69_mainIconBar curIcon?)
			(= gl204_myCachedCursor (g69_mainIconBar curIcon?))
		)
	else
		(= gl204_myCachedCursor 0)
	)
)

handsOn() resets global438 and calls proc0_8(), which restores the cursor and clears the cache.

script 0 - proc0_8()

(procedure (proc0_8)
	(if g204_myCachedCursor
		(g69_mainIconBar curIcon: g204_myCachedCursor)
		(g1_Glory setCursor: (global69 getCursor:))
		(= g204_myCachedCursor 0)

		# If the cached icon was for the item button,
		# but it's not showing an active item, advance the cursor.
		(if
			(and
				(==
					(g69_mainIconBar curIcon?)
					(g69_mainIconBar at: 6)
				)
				(not (g69_mainIconBar curInvIcon?))
			)
			(g69_mainIconBar advanceCurIcon:)
		)
	)
)

[Vhati]

Here's what's going on with the cursor.

handsOff() was intended to never cache the cursor when the current event message is from Open, Trigger, or Fetch. Sometimes it doesn't recognize them.

• At every moment, User::doit() recycles "curEvent" and passes that to User::handleEvent().
  • User's "curEvent" property is a vEvt object.
  • vEvt::new() calls ​kGetEvent to clobber all its own properties with fresh values.
    • Among them are: type, message, modifiers, x, y.
    • When idle, the coords match the cursor's position at each moment, and the rest are 0.
• The event is passed around until it reaches aGate, which defers to Feature::handleEvent().

script 64950 - Feature::handleEvent()

(method (handleEvent param1 &tmp temp0)
	(cond 
		((param1 claimed?) (return 1))
		(
			(and
				(& (param1 type?) $4000)
				(self onMe: param1)
				(self isNotHidden:)
			)
# Important.
			(CueObj
				state: 0
				cycles: 0
				client: self
				theVerb: (param1 message?)
			)
			(= temp0 (param1 claimed: 1))
			(if
				(and
					(g80_User canControl:)
					(& (g0_hero state?) $0002)
					(>
						(GetDistance
							(g0_hero x?)
							(g0_hero y?)
							approachX
							approachY
						)
						approachDist
					)
					g66_gloryApproachCode
					(& _approachVerbs (global66 doit: (param1 message?)))
				)
				(g0_hero
					setMotion: PolyPath approachX (+ (global0 z?) approachY) CueObj
				)
			else
				(g0_hero setMotion: 0)
# Important.
				(if (self facingMe:) (CueObj changeState: 3))
			)
		)
	)
	(return temp0)
)

• CueObj is a Script that holds the message (82) until state 3, when it calls aGate::doVerb(82).

• If hero is already facing aGate, that happens immediately.
  • CueObj is skipped directly to state 3.
  • doVerb() schedules castTriggerScript.
  • handsOff() checks if ((User curEvent?) message?) is 82.
  • The cursor is not cached. This seems to be intended behavior.

• However, if hero is not facing aGate, there is a delay.
  • CueObj state 1 adjusts hero's heading to face aGate.
  • In that extra time, User::doit() resets the event, zeroing out "message".
  • CueObj reaches state 3. Yadda yadda yadda...
  • handsOff() sees ((User curEvent?) message?) is 0. It WILL cache the cursor.

Bonus confusion: hero::normalize() doesn't reset "heading".

After the first casting, hero appears to face north, but technically still faces aGate.
So the next casting will not cache the cursor.


That's more or less what's going on. aside from one case I don't understand... If hero stands below the gate facing north and casts at a spot directly overhead, all castings cache the cursor.


I'm not confident about messing with handsOff and the event system to fix it.
I'll leave it be.

It can be reported as a separate bug someday if it turns out to be serious or if an easy patch is imminent.

[Vhati]

​Pull Request: SCI32: Fix QFG4 Trigger and Open spells

[tomasz89]

Agree that the lockup no longer occurs, however, the Hero should retain the staff in hands after casting. This works for Open, but not Trigger.