Opened 13 years ago

Closed 13 years ago

#3075 closed defect (fixed)

COMI: Subtitles go off-screen

Reported by: SF/mthreepwood Owned by: cyxx
Priority: normal Component: Engine: SCUMM
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)

comibug.jpg (31.5 KB ) - added by SF/mthreepwood 13 years ago.
Screenshot of the bug
1036707.diff (10.0 KB ) - added by cyxx 13 years ago.
bootparam_649_1.tar.bz2 (240.8 KB ) - added by cyxx 13 years ago.
Screenshots of scene with original COMI interpreter (1/2)
bootparam_649_2.tar.bz2 (167.5 KB ) - added by cyxx 13 years ago.
Screenshots of scene with original COMI interpreter (2/2)
actorToPrintStrFor_V8.diff (1.7 KB ) - added by cyxx 13 years ago.
1036707-2.diff (9.8 KB ) - added by cyxx 13 years ago.

Download all attachments as: .zip

Change History (25)

by SF/mthreepwood, 13 years ago

Attachment: comibug.jpg added

Screenshot of the bug

comment:1 by fingolfin, 13 years ago

Already happens with 0.9.0.

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

Owner: set to cyxx

comment:5 by fingolfin, 13 years ago

... oh and maybe cyx feels a bit bored and would be interested in checking some disasm once more *g*

comment:6 by cyxx, 13 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 cyxx, 13 years ago

Attachment: 1036707.diff added

comment:7 by cyxx, 13 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 cyxx, 13 years ago

Attachment: bootparam_649_1.tar.bz2 added

Screenshots of scene with original COMI interpreter (1/2)

comment:8 by cyxx, 13 years ago

File Added: bootparam_649_1.tar.bz2

by cyxx, 13 years ago

Attachment: bootparam_649_2.tar.bz2 added

Screenshots of scene with original COMI interpreter (2/2)

comment:9 by cyxx, 13 years ago

File Added: bootparam_649_2.tar.bz2

comment:10 by bluegr, 13 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 fingolfin, 13 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 fingolfin, 13 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 bluegr, 13 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 cyxx, 13 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 cyxx, 13 years ago

Attachment: actorToPrintStrFor_V8.diff added

comment:15 by cyxx, 13 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 cyxx, 13 years ago

Attachment: 1036707-2.diff added

comment:16 by cyxx, 13 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 fingolfin, 13 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 cyxx, 13 years ago

Resolution: fixed
Status: newclosed

comment:19 by cyxx, 13 years ago

Committed. Thanks for reviewing it.

Note: See TracTickets for help on using tickets.