QFG4: Crash in cave when fighting Pit Horror below the tightrope

[Vhati]

ScummVM 2.1.0git3879-gb203b61b38 (Nov 13 2018 04:24:02)
Windows 7 64bit
QFG4 CD (English)

After returning to the cave late in the game. There's combat with a white grub-like monster below the tightrope. Casting minimally charged projectile spells at it will cause a crash.

"Invalid arithmetic operation (bitwise OR - params: 002e:1694 and 0000:0001) from method wipeSpell::doit (room 810, script 855, localCall ffffffff)!"

Haven't tested with the original interpreter yet.

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

[Vhati]

SavedGame (CD) - Pit Horror, magic-using rogue

[Vhati]

script - 855 wipeSpell::doit()

(super doit:)
(if
	(and
		register
# Crash is here.
		(| (SetNowSeen horror) $0001)
		(or
			(g188_egoSpell
				onMe: (+ (horror nsLeft?) 9) (+ (horror nsTop?) 24)
			)
			(g188_egoSpell
				onMe: (+ (horror nsLeft?) 0) (+ (horror nsTop?) 30)
			)
		)
	)
	(= register 0)
	(g188_egoSpell cue:)
)

​Article: SCI Companion - SetNowSeen (Kernel)

It looks up the view object's x and y properties, as well as the view/cel/loop to determine its width and height, then sets its nsLeft, nsTop, nsRight and nsBottom properties accordingly.

​Source: engines/sci/graphics/compare.cpp - GfxCompare::kernelSetNowSeen

void GfxCompare::kernelSetNowSeen(reg_t objectReference) {
	GfxView *view = NULL;
	Common::Rect celRect(0, 0);
	GuiResourceId viewId = (GuiResourceId)readSelectorValue(_segMan, objectReference, SELECTOR(view));
	int16 loopNo = readSelectorValue(_segMan, objectReference, SELECTOR(loop));
	int16 celNo = readSelectorValue(_segMan, objectReference, SELECTOR(cel));
	int16 x = (int16)readSelectorValue(_segMan, objectReference, SELECTOR(x));
	int16 y = (int16)readSelectorValue(_segMan, objectReference, SELECTOR(y));
	int16 z = 0;
	if (SELECTOR(z) > -1)
		z = (int16)readSelectorValue(_segMan, objectReference, SELECTOR(z));

	view = _cache->getView(viewId);
	view->getCelRect(loopNo, celNo, x, y, z, celRect);

	if (lookupSelector(_segMan, objectReference, SELECTOR(nsTop), NULL, NULL) == kSelectorVariable) {
		setNSRect(objectReference, celRect);
	}
}

From context, the script might be expecting the kernel call to return 1 or 0 to indicate success. I haven't spent much time among kernel calls. That one doesn't seem to be returning anything at all?

[Vhati]

Grepping all the scripts, the bitwise OR idiom "(| (SetNowSeen blah) $0001)" occurs 17 times.

Primarily used on global195 (ego1, in-combat hero). Sometimes global185 (current in-combat monster), revenant, nectar.

[Vhati]

Likely related to...

• #10138 - Revenant combat
  • ​Commit: 01f3e6c - an arithmetic workaround
  • The OP's error pointed to a method with this same idiom, OR'ing the result of SetNowSeen().

• #10419 - Necrotaur combat
  • ​Commit: 4dc9f0e - an arithmetic workaround
  • The OP wasn't specific, but the commit named a script where the idiom is used.

• #10710 - Wraith combat, thief parrying without the skill
  • Not yet fixed.
  • The OP's error pointed to a method with this same idiom.

What do arithmetic workarounds do?
Is that the right approach, or is the kernel func at fault?

[m-kiewitz]

That error message is one implemented by us.

The scripts try to do a bitwise OR operation on an integer and an object, which obviously makes no sense. It's a script bug.

It's only acceptable to do that using 2 integers.

