Author Topic: Found a bug in QFG1 EGA and VGA.. (and fixed it)  (Read 2933 times)

0 Members and 1 Guest are viewing this topic.

Offline 8bitKittyKat

Found a bug in QFG1 EGA and VGA.. (and fixed it)
« on: August 08, 2022, 09:17:55 AM »
So, the other day I was playing QFG1, the VGA version, for the first time in a few years, and ran into a bug I'd never noticed before.  I was in the kobold's cave, a bit low on mana, but still had enough to cast a spell.  Except the game told me I didn't have enough mana for the spell when I tried to cast it.  What the heck?  So I gave up and left, restored my mana and continued on playing.  But I never forgot about that odd bug.  I completed the game, but came back to it after I was done to poke around a bit, and see if I could figure out what had happened.

My first discovery was that a couple of my spells cast in the kobold's cave cost twice the normal amount of mana.  Out of curiosity, I decided to see if the EGA version had a similar quirk.  This is where things got interesting.  Not only did it have the same bug, but it happened with more spells.  So I dug into the SCI Decompilation of QFG1EGA with SCI Companion and (after wayyy too much time looking at the code and trying to figure out exactly where the bug was occurring) eventually discovered that there were 11 separate room scripts that had this bug.  Best I could gather from comparing code between rooms where spells do something unique came down to the unique spell code being nested within a block of code that checked if the spell could be cast.  The rooms where this bug didn't occur put the code that checked if the spell could be cast inside each of the spells coded for the room.  I'm admittedly just a novice programmer, so I'm not exactly sure why this distinction made all the difference, but I found my solution and fixed up the scripts for the 11 rooms this bug occurred in.  The VGA version is a bit of a different beast with a similar bug, but it seems to occur less frequently.  I admittedly haven't fully figured out the bug for the VGA version yet, but I'm working on it.

I've posted my work (forked from ErikOakford's SCI Decompilation) up on Github if anyone wants to take a look.  It hasn't been fully tested yet but with the limited testing I did on my own, everything seems to be working fine with spells now.  You can also download a zip of patch files for the room scripts there, but I'll attach it here as well.  Also, I was working with v1.200 of QFG1EGA so I'm not sure how compatible it would be with any other version, or the original Hero's Quest release.

If anyone more knowledgeable with SCI than me has any insight into the bug, I'd love to hear it.  The original code in theory looks like it should work, and it does look cleaner, but it just doesn't work the same.

Link to the Github repo!
« Last Edit: August 08, 2022, 09:21:17 AM by 8bitKittyKat »



Offline 8bitKittyKat

Re: Found a bug in QFG1 EGA and VGA.. (and fixed it)
« Reply #1 on: August 08, 2022, 09:45:45 AM »
Also, if anyone wants to see the bug themselves before patching the EGA version, the best place I found to test was at the waterfall.  Check your mana, cast fetch, and then check it again.

Offline Charles

Re: Found a bug in QFG1 EGA and VGA.. (and fixed it)
« Reply #2 on: August 08, 2022, 07:42:09 PM »
That is really awesome!

I hope to be able to take the time and really see what’s going on. If I had to guess I’d say for the EGA one, the room-specific code isn’t claiming the Said event as finished, so it runs the room specific code first, then runs the generic code, thus double-casting the spell.

Amazing find, and fix! Great work.

Offline 8bitKittyKat

Re: Found a bug in QFG1 EGA and VGA.. (and fixed it)
« Reply #3 on: August 09, 2022, 10:58:40 PM »
That is really awesome!

I hope to be able to take the time and really see what’s going on. If I had to guess I’d say for the EGA one, the room-specific code isn’t claiming the Said event as finished, so it runs the room specific code first, then runs the generic code, thus double-casting the spell.

Amazing find, and fix! Great work.
Cool find. Here's what I see in the buggy QfG1EGA code.

When "(if (CastSpell spell)" is used before "(switch spell", it's always casting the spell, even if there isn't a switch case for the spell the player typed. So it gets cast twice in these rooms, once by the (CastSpell spell) procedure and then again in Main's default spell handling. Any spell that does have a switch case in the buggy rooms, like (DETMAGIC in bearCave, *should* only consume mana once. This is because the event gets claimed, not sent back to Main for handling like spells that aren't matching a switch case do.
I was looking at the code again earlier because apparently I missed a couple rooms, and was beginning to think it might be something like that.  Thanks for confirming it!
« Last Edit: August 09, 2022, 11:20:05 PM by 8bitKittyKat »

Offline Charles

Re: Found a bug in QFG1 EGA and VGA.. (and fixed it)
« Reply #4 on: August 10, 2022, 11:44:10 AM »
I've looked pretty thoroughly through the code now, and I think I know exactly what's going on. Some of this might help you fix the VGA bug, because it's probably the exact same thing going on there.

It's not effecting all spells in the rooms you've identified. It's not even actually causing problems in all the rooms you fixed, so you can scale back your fixes a bit.  That said, I think your fix is the best way to do so.  The basic problem is exactly as you found -- the (switch spell) is happening after (CastSpell spell).

The basic order of things is that in Main, the glory::handleEvent procedure runs constantly, and for speechEvents (of which typing "Cast anything" is one) it tells the specific room to do what it needs to do first:
Code: [Select]
(super handleEvent: event)
If the room code has done what it needs to do, it will set (event claimed: TRUE). I.e. it did the job, stop there.

