ScummTR corrupts MONKEY1-VGA "v1.0" in ScummVM

dwatteau commented 2 years ago

Summary

Commit dc94b2a5bafdeba50f4e59dd91278610899edd8a added a workaround for the following duplicate offset in MONKEY1-VGA-FR:

ERROR: Duplicate offset in index: 0x18E82 in room 59

Indeed, the commit above let one use ScummRP/ScummTR on MONKEY1-VGA-FR (although I'm not completely sure we're removing the right duplicate yet, see issue #31), but doing so corrupts the game in ScummVM:

scummtr -g monkey -of tmp.txt
scummtr -g monkey -if tmp.txt

# Or:
scummrp -g monkey -od DUMP
scummrp -g monkey -id DUMP

In DOSBox, this screen is OK, but ScummVM tends to be more robust than the original interpreter, so we're probably really corrupting something (I haven't done a full game play yet).

I can't compare this with the original scummtr.exe 0.4.0 release, because this version had the Duplicate offset in index error above. But, recompiling ScummTR 0.4.1 with commit dc94b2a5bafdeba50f4e59dd91278610899edd8a on top of it in Visual C++ 2005 Express (which should be quite close to an original release) has the same behavior, so I don't think that it's because of a recent regression.

This looks quite similar to issue #45, but I do have the std::stable_sort fix, and using an older compiler doesn't seem to change anything.

Impacted games

The Secret of Monkey Island

ScummTR versions

Current Git HEAD

Relevant output (error messages, warnings, or any useful info)

No response

I own a legitimate game

[X] I promise that my game does NOT come from an illegal source, such as "abandonware" websites

dwatteau commented 2 years ago

Comparing the game data files before a simple scummrp reimport show that only 000.LFL is modified, in 4 bytes:

--- 000.LFL.original
+++ 000.LFL.modified
@@ -219,7 +219,7 @@
 00000f20  00 00 00 00 00 00 00 3c  c2 67 00 00 0b 67 d9 00  |.......<.g...g..|
 00000f30  00 2d 84 79 00 00 0e 34  82 00 00 0a bb ae 01 00  |.-.y...4........|
 00000f40  0b a1 de 00 00 0a 9f b0  01 00 3a a4 83 01 00 2d  |..........:....-|
-00000f50  7a db 00 00 55 97 e0 00  00 3b[82 8e 01 00]3b d8  |z...U....;....;.|
+00000f50  7a db 00 00 55 97 e0 00  00 3b[ff ff ff ff]3b d8  |z...U....;....;.|
 00000f60  9b 01 00 25 e0 42 01 00  25 8e 58 01 00 35 14 2b  |...%.B..%.X..5.+|
 00000f70  01 00 35 11 49 01 00 35  84 1c 01 00 35 2f 25 01  |..5.I..5....5/%.|
 00000f80  00 3b bb fc 01 00 00 00  00 00 00 00 00 00 00 00  |.;..............|

The ff ff ff ff probably come from setting the offset to -1 in the workaround. I don't know if it's a valid value there; I doubt it, although the DOS version does appear to tolerate it (but I haven't played a full game to confirm it; the duplicate costume is only triggered at the very end of the game, when LeChuck sends you through the grog machine).

It looks like ScummRp may have a special treatment when an offset is set to -1 for duplicate scripts, but maybe something is wrong/missing when dealing with duplicate costumes.

By the way, my French version (with the bug) shows Version 1.0 in Ctrl-V, while my English version (without the duplicate costume) shows Version 1.1. So this problem is maybe just limited to the earliest VGA floppy versions, but we'd still like to have a proper workaround for them, as as far as I know there's no 1.1 French FLOPPY-VGA version.

Anyway, I've pushed commit 4f92fe2520ca5ff52c6bd0a8755b42c4b1ef0565, so that ScummTr 0.5.0 just rejects this version, instead of corrupting the game. I just don't know how to fix this at the moment, and I'd like to release ScummTR 0.5.0 this month. Since no earlier version of ScummTr had support for these variants anyway, it's not going to be a regression…

dwatteau commented 2 years ago

Looks like this bug may exist in all "v1.0" versions of the VGA game:

https://forums.scummvm.org/viewtopic.php?p=89717#p89717

dwatteau commented 2 years ago

I actually wonder if this corruption doesn't just come from the parsing problem which causes #54 and #56…

EDIT: If you unpack/repack MONKEY1-FLOPPY-VGA with ScummPacker, the same glitch is triggered.

dwatteau commented 2 years ago

FWIW, regarding the "glitch" above, if the issue only shows up in ScummVM, that's maybe because the 000.LFL index file gets changed a bit, and so ScummVM won't recognize it as Monkey1 VGA but Monkey1 EGA (which is wrong), since its fallback detection code is currently a bit limited.

So actually ScummPacker's fix for the index problem in Monkey1 VGA is maybe OK, but the ScummVM detection bug just made me misdiagnose the issue.

gabberhead commented 1 month ago

i have the same problem with the german vga dos version. dosbox is working, scummvm gives the same glitches. i try to maake a re-translation of the game. but it can not be played with scummvm because of that. the german ega version is working fine :)

dwatteau commented 4 weeks ago

Hmm yeah that's maybe another reason why improving the fallback detection code in ScummVM's engines/scumm/detection_internal.h would be helpful.

I don't think there's much interest from the rest of the ScummVM test in working on this improvement, but I do see the cases where it could benefit from it.

And since there are several reasons for me to do this, it increases its priority in my todo-list…

dwatteau / scummtr