dwatteau / scummtr Goto Github PK

In ScummFont, glPalette had a single-byte change after the last "official" release of the tool:

https://github.com/dwatteau/scummtr/blob/main/src/ScummFont/scummfont.cpp#L101

This change was made in 2004, and I've checked that the last version of scummfont.exe does contain the previous palette. So, for the moment, that's the one I'm keeping.

I don't think this change was a temporary test change made in 2004, though. I think that it might be a real bugfix. For example, maybe we were generating slightly invalid BMP files, because of this. Or maybe it was invalid just for some font files.

I need to check the validity of our BMP files, and read the specification of the format.

Summary

With the Amiga version of Monkey1, from LRG's Monkey Island Anthology.

Trying to reimport the original text content, but with an added -A ao flag so that extra padding is inserted (and so that some bytes differ) will result in the following fatal error:

$ scummtr -g monkey -A ao -of test.txt
$ scummtr -g monkey -A ao -if test.txt
ERROR: LFLF 61 differently indexed from one file to another

This looks similar to issue #54, but the error message is different. As far as I can say, the Amiga version is the closest version to the VGA-DOS version.

This error also happens with the original 2003 scummtr.exe, so this isn't a regression.

Impacted games

The Secret of Monkey Island

ScummTR versions

v0.5.1, v0.4.0

I own a legitimate game

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

