SCUMM: LOOM: Fix Hetchel flying with two heads in some EGA releases (Enhancement) by dwatteau · Pull Request #6470 · scummvm/scummvm

dwatteau · 2025-03-06T14:11:49Z

Summary

Some time ago, @eriktorbjorn mentioned on Discord that some EGA releases of Loom have that bug where Hetchel flies with two heads, for a few frames at the forge:

I can reproduce this problem in (at least):

the original English EGA 1.0 release
the original English EGA 1.1 release
the original English Macintosh release

It's been fixed by LEC in (at least) the following EGA releases, though:

original French EGA 1.2 release
Hebrew EGA 3.2 release (as sold by LimitedRunGames)

And then I tested the PC-Engine Japanese release, the FM-TOWNS English release, and the talkie CD release, and there the problem never appears.

Enhancement implementation

Erik suggested this short diff:

diff --git a/engines/scumm/actor.cpp b/engines/scumm/actor.cpp
index 8784d906255..78fd6fb63ed 100644
--- a/engines/scumm/actor.cpp
+++ b/engines/scumm/actor.cpp
@@ -3541,6 +3541,9 @@ void Actor::runActorTalkScript(int f) {
 
                _vm->runScript(script, 1, 0, args);
        } else {
+               if (_vm->_game.id == GID_LOOM && _number == 11 && _moving && (_vm->_currentRoom == 34 || _vm->_currentRoom == 39))
+                       return;
+
                startAnimActor(f);
        }
 }

With a few tweaks (that I can add here if necessary), I'd be OK with it, but since LEC had an official fix for it back then, it's maybe better to just copy their changes?

Here are the script changes between the EGA 1.1 and 1.2 releases, when Hetchel's speaking at the forge:

Script 34-88:

 (11) animateCostume(11,245);
 (80) breakHere();
 (80) breakHere();
-(13) ActorOps(11,[WalkAnimNr(6),StandAnimNr(6)]);
+(13) ActorOps(11,[WalkAnimNr(6),StandAnimNr(6),TalkAnimNr(6,6)]);
...
-(13) ActorOps(11,[WalkAnimNr(2),StandAnimNr(3)]);
+(13) ActorOps(11,[WalkAnimNr(2),StandAnimNr(3),TalkAnimNr(4,5)]);
 (11) animateCostume(11,3);
 (AE) WaitForCamera();
 (80) breakHere();
...
 (11) animateCostume(11,245);
 (80) breakHere();
 (80) breakHere();
-(13) ActorOps(11,[WalkAnimNr(6),StandAnimNr(6)]);
+(13) ActorOps(11,[WalkAnimNr(6),StandAnimNr(6),TalkAnimNr(6,6)]);
 (11) animateCostume(11,9);

Script 38-87:

 (01) putActor(12,288,60);
 (1E) walkActorTo(12,96,60);
 (AE) WaitForActor(12);
-(13) ActorOps(12,[WalkSpeed(8,4),InitAnimNr(6),WalkAnimNr(6),StandAnimNr(6)]);
+(13) ActorOps(12,[WalkSpeed(8,4),InitAnimNr(6),WalkAnimNr(6),StandAnimNr(6),TalkAnimNr(6,6)]);
 (1E) walkActorTo(12,48,60);
...

So, my PR tries to detect these ActorOps calls, and add the TalkAnimNr() options that LEC added above.

It works just fine with the EGA 1.0/1.1/Macintosh releases mentioned above. It doesn't cause any regression with the 1.2 French/Hebrew releases already having these script changes.

Screenshots

For reference, here's what the earliest EGA releases of Loom would display, in that case.

How to test this PR

Start any Loom release, but disable all Enhancements in the game settings
Load an early save where you have at least the distaff
Then you can cheat and move to that scene by doing:
- Debugger (Ctrl-D) > drafts learn, drafts (note the "Sleep" and "Reflection" lines), room 34
- Wake the boy with the Sleep draft (type the notes from right to left, in order to wake him up, instead)
- Then, use the Reflection draft on him
- Go to the right and enter the forge, then go to the right of it to meet the guy who'll put you in jail
- Sleep on the straw
- Wait for the dragon to appear and kill the boy, then Hetchel appears

