Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

COMMON: Last character of some TTF texts was cropped (do not merge) #1734

Closed
wants to merge 1 commit into from

Conversation

@lolbot-iichan
Copy link
Contributor

commented Jul 3, 2019

This fixes https://bugs.scummvm.org/ticket/11007 and some other
Wintermute games.

Issue is an off-by-one error caused by a loop pattern, with inconsistent
pair of increment and break-condition: x += font.getCharWidth(cur) and
if (x + charBox.right > rightX) break;, while char width was unequal
to char box width (it's ok for base Font class, but wrong for child
class TTFFont).

I think that maybe it's the right way to fix it, since bug 11007 is
not reproduced with this code and rendering seems fine at the first
glance, but I really want someone from COMMON maintainers to hardly
review this since this may affect rendering all fonts in all games. I
just don't get how this couldn't affect other engines - maybe
Wintermute's BaseFontTT::measureText is wrong and we should use some
other font methods instead of textWidth = _font->getStringWidth(text)???

COMMON: Last character of some TTF texts was cropped
This fixes https://bugs.scummvm.org/ticket/11007 and some other
Wintermute games.

Issue is an off-by-one error caused by a loop pattern, with inconsistent
pair of increment and break-condition: `x += font.getCharWidth(cur)` and
`if (x + charBox.right > rightX) break;`, while char width was unequal
to char box width (it's ok for base Font class, but wrong for child
class TTFFont).

I *think* that maybe it's the right way to fix it, since bug 11007 is
not reproduced with this code and rendering seems fine at the first
glance, but I really want someone from COMMON maintainers to hardly
review this since this may affect rendering all fonts in all games. I
just don't get how this couldn't affect other engines - maybe
Wintermute's `BaseFontTT::measureText` is wrong and we should use some
other font methods instead of `textWidth =
_font->getStringWidth(text)`???

@lolbot-iichan lolbot-iichan force-pushed the lolbot-iichan:wme_ritter_text branch from e6675a2 to c7e8b2b Jul 3, 2019

@bgK bgK self-requested a review Jul 3, 2019

@bgK

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

I'm sorry, but the changes are definitely incorrect.

ScummVM defines two ways to measure true type rendered text. The bounding box and the "logical size".
The bounding box is the actual dimensions of the text in pixels once rendered (Font::getBoundingBox). The logical size is the rough size of the text to be used for things like layout (Font::getFontHeight and Font::getStringWidth). The bounding box needs to be computed from the individual character's bounding boxes while the logical size is computed from the advance value.

I suspect the issue lies in the Wintermute engine. Perhaps it is using the logical text size to create the target surface and then expecting the actual rendered text to fit. Maybe it's not using the appropriate font size selection mode.
Or maybe it's another bug in ScummVM's ttf code.

@bgK

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

I wonder if it's an issue related to the handling of kTextAlignRight in Font::drawStringImpl. The position where to start rendering the text is computed using the logical width but then the bounds checking happens using character bounding boxes. So if the bounding box of the last character is larger than the logical size then it is not rendered.
Now how to fix it .. Maybe the check should not happen when using kTextAlignRight as we know the last character fits. WDYT?

@lolbot-iichan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

I wonder if it's an issue related to the handling of kTextAlignRight in Font::drawStringImpl.

Well, it seems strange that the last character does not fit in kTextAlignRight case because of 1 pixel short space, not the first one. For me, it means that string may be shifted 1 pixel to the right. So, maybe kTextAlignRight is handled incorrectly.

However, there are other Wintermute issues with lost last letters, even with kTextAlignLeft, for some letters of some fonts. A quick example is russian edition of The Driller Incident game (free & small, get it here: http://questzone.ru/download/gr/drill_inc_ru.rar). On the very first screen captions of items that ends with 'a' letter were cropper as well ('буровая машин[а]', 'ламп[а]').

@lolbot-iichan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

I suspect the issue lies in the Wintermute engine. Perhaps it is using the logical text size to create the target surface and then expecting the actual rendered text to fit.

This is correct. ScummVM's Wintermute code is:

		for (it = lines.begin(); it != lines.end(); ++it) {
			textWidth = MAX(textWidth, _font->getStringWidth(*it));
		}

This fixes issues with kTextAlignLeft at Driller game, but does not fix issues with kTextAlignRight at 1 1/2 Ritter game:

		for (it = lines.begin(); it != lines.end(); ++it) {
			Common::Rect box = _font->getBoundingBox(*it);
			textWidth = MAX(textWidth, box.right - box.left);
		}

I wonder if it TextAlign really matters, or there is something else that make this patch of Wintemute work only for one of those two games, while current PR is fixing both of them...

@criezy

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

I am not familiar with the font code, but have you considered the possibility that there might be a 1 pixel error in the code because it does not take into account that the right edge of a Common::Rect is not in the rect, and thus that x + charBox.right is 1 pixel beyond the end of the char?

@lolbot-iichan lolbot-iichan changed the title COMMON: Last character of some TTF texts was cropped COMMON: Last character of some TTF texts was cropped (do not merge) Jul 4, 2019

@lolbot-iichan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

I'm sorry, but the changes are definitely incorrect.

ScummVM defines two ways to measure true type rendered text. The bounding box and the "logical size".
The bounding box is the actual dimensions of the text in pixels once rendered (Font::getBoundingBox). The logical size is the rough size of the text to be used for things like layout (Font::getFontHeight and Font::getStringWidth). The bounding box needs to be computed from the individual character's bounding boxes while the logical size is computed from the advance value.

It took me some time to understand this, but now I got it.
Just imagine font with absurdly wide overlapping letters. Of course those letters would have relatively small Width and extremely wide Bounding Box. And of course we should advance with something like x += font.getCharWidth(cur) and check borders with something like if (x + charBox.right > rightX) break;.

So, original PR is completely wrong. Will look into it deeper.

@scummvm scummvm deleted a comment from bgK Jul 4, 2019

@bluegr

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Since the design of this change is inherently wrong, I’m closing this pull request. A new PR can be opened, once a more correct approach is found to fix this issue

@bluegr bluegr closed this Aug 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.