(On top of PR #39, because it already removes more similar noise.)

clang-tidy appears to complain about some call to virtual functions inside some of our constructors and destructors:

src/common/file.cpp:68: Call to virtual method 'File::close' during destruction bypasses virtual dispatch
src/common/file.cpp:222: Call to virtual method 'File::read' during destruction bypasses virtual dispatch
src/common/file.cpp:292: Call to virtual method 'File::_zap' during destruction bypasses virtual dispatch
src/common/file.hpp:533: Call to virtual method 'SeqFile::close' during destruction bypasses virtual dispatch
src/ScummRp/block.cpp:956: Call to virtual method 'LFLFPack::_init' during construction bypasses virtual dispatch
src/ScummRp/block.cpp:1053: Call to virtual method 'LECFPack::_init' during construction bypasses virtual dispatch
src/ScummRp/block.cpp:2327: Call to virtual method 'OldRoomV1::_init' during construction bypasses virtual dispatch
src/ScummRp/block.cpp:2467: Call to virtual method 'OldRoomV2::_init' during construction bypasses virtual dispatch
src/ScummRp/block.cpp:2510: Call to virtual method 'OldRoomV2::_ooBX' during construction bypasses virtual dispatch
src/ScummRp/block.cpp:2636: Call to virtual method 'OldRoomV3::_init' during construction bypasses virtual dispatch
src/ScummRp/block.cpp:2679: Call to virtual method 'OldRoomV3::_ooBX' during construction bypasses virtual dispatch
src/ScummRp/toc.cpp:41: Call to virtual method 'TableOfContent::_zap' during construction bypasses virtual dispatch
src/ScummRp/toc.cpp:46: Call to virtual method 'TableOfContent::_zap' during destruction bypasses virtual dispatch
src/ScummRp/toc.cpp:414: Call to virtual method 'GlobalRoomIndex::_zap' during destruction bypasses virtual dispatch

I don't think I can fix this myself, though. Not enough knowledge of best C++ practices.

Note to myself: I need to make sure that _checkDupOffset() doesn't remove the wrong duplicate when it finds one. Otherwise the game could be playable, until an internal script error happens under some particular circumstances…

Ideas on how to check this:

• Check against other variants not having the bug (other platforms, other languages…)
• Try reading the scripts through descumm
• Use another SCUMM resource explorer

On LOOM-EGA-FR, the original scummtr.exe works this way:

$ scummtr.exe -c -gp loom . -of tmp.txt
WARNING: Gap at 0x762F in 53.LFL

But scummtr from a1ab81e instead gives the following fatal error:

$ scummtr -c -gp loom . -of tmp.txt
ERROR: Bad OIv3 offset

so there must be an important regression somewhere.

Summary

If ones downloads the "DOS Short Non-Interactive Demo" (loom-dos-short-demo-en.zip) from the ScummVM demos, it will be rejected by ScummRp/ScummTr because of duplicate content in its index:

$ scummrp -g loom -od DUMP

WARNING: Gap at 0x5A92 in 01.LFL
ERROR: Duplicate offset in index: 0x5B15 in room 1.

Impacted games

Loom

ScummTR versions

v0.5.0, v0.4.2, v0.4.1, v0.4.0

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

No response

I own a legitimate game

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

Summary

There is a TurboGrafx-16/PC-Engine version of Loom (that ScummVM supports). An English and a Japanese version are available.

Unfortunately, ScummRp/ScummTr doesn't support it yet:

$ scummrp -L

[...]
loom        | Loom (EGA)                                         | 00.LFL
loomtowns   | Loom (FM-TOWNS)                                    | 00.LFL
loomcd      | Loom (VGA)                                         | 000.LFL
[...]

I do have the .LFL files (thanks to the ScummVM guide about this), but trying to force ScummRp to interpret them as the EGA/VGA/FM-TOWNS version doesn't work, there's always a parsing error.

It looks like we really need proper parsing support for it.

Impacted games

Loom

ScummTR versions

v0.5.0, v0.4.2, v0.4.1, v0.4.0

I own a legitimate game

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

Summary

With LOOM-EGA-EN (either the 1.0 or 1.1 version from 1990), if one just does this to the original game with ScummTR 0.4.2 on Windows:

# This just reimports the original text of the game with no modification
scummtr -g loom -of tmp.txt
scummtr -g loom -if tmp.txt

and then, if you start a new game, to the forest on the left, and click on the first tree hole, then a graphical glitch will appear in the original DOS interpreter, and ScummVM will plainly refuse it:

ScummTR 0.4.0 and 0.4.1 didn't have this problem on Windows.

Games

Loom

Versions

v0.4.2
current Git HEAD

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

No response

I own a legitimate game

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

https://www.jestarjokin.net/apps/scummimg/ (a very nice tool) says:

[for V1 resources] Note that scummrp also does not output the common colour palette; you can use the included v1col_extract and v1col_insert tools to get this information.

Here are the scripts (MIT too):

https://bitbucket.org/jestar_jokin/scumm-image-encoder/src/master/src/v1col_extract.py
https://bitbucket.org/jestar_jokin/scumm-image-encoder/src/master/src/v1col_insert.py

We could probably add this inside ScummRP.

Observed on macOS Mojave x86-64 with Clang 11.0.0, and OpenBSD/macppc 6.8 with Clang 10.0.1.

scummtr is approximately 30% faster with GCC than with Clang, when importing a translation.

GCC 10.2.0 and GCC 4.2.1 at -O2 are both faster than Clang at -O2 or -O3, so I don't think that it's a recent GCC improvement, since GCC 4.2.1 is from 2007.

There's no need for this set of tools to be extremely performant, but a reproducible 30% difference seems strange.

Possible reasons:

• Clang really is slower than GCC
• GCC enables a useful optimisation that Clang doesn't enable (but Clang was tested with -O3, too)
• we have some undefined behaviour somewhere, and GCC chooses to optimise this away, while Clang is more strict.

I'm told that Loom-EGA-EN currently can't be extracted because of this error:

$ scummtr -cw -gp loom . -of loom_ega_orig.txt
ERROR: Duplicate offset in index: 0x41FB in room 18

Should be similar to #18.

Summary

Trying to extract the text of MANIAC-V2-FR (from my DOTT CD) with the officia: ScummTR 0.5.0 release unfortunately shows a late regression :(

$ scummtr -g maniacv2 -c -l fr -of tmp.txt
ERROR: Bad OIv2 offset

A bit of git bisect shows that this regression appeared with the extra "safety" I added in commit 8322e6d. Looks like I was wrong...

Impacted games

Maniac Mansion

ScummTR versions

v0.5.0

I own a legitimate game

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

The original source of scummrp (and thus scummtr) currently fails to build on most Unix environments.

g++ gives the best error messages:

/src/ScummRp/file.hpp: In member function 'virtual void SeqFile<T>::open(const char*, BackUp&)':
/src/ScummRp/file.hpp:317:7: error: there are no arguments to 'is_open' that depend on a template parameter, so a declaration of 'is_open' must be available [-fpermissive]
  317 |   if (is_open() != _srcFile.is_open())
      |       ^~~~~~~
/src/ScummRp/file.hpp:317:7: note: (if you use '-fpermissive', G++ will accept your code, but allowing the use of an undeclared name is deprecated)
/src/ScummRp/file.hpp: In member function 'virtual void SeqFile<T>::close()':
/src/ScummRp/file.hpp:328:8: error: there are no arguments to 'is_open' that depend on a template parameter, so a declaration of 'is_open' must be available [-fpermissive]
  328 |   if (!is_open())
      |        ^~~~~~~
/src/ScummRp/file.hpp:330:18: error: '_size' was not declared in this scope
  330 |   if (_tmpSize < _size)
      |                  ^~~~~
/src/ScummRp/file.hpp:333:5: error: there are no arguments to 'seekp' that depend on a template parameter, so a declaration of 'seekp' must be available [-fpermissive]
  333 |     seekp(_tmpSize, std::ios::beg);
      |     ^~~~~
/src/ScummRp/file.hpp: In member function 'virtual File& SeqFile<T>::read(char*, std::streamsize)':
/src/ScummRp/file.hpp:354:10: error: there are no arguments to 'tellg' that depend on a template parameter, so a declaration of 'tellg' must be available [-fpermissive]
  354 |   gpos = tellg(std::ios::beg);
      |          ^~~~~
/src/ScummRp/file.hpp:355:18: error: '_size' was not declared in this scope
  355 |   if (gpos + n > _size)
      |                  ^~~~~
/src/ScummRp/file.hpp:356:64: error: '_path' was not declared in this scope
  356 |    throw File::UnexpectedEOF(xsprintf("Unexpected EOF in: %s", _path));
...

clang++ doesn't like this either. It's just fatal for it.

Actually, an old g++ 3.3.5 does accept this construct, though. It seems to be an invalid C++ contsruct, that g++ 3.4 and later just completely refuses. Strangely, cl.exe still accepts it without a single warning, even with MSVC 2019 in C++17 mode.

This should help fixing this:

• https://gcc.gnu.org/gcc-3.4/changes.html#cplusplus
• https://gcc.gnu.org/onlinedocs/gcc-3.4.0/gcc/Name-lookup.html

Happens when compiling with MSVC 19.27.29112 at /W3 on x64. Clang and GCC don't report them.

ScummFont

src\ScummFont\scummfont.cpp(487) : warning C6283: 'unsigned char * glFontBitmap' is allocated with array new [], but deleted with scalar delete.: Lines: 493, 487

seems legit and simple to fix (EDIT: done).

src\ScummFont\scummfont.cpp(548) : warning C6386: buffer overrun: accessing 'glFontBitmap', the writable size is 'maxWidth*maxHeight*numChars*1' bytes, but '2' bytes may be written: Lines: 479, 480, 481, 482, 484, 486, 487, 488, 493, 494, 496, 509, 510, 512, 514, 515, 521, 523, 532, 534, 535, 536, 538, 539, 540, 541, 543, 544, 546, 548, 549, 550, 551, 557, 546, 560, 544, 546, 548

ScummRP

src\ScummRp\block.hpp(105) : warning C26439: This kind of function may not throw. Declare it 'noexcept'. (f.6).

src\ScummRp\file.cpp(728) : warning C6001: using uninitialized memory 'buffer'.: Lines: 716, 718, 719, 723, 728

src\ScummRp\block.cpp(379) : warning C6001: using uninitialized memory 'subblock'.: Lines: 368, 370, 373, 379
src\ScummRp\block.cpp(871) : warning C6001: using uninitialized memory 'subblock'.: Lines: 845, 846, 847, 849, 850, 854, 855, 858, 861, 871
src\ScummRp\block.cpp(969) : warning C6001: using uninitialized memory 'subblock'.: Lines: 943, 944, 945, 947, 948, 952, 953, 956, 959, 969
src\ScummRp\block.cpp(1755) : warning C6001: using uninitialized memory 'subblock'.: Lines: 1629, 1630, 1631, 1632, 1633, 1635, 1636, 1641, 1660, 1661, 1755

While trying to do this with ScummFont, in a directory where there was no CHAR_* files:

for file in CHAR_000? ; do
    scummfont o $file $file.bmp && scummfont i $file $file.bmp
done

this error happened:

ERROR: Cannot open font file
ERROR: Cannot open BMP file

The problem is that I used &&, so it should have stopped right after the first scummfont process. Looks like we're returning 0 even when an exception happens. That's quite dangerous for scripting…

Need to check all the tools for this behavior, and fix it.

See for example this report:
https://zenhax.com/viewtopic.php?t=14391&p=62976

Using ScummTR with non-Latin alphabets is quite inconvenient.

For example, to extract the text from a Japanese SCUMM game:

$ scummtr -b -g loomtowns -of jpn-tmp-bin.txt
$ LC_ALL=C tr '\000' '\n' jpn-tmp-bin.txt | iconv -c -f SHIFT-JIS -t UTF-8 > jpn.txt

But the binary output is not convenient, and we even need to call iconv with -c (probably because of the internal SCUMM escape sequences such as \255\001 etc.).

Using scummtr -H (for hexadecimal output) is maybe a bit better, if you then use a script which can interpret the hexadecimal SHIT-JIS codes. Still, it's not very convenient.

Possible solutions (from the easiest ones to the ones requiring more effort):

• document an acceptable way of dealing with non-Latin alphabets, as is;
• find a way to output a raw SHIFT-IJS files right from ScummTR while keeping internal SCUMM escape sequences (i.e. a mix between -b and the pseudo ASCII-US table for v1/v2 games);
• add a flag which processes everything as UTF-8.

ScummRp::_gameDef contains a list of expected filenames, in order to use the right data to translate each game.

They were mostly DOS games, so the filenames in the table are all in uppercase. That's still fine today for (nearly) all Windows and macOS setups, because they still use a case-insensitive filesystem today.

But it's still possible to have a case-sensitive filesystem on them, and that's the default on most other systems (e.g. Linux, BSDs…).

The current workaround is just to have your local files named in DOS mode, that is all in uppercase. Maybe it would just be simpler if it remains this way.

But it might be convenient if our tools tolerated an A-Z => a-z substitution (only for game data filenames, and only for ASCII letters). ScummVM does this, too.

It should remain simple, though. We don't want the tools to do dozens and dozens of checks just for this.

An idea for an extra tool…

1. Extract the resources of the current game with scummrp -o
2. In each room, descumm each script
3. List every local script (LSCR) being called in this room
4. Compare this to the list of available local scripts in that room
5. If some local scripts are never called: print them, for further analysis.

For example, in MONKEY2, I discovered that LFLF_0075/ROOM/ENCD never starts the LFLF_0075/ROOM/LSCR_0201 script, which contains extra sentences from Wally (was it an oversight or a leftover, that's another question).

This small tool would help finding other potentially "forgotten" scripts in the games.

With my version of ATLANTIS-FLOPPY-FR, the binary mode appears to have problems:

$ scummtr -g atlantis -b -of tmp.txt
$ scummtr -g atlantis -b -if tmp.txt

gives:

ERROR: Empty lines are forbidden (line 37)

This also happens with the original scummtr.exe.

tmp.txt indeed contains \n and \0 bytes when exported, don't know if it's related…

ScummRp and ScummTr both fail on MONKEY1-FLOPPY-VGA-FR (not the EGA version, not the CD-ROM version) with the following error:

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

Original Win32 binaries also have this problem.

RoomPack::_checkDupOffset() in ScummRp/block.cpp has some code to deal with this.

Clang reports this:

src/ScummRp/file.cpp:701:12: warning: cast from 'char *' to 'uint32 *' (aka 'unsigned int *') increases required
      alignment from 1 to 4 [-Wcast-align]
        bufferd = (uint32 *)buffer;
                  ^~~~~~~~~~~~~~~~
1 warning generated.

Indeed, we should maybe use memcpy(), or just access to the buffer in a simple and portable way.

Tested with some games with V1 fonts, such as INDY3 PC VGA.

If you export a V1 font to .bmp, then modify it, and import it back to to the game, the new font is corrupted (in the game and in the new exported bitmap):

scummfont o 98.LFL 98.bmp
open -a preview 98.bmp # or anything that modifies its format a bit
bmptoppm 98.bmp |  ppmtobmp -windows -bpp 8 > fixed.bmp
scummfont i 98.LFL fixed.bmp
scummfont o 98.LFL fixed-but-now-corrupted.bmp

The corruption problem seems to be located somewhere in the saveFont() function.

Original scummfont.exe also has this bug (but then you need to deal with the -new file when reproducing the use-case above).

Happens with the original scummtr.exe as well.

For example with a talkie Monkey Island 2, such as Twang's French translation (scummtr -g monkey2 -w -of output.txt):

-\255\010\202\213\255\010\239\003\255\010\010\000\255\010\000\000\007J'aurais bien besoin\255\001de\016cette cr\138me revitalisante.\255\009\023\000
+\255\010\202\213\255\010\239\003\255\010\010\000\255\010\000\000\007J'aurais bien besoin\255\001\100\101\016cette cr\138me revitalisante.\255\009\023\000

Here, the de immediately after \255\001 inside the sentence is output as \100\101. That's still correct, but not very convenient.

(You can manually fix it and import it back without any problem, but it will get lost at the next output. Should be correctly output by default.)

Fixing this is potentially similar to what has been done in commit 3f040c6.

This was quickly tested on a ppc32 system, but it seems that FontXY and ScummFont have no endianness check, and so they just fail or give improper results on these systems.

Summary

There is a NES version of Maniac Mansion, but unfortunately ScummRp/ScummTr has not support for it.

(Of course, the .LFL files need to be properly extracted, first.)

$ scummtr -L

[...]
maniacv1    | Maniac Mansion (V1)                                | 00.LFL
maniacv2    | Maniac Mansion (V2)                                | 00.LFL
[...]

Well, actually…

/*
 * Game definitions
 */

const GameDefinition ScummRp::_gameDef[] =
{
        { "maniacv1", "Maniac Mansion (V1)", "00.LFL", "%.2u.LFL", GID_MANIAC,
          1, 0xFF, 0xFF, GTCFMT_8SEP16, BFMT_SIZEONLY, 4, GF_OLD_BUNDLE },
        { "maniacv2", "Maniac Mansion (V2)", "00.LFL", "%.2u.LFL", GID_MANIAC,
          2, 0xFF, 0xFF, GTCFMT_8SEP16, BFMT_SIZEONLY, 4, GF_OLD_BUNDLE },
//      { "maniacnes", "Maniac Mansion (NES)", "00.LFL", "%.2u.LFL", GID_MANIAC,
//        1, 0xFF, 0xFF, GTCFMT_8SEP16, BFMT_SIZEONLY, 4, GF_OLD_BUNDLE | GF_NES },

It looks like Thomas tried to work on it back then, but it's still incomplete and that's probably why it has been disabled by default (plus, Thomas probably tried to add support for it back in 2003, while ScummVM had support for it "only" around 2007).

Depending on an extract_mm_nes set of LFL files also means that our job is probably easier, now?

Impacted games

Maniac Mansion

ScummTR versions

v0.5.0, v0.4.2, v0.4.1, v0.4.0

I own a legitimate game

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

Summary

In the VGA 1.1 version (also seen in the 1.0 version) there is an issue with translating from ~ line 360 onwards

Removed (59, 0x13AB3) from the index
BUG: TableOfContent::merge: different roomIds

I have narrowed down the issue by taking the original exported text and changing just 1 character.
When you add or remove a even a single character there, it fails with this kind of error:

When compensating for the character removal/addition with an equal amount of characters on one of the next lines, the error does not show up.

in 1.1; line 362 (not counting the two new scummtr header lines):
I just knew you'd be back!\255\003I knew you wouldn't be able to get that little beauty out of your mind!\255\003Come on. Let's go take another look at her!
in 1.0; line 367:
chickens
This same behavior shows all the way until line 568 (The Sea Monkey in 1.1)
The next lines are fine again to translate/change length... until line 905 where the same kind of error starts (door in 1.1 - while 906 is also chickens as in 1.0... coincidence? Don't think so... In my translation, door had the exact same number of characters, so I got lucky.) Same wrong behavior until line 1033 (It will terrify you) - this is the last line in that section for the Voodoo lady, which also was problematic in the 1.0 game.

Another simple way to trigger the bug is to use the -A ao(v) option to export and reimport the original text, as it is likely to pad one of the lines in the sensitive blocks with @'s. This also renders that padding option unusable for this game as a side effect.

Note: for the EGA version (also 1.0 - but older), the -A aov option round-trip export is ok. [I still need to match my translation files before I can try with an actual translation on that version]

Impacted games

The Secret of Monkey Island

ScummTR versions

v0.5.1

I own a legitimate game

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

Here's what I get so far (to be updated)

openSUSE hosts the Open Build Service, aimed at easing the production and distribution of Linux packages:

https://openbuildservice.org

For now, ScummTR is still a niche tool that's quick and easy to build, and binaries archives are already provided for the most common systems, but providing real packages would nice for updates, testing more compiler versions and architectures, etc.

But I think that we'd only do this if it remains simple enough to maintain and to use.

ScummTR fails on MANIAC-PC-V2-EN (which can be bought on GOG, or extracted from the DOTT Remaster which is also on GOG), probably because of a script error:

$ scummtr -cw -l en -gp maniacv2 . -of tmp.txt

ERROR: Script error at 0x2449 in 07.LFL (roomOps) [007:SCv2#0059]

MANIAC-PC-V1-EN and MANIAC-PC-V2-FR don't have this problem.

descumm (from ScummVM-tools) also has troubles with this script:

$ descumm -2 ./DUMP/LFv2_0007/SCv2_0059

ERROR: do_room_ops_old: unknown subop 12!

Indeed, here's how the script is interpreted on MANIAC-PC-V1-EN:

[00EF] (62) stopScript(0);
[00F1] (49) faceActor(116,VAR_KEYPRESS);
ERROR: do_room_ops_old: unknown subop 12!

while MANIAC-PC-V1-FR looks correct:

[0115] (62) stopScript(0);
[0117] (D8) printEgo("Il est déjà plein."); 
[0128] (A0) stopObjectCode();

It looks like the erroneous script missed a D8 (printEgo()) opcode, and directly used a 49 byte (which is interpreted as as bogus faceActor() opcode) instead of the intended D849... sequence (starting a call to printEgo("I)...). It seems that adding the missing D8 byte would fix this, but then offsets & all must be fixed.

When just applying this to MANIAC-PC-V2, without any other change:

scummrp -gp maniacv2 . -od DUMP # export
scummrp -gp maniacv2 . -id DUMP # reimport the same dump

…scummrp seems to corrupt the game, because right after this, the game always starts in demo mode, at least in ScummVM. This problem also happens with the 0.4.0 scummrp.exe executable.

With a more verbose export, the following warnings appear:

17.LFL should actually end at 0x1F3B
Removed (33, 0x3E77) from the index
Removed (41, 0x160F) from the index
OIv2_0395 might actually be OIv2_0547 (unlikely)

but the first warning about 17.LFL is different on the French version of Maniac Mansion v2.

Removed (17, 0x1F3B) from the index
Removed (33, 0x3E77) from the index
Removed (41, 0x160F) from the index
OIv2_0395 might actually be OIv2_0547 (unlikely)

It appears that the original English v2 version of Maniac Mansion has more script bugs than the one already discovered in #12.

This ScummRP bug is potentially responsible for the fact that PR #14 still doesn't completely fix our ScummTR bug with the same variant of the game.

Seen with g++ 10.2.0, when building in release mode with -O2.

ScummRp/scummrp.cpp: In static member function 'static void ScummRp::_getOptions(int, const char**, const ScummRp::Parameter*)':
ScummRp/scummrp.cpp:528:57: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
  528 |   for (j = 0; pendingParams[j] != 0; pendingParams[j++] = 0)
      |                                      ~~~~~~~~~~~~~~~~~~~^~~
ScummRp/scummrp.cpp:523:7: note: at offset 17 to object 'pendingParams' with size 17 declared here
  523 |  char pendingParams[MAX_PARAMS + 1];
      |       ^~~~~~~~~~~~~

ScummTr/scummtr.cpp: In static member function 'static void ScummTr::_getOptions(int, const char**, const ScummRp::Parameter*)':
ScummTr/scummtr.cpp:464:57: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
  464 |   for (j = 0; pendingParams[j] != 0; pendingParams[j++] = 0)
      |                                      ~~~~~~~~~~~~~~~~~~~^~~
ScummTr/scummtr.cpp:459:7: note: at offset 17 to object 'pendingParams' with size 17 declared here
  459 |  char pendingParams[MAX_PARAMS + 1];
      |       ^~~~~~~~~~~~~

This could be a false positive, though. I'd need to check.

Summary

Commit dc94b2a 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 dc94b2a 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

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

Most environment produce scummtr and scummrp binaries which immediately give the following fatal error:

./scummtr -cw -gp atlantis . -of atlantis.txt
WARNING: Bad data was found and ignored at 0x0 in ATLANTIS.000

The old win32 binaries don't have this problem.

Known affected environments:

• clang++ 11.0.0 on macOS Mojave x64
• g++-10 on macOS Mojave x64
• g++ 3.3.5 on OpenBSD/i386 4.7
• g++ 3.3.5 on OpenBSD/amd64 4.7
• g++ 4.2.1 on OpenBSD/amd64 6.8
• clang++ 10.0.1 on OpenBSD/amd64 6.8

Known OK environments:

• MSVC 2019 cl.exe x86 build
• MSVC 2019 cl.exe x64 build
• g++-4.0.1 on macOS 10.5 ppc32
• g++-4.2.1 on macOS 10.5 ppc32
• g++-mp-4.8 on macOS 10.5 ppc32

Pretty strange list. There's probably some undefined behaviour, or some non-portable construction somewhere, which causes this?

Steam sells some LucasArts titles, such as Indy3, Indy4, Loom, The Dig or Sam & Max. Some of them are a bit different from the original floppy/CD releases (which is what GOG mostly uses).

The ScummTR tools predate the Steam versions, which appear to embed the index files (000.LFL and so on) into the main executables.

(For example, for my Steam version of Loom, 000.LFL is at offset 170464+8307 of the Loom macOS binary file.)

I don't think that the ScummTR tools should to find this content themselves, but we should at least document how to do this somewhere.

References:

https://github.com/symm/LAAExtract
https://github.com/scummvm/scummvm/blob/master/engines/scumm/detection_steam.h

Games to be checked:

• Loom (Steam/Windows)
• Loom (Steam/macOS)
• Indy3 (Steam/Windows)
• Indy3 (Steam/macOS)
• Indy4 (Steam/Windows)
• Indy4 (Steam/macOS)
• Sam & Max (Steam/Windows)
• Sam & Max (Steam/macOS)
• The Dig (Steam/Windows)
• The Dig (Steam/macOS)

There's a \x85 character in the Text::CT_V1EN array of ScummTR.

This file contains escape sequences for some ISO-8859-1 characters, such as \xa3 for a © symbol (original file was in Windows-1252, but I used escape sequences while converting it to UTF-8; it's also better for compiler portability).

The problem is that the \x85 value only makes sense in ISO-8859-1, not in Windows-1252. And nowadays, it's probably better to limit ourself to the identical subset between ISO-8859-1/Windows-1252 (this character isn't part of it).

Sometimes I do this, by mistake:

scummrp -g monkey2 -of tmp.txt

i.e. I call scummrp instead of scummtr.

The problem is that scummrp doesn't complain if you give it a -f tmp.txt option, even if it doesn't have any -f option.

So, we should probably error out when someone tries to use an unknown option, because it's quite likely that the user made a mistake.