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

ImGuiIntegration: use single-channel format for font texture atlas, if available #115

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

pezcode
Copy link
Contributor

@pezcode pezcode commented Nov 22, 2024

On OpenGL and GLES3 we can use a single-channel red texture together with 111r-swizzling, instead of wasting 3 bytes per pixel on RGB channels that get set to 1 anyway. Also adds a test for text rendering, mainly to test that the atlas uploading + sampling works for both configurations.

There's also a drive-by fix for an MSVC warning 🍒

@pezcode pezcode force-pushed the imgui-font-atlas-single-channel branch from 48d7bee to 2e45f10 Compare November 22, 2024 09:11
@pezcode
Copy link
Contributor Author

pezcode commented Nov 22, 2024

This may seem like a silly optimization, but for UI that supports things like Chinese characters, you quickly reach an atlas size of 100+MB. Also reduces bandwidth by a little bit, but not sure if font texture sampling is really a bottleneck for UI rendering.

Mainly to test that font atlas upload and sampling works, especially
when using single-channel format + swizzling on OpenGL and GLES3
@pezcode pezcode force-pushed the imgui-font-atlas-single-channel branch from 2e45f10 to 978dda8 Compare November 22, 2024 09:34
Slight differences on GLES, could be swiftshader?
Copy link

codecov bot commented Nov 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.32%. Comparing base (50ac4db) to head (ead3e8a).
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #115      +/-   ##
==========================================
+ Coverage   78.18%   78.32%   +0.14%     
==========================================
  Files          22       22              
  Lines        1022     1029       +7     
==========================================
+ Hits          799      806       +7     
  Misses        223      223              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mosra mosra added this to the 2024.0a milestone Nov 25, 2024
@mosra
Copy link
Owner

mosra commented Nov 25, 2024

Ha, few days ago I was actually thinking about doing the exact opposite for the Ui library... "if ImGui can get away with using a RGBA atlas for font glyphs, maybe the extra memory usage is not so bad in practice nowadays and I could just opt-in to use a RGBA format if people want to use emoji fonts?"

The patch makes sense to me (and thanks for the test as well), but I have some questions:

  • How does this work with emoji, or multi-color fonts in general? Does ImGui have a specialized secondary atlas for that? Or does this change break those?
  • Can ImGui be told to have a premultiplied alpha workflow? Then the swizzle would be r, r, r, r instead of 1, 1, 1, r and it could work with the ancient GL_LUMINANCE format as well, I think? On GLES2 at least.
  • Another way to make this work on GLES2 / WebGL would be to have a shader variant that performs the swizzle directly in code. But that means shader switching depending on what texture is used :/
  • For color font / emoji support in the Ui library (or the Text library in general) I was thinking of carrying some mask along with draw data to allow both single-channel and four-channel glyphs to live in a single atlas (single-channel glyphs making use of any of the R/G/B/A planes, multi-channel spanning all). That could solve the problem of shader / texture switching, but is probably not really applicable here.

@pezcode
Copy link
Contributor Author

pezcode commented Nov 25, 2024

How does this work with emoji, or multi-color fonts in general? Does ImGui have a specialized secondary atlas for that? Or does this change break those?

ImGui has no support for color fonts - GetTexDataAsRGBA32() simply fills in RGB with 111, and it exists for backends that don't support single-channel-alpha sampling.

Can ImGui be told to have a premultiplied alpha workflow?

That's a good question. Haven't found anything from a cursory glance. From what I understand GL_LUMINANCE returns rrr1, so if the premultiplied blend state needs alpha (and I believe it does), it'd be wrong. GL_ALPHA has the opposite problem, it becomes 000r.

Another way to make this work on GLES2 / WebGL would be to have a shader variant that performs the swizzle directly in code. But that means shader switching depending on what texture is used :/

Yeah I didn't like that too much since it means needing a custom shader instead of using the built-in Flat. But it would be a somewhat sane alternative. Personally I'm OK with leaving out GLES2/WebGL from this optimization. 🤷

I was thinking of carrying some mask along with draw data to allow both single-channel and four-channel glyphs to live in a single atlas

This briefly crossed my mind as well, it would definitely be useful to avoid running into texture size limits for the atlas. Our ImGui glyph atlas is already 8k by 4k, not hard to imagine going beyond 8k width if you include some exotic script support.

