Opened 17 years ago
Closed 17 years ago
#3075 closed defect (fixed)
COMI: Subtitles go off-screen
| Reported by: | SF/mthreepwood | Owned by: | cyxx |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCUMM |
| Version: | Keywords: | ||
| Cc: | Game: | Monkey Island 3 |
Description
On Skull Island, I found the bug (see attached screenshot). You can use boot param 649 to see it.
According to Quietust this happened during the swordfight with Captain Rottingham as the crew members are looking at the whale.
(I used Mac OS X 0.10.0svn 2/4/07 under 10.2.8 -- the usual)
Ticket imported from: #1662610. Ticket imported from: bugs/3075.
Attachments (6)
Change History (25)
by , 17 years ago
| Attachment: | comibug.jpg added |
|---|
comment:2 by , 17 years ago
I guess this could be either wrong positioning or wrong line breaking.
It would be interesting to compare how the original draws subtitles to how ScummVM does it. To me, it looks as if ScummVM's subtitles are sometimes drawn rather narrowly (resulting in more line breaks) when the talking character is near the edge of the screen, but perhaps that is the correct behaviour?
comment:3 by , 17 years ago
Perhaps it is correct, perhaps not -- as you said yourself, it would be very helpful if somebody checked out this scene with the original engine, and could provide some screenshots for comparision.
comment:4 by , 17 years ago
| Owner: | set to |
|---|
comment:5 by , 17 years ago
... oh and maybe cyx feels a bit bored and would be interested in checking some disasm once more *g*
comment:6 by , 17 years ago
Same problem as #1036707, I'd say...
In V8, the CHARSET_1 function has been simplified a lot. Now, it just enqueues the text and doesn't perform any text clipping/word wrapping. This is done at the "last moment", in drawBlastTexts, handling the various string flags (centered, word wrapped "correctly").
Some more differences : - in the original, the clipping rect for text is (10, 10, screen.width - 10, screen.height - 10) - decodeParseString/SO_PRINT_WRAP isn't a no-op in the original :)
I will try to have a closer look...
by , 17 years ago
| Attachment: | 1036707.diff added |
|---|
comment:7 by , 17 years ago
Here's a first version of a (hacky) patch to fix the issue.
Also attached screenshots of the scene with the original COMI interpreter.
File Added: 1036707.diff
by , 17 years ago
| Attachment: | bootparam_649_1.tar.bz2 added |
|---|
Screenshots of scene with original COMI interpreter (1/2)
by , 17 years ago
| Attachment: | bootparam_649_2.tar.bz2 added |
|---|
Screenshots of scene with original COMI interpreter (2/2)
comment:10 by , 17 years ago
It does look a bit hackish, but shouldn't there be checks for those cases where the character is near the edges of the screen anyway, like the ones cyx is using?
As for that scene, I remember that the screen scrolls up to show the rest of Skull island, perhaps this scrolling is affecting subtitle positioning?
comment:11 by , 17 years ago
Yes, the are "checks for those cases where the character is near the edges of the screen", i.e. in other words, we do perform clipping. It just happens that for COMI it needs to be done differently.
Anyway, if you had a look at the code and suspect specific things, please say so, it is welcome. Otherwise, looking at the current buggy behavior and telling us your guesses for the causes, when you apparently don't really know the code (yet?), is meant well, but more a distraction than actual help.
comment:12 by , 17 years ago
Regarding the patch: I didn't have time so far to properly review it, but one thing I want to mention, lest it be forgotten: We likely would have to store the "wrapping" flag in the savegame, like all other StringTab members. So this patch should not be commited as is.
comment:13 by , 17 years ago
I'm sorry, I just saw some checks in cyx's patch for character positioning, that's why I mentioned this
comment:14 by , 17 years ago
Yes, that patch shouldn't be committed as-is (look at the XXX comments present in the code for the missing/TODO items).
I submitted it to get some feedback and opinion whether patching/overloading CHARSET_1 for v8 is a "correct" way to fix the issue or not.
Looking at it again, it seems that I forgot to handle VAR_CHARINC.
by , 17 years ago
| Attachment: | actorToPrintStrFor_V8.diff added |
|---|
comment:15 by , 17 years ago
I also noticed a difference with _actorToPrintStrFor. In the original interpreter, it must be compared/set to 0 where 0xFF was used in <= V7. It seems a change was done in stopTalk to reset it to 0 in case of V8 (to match disasm I suppose), but then this is not consistant anymore with the actorTalk method.
Here's a patch to fix this inconsistency (this is not directly related to the issue, but I thought I should mention it somewhere).
File Added: actorToPrintStrFor_V8.diff
by , 17 years ago
| Attachment: | 1036707-2.diff added |
|---|
comment:16 by , 17 years ago
Updated patch : - turned CHARSET_1 into a virtual method overloaded for v8 - fixed computation of _talkDelay (using VAR_CHARINC) - added StringTab.wrapping to the saveload data
This patch should also fix #1036707.
File Added: 1036707-2.diff
comment:17 by , 17 years ago
That patch looks pretty good to me. Feel free to commit it & close these two bugs. Good work, as usual :-)
comment:18 by , 17 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
Screenshot of the bug