Opened 13 years ago

Closed 13 years ago

Last modified 13 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 13 years ago.
save game to reproduce the glitch
tentacle.s60 (27.0 KB ) - added by eriktorbjorn 13 years ago.
Savegame from English talkie version
costume-height.diff (1.0 KB ) - added by eriktorbjorn 13 years ago.
Possible patch against current SVN

Download all attachments as: .zip

Change History (12)

by lordhoto, 13 years ago

Attachment: dott.s06 added

save game to reproduce the glitch

comment:1 by eriktorbjorn, 13 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, 13 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, 13 years ago

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

by eriktorbjorn, 13 years ago

Attachment: tentacle.s60 added

Savegame from English talkie version

by eriktorbjorn, 13 years ago

Attachment: costume-height.diff added

Possible patch against current SVN

comment:4 by eriktorbjorn, 13 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, 13 years ago

Owner: set to fingolfin

comment:6 by Kirben, 13 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, 13 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, 13 years ago

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

comment:9 by fingolfin, 13 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.