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
base: mindsnacks-dev
Are you sure you want to change the base?
Conversation
β¦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.
There was a problem hiding this 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++) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this 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.
While working on a way to let
SizeConstrainedTextArea
find the optimal font size without actually rendering any textures, I ran into an issue withMOAIFreeTypeFont
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, callingmoaiFont:dimensionsOfLine("Γ₯pple", 10.0)
will (sometimes) cause a crash. π That's probably why we've avoided usingdimensionsOfLine
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 afor
loop so that it uses the correct index variable in its condition statement. Previously, thefor
loop was comparing to a different index variable... which got incremented by more than1
whenu8_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 calledFT_Done_Glyph
on them. π£After that fix, I also had to switch the calculation for
maxGlyphs
fromstrlen
(which doesn't handle multi-byte Unicode characters properly) to a different function defined inMOAIFreeTypeFont
(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. πππ