Opened 4 years ago

Last modified 9 months ago

#9646 new defect

SCI: QFG1: Running on ice till health points depletion won't kill hero.

Reported by: OmerMor Owned by:
Priority: normal Component: Engine: SCI
Keywords: Cc:
Game: Quest for Glory 1

Description

First reported here:
https://twitter.com/darthhelmet86/status/796973009558913024

Appears to be a script bug.

When in room 58, and type "run", the hero will fall and his HP will decrease by 1 point. This can be repeated, but will never cause the hero to die.

Change History (8)

comment:1 by OmerMor, 4 years ago

This is reproducible in QFG1VGA as well.

comment:2 by OmerMor, 4 years ago

When typing "run" the ego will be assigned with the script egoRuns.
egoRuns::changeState calls proc0_36 in script 0 which is deducing damage from the hero's HP.
This procedure returns TRUE if the hero is still alive, but the return value is never observed in egoRuns.

comment:3 by OmerMor, 4 years ago

In QFG1VGA, it's pretty much the same.
The main difference is that the procedure that takes the damage is proc814_19 in script 814.

comment:4 by OmerMor, 4 years ago

Here's the content of the twitter report, by DarthWizard86:

Uhhhh still alive have 0 health. Running on the ice takes off 1 health till you have 0 then you don't die?
https://pbs.twimg.com/media/Cw9quUGUoAAUWBl.jpg

comment:5 by sluicebox, 10 months ago

I'm putting this bug "on notice" for being closed as WONTFIX if we can't come up with something in a reasonable amount of time.

This is a very real script bug. Omer did a great job in documenting this; they called their function that deducted health and then didn't write any code to test the return value and handle it, presumably, by calling the death function with an appropriate message.

So what are we to do? Which message should we display when dying? That question assumes there's even room to patch in all that new code, which we'd have to do twice for the original and the remake, and it's likely there's not enough room to do that in either.

I hope we can fix this! At the same time, I see that QFG1VGA calls proc814_19 frequently without testing the return value. I really like this game, but it seems like they screwed up basic stat stuff like this all the time.

comment:6 by OmerMor, 9 months ago

So what are we to do? Which message should we display when dying?

How about message 57 "That was a little too much for you. You collapse from exhaustion and die." ?

comment:7 by ZvikaZ, 9 months ago

I have added a new patching mechanism, hooks to VM, that allows adding new instructions.
It was merged at https://github.com/scummvm/scummvm/pull/2109

Now engines\sci\engine\vm_hooks.cpp contains a patch that fixes QFG1 (not VGA) according to @OmerMor's detailed description.

However, the question is how do we continue?
@sluicebox said that there are more scenerios like this at QFG1VGA, so I see two alternatives:

  • go over all these places, and add a patch similar to the one that I already did
  • or, patch proc814_19, and if needed, kill the hero from there (instead of checking all places that call proc814_19 for return value, and kill him if needed). However, maybe there are places that call proc814_19, and the hero *shouldn't* die, on purpose?

Also, it's not clear if there are more problems at legacy QFG1, or is it the only one.

@sluicebox, @OmerMor, I understand that you've already investigated this bug, so, what do you think?

Last edited 9 months ago by ZvikaZ (previous) (diff)

comment:8 by OmerMor, 9 months ago

Very nice work @ZvikaZ!

Patching proc814_19 seems wrong to me: if you kill the hero, you need context to show the right message to the player.

The right approach IMO is to go over each call-site of proc814_19, see if it does not check the return value, and handle it appropriately.

Note: See TracTickets for help on using tickets.