Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#2729 closed defect (fixed)

DOTT: Graphic glitches while kite comes down

Reported by: lordhoto Owned by: eriktorbjorn
Priority: normal Component: Engine: SCUMM
Keywords: Cc:
Game: Day of the Tentacle

Description

I'm using a svn trunk version from today (08072006) on Linux/amd64. When the kite got hit by the thunderbolt and it comes down the tail (the red thing) leaves some red pixels which shouldn't be there I suppose (at least it looks wrong). I attached a savegame (from the german CD talkie version). Just push the kite in the right moment as said. And you'll se the graphics glitch.

Ticket imported from: #1519667. Ticket imported from: bugs/2729.

Attachments (3)

dott.s06 (26.5 KB ) - added by lordhoto 14 years ago.
save game to reproduce the glitch
tentacle.s60 (27.0 KB ) - added by eriktorbjorn 14 years ago.
Savegame from English talkie version
costume-height.diff (1.0 KB ) - added by eriktorbjorn 14 years ago.
Possible patch against current SVN

Download all attachments as: .zip

Change History (12)

by lordhoto, 14 years ago

Attachment: dott.s06 added

save game to reproduce the glitch

comment:1 by eriktorbjorn, 14 years ago

I don't have the German version, so I've been trying on an old savegame of mine. A couple of observations:

The glitch happens for me in ScummVM 0.8.2 as well, so it's apparently not a recent regression.

The kite is actor 7. The glitch happens while drawing a scaled sprite for it.

From what I understand, ClassicCostumeRenderer::mainRoutine() calculates the height of the sprite, and these values are later used by ScummEngine::resetActorBgs(). However, in some cases it appears that proc3() draws a sprite that is slightly taller than the calculated height.

comment:2 by eriktorbjorn, 14 years ago

I see what's happening, though I'm not entirely certain what the correct behaviour is.

When calculating the height of a scaled sprite, we do this:

for (i = 0; i < _height; i++) { if (v1.scaletable[_scaleIndexY++] < _scaleY) rect.bottom++; }

When drawing it, we do... well, we do quite a lot, but if we simplify it a lot, it would look somewhat like this:

scaleytab = &v1.scaletable[_scaleIndexY]; for (i = 0; i < _height; i++) { if (*scaleytab++ < _scaleY) y++; }

In both cases, _scaleIndexY start out as the same value. The two code fragments look deceptively simple. But in this case, _scaleIndexY overflows and wraps around to zero in some cases when calculating the height. That does not, of course, happen when drawing.

Question is, should we prevent wrap-around when calculating the height, or should we allow wrap-around when drawing? Either would fix the problem, I guess.

I also guess that we have the same potential problem in the AKOS renderer. At least in code1_genericDecode().

comment:3 by eriktorbjorn, 14 years ago

Bah, I meant of course that the two code fragments look deceptively *similar*, not simple.

by eriktorbjorn, 14 years ago

Attachment: tentacle.s60 added

Savegame from English talkie version

by eriktorbjorn, 14 years ago

Attachment: costume-height.diff added

Possible patch against current SVN

comment:4 by eriktorbjorn, 14 years ago

Looking a bit closer at it, it seems the overflow won't happen in AKOS since startScaleIndex is an int, not a byte.

I've attached a patch that introduces the same overflow/wrap-around behaviour in proc3() as in mainRoutine(), and it does fix the glitch. But is it correct to use the scale table that way?

Assigning to Fingolfin, since he claims to understand the scale table.

comment:5 by eriktorbjorn, 14 years ago

Owner: set to fingolfin

comment:6 by Kirben, 14 years ago

The costume scale table is accessed via byte sized variables in the code of original game, so would overflow in this situation too.

So the patch to allow overflow/wrap-around, by using a byte sized variable sounds fine.

comment:7 by eriktorbjorn, 14 years ago

Thanks. I've applied my patch, after adding a comment about the possibility of the scale index wrapping around.

comment:8 by eriktorbjorn, 14 years ago

Owner: changed from fingolfin to eriktorbjorn
Resolution: fixed
Status: newclosed

comment:9 by fingolfin, 14 years ago

Only noticed this report today. But yeah, the patch is perfectly valid, even from a logical point of view. The problem more or less stems from the fact that AKOS uses two different kinds of scale tables: An old one, identical to the table used in costume.cpp, and a newer one which is bigger (768 bytes IIRC), and for which we must *not* do wrapping. I am not sure if this was ever implemented quite correctly, or if maybe somebody (most likely I :-) broke it during on of the costume code cleanups years ago...

Note: See TracTickets for help on using tickets.