Opened 5 years ago

Closed 5 years ago

#10765 closed defect (fixed)

QFG4 floppy: Assertion Error celobj32.cpp:179 at the swamp

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

Description (last modified by digitall)

ScummVM 2.1.0git3770-g15306581ab (Oct 18 2018 04:27:32)
Windows 7 64bit
QFG4 Floppy 1.1a + note patch (English)

After starting a new game, something about the wisp swamp screen is causing an assertion failure shortly after entering it. Arrival triggers an autosave, but the error ranges from immediately to a couple seconds later.

Restore the attached savegame and just stand still.

Popup
" " "
MinGW Runtime Assertion

Assertion failed!

Program: scummvm.exe
File: ../../src-master/src/engines/sci/graphics/celobj32.cpp, Line 179

Expression: _minX <= _maxX
" " "

Console
" " "
Running Quest for Glory IV: Shadows of Darkness (DOS/English)
ADLIB: Starting driver in SCI1 mode
kFileIO(fileExists) 18.scr -> 0
kFileIO(fileExists) 18.scr -> 0
kFileIO(fileExists) 18.scr -> 0
kFileIO(fileExists) flythru.seq -> 0
# Restore Menu
kValidPath() -> 0
# Walk west
kFileIO(fileExists) 18.scr -> 0
kGetCWD() -> /
Game name Glory save 2 desc Automatic Save ver
" " "

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

Attachments (1)

sci.002 (43.0 KB ) - added by Vhati 5 years ago.
SavedGame - WalkWest

Download all attachments as: .zip

Change History (16)

by Vhati, 5 years ago

Attachment: sci.002 added

SavedGame - WalkWest

comment:1 by Vhati, 5 years ago

Correction: Restore the attached savegame, walk west, then just stand still.

comment:2 by digitall, 5 years ago

Description: modified (diff)
Summary: QFG4: Assertion Error, selobj32.cpp 179 at the swampQFG4: Assertion Error celobj32.cpp:179 at the swamp

comment:4 by Vhati, 5 years ago

Disabling autosave didn't help.
Adjusting speed to its lowest setting and running didn't help.

I was afraid I'd never get past this, but I seem to have some luck if, from the start, I never adjust the speed slider and run through. Still crashes if I backtrack to that area.

comment:5 by Vhati, 5 years ago

Good news! This isn't quite fatal. Just very annoying.
The assertion failure popup has an "ignore" button: that works.

Players have to click "ignore" repeatedly, once for every invalid animation tick. Sometimes it's constant. Sometimes there's a lull that allows players a little time to click WALK, leap, etc. and mash "ignore" until the hero completes the action.

Having traversed the swamp eastward from the tomb side. I suspect the undead hands are at fault, given the timing and unevenness of the errors. And during the day, there are no wisps around to blame.

comment:6 by bluegr, 5 years ago

Summary: QFG4: Assertion Error celobj32.cpp:179 at the swampQFG4 floppy: Assertion Error celobj32.cpp:179 at the swamp

This happens in the floppy version only. The CD version works fine.

comment:7 by Vhati, 5 years ago

Investigated a bit.

  • Walk to the swamp shore west of the goo.
  • Find the plane with ~24 screen items.
    • vpl
  • List the screen items on that plane.
    • vpi 0012:0003
  • send {every 'hands' address} hide
    • "hands.0" ... "hands.5" address notation is convenient.
  • send {one 'hands' address} show

All but one sank and rose without problems.

One hand near the back (0012:05a5, x:48 y:108 view:535 loop:5 cycler:cueEndLoop) was throwing assertion errors repeatedly the whole time it was submerged. It was fine while it was up. No where near the edge of the screen.

Hands are positioned with randomly chosen values from lists and given a randomly chosen loop. (script 10 - Swamp::init())

All the possible hand loops have a submerged cell 0 with dimensions: 1x1.


I brought the hand closer to the foreground ("send 0012:05a5 y 150"), and it stopped throwing errors.

I moved it far away ("send 0012:05a5 y 100"), and it threw errors again.


I can't set a good breakpoint for the error and don't know how to check its width.
I'm guessing the scaler is reducing the 1x1 dimensions to 0.

Source: celobj32.cpp

_minX(targetRect.left),
_maxX(targetRect.right - 1),
[...]
assert(_minX <= _maxX);

I don't know why the CD edition *doesn't* throw errors. Diff didn't reveal any interesting changes. SCI Companion says the View is scalable in both editions.

I fired up the CD edition, hid all but one hand, and gave it the above x, y, and loop values. CD edition behaves.

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

comment:8 by Vhati, 5 years ago

Oh. The Scaler class changed.


Floppy, script 64935 - Scaler::doit()

(method (doit &tmp temp0 temp1 temp2 temp3)
	(= temp2 (client scaleX?))
	(= temp3 (client scaleY?))
	(cond 
		((< (= temp0 (client y?)) backY) (= temp1 backSize))
		((> temp0 frontY) (= temp1 frontSize))
		(else (= temp1 (+ (/ (* slopeNum temp0) slopeDen) const)))
	)
	(if
		(or
			(!= temp2 (= temp1 (/ (* temp1 128) 100)))
			(!= temp3 temp1)
		)
		(client scaleX: temp1 scaleY: temp1)
	)
)

CD, script 64935 - Scaler::doit()

(properties
# added a property
	lastY -9999
)

# ...