Expected behavior:

with Enhancements off, either Hetchel's head glitches as in the screenshots above (and then your release is impacted), or it doesn't and then you have a fixed release
with Enhancements on: Hetchel's head should not glitch anymore. If it still does, or if you see any other issue, please report it.

dwatteau · 2025-03-06T14:14:35Z

Tests on the Atari or Amiga releases (English or other languages) could be useful, for example. I know they sometimes have their room or scripts reordered, and so I'd like to make sure it's working there as well.

Thanks!

bluegr · 2025-03-06T14:25:04Z

@dwatteau nice!

Would it be possible to move the workaround code in a separate function, so that it's cleanly separated from the normal code?
In general, it would be great if we could do this kind of separation for all workarounds

dwatteau · 2025-03-06T14:39:41Z

@dwatteau nice!

Would it be possible to move the workaround code in a separate function, so that it's cleanly separated from the normal code? In general, it would be great if we could do this kind of separation for all workarounds

Sure. I agree that the workarounds add noise to the main functions. But I wonder if we shouldn't move to a cleaner approach at some point, such as what the SCI engine does for patches, or a pseudo-.ips capability that would let us detach that from any deep engine logic.

Anyway, having dedicated functions for workarounds is a first step.

@bluegr: Do you want a single function for each workaround, on its own? Or should such workaround functions contain various similar workarounds?

eriktorbjorn · 2025-03-06T18:16:41Z

And then I tested the PC-Engine Japanese release, the FM-TOWNS English release, and the talkie CD release, and there the problem never appears.

I've tested with the English TurboGrafx-16 and Japanese FM Towns versions and the glitch, unsurprisingly, doesn't happen there either.

but since LEC had an official fix for it back then, it's maybe better to just copy their changes?

Sounds like a good idea to me. It never occurred to me to check the other versions that I own.

bluegr · 2025-03-06T21:15:57Z

@bluegr: Do you want a single function for each workaround, on its own? Or should such workaround functions contain various similar workarounds?

For now, let's start moving big chunks of code into separate functions, I.e. each workaround with a lot of business logic should be put into its own function, so that the workaround code will not disrupt reading the main engine logic

dwatteau · 2025-03-08T16:49:35Z

@bluegr: Do you want a single function for each workaround, on its own? Or should such workaround functions contain various similar workarounds?

For now, let's start moving big chunks of code into separate functions, I.e. each workaround with a lot of business logic should be put into its own function, so that the workaround code will not disrupt reading the main engine logic

Sure. Are you OK with the new PR update update, then?

