[Scummvm-devel] descumm defects/patches

[Laurence Dougal Myers]

Hi all,

I'm Laurence Myers (aka jestar jokin). I've been contributing technical
information on SCUMM to the wiki. I have also written a tool called
Scummbler that will compile SCUMM scripts as output by descumm.

I have found a few issues in descumm that I would like to fix, but I first
wanted to see whether these changes would be acceptable, before I do too
much work. The issues are all related to versions of SCUMM < 6, since I
imagine the new bytecode decompiler would handle SCUMM >= 6. Here are
issues that I would like to fix, with the benefit of making descumm
scripts easier to parse by a machine:
- Inconsistent usage of semicolons.
- Some instructions have an extra closing parenthesis, or a missing
closing parenthesis.
- Instructions like "beginOverride" and "beginCutscene" do not have any
parentheses.
- "startScript" does not output enough information (it's missing flags
indicating if the script should be recursive or freeze-resistant)
- Some SCUMM functions are poorly named. I would like to remove question
marks from all function names. I would also like to rename roomOps sub-op
0x07 from "Unused" to "setRoomScale", and also add the 5 expected
arguments for it.
- "findObject" looks for words or vars, whereas ScummVM looks for bytes or
vars. I have only ever seen this function used with vars, but I would like
to change it to match ScummVM anyway.

I would also like to make the following change for my own reasons:
- Output extra object metadata for SCUMM V3-4 object scripts, such as
object ID, x- and y-positions, width & height, etc. I want to do this so a
script output with descumm can be compiled using Scummbler and result in a
file that is bit-identical to the original script block.

-------

I've also encountered an actual bug with the creation of if/else blocks.
I've found a script that seems to have a weird combination of jumps, and
when descumm tries to decompile it, one instruction gets left out of the
resulting script.

Here is the start of the script, output with "if" blocks:
[0000] (44) if (VAR_CURSORSTATE > 0) {
[0007] (A8)   if (Bit[726]) {
[000C] (44)   } else if (VAR_MOUSE_Y > 152) {
[0016] (44)     if (VAR_MOUSE_X > 160) {
[001D] (AC)       Exprmode Local[2] = ((VAR_MOUSE_X - 160) / 40);
[002C] (AC)       Exprmode Local[2] = (Local[2] + (((VAR_MOUSE_Y - 152) /
24) * 4));
[0043] (9A)       Local[0] = Var[134 + Local[2]];
[004A] (48)       if (Var[108] == 11) {
[0051] (9A)         Var[154] = Var[108];
[0056] (1A)         Var[108] = 8;
[005B] (**)       }
[005B] (48)       if (Var[108] == 9) {
[0062] (9A)         Var[154] = Var[108];
[0067] (1A)         Var[108] = 8;
[006C] (**)       }
[006C] (18)     } else {
[0072] (18)       goto 008B;
[0075] (**)     }
[0075] (**)   }
[0075] (F5)   Local[0] = findObject(VAR_VIRT_MOUSE_X,VAR_VIRT_MOUSE_Y)
...

And here is the same script, output without "if" blocks:

[0000] (44) unless (VAR_CURSORSTATE > 0) goto 019F;
[0007] (A8) unless (Bit[726]) goto 000F;
[000C] (18) goto 0075;
[000F] (44) unless (VAR_MOUSE_Y > 152) goto 0075;
[0016] (44) unless (VAR_MOUSE_X > 160) goto 006F;
[001D] (AC) Exprmode Local[2] = ((VAR_MOUSE_X - 160) / 40);
[002C] (AC) Exprmode Local[2] = (Local[2] + (((VAR_MOUSE_Y - 152) / 24) *
4));
[0043] (9A) Local[0] = Var[134 + Local[2]];
[004A] (48) unless (Var[108] == 11) goto 005B;
[0051] (9A) Var[154] = Var[108];
[0056] (1A) Var[108] = 8;
[005B] (48) unless (Var[108] == 9) goto 006C;
[0062] (9A) Var[154] = Var[108];
[0067] (1A) Var[108] = 8;
[006C] (18) goto 0072;
[006F] (18) goto 0075;
[0072] (18) goto 008B;
[0075] (F5) Local[0] = findObject(VAR_VIRT_MOUSE_X,VAR_VIRT_MOUSE_Y)
...

Look at lines 006C to 0072. In the version with "if" blocks, we have lost
the instruction at 006F. Additionally, we've completely changed the
program flow; instead of 006C jumping to 0072 (which then jumps to 008B),
006C will instead jump to 0075.

-------

So, I am prepared to fix all of these issues immediately, except for the
"if" block bug which might require a fair bit of refactoring (such as
adding two-pass parsing). I have already cut some code that fixes the
issue with inconsistent semi-colons.

If you all are happy with these changes to go ahead, should I submit the
patches to this mailing list?

Cheers,
Laurence