Those are typically typos in the script code somewhere.

[m-kiewitz]

Arithmetic workarounds may either still do the call, or skip the call.

We need to check the script code and verify what is actually happening.
In some cases a workaround may make sense. In other cases the script needs to get patched.

The original interpreter simply ate up anything.

The kernel function is definitely not at fault. It is a script bug in any case.

[m-kiewitz]

SetNowSeen32 returns a boolean, so that can't be it.

It looks to me as if it's g188_egoSpell, that is returning an object.

How can I reproduce the bug? I never played through Quest for Glory 4.

[Vhati]

(if
  (and
    "(| (SetNowSeen horror) $0001)"

Come to think of it. That can't have been expecting a 1 or 0.
Bitwise OR-ing 1 or 0 with 1 would guarantee 1... useless as an if condition.

I was charitably thinking of AND, which would've been bizarrely redundant but at least not guaranteed to be 1. I don't know what they were thinking.

[Vhati]

@m-kiewitz:

How can I reproduce the bug? I never played through Quest for Glory 4.

• Restore the attached savegame.

• From the bar at the top, choose the grappling hook.
• Place the hook on the ledge directly below the left stalagmite.
• Hero will climb down.
• Switch to HAND as if to grab the conspicuous item on the floor beside the sleeping monster.
• It will awaken and combat will begin. Not threatening. Don't worry about getting hit.

• During combat, there will be some yellow buttons at the bottom for spells.
  • Zap, Flame Dart, Force Bolt, Frostbite. A pure mage class would have more. And "S".

• Ignore the first and last buttons.
  • Clicking the first (Zap), adds a damage boost to your next melee.
  • The last ("S") is for strategy mode - an AI controls your character. I'm not familiar with it.

• Anyway... The middle buttons.
  • Hold any of them down to charge. Release to fire.
  • A brief charge yields a puny blast that causes this bug.

• Cursor keys maneuver/block, but they won't matter here.
• Left or right clicking on enemies, the air, yourself will melee, jump, block... right-clicking empty space on the bottom panel will throw daggers.

• If you feel like winning, walk up to it and left-click a bunch. It's resistant to magic.

[Vhati]

@m-kiewitz:

It looks to me as if it's g188_egoSpell, that is returning an object.

I thought the onMe(x, y) would do hit detection and return a boolean?

Digging up g188_egoSpell's class...

script 38 - sActor::onMe()

(method (onMe param1 param2)
	(OnMe param1 param2 self (& signal skipCheck))
)

SCI Companion highlights the inner OnMe brown: it's asking the kernel to handle it.


In any case, the error is complaining about a bitwise operator.
(| a b) is a bitwise operator.
(or a b) is a conditional operator.


Debugger's "registers" command reports pc=002e:0410 when the error occurs.

002e:0404: 78             push1
002e:0405: 72 10 00       lofsa horror[1694]
002e:0408: 36             push
002e:0409: 43 0a 02 00    callk SetNowSeen[a],  0002
002e:040d: 36             push
002e:040e: 35 01          ldi   01
002e:0410: 14             or

It's doing a bitwise OR between what I'd expect to be the kernel call's result and 1.

Breaking just after the call, acc=002e:1694, which is the address of horror. The kernel is either leaving the accumulator untouched - horror was the last thing in acc - or the kernel is returning the object it was given.

"Invalid arithmetic operation (bitwise OR - params: 002e:1694 and 0000:0001)"

[Vhati]

(if
  (and
    register
    "(| (SetNowSeen horror) $0001)"
    ...

*squint*
If SetNowSeen() were expected to return 1/0. And bitwise OR-ing that guarantees a 1...

Did they... Did they micro-optimize an action by inserting it into the middle of a series of conditions, so it could be short-circuited when the "register" variable was 0?

When the action did run, always forced to evaluate as 1, it would not abandon the block when the kernel returned 0. Because it wasn't there to BE a condition, just something to run.

It makes sure horror's properties are set, so the subsequent onMe() conditions can test them.

[Vhati]

@m-kiewitz:

The kernel function is definitely not at fault.
[...]
SetNowSeen32 returns a boolean, so that can't be it.

I'm sorry to ask but... Does it?


​Source: sci/sci.h

enum SciVersion {
...
	SCI_VERSION_2, // GK1, PQ4 floppy, QFG4 floppy
	SCI_VERSION_2_1_EARLY, // GK2 demo, KQ7 1.4/1.51, LSL6 hires, PQ4CD, QFG4CD
...
}

​Source: sci/engine/kgraphics32.cpp - kSetNowSeen32()

reg_t kSetNowSeen32(EngineState *s, int argc, reg_t *argv) {
	const bool found = g_sci->_gfxFrameout->kernelSetNowSeen(argv[0]);

	// MGDX is assumed to use the older kSetNowSeen since it was released before
	// SQ6, but this has not been verified since it cannot be disassembled at
	// the moment (Phar Lap Windows-only release)
	// (See also getNowSeenRect)
	if (getSciVersion() <= SCI_VERSION_2_1_EARLY ||
		g_sci->getGameId() == GID_SQ6 ||
		g_sci->getGameId() == GID_MOTHERGOOSEHIRES) {

		return s->r_acc;
	}

	return make_reg(0, found);
}

I got lost. Can't tell if it's returning acc as-is or deferring to something behind frameout that would modify it.

[m-kiewitz]

It returns "found", which is defined as a boolean.
So it's 0 or 1.

Can you please use "vo 002e:1694" (or whatever the first value is).
That way you can check what kind of object it is.

SCI internally uses 16 bit integer, and we added a segment on top to also return objects and such (that's ScummVM exclusive) in a way that we can distinguish between them.

Original SCI simply returned either a regular value or an offset, there was no way to differentiate between them. It always depends on how the value was used later.

This is not the case for us anymore, because of script bugs like these. The original behavior caused memory corruption in some games and other random problems and through uninitialized read + the detection above we can detect such problems and fix them.

[m-kiewitz]

Reproduced it.

002e:1694 is "horror", based on "SActor"
Script "horrorCombat"
Script #885

Really looks like a typo somewhere.
Does this bug really only occur here, or everywhere?

[m-kiewitz]

Original SCI was also limited to 64kb of data in total, because of those 16-bit integers btw.
We aren't. The VM is based on the VM of FreeSCI.

Backtrace:
combat:.init (script 810, which is the current room)
pCombat::Dispose (script 38)
pCombat::show (script 34)
pCombat::hide (script 34)
pCombat::doit (script 34)
pCombat::doit (script 38)
Cast:doit (script 64999)
Cast::eachElementDo (script 64999)
kListEachElementDo(1c:147, 45h)
horror::doit (script 38)
horrorCombat::doit (script 64999)
wipeSpell::doit(855)

[m-kiewitz]

Ah wait, it seems that maybe the detection here is a false positive.

For original SCI it was fine to do a bitwise OR against a value plus an offset, because both were 16-bit integers, but in some cases there are script bugs which break things, that's why we got this detection going.

I guess maybe that code is really working as intended (checking for an object OR that kernel call returning 1) and we can simply pass it through.

[Vhati]

@m-kiewitz:

Reproduced it.
002e:1694 is "horror"

maybe that code is really working as intended (checking for an object OR that kernel call returning 1)

The object in question is horror.

This

(| (SetNowSeen horror) $0001)

is this

002e:0404: 78             push1
002e:0405: 72 10 00       lofsa horror[1694]
002e:0408: 36             push
002e:0409: 43 0a 02 00    callk SetNowSeen[a],  0002
002e:040d: 36             push
002e:040e: 35 01          ldi   01
002e:0410: 14             or

"Invalid arithmetic operation (bitwise OR - params: 002e:1694 and 0000:0001)"

The things being OR'd are acc following a kernel call (pushed to the stack for this op) and a literal 1.
The acc value after the kernel call is an object, 002e:1694, horror.


Believe me, I am painfully aware of my Dunning-Kruger risk as I press this. I can't see how you're reading something different.

​Article: ScummVM Wiki - SCI Spec, Kernel funcs

Return values are returned into the accumulator, unless stated otherwise. If return type is stated as (void), then the accumulator is not modified.

[SetNowSeen32] returns "found", which is defined as a boolean.
So it's 0 or 1.

reg_t kSetNowSeen32(EngineState *s, int argc, reg_t *argv) {
	const bool found = ...

	if (getSciVersion() <= SCI_VERSION_2_1_EARLY ... ) {
		return s->r_acc;
	}

	return make_reg(0, found);

}

It has two return statements.
The SciVersion enum comments say QFG4 is <= SCI_VERSION_2_1_EARLY.
It doesn't look to me like the "found" bool is being returned. At least not the "found" that is declared there.

If it were returning 1 or 0, acc would be clobbered, and 002e:1694 wouldn't be there after the call.



EDIT: Changed the SCI Spec link, from SierraHelp's wiki to ScummVM's wiki.

[Vhati]

@m-kiewitz

maybe the detection here is a false positive [...] and we can simply pass it through.

For original SCI it was fine to do a bitwise OR against a value plus an offset, because both were 16-bit integers

@Vhati:

If SetNowSeen() were expected to return 1/0. And bitwise OR-ing that guarantees a 1...

it wasn't there to BE a condition, just something to run.

Hm. I can't say what QFG4 is expecting from the call. Technically all the OR does is guarantee a non-zero result. So SetNowSeen() could get away with returning a bool, an object address, or void (de facto object, with the call's obj param left unmodified in acc). They'd all be forced to evaluate as true and not abandon the if block.

If SetNowSeen() is returning void (de facto object), and if that is intended behavior for QFG4, then the invalid-arithmetic detection would be a false positive. Everywhere this "(| (SetNowSeen blah) $0001)" idiom is used.

[m-kiewitz]

Hmm right.
I should have checked using a debugger.

I have to look into this further, was pretty sure that a boolean is returned.
I see - Quest for Glory 2 is SCI2.1 EARLY. That explains it all. I was assuming QfG2 used a newer version.

I guess it looks like a script bug then, probably really a typo.
Effectively the code makes no sense. It's always active, so why even bother to check.

I guess what they wanted to do is to check if an object is returned by kSetNowSeen().

[Vhati]

@m-kiewitz:

It's always active, so why even bother to check.

I'm warming to the hypothesis that it's not really a condition to 'check'. The OR is a micro-optimizing hack to sneak a void func into the middle of a list of conditions for an if block.

if (register && (SetNowSeen(horror) | 1) && (boundary tests)) {
	register = 0          // Got a hit, stop checking.
	g188_egoSpell::cue()  // Advance the spell Script's state to disappear?
}

• If "register" is 0, short-circuit.
  • doit() runs frequently, I imagine. Every optimization helps on slow hardware.
  • Script objects in script 855 tend to set their own "register" property occasionally when changeState() wants to toggle the frequent collision detection.

• The func is there to update boundary properties so that collision tests always have current data, but only when "register" deems it necessary.

• The author may have been acting out of a general sense that the truth evaluation of void funcs is erratic.
  • In the specific case of what I disassembled above, SetNowSeen() could exist as a pseudo-condition unaided, always non-zero. This is only because its stack was pushed immediately before the callk, with an object arg.
  • Wrapping a void func in a bitwise OR ensures that however it compiles, it will evaluate to non-zero.
  • [EDIT: Removed bad speculation about null handling that would absolutely explode.]

Why not this? Dunno. Brevity?

if (register) {
	SetNowSeen(horror)

	if (boundary tests) {
		register = 0
		g188_egoSpell::cue()
	}
}

[Vhati]

In any case, it is ugly. Apparently innocuous.
And occurs in *scrolls up* 17 places with what ought to be a very signature-friendly form.

Scripts 41, 810, 830, 835, 840, 855, 870.

Grepping all scripts for the bitwise OR, I didn't find any other function wrapped like this.

[Vhati]

@m-kiewitz:

In some cases a workaround may make sense. In other cases the script needs to get patched.

The function definitely needs to run, in preparation for the boundary tests.
The function, wrapped or not, wasn't capable of abandoning the tests.

The following patch would allow that, make the arithmetic valid, and I expect would apply to any of those scripts. Would that be preferred over workaround entries?

002e:0404: 78             push1
002e:0405: 72 ** **       lofsa anyObject
002e:0408: 36             push
002e:0409: 43 0a 02 00    callk SetNowSeen[a],  0002
002e:040d: 36             push      // Make this 0x78 push1
002e:040e: 35 01          ldi   01
002e:0410: 14             or        // Result is (1 | 1, true)

[m-kiewitz]

In this case it makes the most sense to simply add a workaround entry with STILLCALL.

Does this only occur in CD, or also in Floppy?
If so, you will have to check if the exact same call exists and if the workaround applies to both.

[Vhati]

@m-kiewitz:

Does this only occur in CD, or also in Floppy?

Floppy also has 17 occurrences. Same scripts.

Bug #10778 makes it impossible for floppy to naturally get this far in the plot.


I managed to confirm the OP crash through hacked circumstances.

• Create a new magic user. Name them. No need to allocate skill points.
• Wake up in the starting room.
• With the debugger. set the global for Sierra's debug mode.
  • vv g 201 1
  • The room you're about to go to will react to that flag.
• Teleport.
  • room 710
• Dismiss the debugger.

• Hero will start at an inappropriate position, causing endless 'you can't leave' nags.
• Reposition with the debugger, then dismiss it... and the lingering nag.
  • send hero posn 28 55

• From the bar at the top, click the star. That's the spells menu.
• Click HAND on the Levitate spell.
  • It's the last icon in the second row, a circle with streaks suggesting upward motion.
  • hero will automatically walk to the ledge.
• The monster will immediately wake when you descend.
  • When I created a rogue with magic points, I had to walk at it a couple times.

• Combat begins.
• Cast a minimally charged Flame Dart spell, as before.

[Vhati]

@m-kiewitz:

it makes the most sense to simply add a workaround entry with STILLCALL.

That wasn't accepted as a legal value for arithmeticWorkarounds.

"Assertion failed: solution.type == WORKAROUND_FAKE, file engines/sci/engine/vm_types.cpp, line 73"


​Source: vm_types.cpp

reg_t reg_t::lookForWorkaround(const reg_t right, const char *operation) const {
	SciCallOrigin originReply;
	SciWorkaroundSolution solution = trackOriginAndFindWorkaround(0, arithmeticWorkarounds, &originReply);
	if (solution.type == WORKAROUND_NONE)
		error("Invalid arithmetic operation (%s - params: %04x:%04x and %04x:%04x) from %s", operation, PRINT_REG(*this), PRINT_REG(right), originReply.toString().c_str());
	assert(solution.type == WORKAROUND_FAKE);
	return make_reg(0, solution.value);
}

Should there be a new workaround table for kSetNowSeen32()?

[Vhati]

Oh, I see.
WORKAROUND_FAKE on the OR's arithmetic.

The kernal call will have already happened.
The faked OR will disregard the non-number it was given.

[m-kiewitz]

Ah, sure. You may also do that.
Actually executing it would also work, especially because the result is not really used for anything except checking if it's not zero.

Faking it makes sense as well, so I guess maybe fake the result.

[m-kiewitz]

btw. how do you edit comments?
That feature is probably hidden in some way, or it doesn't work correctly in my browser.

[m-kiewitz]

And right, I just checked the code.
We only made faking possible for those arithmetic operations.
Actually makes some sense, although the operation would still work even when using offset + value.

[m-kiewitz]

Is the exact same error triggered in floppy?
Just to be sure.

[Vhati]

@m-kiewitz:

how do you edit comments?

On the right, where you see a permalink ("comment:27") hover below that, and buttons will appear.

Actually executing it would also work

Perhaps I shouldn't have redacted my speculation. Relevant now.

" " "

• The author may have been acting out of a general sense that the truth evaluation of void funcs is erratic.
  • In the specific case of what I disassembled above, SetNowSeen() could exist as a pseudo-condition unaided, always non-zero. This is only because its stack was pushed, from acc, immediately before the callk, with an object arg.
  • Wrapping a void func in a bitwise OR ensures that however it compiles, it will evaluate to non-zero.

" " "


I'll quote the disasm of wipeSpell again, for juxtaposition.

002e:0404: 78             push1
002e:0405: 72 10 00       lofsa horror[1694]
002e:0408: 36             push
002e:0409: 43 0a 02 00    callk SetNowSeen[a],  0002
002e:040d: 36             push
002e:040e: 35 01          ldi   01
002e:0410: 14             or

I just stepped through one of those other occurrences that weirdly *was not* crashing.


script 870 - attackRight::doit()

002d:0494: 67 16          pTos  state[16]
002d:0496: 35 01          ldi   01
002d:0498: 1a             eq?
002d:0499: 30 88 00       bnt   0088  [0524]

002d:049c: 78             push1
002d:049d: 89 c3          lsg   c3              ; 195
002d:049f: 43 0a 02 00    callk SetNowSeen[a],  0002
 Kernel params: (0060:338f)
002d:04a3: 36             push
002d:04a4: 35 01          ldi   01
002d:04a6: 14             or

This time, the void func 'returns' a 1.

Which makes the OR arithmetic superficially valid. The object arg had been loaded directly into the stack from a global (lsg).

Looking back farther, the last value of acc came from... [EDIT] An unrelated "state == 1" property test!

The leaked accumulator could get weirder, even causing the void func to 'return' 0.



EDIT: I'd initially blamed the arg count (push1), but that doesn't involve acc.

[Vhati]

Should I add arithmetic workarounds for all wrapped occurrences - even the ones that don't crash - out of principle?

Should I patch them all, as detailed before, with a commentary to make it explicit what is going on?

Should I leave them be?

[Vhati]

/Revised where I placed the blame for attackRight's 'return' value in comment:30.

[Vhati]

EDIT: [Mistakenly doubted my push1 patch earlier. Considered erasing the wrapper entirely, with ldi 01.]

[m-kiewitz]

No patch please.
Just add workaround entries.

[Vhati]

@m-kiewitz:

Just add workaround entries.

Okay. Only to suppress known crashes, or preemptively for all 17 places where this idiom is used?

[m-kiewitz]

Is it exactly the same code?

You can use placeholders as well for the table, do you have another easy spot to test?
Then please check what's the exact workaround information for it.

[Vhati]

@m-kiewitz:

Is it exactly the same code?

The idiom is always wrapping SetNowSeen(), given either a named instance or a global.

grep "SetNowSeen" *.sc | grep "|"

n041.sc:	(| (SetNowSeen global185) $0001)
n041.sc:	(| (SetNowSeen global195) $0001)
n041.sc:	(| (SetNowSeen global195) $0001)
n810.sc:	(| (SetNowSeen global185) $0001)
n830.sc:	(| (SetNowSeen global195) $0001)
n830.sc:	(| (SetNowSeen revenant) $0001)
n835.sc:	(| (SetNowSeen global195) $0001)
n835.sc:	(| (SetNowSeen global195) $0001)
n835.sc:	(| (SetNowSeen global195) $0001)
n840.sc:	(| (SetNowSeen global195) $0001)
n840.sc:	(| (SetNowSeen global195) $0001)
n855.sc:	(| (SetNowSeen horror) $0001)
n870.sc:	(| (SetNowSeen global195) $0001)
n870.sc:	(| (SetNowSeen global195) $0001)
n870.sc:	(| (SetNowSeen global195) $0001)
n870.sc:	(| (SetNowSeen nectar) $0001)
n870.sc:	(| (SetNowSeen global195) $0001)

Those similar lines are all void funcs, wrapped in a bitwise OR, and inserted among if-block conditions.

In context, the condition preceeding each varies: parameter truth/comparisons, global truth/comparisons. In places there are multiple instances of the idiom in a row, for different objects.

My expectation:

• When named, the object is stacked up via lofsa+push, and ScummVM will crash on invalid arithmetic.
• When global, it'd be stacked up via lsg, and ScummVM *might* crash depending on whether acc's last use - in the previous condition - held an object.
• This was someone's clever hack exploiting the unsafe nature of the original interpreter. ScummVM can only tolerate it sometimes by accident.

script 41 - xSlash::doit(), xDuck::doit(), xParryLow::doit()
script 810 - slash::doit()
script 830 - revenantForward::doit() twice
script 835 - doRSlash::doit(), doLSlash::doit(), tailAttack::doit()
script 840 - doLSlash::doit(), doRSlash::doit()
script 855 - wipeSpell::doit()
script 870 - attackLeft::doit(), attackRight::doit(), headAttack::doit(), hurtMyself::changeState() twice

[m-kiewitz]

I guess a script patch is really better in this case.
You can apply the same patch per script multiple times.

[Vhati]

@m-kiewitz:

I guess a script patch is really better in this case.

Okay. Will do.

[m-kiewitz]

Simply set the number after the description to the amount of times the patch is supposed to get applied for the current script.

That way we will use the minimum amount of text lines.

[Vhati]

​Pull Request: SCI32: Fix QFG4 conditional void calls

[Vhati]

SavedGame (CD) - Combat, Badder

[Vhati]

SavedGame (CD) - Combat, Chernovy

[Vhati]

SavedGame (CD) - Combat, Necrotaur

[Vhati]

SavedGame (CD) - Combat, Pit Horror

[Vhati]

SavedGame (CD) - Combat, Revenant

[Vhati]

SavedGame (CD) - Combat, Vorpal Bunny

[Vhati]

SavedGame (CD) - Combat, Wraith

[Vhati]

SavedGame (CD) - Combat, Wyvern

[Vhati]

/Collecting saves for floppy combat...

[Vhati]

SavedGame (Floppy) - Combat, Badder

[Vhati]

SavedGame (Floppy) - Combat, Chernovy

[Vhati]

SavedGame (Floppy) - Combat, Necrotaur

[Vhati]

SavedGame (Floppy) - Combat, Pit Horror (hacked)

[Vhati]

SavedGame (Floppy) - Combat, Revenant

[Vhati]

SavedGame (Floppy) - Combat, Vorpal Bunny

[Vhati]

SavedGame (Floppy) - Combat, Wraith

[Vhati]

SavedGame (Floppy) - Combat, Wyvern

[Vhati]

@m-kiewitz:

Is the exact same error triggered in floppy?
Just to be sure.

Yes. Same error. And the patch makes it go away.

[m-kiewitz]

kSetNowSeen in this game does not modify ACC.
This has been working correctly and is also still working correctly.
In these specific cases ACC already holds an object, which is not removed and which then causes this arithmetic operation fault by us.

In a sense it's valid script code, but for us it isn't.
We could add workarounds anyway, or better patch it, so that it doesn't do this anymore.

[bluegr]

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