Opened 15 years ago

Closed 13 years ago

#1791 closed defect (fixed)

COMI: Subtitles positioned incorrect

Reported by: Kirben Owned by: cyxx
Priority: normal Component: Engine: SCUMM
Keywords: Cc:
Game: Monkey Island 3

Description

Latest ScummVM cvs version.
Compiled under mingw with GCC 3.4.1 and running under
Windows XP.
English version of Curse of Monkey Island.

At the start of Part III, when a ship is shown
approaching Guybrush's ship, the subtitles are shown in
incorrect position or completely missing.
The original game never centered the subtitles like that
either.

These subtitles were positioned correctly at one point:
scumm/gfx.cpp revision 2.289 broke the subtitles
position, although it fixed an invalid write in the Dig (Bug
#1008621).

Use boot param. 465 to see the issue.

Ticket imported from: #1036707. Ticket imported from: bugs/1791.

Attachments (3)

comi.png (54.3 KB ) - added by Kirben 15 years ago.
Example from COMI
comi.zip (235.7 KB ) - added by Kirben 15 years ago.
Screenshots of scene from original COMI
comi_default_charset.diff (2.6 KB ) - added by cyxx 13 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 by fingolfin, 15 years ago

Which subtitles in which scene are you talking about? On the navigation
chart? During shipt-to-ship combat? Both seem fine... Maybe you could
provide a screenshot of what you mean, with instructions to get there
(besides the boot param, I mean).

comment:2 by Kirben, 15 years ago

As a ship is shown approaching Guybrush's ship, which occurs
right after the Part III title screen, before those others
scenes you mentioned. The subtitles should be shown along
top of screen, but are shown close to ships instead.
Example screenshot attached.

by Kirben, 15 years ago

Attachment: comi.png added

Example from COMI

comment:3 by fingolfin, 15 years ago

OK, I see that.

The problem stems from the fact that CHARSET_1 clips
_string[0].ypos *before* _screenTop is added to it; but
in this example, ypos actually has a negative value.

If I change the clipping from
if (_string[0].ypos < 1)
_string[0].ypos = 1;
to
if (_string[0].ypos + _screenTop < 1)
_string[0].ypos = 1 - _screenTop;
Then things look better.
The direct cause for the whole mess, though, is the fact that actors talk
here which are outside the room, at coords (0,0), with a talkPosY = -80.
No good...
It would be interesting to know at which positions the original engine
drew that text...

comment:4 by fingolfin, 15 years ago

Problem was in charset.cpp, which didn't pass the right y value to the
font renderer (it passed it a y value in room coordinates, not in the
screen/surface coordinates it should have used...). My fix also cures a
potential clipping bug (i.e. a memory out of bounds access).

comment:5 by fingolfin, 15 years ago

Owner: set to fingolfin
Resolution: fixed
Status: newclosed

by Kirben, 15 years ago

Attachment: comi.zip added

Screenshots of scene from original COMI

comment:6 by Kirben, 15 years ago

The vertical position of the subtitles is fixed now but the
formating of the subtitles is still wrong. The subtitles lines are
been wrapped around, when they shouldn't be.
Screenshots of that scene from original COMI are attached.

comment:7 by Kirben, 15 years ago

Resolution: fixed
Status: closednew

comment:8 by fingolfin, 15 years ago

Owner: fingolfin removed

comment:9 by fingolfin, 15 years ago

Ah. Well, the bug report title was about the subtitle
*position*, which I fixed.

comment:10 by fingolfin, 15 years ago

Another thing which is apparent here is that the original engine
spaced characterd in this scene wider than we do. I have no idea
what might control that, though.

comment:11 by sev-, 14 years ago

What is the status of this item?

comment:12 by Kirben, 14 years ago

The problem still occurs, the subtitles are still wrapped
onto next line in this scene under ScummVM.

by cyxx, 13 years ago

Attachment: comi_default_charset.diff added

comment:13 by cyxx, 13 years ago

As Max mentionned, the space between characters is wider in the original engine than in ScummVM (this happen in every scene of the game, not just the one described here). There's nothing to control that, it's just we're not using the "correct" nut font (it should be 1, instead of 0). Attached patch which fixes this problem (this matches disasm).

This, of course, doesn't fix the bug mentionned here, but only a part of it. The fix the "rest", some modifications to CHARSET_1 will be necessary (it seems _charset->_nextLeft is "responsible" for the "incorrect" line wrapping).

File Added: comi_default_charset.diff

comment:14 by fingolfin, 13 years ago

Good work, cyx!
Does your change to SO_CHARSET_COLOR imply that it is a no-op in disasm, too?

comment:15 by Kirben, 13 years ago

I can confirm SO_CHARSET_COLOR is ignored in disasm of the original COMI. It gets a stack list of 4 items and doesn't do anything with the results.

comment:16 by fingolfin, 13 years ago

Great! Well, cyx, do you want to commit this then? Possibly with a comment added to SO_CHARSET_COLOR stating something like
// This opcode does nothing (confirmed by disasm)
or so.

comment:17 by fingolfin, 13 years ago

Since I didn't hear again from cyx, I have no commited his patch.

comment:18 by fingolfin, 13 years ago

So, I wanted to take a look at this. Yet oddly enough, I somehow can't seem to get any subtitles in the ship-to-ship combat right now. I have a savegame from which I start it (click on navigation chart, move into another ship, combat starts) -- the only message I get during the combat is the "defeat" message. Am I missing something?

comment:19 by Kirben, 13 years ago

Just use boot param 465 to see the subtitle issues, as mentioned in original bug report.

These sequence are shown at the start of Part III, before the pirate song and ship to ship combat.

I don't recall any subtitles during the actual ship to ship combat.

comment:20 by fingolfin, 13 years ago

In string.cpp, around line 517-526, we call addLinebreaks. If I remove the
if (maxwidth > _nextLeft)
maxwidth = _nextLeft;
then the line breaks do not get added. The result is still not quite right, the text is printed too far to the right.

Logically, this check is there to ensure we do not print outside the left border of the screen. But it's in the wrong spot there, as we adjust the value of _nextLeft later on.

The code is pretty old, and works fine elsewhere (and the peculiar behavior caused by it can be reproduced with the original interpreters, too, I think). But maybe this is yet another thing they finally "fixed" in COMI?

comment:21 by cyxx, 13 years ago

Most functions dealing with the charset drawing have been rewritten in v8 (CHARSET_1, actorTalk). It seems that all the clipping, centering of the texts are done at last moment, when drawing in drawBlastText().

Anyway, while looking at this the other day, I also noticed a small difference in CHARSET_1 between ScummVM and v5/v7 interpreters. In ScummVM, we have something like :

if (_game.version > 3) {
int w = _charset->_right - string[0].xpos - 1;
if (_charset->_center) {

The original interpreters have the following code :

int w = _charset->_right - _nextLeft - 1;
if (_charset->_center) {

This shouldn't make much a difference unless CHARSET_1 is called with _keepText set to false (in that case _nextLeft is initialised with _string[0].xpos). As I am not sure how to properly test if that makes a difference or not, I haven't committed anything. That was for information.

comment:22 by cyxx, 13 years ago

See also bug report #1662610

comment:23 by cyxx, 13 years ago

Owner: set to cyxx
Resolution: fixed
Status: newclosed

comment:24 by cyxx, 13 years ago

Should be fixed with latest SVN.

Note: See TracTickets for help on using tickets.