@mosra
Copy link
Owner

mosra commented Nov 25, 2024

ImGui has no support for color fonts

I'm vaguely aware of a "relatively recent" ImGui feature that supports SVG fonts, by calling into lunasvg / plutosvg to turn those into raster: ocornut/imgui@2ad8c60 Never used it personally, don't know any details, I just know it exists. Maybe it's a completely different code path that isn't affected by this change, maybe it's even something that would need additional changes in the integration lib for proper support (tangentially, I have WIP LunaSvgImporter, PlutoSvgImporter and ResvgImporter plugins that could be useful here, let me know if you'd have a use for those outside of just imgui/font support).

What I want to say -- if SVG fonts work already with the integration lib in some way, I fear of breaking existing use cases if I merge this PR as-is. Then there would need to be some kind of an option where one can control the channel count during context creation, and it would possibly need to be opt-in instead of opt-out. If SVG fonts don't work, and would need extra changes to work, then merging the PR as-is would be fine I think.

From what I understand GL_LUMINANCE returns rrr1

Yeah, true. And GL_LUMINANCE_ALPHA is useless because it's a two-channel format. So that's not a way to go, after all.

Needing a custom shader instead of using the built-in Flat

Quite a few times I ran into a problem where a generic asset loaded wrong because a monochrome PNG got optimized to single-channel, and the only portable solution was to manually expand or fiddle with plugin-specific config to get a RGBA again. So adding such an option as a builtin flag, to this shader and all others as well, makes complete sense. Or there's mosra/magnum#569 which could be finally kicked off by this (though such an implementation would make the eventual wgpu/vulkan backend harder).

Our ImGui glyph atlas is already 8k by 4k

In the Ui lib (and in the reworked Text::AbstractGlyphCache) I added support for texture atlases for this, to avoid hitting the limits. Again not sure how or if at all ImGui supports those, I'm thinking one could fake it with a specially crafted ImTextureId where some of the bits are reserved for layer index. But even then ImGui has to be aware of there being multiple atlases (can it do that? can it do atlas packing into N separate textures?), or there has to be some artificial partitioning of the 8K+ atlas into texture array slices...

@pezcode
Copy link
Contributor Author

pezcode commented Nov 26, 2024

Then there would need to be some kind of an option where one can control the channel count during context creation, and it would possibly need to be opt-in instead of opt-out.

Hum, I definitely misread the imgui code, good thing you poked a bit more at this 😅 The FAQ even explicitly mentions color fonts.

Luckily, ImGui::FontAtlas has a bool TexPixelsUseColors, which is precisely for this use case. If someone loads a color font, they're meant to set this to true. If it's false we can use the single-channel texture optimization. I'll fix that.

monochrome PNG got optimized to single-channel

That one is infamous. Up until recently we displayed red thumbnails for monochrome PNGs 😹

But even then ImGui has to be aware of there being multiple atlases (can it do that? can it do atlas packing into N separate textures?), or there has to be some artificial partitioning of the 8K+ atlas into texture array slices...

Haven't really looked into that, but from what I understand there's only one atlas. So you probably have to fix up glyph UVs manually after splitting the atlas? The texture ID is set on the atlas, so I suppose sneaking the z-slice in there won't work.

@pezcode pezcode force-pushed the imgui-font-atlas-single-channel branch from cd92508 to ead3e8a Compare November 27, 2024 14:23
@mosra
Copy link
Owner

mosra commented Nov 27, 2024

I'll merge as soon as I get the time, thank you. My only remaining question is whether there are any consequences to font loading that would be worth putting into the docs? Such as making sure color fonts are loaded early enough for the texture format to be correct, etc.

@pezcode
Copy link
Contributor Author

pezcode commented Nov 28, 2024

Such as making sure color fonts are loaded early enough for the texture format to be correct

You have a great talent of finding bugs by asking seemingly innocent questions 😄

The official freetype atlas builder sets TexPixelsUseColors during Build(), which is called on demand by GetTexDataXX(). relayout() however calls GetTexDataXX() after checking TexPixelsUseColors. So I suppose relayout() should explicitly call Build() first, then this will work out of the box. I'll try to test with the official freetype atlas builder and make that change 🙇‍♂️

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

Successfully merging this pull request may close these issues.

2 participants