#8137 closed patch (outdated)
MI1VGA: Possible scroll arrow fix
| Reported by: | eriktorbjorn | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCUMM |
| Version: | Keywords: | ||
| Cc: | Game: | Monkey Island 1 |
Description
I've taken a closer look at the second issue of bug #599442 ("MI1VGA: the last few glitches") -- the one with scroll arrows covering part of the text.
I've compared ScummVM to the original (that is, if you consider DOSBox to be a faithful rendition of the original), and the arrows are positioned correctly. The problem is that restoreVerbBG() thinks the arrows are wider than they really are, so it erases too much.
What this patch does is to make the charset renderer keep track of how wide the text it draws is (the arrows are being drawn as verb text, apparently) if you only consider the pixels that are actually painted. I originally intended to do the same for the text height but a) I couldn't get it to work properly for some reason, and b) it wasn't needed.
One possible reason I couldn't get it to work is that restoreBG() seems a bit dodgy. It has the lines
height = bottom - top; width = right - left;
which I take to mean that both height and width will be one too small. Well, width won't be since we already increase 'right' by one (with a FIXME comment), but what about height? On the other hand, even when "fixing" this I couldn't get it to work, so maybe I'm just mistaken.
My main concern about this patch is that it adds to a part of the code that already looks dangerously overloaded with strange variables. Maybe a cleaner solution would be to make restoreVerbBG() more clever, e.g. by drawing the verb string again, with the verb's background colour. But I couldn't see any easy way of doing that.
Ticket imported from: #643464. Ticket imported from: patches/242.
Attachments (1)
Change History (8)
by , 22 years ago
| Attachment: | insult-arrows.diff added |
|---|
comment:1 by , 22 years ago
I am not so happy about this patch, for the same reasons as you - i.e. I trust you it'll fix the issue, but obviously it would be preferable to fix the real cause instead of applying a work around... I have some ideas, and will try to look into them, but it might be a couple of days till I get a chance to do so.
comment:2 by , 22 years ago
Sure. I look forward to seeing what you'll come up with. At least we know what's causing the glitch now, and that's always a good first step.
In the meantime, do you have any idea if my concerns about restoreBG() are misplaced or not? I mean, I haven't actually *seen* it misbehave - that I know of - but it sure looks strange to me...
comment:3 by , 22 years ago
Yeah I agree, restoreBG is weird. There are too many hacks to fix patches for incorrect fixme's in it :-)
comment:4 by , 22 years ago
The problem is indeed in the restore function "restoring" too much. I analyzed the drawing, and _strRight is set to 16, and indeed the last pixel of the arrows is at coordinate 15, then one black pixel at coordinate 16, and the green text (the insults) starts at 17. Since _strRight=16, that would indeed be correct. However, one more pixel is "restored" to the black background. I'll look into fixing this.
comment:5 by , 22 years ago
| Owner: | set to |
|---|---|
| Resolution: | → outdated |
| Status: | new → closed |
comment:6 by , 22 years ago
The fix I checked in comments out a suspiscious right++; in restoreBG(). That may cause regressions elsewhere, but I'll deal with those once we encounter them.
comment:7 by , 6 years ago
| Component: | → Engine: SCUMM |
|---|---|
| Game: | → Monkey Island 1 |
Patch against a November 23 CVS snapshot