So, what should be happening is that the room specific stuff should check only for spells that do something unique in that room, and if so cast the spell (via (CastSpell), do the animations, whatever, and claim the event.

Instead, what's happening is that those buggy rooms are casting the spells first thing with a (CastSpell) call -- that improves your skill with the spell, drains your mana, and flags the event as claimed, so nothing else will try to claim the spell -- then it's checking if it should actually do anything specific with the spell.  If so, do that animation, show that text, otherwise if it's a spell we're not doing anything special with reset the event claimed flag back to FALSE, so the main script can deal with it.

But we're already cast the spell... so we're casting it twice.  Once in secret, then finally with the animation and text.

There's two ways to fix it... one, as you've already done is to swap around the switch statement and the CastSpell statement.  So you check to see which spell the player typed in, and only CastSpell if it's one this room recognizes... otherwise, leave the claimed flag as false and let the main script handle it.  You can see the original programmers already did this in some rooms, like the Stags in Rooms 77 and 78.

The other way is to add cases for all 7 spells, and don't let the main script handle any of that.  That's actually the approach done in the RandomEncounter.sc script.  So that room you didn't actually need to switch the order around, because it's not leaving the event unclaimed for the main script to handle.  It's not my favorite approach though, because it means needlessly copying code into all the rooms. Something the original programmers did a fair bit... there's like 5 places where they handle the Zap spell, and it does the exact same thing in each... scratch that, almost the exact same thing. They slightly changed the text for when you're not holding a weapon in all but the Main script.

Best I can figure, there were 31 separate instances that check if the user tried to cast a spell (calls to the SaidCast function), not counting the one in Main.  So to be sure, I think you'd need to check each of those rooms, and see if they're casting the spell first, then accounting for a fraction of the spells then flagging the event as not claimed.  Those would be your problems rooms, and I suspect it'll be the exact same in the VGA version.

Offline 8bitKittyKat

Re: Found a bug in QFG1 EGA and VGA.. (and fixed it)
« Reply #5 on: August 10, 2022, 02:55:45 PM »
When I started working on my bug fix I did a search for any scripts with "if (CastSpell spell)" outside of a switch and restructured those bits of code, so yeah, I probably didn't need to every instance.  I'll go through and see what rooms were unnecessary.  Also I did see the redundant spells in some rooms.. I had a bit of an itch to tidy it up when I saw it, but I had to remember I'm trying to patch a bug and not needlessly rewrite a whole bunch of code!  That would be.. a whole other beast.

Thanks for the thorough explanation.  I'm very new to SCI and still a novice programmer so it's appreciated. I'll be fixing up the EGA patch soon and doing more testing on it.  Then on to the VGA patch.

Offline OmerMor

Re: Found a bug in QFG1 EGA and VGA.. (and fixed it)
« Reply #6 on: August 10, 2022, 03:24:25 PM »
That's a very interesting bug!

Have you considered writing a patch for ScummVM as well?

Offline 8bitKittyKat

Re: Found a bug in QFG1 EGA and VGA.. (and fixed it)
« Reply #7 on: August 11, 2022, 07:00:58 AM »
Just did some quick testing and it's looking like the bug isn't in v1.000 of Hero's Quest.  I don't have a decompilation of that version aside from what I can get through SCI Companion's tools unfortunately.

That's a very interesting bug!

Have you considered writing a patch for ScummVM as well?
ScummVM works with SCI patch files, which is what's I've included in the zip, so it should just work if you put the files in the game folder.
« Last Edit: August 11, 2022, 07:05:46 AM by 8bitKittyKat »

Offline OmerMor

Re: Found a bug in QFG1 EGA and VGA.. (and fixed it)
« Reply #8 on: August 11, 2022, 09:51:21 AM »
ScummVM works with SCI patch files, which is what's I've included in the zip, so it should just work if you put the files in the game folder.

Of course, but ScummVM also adds their own internal patches to SCI script bugs so you won't have to manually patch your game.

You can take a look at one such fix: https://github.com/scummvm/scummvm/commit/87afecfe1394398c18324ec06dc628eb2e923e9d

It's a bit involved to fix it in ScummVM but you can try seek the devs (especially SluiceBox) on discord for help.

Offline 8bitKittyKat

Re: Found a bug in QFG1 EGA and VGA.. (and fixed it)
« Reply #9 on: August 11, 2022, 10:17:08 AM »
Of course, but ScummVM also adds their own internal patches to SCI script bugs so you won't have to manually patch your game.

You can take a look at one such fix: https://github.com/scummvm/scummvm/commit/87afecfe1394398c18324ec06dc628eb2e923e9d

It's a bit involved to fix it in ScummVM but you can try seek the devs (especially SluiceBox) on discord for help.
Ah, I didn't know they had their own patches in the engine.  Personally I use DOSbox 99% of the time.  I'm not sure I want to add additional work by writing a patch for ScummVM too, especially since I'm not done yet and this bug is proving to be stranger than I first thought, but my github repo is public and if when I'm finished someone wants to port my work, that's fine by me.


SMF 2.0.19 | SMF © 2021, Simple Machines
Simple Audio Video Embedder

Page created in 0.034 seconds with 22 queries.