Opened 20 years ago
Closed 17 years ago
#1791 closed defect (fixed)
COMI: Subtitles positioned incorrect
| Reported by: | Kirben | Owned by: | cyxx |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCUMM |
| Version: | 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)
Change History (27)
comment:1 by , 20 years ago
comment:2 by , 20 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.
comment:3 by , 20 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 , 20 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 , 20 years ago
| Owner: | set to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
comment:6 by , 20 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 , 20 years ago
| Resolution: | fixed |
|---|---|
| Status: | closed → new |
comment:8 by , 20 years ago
| Owner: | removed |
|---|
comment:9 by , 20 years ago
Ah. Well, the bug report title was about the subtitle *position*, which I fixed.
comment:10 by , 20 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:12 by , 18 years ago
The problem still occurs, the subtitles are still wrapped onto next line in this scene under ScummVM.
by , 17 years ago
| Attachment: | comi_default_charset.diff added |
|---|
comment:13 by , 17 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 , 17 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 , 17 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 , 17 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:18 by , 17 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 , 17 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 , 17 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 , 17 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:23 by , 17 years ago
| Owner: | set to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
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).