SCI: LB1: Commands lockup game when colliding with obstacles

[sluicebox]

The Colonel's Bequest contains over 20 commands which lockup the game if entered while you're colliding with an obstacle. The prime example is opening the upstairs secret passages with "move closet". I've seen old walkthroughs recommend saving before every attempt just in case. This bug affects all versions (DOS/Amiga/Atari ST) and also occurs with Sierra's interpreter.

I've written a script patch that fixes all instances of this.

Attached are saves for all versions in front of a closet. The speed is turned down which makes this easy to reproduce:

1. Walk towards the closet, which since you're already up against it will briefly switch Laura's view from standing to walking.
2. Quickly press a letter key to bring up the input window while the walking view is displayed. (before she goes back to standing)
3. Enter "move closet" or "open closet". Laura will turn around and the game will lockup.

These commands also lockup when entered while colliding:

"open door" at top-of-stairs closet
"look in doghouse"
"knock door" at cellar door
"give necklace" when Celie is in rocking chair
"open door" at dollhouse
"sit" at swing
"pull ring" / "ring bell with cane" at bell tower
"look holes" in secret passages
"open vault with crowbar" / "close vault"
"get crowbar"
"put valve in shaft"
"turn valve"
"flush toilet"
"look bottle with monocle"
"read newspaper" in attic
"tell lillian about gertie" in kitchen or study

Those are the ones I'm certain of and there might be more but the script patch fixes the root problem.

The root cause is code in CB1:doit that performs an elaborate check every cycle to see if Laura appears blocked and stops her motion if so. I don't know why this game needs this code to manage basic motion that the core classes should have covered but that's what introduces this edge case. This code doesn't appear in other SCI games I've looked at so only this game has this bug.

[sluicebox]

​https://github.com/scummvm/scummvm/pull/1353

[m-kiewitz]

I personally always didn't like making such generic patches, because there may be some special case somewhere that gets broken by it. Sierra games quite often have special cases somewhere that sometimes even rely on certain bugs.

This not only needs a review (obviously), but also intensive game testing for any regressions. LB1 especially is somewhat "open", so testing everything is probably not as easy as with other Sierra games.

If I remember correctly there are also multiple versions of the game. Both need to get tested. I'm not sure how many things Sierra changed for the platform ports.

The bugs are way too many to fix them all separately, so in this case generic patching makes sense.

I think I already fixed some situations with specific fixes, those can probably get removed after adding this patch.

[sluicebox]

I agree and I'm not wild about patching setMotion, it's risky, but yeah there are just too many. I've played through once but this one needs a lot of testing.

DOS, Amiga, and Atari ST's setMotion have the same byte code and from my tests with those versions all of this behavior, before and after the patch, is consistent.

Since this patch clears a signal flag the question is who depends on that flag's value and when. As far as I can tell from ScummVM source the interpreter only writes the flag in kDoBresen and never depends on its value. The scripts should be testing the flag through isBlocked but could be testing signal directly. The only other scripts that call isBlocked are 245 and 269. They test to see if Clarence or Gloria are blocked when playing billiards and if Jeeves is blocked when sweeping so that they can tell you to get out of the way. From my tests that behaves the same. It's trickier to search for any instances of directly testing signal for the flag but I've tried and haven't found any. So it seems to me that if this is going to have any side effects it would be with ego's movement.

From what I've seen this is a separate bug from the movement lockups that have already been fixed. I tried disabling the armor script patches and that lockup still occurred. The armor scripts get stuck in an infinite movement loop whereas this bug is CB1:doit terminating a script's movement.

[bluegr]

I've reviewed this, and merged it. With a first glance, there don't seem to be any obvious regressions, but the game will need a full playthrough with this change. There wasn't any other good way of testing these changes, than merging this patch. If we find that this change breaks something in the game, we can revert it.

Thus, we should mark the game as needing a full playthrough for the next release.

Closing as resolved, nice work again! :)