@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.

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!

@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.

@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.

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

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.

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

@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.

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.

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

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"

@bluegr:

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

Noted.

Thanks! Merging

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.