Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SCI32: Fix QFG4 fortune teller crash #1421

Merged
merged 2 commits into from Dec 1, 2018

Conversation

Projects
None yet
5 participants
@Vhati
Copy link
Contributor

Vhati commented Nov 27, 2018

Fixes room disposal when leaving Magda's wagon, bug #10778

// When the fortune teller's wagon room is disposed, it attempts to call
// hero::show(), hero has a null "plane" property, and ScummVM crashes.
//
// The problematic line was removed in the CD edition. We remove it, too.
//
// Note: This patch is a workaround. The floppy edition SSCI did not crash, and
// its implementation of AddScreenItem() should be checked to find out why.

@Vhati Vhati force-pushed the Vhati:qfg4_magda_disposal branch from 0d893c0 to d39384b Nov 27, 2018

@m-kiewitz

This comment has been minimized.

Copy link
Contributor

m-kiewitz commented Nov 27, 2018

@lskovlun
Sorry for contacting you again, but could you please check in your .IDB what Sierra did in a case like this? Did they error out/crash, or did they ignore a case like this?

I just want to make sure that Sierra did it that way. It wouldn't surprise me, because quite often Sierra did not check for certain problematic conditions, as you of course also know.

@sluicebox

This comment has been minimized.

Copy link
Member

sluicebox commented Nov 27, 2018

Haha, coincidentally I just submitted some script patches for the fortune teller in Gabriel Knight 1, then immediately confused them with this PR. Sorry if that confuses anyone else!

@lskovlun

This comment has been minimized.

Copy link
Member

lskovlun commented Nov 27, 2018

@m-kiewitz It looks like this is a rather deeper bug. I get the same crash just from pressing F5 (but then again, I have a nasty habit of leaving stray patch files around... it may be due to that). As @Vhati says, none of this occurs in the CD version. I would like to investigate a bit more before we merge this.

@Vhati

This comment has been minimized.

Copy link
Contributor Author

Vhati commented Nov 27, 2018

@lskovlun:

I get the same crash just from pressing F5 (but then again, I have a nasty habit of leaving stray patch files around... it may be due to that).

Pressing F5 in the fortune teller room does not crash for me, with master + this commit, or with the Nov 13 daily build.

@m-kiewitz

This comment has been minimized.

Copy link
Contributor

m-kiewitz commented Nov 28, 2018

@bluegr
Lars wanted to check this problem in depth. It shouldn't have been pushed yet.

@lskovlun

This comment has been minimized.

Copy link
Member

lskovlun commented Nov 28, 2018

Well, the saving crash only occurs if you've got the original save/load dialogs turned on, which is an option. So there's a workaround for that already. Still, it's interesting that it too ends up calling AddScreenItem on a screen item with no plane. In the saving case, because an edit widget has not been inited yet and therefore has a zero plane (the responsible method actually gets called twice, which is part of the problem); in the gypsy case, because the hero view has been disposed and therefore has had its plane reset. Sequencing issues in both cases.

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Nov 28, 2018

Interesting. So, if the save/load dialog crashes as well this is more complicated than it looks. If we do manage to find the root cause for this crash, and ensure that we won't get any related crashes in that scene, we can merge this

@Vhati

This comment has been minimized.

Copy link
Contributor Author

Vhati commented Nov 29, 2018

@bluegr:
I took lskovlun to be saying there are two different bugs of a similar kind ("AddScreenItem on a screen item with no plane"), that should be patched separately?

  • "In the saving case, because an edit widget has not been inited yet and therefore has a zero plane (the responsible method actually gets called twice, which is part of the problem)"
  • "in the gypsy case, because the hero view has been disposed and therefore has had its plane reset."

There's another crash like this when Cranium prompts for potion formulas (#10773). I that case, it disposed items, displayed copy protection, then tried to show those items again afterward.

@sluicebox

This comment has been minimized.

Copy link
Member

sluicebox commented Nov 30, 2018

