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

Tester wanted: Font atlas builder fixes (stb_truetype and freetype version) #2270

Closed
ocornut opened this issue Jan 10, 2019 · 7 comments
Closed

Comments

@ocornut
Copy link
Owner

ocornut commented Jan 10, 2019

I have rewritten some of the (messy) ImFontAtlas building code and pushed it in the atlas_fixes branch:
https://github.com/ocornut/imgui/tree/atlas_fixes
Before merging back into master I would appreciate if some testers could confirm that it works for them.

Testing NOT necessary if you are only using dear imgui with the default font.

Testing welcome IF YOU ARE:

  • using multiple fonts
  • using icon/merged fonts
  • using asian fonts or other fonts with non-default glyph ranges.
  • using freetype builder

I would appreciate if you could quickly merge this branch and verify that this works ok for you.
I have verified that you can easily merge atlas_fixes into docking or viewport without conflict, in case you are on one of those branches.

There's no change to API.

Those are the things that I changed:

  • FreeType builder: Fixed abnormally high atlas height and waste of texture space.
  • FreeType builder: Fixed support for any values of TexGlyphPadding (not just only 1).
  • Stb and FreeType builders: Atlas width is now properly based on total surface rather than glyph count (unless overridden with TexDesiredWidth).
  • Stb and FreeType builder: Fixed atlas builder so missing glyphs won't influence the atlas texture width.
  • Stb and FreeType builder: Fixed atlas builder so duplicate glyphs (when merging fonts) won't be included in the rasterized atlas.

It is now possible to safely load a font entirely by passing e.g. { 0x0020, 0xFFFF, 0 } range. Previously this would have various side effects on the building process, it now should be ok. (#2233).

Additionally the code is now a little less confusing and both the FreeType and Stb builders share a more similar code structure. There are various simplifications that should be made to the Stb builder (it is still a little more complicated than necessary, and some duplicate code between both builders will probably be exported into helper imgui_internal.h functions, but none of this is very important.

image

image

You may reply to this post with simple report e.g. "I tested the FreeType builder with a merged icon font and it was ok".

Thank you!

PS: Pro-tip, unrelated, but if your engine/renderer supports non power of two textures, you can set io.Fonts->Flags |= ImFontAtlasFlags_NoPowerOfTwoHeight to save texture space.

@ocornut
Copy link
Owner Author

ocornut commented Jan 11, 2019

Thanks for testing @Stfx !

Generated texture changed from 512x4096 to 1024x2048

Yes I tweaked the rules to approach a square texture.

Are you sure there are no performance downsides when using NPOT textures on OpenGL 3.3+ since the reduced texture height is marginal?

Absolutely no idea about that to be honest. Features such as mipmapping may affect it.

@Vuhdo
Copy link

Vuhdo commented Jan 11, 2019

Hi!
Judging from my quick & dirty test (well, not so quick giving the fact we haven't been updating upstream for a while) - the branch working (almost*) fine, no visible glitches or performance problems.

We use multiple fonts (Verdana for most texts, Consolas for monospaced texts, both with cyrillic glyphs merged with Font Awesome). The atlas size went down from 2048x2048 (80% wasted) to a nicely packed 1024x1024: http://gyazo.targem.ru/images/64f01fde6f1575a1d292ec905908.png

I haven't bother to check default stb rasterizer since we use FreeType backend exclusively.

  • There's some issue which I believe is a bug:

src_tmp.GlyphsSet.Resize(src_tmp.GlyphsHighest);

src_tmp.GlyphsSet.Resize(src_tmp.GlyphsHighest);

Here the bitset is being resized so that we can index the bits in [0,src_tmp.GlyphsHighest - 1] range, but we also need to take GlyphsHighest bit into account, so I've just quickly hacked around by adding +1 to make our code work.

ocornut added a commit that referenced this issue Jan 11, 2019
@ocornut
Copy link
Owner Author

ocornut commented Jan 11, 2019

Thanks for testing @Vuhdo !

Here the bitset is being resized so that we can index the bits in [0,src_tmp.GlyphsHighest - 1] range,

Correct, I fixed it now, thanks! It would have made an invalid access if ((GlyphsHighest % 32) == 0)

@Vuhdo
Copy link

Vuhdo commented Jan 11, 2019

Great, @ocornut !

In case you wonder how we use all those fonts here's the screenshot with some of our in-game tools, pretty intense text wise I guess: http://gyazo.targem.ru/images/190fd6648bea9037aa1e64803142.png
(we scale fonts at diffrent resolutions, I took this screenshot at 1440p).

So I'll take this opportunity to say - thanks a lot, Omar, for all your great work on ImGui!

@Vuhdo
Copy link

Vuhdo commented Jan 12, 2019

One more minor issue, while compiling with Clang (GCC issues similar error):


error : cannot initialize return object of type 'const FT_Bitmap *' (aka 'const FT_Bitmap_ *') with an rvalue of type 'bool'

ocornut added a commit that referenced this issue Jan 12, 2019
@ocornut
Copy link
Owner Author

ocornut commented Jan 12, 2019

One more minor issue, while compiling with Clang

Fixed and removed some redundant code that wasn't needed as well. Initially I tried to pull size information from the metrics, as your old comment didn't explicitely rule out that possibly. Then I found metrics on load weren't a reliable indicator of rasterized size so I switched to rendering in a temporary buffer.

Warnings: I added myself a note to add imgui_freetype to my testing/build scripts (I have scripts to build core imgui with many compilers but not imgui_freetype.cpp nor the back-ends).

@ocornut
Copy link
Owner Author

ocornut commented Jan 15, 2019

Merged this. Thank you for the feedback (here and sent privately).

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

No branches or pull requests

2 participants