Opened 6 years ago

Closed 6 years ago

#10773 closed defect (fixed)

QFG4 floppy: Crash when asking Cranium for potions

Reported by: Vhati Owned by: bluegr
Priority: high Component: Engine: SCI
Version: Keywords: SCI32 has-pull-request
Cc: Game: Quest for Glory 4

Description

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

In Cranium's lab... If you ask about "science", then either "healing" or "poison cure" potions, you'll be prompted for a formula (copy-protection). After the dialog is dismissed (by answering correctly, or clicking EXIT), there's a crash.

It's always the same error.

ERROR: kAddScreenItem: Plane 0000:0000 not found for screen item 00bb:03a6!

Note: This ScummVM build has the fix for Cranium's TRAP robot assertion error (#10766). The new build allowed me to progress into Cranium's lab and encounter the bug naturally, without dubious use of the debugger to teleport.

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

Attachments (2)

sci.037 (49.0 KB ) - added by Vhati 6 years ago.
SavedGame (Floppy) - Request potions
sci.053 (63.0 KB ) - added by Vhati 6 years ago.
SavedGame (Floppy) - Request rehydration

Download all attachments as: .zip

Change History (17)

by Vhati, 6 years ago

Attachment: sci.037 added

SavedGame (Floppy) - Request potions

comment:1 by Vhati, 6 years ago

The CD edition under ScummVM did not have this bug.


ScummVM 2.1.0git3797-ge7d23d2cd9 (Oct 25 2018 04:17:12)
Windows 7 64bit
QFG4 CD (English)

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

comment:2 by Vhati, 6 years ago

Also happens when asking for the "rehydration solution" (available later on).

This savegame throws the same error for all potions, but it's a different one from the OP.

"kAddScreenItem: Plane 0000:0000 not found for screen item 00f2:03a6!"

by Vhati, 6 years ago

Attachment: sci.053 added

SavedGame (Floppy) - Request rehydration

comment:3 by Vhati, 6 years ago

Summary: QFG4: Crash when asking Cranium for healing/cure potionsQFG4: Crash when asking Cranium for potions

comment:4 by bluegr, 6 years ago

Summary: QFG4: Crash when asking Cranium for potionsQFG4 floppy: Crash when asking Cranium for potions

comment:5 by Vhati, 6 years ago

Workaround: Add items via the debugger (ctrl-shift-d, return to game with exit).

# Poison Cure
send hero get 2

# Healing Potion
send hero get 3

# Rehydration Solution
send hero get 32

I've confirmed this works to rescue the dry Domovoi and unlock the doll.

Version 1, edited 6 years ago by Vhati (previous) (next) (diff)

comment:6 by Vhati, 6 years ago

Both error addresses above (00bb:03a6 and 00f2:03a6), in their respective savegames, refer to craniumThumbs.

script 69 - craniumTalker::showAgain() is calling craniumThumbs::show(). At that moment, craniumThumbs has no valid plane to be drawn on. While chatting prior to the formula prompt, plane had been 0012:0003.

During a chat, I can toggle his visibility in the debugger without issue ("send craniumTalker hide" and "send craniumTalker showAgain"). The transition to the formula prompt, and back, must be removing Views from the plane, and not putting them back.

Just before the formula prompt, "bp_kernel DeleteScreenItem break" says craniumTalker is getting disposed.

See also: #10778 where hero's visibiliy wasn't getting restored as the room was disposed.

Last edited 6 years ago by Vhati (previous) (diff)

comment:7 by Vhati, 6 years ago

Floppy Edition (Script 69, craniumTalker)

	(method (dispose param1)
		(if (or (not argc) param1)
			(craniumThumbs dispose:)
			(craniumBrow dispose:)
		)
		(super dispose: param1)
	)

CD Edition (Script 69, craniumTalker)

	(method (dispose param1)
		(if
			(and
				(or (not argc) param1)
				(or
					(not (proc0_4 147))
					(and global194 (!= (global194 talker?) self))
				)
			)
			(craniumThumbs dispose:)
			(craniumBrow dispose:)
		)
		(super dispose: param1)
	)

Debugger command "vv g 194" says global194 is craniumTeller, whose talker property points to craniumTalker. If I'm reading this right, the CD edition seems to be sparing craniumThumbs from disposal?

The lab is script 370. It declares craniumTeller, whose cue() method hides itself and starts a script named delayMsg (also declared there)... whose changestate() method starts the 'protection' puzzle (script 88) and calls showAgain().

Last edited 6 years ago by Vhati (previous) (diff)

comment:8 by Vhati, 6 years ago

In other rooms, global194 is the current Teller object (dialogue choices). Its "talker" property is a Talker object (animated character beside the menu).


proc0_4() checks plot flags. Flag 147 is only set in a few other places.

The clearest example is script 800 - sDoTheDarkSign::changeState().

  • It sets the flag with proc0_2(). Then shows a puzzle (script 801 - runesPuz).
  • Afterward, it unsets the flag with proc0_3().

But also script 23.

  • Teller::showCases() sets, just after it...
    • shows its talker
    • assigns itself, the Teller, to global194
  • Teller::clean() unsets just before disposing its talker.



Teller does these things in the floppy edition as well. craniumTalker::dispose() could check them, in principle. I don't see an easy way to cram in the extra bytes.

Last edited 6 years ago by Vhati (previous) (diff)

comment:9 by Vhati, 6 years ago

The floppy edition's original interpreter did not crash.

comment:10 by sluicebox, 6 years ago

Is making copy protection not crash considered cracking? =)

I took a look, and thanks to your great notes, I came up with a patch. Can you please test this? https://github.com/sluicebox/scummvm/commit/e1aa5b3be598b24be3f8a70d1abab12636ecf656

You're right that the difference in craniumTalker:dispose shows the problem. Weird that they they fixed it in the CD version since it doesn't run the copy protection script. I don't see any other scripts that depend on this. I used the debugger to get the CD version to run the copy protection and it doesn't crash, so their fix works.

The bug is that delayMsg:changeState(0) calls craniumTalker:showAgain even though it's already been disposed. This patch re-initializes craniumTalker so that showAgain is safe to call. craniumTalker is re-initialized as you navigate between conversation sections so that's normal.

delayMsg:changeState(0):

(if ((ScriptID 88 0) init: local4 show: dispose:) ; protection:show returns 1 or 0
	(= register 1)
else
	(= register 0)
)
((craniumTeller talker?) showAgain:) ; showAgain crashes


becomes:

(= register ((ScriptID 88 0) init: local4 show: dispose:))
((craniumTeller talker?) init: showAgain:)


comment:11 by Vhati, 6 years ago

@sluicebox:

Can you please test this?

No crash for my 1.1a English and German floppy editions.


Weird that they they fixed it in the CD version since it doesn't run the copy protection script.

Mine does.
Quest for Glory Collection Series (1997)

Your patch didn't kick in for it, thanks to the hardcoded showAgain for floppies.
Of course, it didn't need to.


craniumTalker is re-initialized as you navigate between conversation sections so that's normal.

Good to know.

Last edited 6 years ago by Vhati (previous) (diff)

comment:12 by sluicebox, 6 years ago

Thanks for testing that and clearing up the CD copy protection. I didn't realize they enabled it on the CD version since they tended to disable it on most other CD releases.

https://github.com/scummvm/scummvm/pull/1431

comment:13 by digitall, 6 years ago

Keywords: has-pull-request added

comment:14 by bluegr, 6 years ago

Thanks for your work. The PR has been merged, so this can be closed now

comment:15 by bluegr, 6 years ago

Owner: set to bluegr
Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.