I have a script patch for #10773 which I'm holding off on until AddScreenItem gets figured out. If we end up wanting to keep the current behavior of erroring on zero-planes I'll submit it. It would be good to know why this doesn't cause problems in SSCI.

What's interesting about that bug is that even though it didn't appear to cause problems in the floppy version on SSCI, they did fix the script in CD so that they wouldn't use disposed items, so someone noticed.

@digitall digitall requested review from bluegr and m-kiewitz Nov 30, 2018

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Dec 1, 2018

The SCI script code that you're removing with this patch is indeed questionable. It seems that they tried to move a destroyed object (Ego) outside of the screen coordinates, to make it disappear. Perhaps it was added during SCI32 development, as QFG4 floppy was one of the first titles that was released with the SCI32 engine.

Thus, I do agree that removing this line is indeed a good idea. What I don't understand is why the original interpreter allowed modification of disposed objects.

I propose to merge this patch, since the rationale for adding it is correct. However, it should be marked as a workaround, and that we need to figure out if QFG4 had any differences in the implementation of AddScreenItem, which allowed such buggy scripts to work without throwing errors.

I propose to follow the same approach with the bug with Dr. Cranium's potions.

Overall, these two cases (this one, and Dr. Cranium) are indeed suspicious

@m-kiewitz

This comment has been minimized.

Copy link
Contributor

m-kiewitz commented Dec 1, 2018

Thus, I do agree that removing this line is indeed a good idea. What I don't understand is why the original interpreter allowed modification of disposed objects.

That Larry 5 problem was the exact same.
Destroyed an object, created new one and excepted the data of the old object to be in the new object.
That's probably some kind of "feature"

SCI32: Fix QFG4 fortune teller crash
Fixes room disposal when leaving Magda's wagon, bug #10778

@Vhati Vhati force-pushed the Vhati:qfg4_magda_disposal branch from a2ffaf6 to db61593 Dec 1, 2018

@Vhati

This comment has been minimized.

Copy link
Contributor Author

Vhati commented Dec 1, 2018

@bluegr:

it should be marked as a workaround, and that we need to figure out if QFG4 had any differences

Noted.

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Dec 1, 2018

Thanks! Merging

@bluegr bluegr merged commit e6dbdb8 into scummvm:master Dec 1, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@Vhati Vhati deleted the Vhati:qfg4_magda_disposal branch Dec 4, 2018

@lskovlun

This comment has been minimized.

Copy link
Member

lskovlun commented Dec 27, 2018

Root cause analysis: (putting it here because the bugs are closed)

SCI32 creates a number of internal planes for various purposes. In most interpreters, these planes have ID numbers starting at 20000. These planes do not have a corresponding script object, and are denoted by (scummvm) in the output of the plane_list command. One such plane (and the only one relevant to GK1 and QfG4) is called initPlane in the scummvm source code. Most memory handles are smallish, so starting at 20000 makes conflicts with script-created planes much less likely (but not impossible). In the early interpreters of GK1 and QfG4 floppy, the numbering actually starts at zero. This means that operations on screen items with plane == 0 will operate on the initPlane implicitly. As more features were implemented that used this kind of plane, conflicts became likely, which probably explains why Sierra changed it. This also explains why they had to fix this bug even though it was asymptomatic in SSCI 2.0: As soon as they rebuilt the game with SCI2.1 for the CD release, the bug would have become symptomatic.

I have a potential fix in the pipeline (which just changes one numeric constant, gah) which also fixes the Dr. Cranium bug and the save dialog bug I referred to above, obviating the corresponding script patches. Outstanding issues are backward compatibility with existing savegames (it seems this is not going to be a problem) and addition of another version difference in our engine that allows us to select the plane numbering base instead of hardcoding it.

rsn8887 added a commit to rsn8887/scummvm that referenced this pull request Mar 20, 2019

SCI32: Fix QFG4 fortune teller crash (scummvm#1421)
Fixes room disposal when leaving Magda's wagon, bug #10778
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.