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

Fixing a crash on mutli-byte Unicode characters in MOAIFreetypeFont:dimensionsOfLine #132

Open
wants to merge 2 commits into
base: mindsnacks-dev
Choose a base branch
from

Conversation

aab29
Copy link

@aab29 aab29 commented Nov 4, 2021

While working on a way to let SizeConstrainedTextArea find the optimal font size without actually rendering any textures, I ran into an issue with MOAIFreeTypeFont and decided to address that first instead. πŸ” πŸ”§

Basically, the method _dimensionsOfLine crashes (intermittently) when called on strings that contain multi-byte Unicode characters. πŸ’£πŸ’₯ For example, calling moaiFont:dimensionsOfLine("Γ₯pple", 10.0) will (sometimes) cause a crash. πŸ™ˆ That's probably why we've avoided using dimensionsOfLine in our entire codebase. πŸ’β€β™‚οΈ

It turns out the crash was happening because there were glyphs that were getting freed by FT_Done_Glyph that weren't allocated to begin with. πŸ™… I fixed it by changing a for loop so that it uses the correct index variable in its condition statement. Previously, the for loop was comparing to a different index variable... which got incremented by more than 1 when u8_nextchar encountered a multi-byte Unicode character. ❌ That threw off the loop that initialized the glyphs... which is why some of them weren't initialized when we called FT_Done_Glyph on them. πŸ’£

After that fix, I also had to switch the calculation for maxGlyphs from strlen (which doesn't handle multi-byte Unicode characters properly) to a different function defined in MOAIFreeTypeFont (glyphsInText). Before I changed that, labels that included multi-byte Unicode characters would sometimes render with garbage characters at the end. πŸ—‘οΈ

The code in this file is pretty sloppy (to say the least 😬), so after I tested out the fixes, I started to try renaming variables and doing some light refactoring and tidying up. πŸ•³οΈπŸ‡ I got through the two methods I modified, but I ended up changing practically every line of code in them. 🧹🧹🧹 I still have that cleanup on a separate branch, but I figured we should start with just these three lines for now to keep the important change isolated and minimize risk. 🎲 If we do decide to go ahead with code cleanup in MOAIFreeTypeFont, I would advocate for rolling it out over the course of several sprints so we can have more confidence we're not introducing bugs to such a sensitive part of the games stack. πŸ—ΏπŸš«πŸ›

For what it's worth, I made good progress on the no-render-SizeConstrainedTextArea project, but since it depends on _dimensionsOfLine, it'll have to wait for a future 5% Day. πŸ˜πŸ”œπŸ“†

…OfLine`. Previously, there was an intermittent crash in this method that happened when `deleteGlyphArray` got called, which called `FT_Done_Glyph` on glyphs that hadn't been initialized. They weren't initialized because the `for` loop ended before `maxGlyphs`... since `u8_nextchar` incremented `n` by a value greater than 1 when it encountered multi-byte characters.
…ngleLine` to use `glyphsInText` instead of `strlen`. `strlen` did not properly count multi-byte characters, which sometimes led to extra glyphs getting rendered at the end of a line.
Copy link

@andrewcousins7 andrewcousins7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find! It always sort of bothered me that I had to render text to determine the size it would be (hence the problem you were trying to solve in SizeConstrainedTextArea), but I guess "the code that can do that crashes sometimes" is as good a reason as any.

@@ -590,7 +590,7 @@ USRect MOAIFreeTypeFont::DimensionsOfLine(cc8 *text, float fontSize, FT_Vector *
// Gather up the positions of each glyph
FT_Int penX = 0, penY = 0;

for (size_t n = 0; n < maxGlyphs; ) {
for (size_t n = 0; numGlyphs < maxGlyphs; numGlyphs++) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This for loop feels really weird. I'm seeing how trying to clean it up expands into a much larger task. How would you feel about just switching it to a while loop until that cleanup happens, since it might make it a little more readable?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Does the value of n ever change in this loop?

Copy link

@jpoka jpoka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on an iPhone 13 Pro Max in standard and zoomed mode. Looked good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants