Opened 14 years ago

Closed 14 years ago

Last modified 20 months ago

#2894 closed defect (fixed)

IMUSE: Typo in sysex_scumm.cpp ?

Reported by: SF/albeu Owned by: sev-
Priority: normal Component: Engine: SCUMM
Keywords: Cc:
Game:

Description

Hi,

while digging the iMUSE code for ScummC i found some code that look very much like a typo. At the end of sysex_scumm.cpp (around line 190) there is:

case 96: // Set instrument part = player->getPart(p[0] & 0x0F); a = (p[1] & 0x0F) << 12 |(p[2] & 0x0F) << 8 |(p[4] & 0x0F) << 4 |(p[4] & 0x0F);

Which should read a 16 bit value from the "one-nibble-per-byte" encoding used in the imuse sysex, but that should probably be:

a = (p[1] & 0x0F) << 12 |(p[2] & 0x0F) << 8 |(p[3] & 0x0F) << 4 |(p[4] & 0x0F);

If not then a comment would be a good idea because the code just doesn't look logical. I attached a patch with the fix above.

Ticket imported from: #1592006. Ticket imported from: bugs/2894.

Attachments (2)

sysex_scumm-typo.diff (532 bytes ) - added by SF/albeu 14 years ago.
Patch against r24646
op96_set_instrument.txt (2.4 KB ) - added by cyxx 14 years ago.
op96 disasm from dott imuse

Download all attachments as: .zip

Change History (9)

by SF/albeu, 14 years ago

Attachment: sysex_scumm-typo.diff added

Patch against r24646

comment:1 by fingolfin, 14 years ago

I agree with your observations, the code looks fishy (good catch). But I also just verified that it has been just like this since the very first incarnation of that code. So *maybe* it is right, maybe it was a mistake made during RE.

So, two things could be done: Somebody checks against disassembly, or we try to find spots where this change would matter, i.e. where p[3]&0xF != p[4]&0x0F.

comment:2 by fingolfin, 14 years ago

Summary: Typo in sysex_scumm.cpp ?IMUSE: Typo in sysex_scumm.cpp ?

comment:3 by SF/albeu, 14 years ago

A small update. Using some of my tools i checked all the SOUN ressource in dott and none of them use this SysEx.

by cyxx, 14 years ago

Attachment: op96_set_instrument.txt added

op96 disasm from dott imuse

comment:4 by cyxx, 14 years ago

I just checked against dott imuse disasm, p[3] is clearly used here. Attached disasm for reference.

comment:5 by sev-, 14 years ago

Ok, fixed in SVN. Thanks for reporting and cyx for research.

comment:6 by sev-, 14 years ago

Owner: set to sev-
Resolution: fixed
Status: newclosed

comment:7 by digitall, 20 months ago

Component: Engine: SCUMM
Note: See TracTickets for help on using tickets.