Fix on-demand glyph loading #4779
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Web-eWorks
requested changes
Jan 30, 2020
There was a problem hiding this comment.
Looks pretty good to me at a first glance; I'll try to find the time to dig deeper into it soon and back up @ecraven's review.
One thing stands out to me; is there a reason for using std::unordered_map over std::unordered_set? Since we're mapping the glyph value to the... glyph value, it seems the latter might be the wiser choice here? AFAIK there's no particular difference in time complexity or performance.
vakhoir
force-pushed
the
bugfix/font_bake_memory_leak
branch
from
a095480
to
13a161f
Compare
Rename containsGlyph to isValidGlyph Change the approach to checking if a font can handle a glyph. The original approach was to pass the valid glyph range to the constructor. When it's not found by ImGui we call AddGlyph where we check if it's valid for the font. In the event where ImGui couldn't handle a glyph that was defined as valid in the constructor, that led to constant re-baking of fonts. The new approach is to change m_ranges (which used to contain valid glyph ranges) to m_invalid_glyphs, which contains only invalid glyphs that we tried to add to the font but failed. The next time AddGlyph is called, we skip the font since it's unable to handle the glyph.
vakhoir
force-pushed
the
bugfix/font_bake_memory_leak
branch
from
13a161f
to
13f7645
Compare
|
Alright, I'll pull this PR locally in about twelve hours and try to dig into it to double-check everything is working. Thank you @vakhoir for this bugfixing (and bugfinding too!) |
Web-eWorks
approved these changes
Feb 2, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #4778
For all the talk in the issue, the fix seems relatively simple, the following description is also in the commit message:
Change the approach to checking if a font can handle a glyph. The original approach was to pass the valid glyph range to the constructor. When it's not found by ImGui we call AddGlyph where we check if it's valid for the font. In the event where ImGui couldn't handle a glyph that was defined as valid in the constructor, that led to constant re-baking of fonts. The new approach is to change m_ranges (which used to contain valid glyph ranges) to m_invalid_glyphs, which contains only invalid glyphs that we tried to add to the font but failed. The next time AddGlyph is called, we skip the font since it's unable to handle the glyph.
But I have to say, I'm not entirely confident about what I did here. @ecraven I think you're the one that wrote all this, I also saw an issue you opened in the ImGui repo, is there any chance you can chime in on this?