Opened 18 years ago

Closed 18 years ago

Last modified 2 years ago

#8137 closed patch (outdated)

MI1VGA: Possible scroll arrow fix

Reported by: eriktorbjorn Owned by: fingolfin
Priority: normal Component: Engine: SCUMM
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)

insult-arrows.diff (3.0 KB ) - added by eriktorbjorn 18 years ago.
Patch against a November 23 CVS snapshot

Download all attachments as: .zip

Change History (8)

by eriktorbjorn, 18 years ago

Attachment: insult-arrows.diff added

Patch against a November 23 CVS snapshot

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

Owner: set to fingolfin
Resolution: outdated
Status: newclosed

comment:6 by fingolfin, 18 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 digitall, 2 years ago

Component: Engine: SCUMM
Game: Monkey Island 1
Note: See TracTickets for help on using tickets.