Opened 10 years ago

Last modified 7 years ago

#4535 new defect

MONKEY: Voodoo lady palette glitches in Amiga version

Reported by: SF/quietust Owned by:
Priority: normal Component: Engine: SCUMM
Keywords: Cc:
Game: Monkey Island 1

Description

Several spots in the game exhibit significant palette glitches, possibly related.

1. When entering the forest maze, the yellow flowers aren't yellow, but are being colored red instead.
2. When talking to the voodoo lady, the entire screen flashes blue in a rather glitchy way when the boiling pot rises out of the floor or retracts back in.

Ticket imported from: #2838205. Ticket imported from: bugs/4535.

Change History (18)

comment:1 Changed 10 years ago by SF/quietust

Also, this is happening with latest code from SVN.

comment:2 Changed 10 years ago by sev-

could you, please, try GSoC Amiga modules branch? There were palette-related fixes. The code is in branches/gsoc2009-mods

comment:3 Changed 10 years ago by SF/quietust

That's actually where I first noticed the problem (so nice to finally have music and sound effects in the game), and I double-checked with the base version to verify that it also happened there - a quick recheck indicates that the latest svn for gsoc2009-mods also exhibits the problem.

comment:4 Changed 10 years ago by SF/quietust

Further inspection reveals that this apparently stopped working between the 0.7.1 release and 0.8.0. Best I can tell, o5_roomOps() case 2 (SO_ROOM_COLOR) seems to not be working as it should (I suspect _paletteMod is confusing things).

comment:5 Changed 9 years ago by lordhoto

I guess it must be some regression introduced in one of those functions using "_roomPalette" in gfx.cpp.

It seems 0.7.1 still had this code in the Gdi constructor:

_roomPalette = vm->_roomPalette;
if ((vm->_features & GF_AMIGA) && (vm->_version >= 4))
_roomPalette += 16;

While 0.8.0 started to use just:

_roomPalette = vm->_roomPalette;

To compensate that it seems there was "_paletteMod" introduced, which is set in "Gdi::decompressBitmap". It seems some of the functions called from Gdi::decompressBitmap were changed to use "_paletteMod" in their "_roomPalette" access.

Here's an example of drawStripEGA:

0.7.1:

*(dst + y * dstPitch + x) = (z & 1) ? _roomPalette[color & 0xf] : _roomPalette[color >> 4];

0.8.0:

*(dst + y * dstPitch + x) = (z & 1) ? _roomPalette[color & 0xf] + _paletteMod : _roomPalette[color >> 4] + _paletteMod;

I have to say that looks strange. For example why isn't the "+ _paletteMod" after the indexed access to _roomPalette and not added to the index, which looks like what the 0.7.1 code did?

Maybe there is a good explanation for this, I don't know whether the palette handling itself changed during 0.7.1 to 0.8.0 changes. But the above mentioned looks odd.

Also maybe some other functions called from "Gdi::decompressBitmap" need adaption to take "_paletteMod" into calculation, it looks to me like not every function was changed. Those functions are:

unkDecode8
unkDecode9
unkDecode10
unkDecode11
drawStrip3DO
drawStripHE

Judging by the names the latter 3 should not be used in MI1 Amiga though. The former four seem to be only used by FM-Towns games, according to the comments. So probably they do not need to be changed.

There's another function accessing _roomPalette and not taking _paletteMod into calculation:

drawStripC64Background

Maybe that's just fine though, since it seems for C64 "_paletteMod" is always 0 :-).

Anyway I guess the problem lies in the above mentioned change in "drawStripEGA" (and maybe the other functions changed in a similar way). I do not have MI 1 Amiga to test that though, so it would be nice if someone with that game version could look into it. Personally I would for example just change the above mentioned line in drawStripEGA to:

*(dst + y * dstPitch + x) = (z & 1) ? _roomPalette[(color & 0xf) + _paletteMod] : _roomPalette[(color >> 4) + _paletteMod];

And of course all other places doing the same addition of _paletteMod after _roomPalette index look up :-). Maybe that fixes the problem or it will just look even more incorrect and thus the problem is caused by another change...

comment:6 Changed 9 years ago by fingolfin

Your analysis is 100% correct. In particular, in the old code base, the _palette_mod was indeed added inside the array access, which is the only logical way, too ;). See also <http://scummvm.svn.sourceforge.net/viewvc/scummvm/scummvm/trunk/engines/scumm/gfx.cpp?r1=10776&r2=10778>

So please go ahead and fix it ;).

comment:7 Changed 9 years ago by fingolfin

Owner: set to lordhoto

comment:8 Changed 9 years ago by lordhoto

I did commit it to trunk now, would be still nice if someone could verify it and when it's working just backport it to branch maybe.

comment:9 Changed 9 years ago by lordhoto

Talking with Kirben showed that my simple patch did cause a regression in FOA/Amiga. I also fixed that and backported it to 1.0.0.

Kirben also pointed out that the voodoo lady issue is still present (as it was in 0.7.1 according to him).

I'll leave this bug report open till that is fixed. I'm also reassigning the report to "None", since I do not have any way to fix that issue :-).

comment:10 Changed 9 years ago by lordhoto

Owner: lordhoto deleted

comment:11 Changed 9 years ago by Kirben

There were no effects when the Voodoo lady rises and retracts from the floor, in the original Amiga version. So it looks like some effects were disabled in the Amiga version, and need to be disabled under ScummVM too.

I noticed that roomOps case 4 is been used at these points, so maybe_shadowPalette was not used in Amiga version?

comment:12 Changed 9 years ago by fingolfin

Summary: MONKEY: Palette glitches in Amiga versionMONKEY: Voodoo lady palette glitches in Amiga version

comment:13 Changed 9 years ago by fingolfin

Possible. Only way to know for sure is to look at the disassembly, I guess. But in the meantime, you could try modifying / disabling roomOps case 4, resp. shadowPalette handling, to see if that resolve the issues. If it does, then we'd also have to test the rest of the game for regressions, though :)

comment:14 Changed 9 years ago by fingolfin

What is the status of this item?

comment:15 Changed 9 years ago by Kirben

Unchanged, the problem still occurs with ScummVM SVN.

comment:16 Changed 7 years ago by sev-

This bug is nice to get fixed before the release. Raising priority for keeping the track.

comment:17 Changed 7 years ago by sev-

Priority: normalhigh

comment:18 Changed 7 years ago by sev-

Priority: highnormal
Note: See TracTickets for help on using tickets.