(method (doit &tmp temp0 temp1 temp2 temp3)
	(if (!= (= temp0 (client y?)) lastY)
		(= lastY temp0)
		(= temp2 (client scaleX?))
		(= temp3 (client scaleY?))
		(cond 
			((< temp0 backY) (= temp1 backSize))
			((> temp0 frontY) (= temp1 frontSize))
			(else (= temp1 (+ (/ (* slopeNum temp0) slopeDen) const)))
		)
		(if
			(or
				(!= temp2 (= temp1 (/ (* temp1 128) 100)))
				(!= temp3 temp1)
			)
			(client scaleX: temp1 scaleY: temp1)
		)
	)
)

comment:9 by Vhati, 5 years ago

The only functional difference I'm seeing between Scaler versions is CD avoids recalculating if the y position hasn't changed since last time. And I don't think hands ever move.


script 10 - Swamp::init()

(= local7 (IntArray with: 54 48 95 158 132))
(= local8 (IntArray with: 175 108 119 142 178))
# ...
(switch g11_myCurrentRoomNum
# ...
	(535
		(= temp0 0)
		(while (< temp0 5)
			((hands new:)
				init: temp0 (local7 at: temp0) (local8 at: temp0)
			)
			(++ temp0)
		)
	)
# ...
)



script 10 - hands::init()

	(method (init param1 param2 param3)
		(super init:)
		(= x param2)
		(= y param3)
		(= loop (Random 0 5))
		((Timer new:) setReal: self (Random 5 25))
		(self
			signal: (| (self signal?) $0001)
			setScaler: Scaler 100 60 125 65
		)
	)



hands is a TargActor, whose ancestry is Feature/View/Prop/Actor.
Prop::setScaler() basically forwards args to Scaler::init(self, &rest).


script 64935 - Scaler::init()

(method (init param1 param2 param3 param4 param5)
	(if argc
		(= client param1)
		(= frontSize param2)
		(= backSize param3)
		(= frontY param4)
		(= backY param5)
	)
	(= slopeNum (- frontSize backSize))
	(if (not (= slopeDen (- frontY backY)))
		(proc64921_0 {<Scaler> frontY cannot be equal to backY})
		(return 0)
	)
	(= const (- backSize (/ (* slopeNum backY) slopeDen)))
	(return (self doit:))
)



A hands scaler would have...

  • frontSize = 100
  • backSize = 60
  • frontY = 125
  • backY = 65
  • slopeNum = 100 - 60 = 40
  • slopeDen = 125 - 65 = 60
  • const = 60 - (100 * 65 / 60) = -48

Okay. frontY & backY are capping the scale. If a client moves lower/higher than those values, it won't get any bigger/smaller.

The problem hand earlier was at y:108.

(else (= temp1 (+ (/ (* slopeNum temp0) slopeDen) const)))

40 * 108 / 60 + -48 = 24

If the client's "scaleY" property isn't already 24...
(Debugger says a hands at that position has scaleY:113. Dunno where 113 came from.)

... the scaler will attempt to set scaleY to 24.

The assertion doesn't abort anything though.

"bp_write hands.3::scaleY" isn't stopping as the hand bobs up and down, so scaleY isn't constantly being set (or it is constantly set to 113?). Confused.

comment:10 by Vhati, 5 years ago

Hrm.
I tried disposing the scaler and nulling the hand's "scaler" property.
Errors persisted.

I set the hand's "scaleX" and "scaleY" to 200.
Errors persisted.

The constant errors seem to begin the moment it submerges.
And they stop while it is still submerged, before it has risen.

Except sometimes it goes through an entire cycle without ANY errors!?


Hiding the last hand definitely makes the errors stop.

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

comment:11 by Vhati, 5 years ago

"bp_write hands.3::scaleY" isn't stopping

I probably just can't set breakpoints on {nnnn:nnnn} or {name.n}.

I couldn't get the hand's cycler to trip until I set a breakpoint on the bare name.

comment:12 by Vhati, 5 years ago

I disposed all the other hands and wisps.
The lone hand was only rarely throwing errors.

Went to restore and start over and... the control panel triggers an error!
If I show the control panel, specifically while the hand is submerged, there's a single error.

The assertion itself is failing during repaints, which don't happen unless the screen region is dirty. Of course, the painted object's properties could be set any time before that.


Prop::setScaler() was also setting the hands "scaleSignal" property to 1.
"send {hands} scaleSignal 0"
No more scaling (I'd manually set large scaleX/scaleY values earlier).
No more errors while submerged when the control panel is shown.

I toggled "scaleSignal" on and off, and I could cause the errors almost exclusively while it was on.
There was a little lag. I think the graphics needed to re-sync with the object before it really stopped scaling.

Weirdly, even scaling up is a problem (tried scaleX/scaleY = 400).

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

comment:13 by sluicebox, 5 years ago

Keywords: has-pull-request added
Owner: set to sluicebox

comment:14 by Filippos Karapetis <bluegr@…>, 5 years ago

In 2fd416b0:

SCI32: Extend scaler sanity checks to all SCI32 versions

Fixes QFG4 bug #10765. It's preferable to have sanity checks in the
code, rather than crashing due to invalid draw rectangles from buggy
game scripts. It's no use checking which specific interpreter versions
had sanity checks and trust the game scripts of the other interpreters.
Thus, it's easier and safer to always enable these sanity checks.

comment:15 by bluegr, 5 years ago

Owner: changed from sluicebox to bluegr
Resolution: fixed
Status: newclosed

Sanity checks have been put in place. Many thanks to everyone's work on this!
Closing

Note: See TracTickets for help on using tickets.