-
-
Notifications
You must be signed in to change notification settings - Fork 362
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
Bug in font charset detection causing memory leak #4778
Comments
Just notice that the leak starts when you keep open the language select menu for a while yet, and this whatever your actual/new chosen language is. Also, the ship market tab is not affected (does not use decimal separator, but space as thousand separator is displayed correctly) |
FIX FAILED? Complete Console log of self-compiled program execution:
MADE EVEN WORSE? As mentioned above, this has been displayed correctly in Version 20191117, now there are also question marks and data are written to the swap partition: |
@Bodasey please remember to use the |
@Web-eWorks I took the liberty to edit his post. |
Does this mean you're on the master branch? The fix hasn't been merged yet, and is only available on my fork: https://github.com/vakhoir/pioneer/tree/bugfix/font_bake_memory_leak It should fix both the question marks and the memory leak (although a backup font is likely to be used, since ImGui doesn't detect a character that should be handled by the font).
Yeah, this is a PiGui bug. The version you had previously didn't have the ship market ported to PiGui yet, so that view was still fine. |
FIX CONFIRMED for vakhoir's version and German language |
Detected while testing #4695
Observed behaviour
Certain unicode characters are not displayed properly with some of the fonts, even when the font contains the character. This leads to constant re-baking of the font, and a huge (~100MB/s) memory leak.
Expected behaviour
Characters that are in a font are rendered correctly. If they aren't handled by the font, a fallback is used or an error is thrown. In either case, there should be no memory leaks.
Steps to reproduce
Set language to German
Start new game
Open the Equipment Market (or any view that displays a formatted money amount > $1 000). The formatted amount of money displayed should contain a '?' like in the following screenshot:
Causes
This is one of those bugs where a number of things has to happen at the same time for it to come up. First of all in the
format_money
function, if the locale has a white space set as the thousand-separator, we overwrite that with a unicode fixed space:pioneer/src/utils.cpp
Lines 43 to 45 in 99a2e0b
That's why it's necessary to switch to German, or another language that use a white space as the thousand-separator, in order to reproduce the bug.
When we try to render the character ImGui's
FindGlyph
function says it's not there, so a fallback character is displayed instead, and the fixed space is "reported missing", therefore PiGui tries to add it explicitly to the font, and rebake it. The twist is that PiGui saysPionilliumText22L-Medium.ttf
contains the\u00a0
fixed space, so it wants to keep using that font:pioneer/src/PiGui.cpp
Lines 139 to 146 in 99a2e0b
But when ImGui calls
stbtt_FindGlyphIndex
it can't find it, 0 is returned and the whole thing starts over in the next framepioneer/contrib/imgui/imstb_truetype.h
Lines 1501 to 1518 in 99a2e0b
So is it a Pioneer bug or an ImGui bug? A quick test seems to indicate that the "fixed space" character is indeed defined in the Pionillium font:
So it seems the problem is on ImGui side, but at the moment I can't make heads or tails out of what's going on in
stbtt_FindGlyphIndex
. Maybe someone else knows off the top of their head, if not I'll resume work on this later.P.S.
I'm pretty sure the bug is in font-parsing now. When I was debugging this I originally misunderstood the meaning of the
containsGlyph
function here:pioneer/src/PiGui.cpp
Lines 139 to 146 in 99a2e0b
So I thought "Wait, why are we adding a glyph when the font already contains it? Shouldn't it be the other way around?" and added a negation. It actually helped! Kind of. The character ranges on Pigui-side are hardcoded (we
should probablyhave to (see below) change that somehow, as well as the name of thecontainsGlyph
function andm_ranges
variable), so the alternative fallback fonts like DejaVuSans.ttf jumped into the AddGlyph block. On ImGui side the fixed space was also detected and displayed correctly, that's valid because that font also contains the character:Although it looks like it's defined differently, maybe that has something to do with it? Can't say I know much about the ttf format.
P.P.S
After thinking about it more, I guess the bug is also on the Pioneer side.
pioneer/src/PiGui.cpp
Lines 745 to 748 in 99a2e0b
This is the bit I mentioned, where the charsets are hardcoded, the last argument in each
PiFace
constructor is the range of valid characters the font is supposed to support, and Orbiteer and Pionillium have it set to from 0x0000 to 0xffff. Even if the fonts contained every single unicode character, this would still be wrong to have the range hardcoded. But since they don't have every character, this means we're treating that range as a preference - "ImGui, try to use this font to handle these characters, if you can, please". In that case we need to also handle the case when ImGui tells us it cannot handle that character with that font.Conclusions
We need to remove the hardcoding of
m_ranges
inPiFace
. Either try to read the valid character ranges from the file directly, or to save the glyphs we already tried loading with ImGui in a variable, and use it to return false incontainsGlyph
the next time the same glyph comes up.Should we drop that whitespace / unicode fixed space replacement in format_money? We seem to be able to render whitespaces just fine.
(Optional) figure out why ImGui can't read the fixed space character for Pionillium
(Optional) Rename
m_ranges
to something likem_valid_ranges
andcontainsGlyph
tocanHandleGlyph
, or describe what they do in comments. It's hard to figure out what these are actually for at first glance.The text was updated successfully, but these errors were encountered: