Opened 22 years ago
Closed 22 years ago
#720 closed defect (fixed)
LOOM: Distaff not properly erased (regression)
| Reported by: | eriktorbjorn | Owned by: | eriktorbjorn |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCUMM |
| Version: | 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 by , 22 years ago
comment:2 by , 22 years ago
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 by , 22 years ago
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 by , 22 years ago
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 by , 22 years ago
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 by , 22 years ago
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 by , 22 years ago
| Owner: | set to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
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...