imgui icon indicating copy to clipboard operation
imgui copied to clipboard

Keep track of missing glyphs

Open ecraven opened this issue 8 years ago • 9 comments

I've patched imgui locally to keep track of which glyphs are missing in a font, and provide that info to the library user, so that the font can be re-created with an appropriate glyph_range. I'd love for this to get into mainline imgui. Should I hide things behind an #ifdef, so people can disable it at compile-time, or is a runtime check good enough, for providing an actual pull request for you all to look at?

ecraven avatar Feb 06 '17 09:02 ecraven

Runtime check is probably only good enough if done very efficiently in a way that doesn't harm the performance of the CalcTextSize function. However that feature would pretty much be overlapping with the more desirable atlas update requirement.

If you let a frame elapse before rendering then those glyphs will be missing for one frame, so in theory the data needs to be computed immediately.

ocornut avatar Feb 06 '17 09:02 ocornut

I'm currently using this for a font merged from multiple TTFs, so I don't think imgui could update things correctly internally :-/ you are correct that glyphs are missing for a frame, but that's better than having to load all of CJK in multiple fonts and sizes, which I'd have to do right now :-/ Implementation-wise, I've just added code after the if in ImFont::FindGlyph (but before the return FallbackGlyph;). Not sure whether that would impede performance in some way :-/

ecraven avatar Feb 06 '17 09:02 ecraven

FindGlyph() is probably the best place to put it in term of performances, unfortunately CalcTextSize() doesn't uses it (it rather uses the densely packaged IndexXAdvance[] array), so that would also create an issue for one frame where both the Calc and Draw functions would use the fallback width.

It's actually a fairly bearable side-effect, and it avoid a lots of the trouble of getting it right, so it might be a great fit in term of the project philosophy. Maybe it is the best solution and those who want to avoid artefact that request glyphs to be loaded ahead.

I'll have to think about it. For now but it should be a reasonably easy patch to maintain locally if you are only adding to FindGlyph() - function is trivial and not likely to change.

Can you post of the gist of your changes here? (not as a formal PR, just a loose patch/dump of code) for reference.

ocornut avatar Feb 06 '17 10:02 ocornut

https://github.com/ecraven/imgui/tree/missing-glyphs The vector could be a boolean set, which would add about 8k per font, but might be faster. I'm not hung up on the actual code, just the general idea, I'd love to have a way to find out which glyphs are currently being rendered, but not in a font. What I have also done locally (not shown in the commit, to make it easier to see what is going on) is add a second optional parameter to FindGlyph, so that it only registers missing glyphs in ImFont::RenderChar and RenderText.

ecraven avatar Feb 06 '17 10:02 ecraven

Hi @ecraven . I've integrated your patch into my local copy of ImGui but am having issues. It keeps putting the glyphs back into the MissingGlyphs vector even after they are merged into the existing font. This a gist for the code I'm using in the master while loop - https://gist.github.com/chetan-prime/b8ab0382c64a2bf0500a3ea561106cdb

Expected Result: Missing Glyphs added to ImFont

Actual Output: The Glyphs don't get added or FindGlyph() keeps adding them back to the MissingGlyphs vector . I can't see the missing glyphs on screen also.

The below keeps looping endlessly -

ImFont::FindGlyph(20116)/adding 20116 to MissingGlyphsVector ImFont::FindGlyph(26376)/adding 26376 to MissingGlyphsVector ImFont::FindGlyph(36130)/adding 36130 to MissingGlyphsVector ImFont::FindGlyph(23500)/adding 23500 to MissingGlyphsVector ImFont::FindGlyph(21916)/adding 21916 to MissingGlyphsVector ImFont::FindGlyph(27426)/adding 27426 to MissingGlyphsVector ImFont::FindGlyph(20320)/adding 20320 to MissingGlyphsVector ImFont::FindGlyph(20170)/adding 20170 to MissingGlyphsVector ImFont::FindGlyph(22825)/adding 22825 to MissingGlyphsVector ImFont::FindGlyph(26159)/adding 26159 to MissingGlyphsVector ImFont::FindGlyph(20010)/adding 20010 to MissingGlyphsVector ImFont::FindGlyph(22909)/adding 22909 to MissingGlyphsVector ImFont::FindGlyph(26085)/adding 26085 to MissingGlyphsVector ImFont::FindGlyph(23376)/adding 23376 to MissingGlyphsVector glyph_ranges[0/1]=20116 glyph_ranges[2/3]=26376 glyph_ranges[4/5]=36130 glyph_ranges[6/7]=23500 glyph_ranges[8/9]=21916 glyph_ranges[10/11]=27426 glyph_ranges[12/13]=20320 glyph_ranges[14/15]=20170 glyph_ranges[16/17]=22825 glyph_ranges[18/19]=26159 glyph_ranges[20/21]=20010 glyph_ranges[22/23]=22909 glyph_ranges[24/25]=26085 glyph_ranges[26/27]=23376 missing_new.size()=0/AreGlyphsMissing=0/font=1b70180/ifont=1b70180

chetan-prime avatar Feb 16 '17 06:02 chetan-prime

I have it solved, @ecraven For anyone else that has the same issue using this patch, I'd like to confirm that the issue was solved by calling ResetMissingGlyphs() AFTER calling GetTexDataAsRGBA32(..). Calling GetTexDataAsRGBA32 to recreate the font texture seems to add the missing glyphs back into the MissingGlyphs vector (I think this isn't easy to avoid as the process of regenerating the textures must make a lot of calls to FindGlyph). Anyway the solution was dead simple. Works great now

chetan-prime avatar Feb 16 '17 18:02 chetan-prime

It's a really good news.! I may look into making an official version of something like that before implementing the full thing.

ocornut avatar Feb 16 '17 18:02 ocornut

I have it solved, @ecraven For anyone else that has the same issue using this patch, I'd like to confirm that the issue was solved by calling ResetMissingGlyphs() AFTER calling GetTexDataAsRGBA32(..). Calling GetTexDataAsRGBA32 to recreate the font texture seems to add the missing glyphs back into the MissingGlyphs vector (I think this isn't easy to avoid as the process of regenerating the textures must make a lot of calls to FindGlyph). Anyway the solution was dead simple. Works great now

Sorry, this is a detail I neglected to mention, I actually changed FindGlyph locally to take a second parameter (bool registerMissing = false), to make it only register missing glyphs on "external" calls to FindGlyph, not internal imgui bookkeeping ones.

I'll try to start an actual PR over the next week.

ecraven avatar Feb 17 '17 09:02 ecraven

Hi all,

I was about to make a similar feature request, but then found this existing issue. So +1 from my side for adding something like this in the official version :-)

The idea/motivation for my request was very similar. But I had a different implementation in mind:

  • Instead of adding an extra bool and ImVector inside ImFont (and corresponding getter/setters), I'd add a customizable callback function. Maybe with this prototype and default implementation:
const ImFont::Glyph* defaultMissingGlyphCallback(const ImFont* font, ImWchar missing) {
    return font->FallbackGlyph;
}
  • Users can then override this callback to build similar functionality as in ecraven's patch. Or still other functionality.

This callback approach might be less intrusive (smaller diff compared to current code). And less overhead for the majority of the users who don't need this.

Thanks for considering this.

m9710797 avatar Mar 09 '24 14:03 m9710797