Opened 16 years ago

Closed 16 years ago

#720 closed defect (fixed)

LOOM: Distaff not properly erased (regression)

Reported by: eriktorbjorn Owned by: eriktorbjorn
Priority: normal Component: Engine: SCUMM
Keywords: Cc:
Game: Loom

Description

This bug has been listed on the ScummVM regressions
page for some time now, but I figured it'd be a good
idea to have it in the tracker as well since we're
nearing 0.4.0.

As the subject line says, the distaff is not properly
erased. There's a screenshot of this at
http://users.bigpond.net.au/tgray2/loombug.png

Since it happens so early in the game, I haven't
bothered to provide a savegame.

I believe this bug was introduced when Fingolfin fixed
a graphics glitch with the MonkeyVGA scroll arrows by
removing a "right++;" from restoreBG(). But since it
calculates the width as right - left, that probably
makes it one pixel too narrow. Another thing that looks
suspicious in this function is that it checks if right
> _realWidth. Surely that should be _realWidth - 1?

The same reasoning also applies to the height
calculation and check. Note however that whenever I've
tried to "fix" this, I've ended up introducing new
graphics glitches.

Maybe we should justrevert the bugfix and work around
the problem like we do for Indy 3 and Zak256 in
CharsetRendererOld256::getCharWidth(). At least I
assume that FIXME code is for a similar bug.

Ticket imported from: #706178. Ticket imported from: bugs/720.

Change History (7)

comment:1 Changed 16 years ago by fingolfin

No we should most definitly not revert this - the "right++;" was *wrong*. I compared it to the original code. Also, I think most (all?) of our checks in there are in fact correct. I can go over it again, though. But it's very easy to draw wrong conclusions from this code, as not everything is what it might appear to be...

comment:2 Changed 16 years ago by eriktorbjorn

If you say the right++ was wrong, then I believe you.

But it still looks strange to me. Maybe I misunderstand the
semantics of the function. Should it really mean "restore
pixels from the left pixel to the right pixel, except don't
touch the rightmost pixel"? Because that's how it looks to
me right now.

comment:3 Changed 16 years ago by fingolfin

Yes the code is indeed strange, even in the original, and the fact that we have to cover various versions of Scumm probably doesn't help either. I will look into this again, I can't say anything detailed from the top of my head.

comment:4 Changed 16 years ago by eriktorbjorn

I looked a bit more at the problem. These are the rectangles
used for erasing the wooden part of the distaff:

Verb | Left | Right | Top | Bottom
-----+--------+--------+--------+--------
41 | 16 | 47 | 144 | 167
42 | 48 | 71 | 144 | 167
43 | 72 | 95 | 144 | 167
44 | 96 | 119 | 144 | 167
45 | 120 | 143 | 144 | 167
46 | 144 | 167 | 144 | 167
47 | 168 | 191 | 144 | 167
48 | 192 | 231 | 144 | 159

You say changing restoreBG() probably isn't an option, so I
can think of only three other options:

* Change restoreVerbBG() to add one to the right (and
bottom?) corrdinates. This will probably affect the
undrawing of all kinds of verbs, not just verb images, so
this may be a bad idea.

* Add one to the verb's right (and bottom?) coordinates. We
can do this for the case of verb images only, which is good,
but it will also affect the behaviour of checkMouseOver(),
which may be bad.

* Add one to the verb's oldright (and oldbottom?)
coordinages. We can do this for the case of verb images
only, and as far as I can tell it's only used for undrawing
the verb. Maybe this is the least intrusive fix?

comment:5 Changed 16 years ago by fingolfin

Any of these changes could introduce subtle regressions in other games (think of the "arrow" bug in the insult fights in MI). I really really should compare this code again to the original, I did in the past, and it's quite annoying to do because we changed quite some things in the meantime compared to the original code <sigh>.

comment:6 Changed 16 years ago by eriktorbjorn

Changing restoreVerbBG() would likely break the MonkeyVGA
scroll arrows, yes. Changing the behaviour of
drawVerbBitmap() shouldn't since, if I recall correctly, the
arrows are actually special text characters.

Of course, that doesn't rule out *other* possible regressions.

This is the part of drawVerbBitmap() that looks most
suspicious to me:

vst->right = vst->x + imgw * 8 - 1;
vst->bottom = vst->y + imgh * 8 - 1;

I take this to mean that we subtract 1 from right and bottom
so that they will point to the rightmost and bottommost
pixel. Yet when we undraw the verb, we only restore to the
rightmost (and bottommost?) pixel, minus one. I don't see
how that can possibly be correct.

Actually, when erasing the verb we use oldright and
oldbottom, but they're set to right and bottom respectively.
I don't know why we have two sets of coordinates, but one of
them is used for checking mouseovers (where I think we *do*
include the rightmost and bottommost coordinates, which is
why I said changing right and bottom may be wrong) and one
is used for erasing the verb image. There may be other
differences for other kinds of verbs, I guess.

comment:7 Changed 16 years ago by fingolfin

Owner: set to eriktorbjorn
Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.