(Though, I wonder how this approach is going to "scale"? For example, in those o5_actorOps workarounds, some need to be pre op-code, other have to be mid op-code, or post op-code (first iteration of the current PR). Some need to tweak local variables, other need to interrupt the control frow (break or return insertions), while others need the current control-flow to continue… So I don't know how we are going to have something really cleaner with these technical constraints.)

This debate is not the subject of this PR, though. But I guess at some point we'll reach the point where we'll want to have a cleaner implementation for SCUMM enhancements. Maybe they should mostly be written in a way where they don't interrupt the main engine logic at all, thanks to a better hot-patching, or dynamic resource patching mechanism. We call the Enhancements "workarounds" in the engine, because they're a bit dirty, I guess. A more elegant solution could probably be found (hence my thoughts about what SCI does), but I personally don't have the proficiency to submit such a solution :/

Anyway, as always, I digress… but I guess at some point this topic will come back.

…in some releases Some EGA releases of Loom have a bug where Hetchel may appear with two heads for a few frames, when she's talking as she's flying, when she appears at the forge to help Bobbin. I can reproduce this problem in the original English EGA 1.0, EGA 1.1 and Macintosh releases. It's been fixed by LucasArts in the French EGA 1.2 and in the Hebrew EGA releases (possibly others). The TG16/PC-Engine release, and all later 256-color releases, appear to be fixed by default. So, for the earliest EGA releases, bring the script changes that LEC made back then, i.e. some new `TalkAnimNr` options to some of the `ActorOps()` calls of scripts 34-88 and 38-87. Script 34-88: (11) animateCostume(11,245); (80) breakHere(); (80) breakHere(); -(13) ActorOps(11,[WalkAnimNr(6),StandAnimNr(6)]); +(13) ActorOps(11,[WalkAnimNr(6),StandAnimNr(6),TalkAnimNr(6,6)]); ... -(13) ActorOps(11,[WalkAnimNr(2),StandAnimNr(3)]); +(13) ActorOps(11,[WalkAnimNr(2),StandAnimNr(3),TalkAnimNr(4,5)]); (11) animateCostume(11,3); (AE) WaitForCamera(); (80) breakHere(); ... (11) animateCostume(11,245); (80) breakHere(); (80) breakHere(); -(13) ActorOps(11,[WalkAnimNr(6),StandAnimNr(6)]); +(13) ActorOps(11,[WalkAnimNr(6),StandAnimNr(6),TalkAnimNr(6,6)]); (11) animateCostume(11,9); Script 38-87: (01) putActor(12,288,60); (1E) walkActorTo(12,96,60); (AE) WaitForActor(12); -(13) ActorOps(12,[WalkSpeed(8,4),InitAnimNr(6),WalkAnimNr(6),StandAnimNr(6)]); +(13) ActorOps(12,[WalkSpeed(8,4),InitAnimNr(6),WalkAnimNr(6),StandAnimNr(6),TalkAnimNr(6,6)]); (1E) walkActorTo(12,48,60); ... I guess more official EGA 1.0 > 1.1 > 1.2 script fixes could be backported as enhancements...

bluegr · 2025-03-08T17:36:33Z

Looks good now, thanks!

Yeah, we'll have to find a cleaner long-term solution for such workarounds/patches, cause the SCUMM engine is full of them :(

Anyway, awaiting for the github action builds to finish, and merging

bluegr · 2025-03-08T20:38:14Z

Thanks, merging

dwatteau force-pushed the feat/scumm-loom-enhancement-fix-double-duck-head branch 2 times, most recently from dbad3c2 to 69abd97 Compare March 6, 2025 14:24

dwatteau force-pushed the feat/scumm-loom-enhancement-fix-double-duck-head branch from 69abd97 to baa6e7f Compare March 8, 2025 16:22

dwatteau force-pushed the feat/scumm-loom-enhancement-fix-double-duck-head branch from baa6e7f to 5a3f9a8 Compare March 8, 2025 17:34

bluegr merged commit 6eb9442 into scummvm:master Mar 8, 2025
8 checks passed

dwatteau deleted the feat/scumm-loom-enhancement-fix-double-duck-head branch March 9, 2025 12:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

SCUMM: LOOM: Fix Hetchel flying with two heads in some EGA releases (Enhancement)#6470

SCUMM: LOOM: Fix Hetchel flying with two heads in some EGA releases (Enhancement)#6470
bluegr merged 1 commit intoscummvm:masterfrom
dwatteau:feat/scumm-loom-enhancement-fix-double-duck-head

dwatteau commented Mar 6, 2025

Uh oh!

dwatteau commented Mar 6, 2025

Uh oh!

bluegr commented Mar 6, 2025

Uh oh!

dwatteau commented Mar 6, 2025 •

edited

Loading

Uh oh!

eriktorbjorn commented Mar 6, 2025

Uh oh!

bluegr commented Mar 6, 2025 •

edited

Loading

Uh oh!

dwatteau commented Mar 8, 2025 •

edited

Loading

Uh oh!

bluegr commented Mar 8, 2025

Uh oh!

bluegr commented Mar 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

dwatteau commented Mar 6, 2025

Summary

Enhancement implementation

Screenshots

How to test this PR

Uh oh!

dwatteau commented Mar 6, 2025

Uh oh!

bluegr commented Mar 6, 2025

Uh oh!

dwatteau commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eriktorbjorn commented Mar 6, 2025

Uh oh!

bluegr commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwatteau commented Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluegr commented Mar 8, 2025

Uh oh!

bluegr commented Mar 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dwatteau commented Mar 6, 2025 •

edited

Loading

bluegr commented Mar 6, 2025 •

edited

Loading

dwatteau commented Mar 8, 2025 •

edited

Loading