-
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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
Apply gamma correction to fonts #4950
base: master
Are you sure you want to change the base?
Conversation
3f47dd4
to
c136d5a
Compare
I think this vaguely relates to #2943, since it ties into Dear ImGui's general handling of colour spaces. |
c817acb
to
8d39063
Compare
Hello, thank you for finding this problem and it gave me a lot of inspiration. Update: |
I think this is ready for merging. I've updated the title and description to be a bit clearer and also reference a FreeType webpage that explains the need for gamma-correction. |
d20d207
to
9bfcbe1
Compare
Would it be possible to have this change behind a define that a user can change if they don't want the fonts converted to gamma space? |
I haven't looked at it yet but either way I see no reason that this can't be configured by a runtime flag if needed. |
Great! Compile time or runtime flag would both work for me. I run my application in linear mode and convert to sRGB at the end and would have to "undo" this step above otherwise. |
After converting my app to use linear colors, sRGB textures, sRGB framebuffers, etc. (incl. ImGui, where I linearize the drawlist, at the cost of some precision loss), I ran into this odd behavior. It manifested as "wow my fonts look better when using ❤️ for noticing this! However, I'd echo @emoon 's request to put this behind some kind of runtime flag. The reason why is because I, probably like other devs in the wild, have gone through the effort of making a downstream project (this) use "correct" colors/lighting equations, but this PR would actually mean that I'd need to un-correct the fonts. |
Right now this is something of a hacky proof-of-concept draft commit. You see, the documentation of Dear ImGui's FreeType support (https://github.com/ocornut/imgui/tree/master/misc/freetype) mentions that FreeType outputs in linear space rather than gamma space. What I find strange is that the docs suggest that users should convert their entire app to render in linear space, when I'd argue that it's better to just convert the rendered glyphs from linear space to gamma space. This is exactly what I did in an old project of mine which used FreeType (https://github.com/EwanGreen4/CSE2EX/blob/master/src/Font.cpp#L964). It's also a fix that I provided to the GLideN64 project (gonetz/GLideN64#2030). Another thing that I found strange was that the documentation seems to be wrong about linear space rendering being exclusive to FreeType: when compared side-by-side, however, both FreeType *and* stb_truetype appear to produce identical-looking bitmaps when it comes to their gamma curve (or lack thereof). Unfortunately, I couldn't find anything in stb_truetype's documentation which specifies whether stb_truetype purposefully outputs in linear space or gamma space. I suppose I could ask a maintainer via a GitHub Issue or Discussion, or somehow try to comprehend the source code. With all of this in mind, I've applied de-linearisation to both font renderers, by hijacking the ImFontAtlasBuildMultiplyCalcLookupTable function. This function conveniently applies a post-processing effect to the glyph bitmaps, which is exactly what I need to apply my de-linearisation effect. Now, this function both applies brightening/ darkening, *and* de-linearisation. This does technically mean that glyph post-processing is now *always* applied, even when cfg.RasterizerMultiply is 1.0f. This makes some `multiply_table == NULL` checks in the FreeType renderer redundant. I can remove them in another commit if needed. I'm worried that I might be missing something obvious, which is why this commit is described as a draft: I'm really confused as to why there's all this strangeness about the linear space->gamma space process, and I think I might be misunderstanding something on my end. In the associated Pull Request for this commit, I'll try to provide some screenshots to help illustrate my points.
Made redundant by the previous commit
As explained in 9b17026, I'm pretty sure that both stb_truetype and FreeType output linear - it's not a quirk exclusive to FreeType. Additionally, said commit makes this a non-issue anyway, so this statement is no longer needed.
Making it static ensures that it isn't copied to the stack.
9459d0c
to
a3dda47
Compare
The documentation of Dear ImGui's FreeType support mentions that FreeType outputs in linear space rather than gamma space. What I find strange is that the docs suggest that users should convert their entire app to render in linear space, when I'd argue that it's better to just convert the rendered glyphs from linear space to gamma space. This is exactly what I did in an old project of mine which used FreeType. It's also a fix that I provided to the GLideN64 project.
Another thing that I found strange was that the documentation seems to suggest that linear space rendering is exclusive to FreeType: when compared side-by-side, however, both FreeType and stb_truetype appear to produce identical-looking bitmaps when it comes to their gamma curve (or lack thereof). Unfortunately, I couldn't find anything in stb_truetype's documentation which specifies whether stb_truetype purposefully outputs in linear space or gamma space. I suppose I could ask a maintainer via a GitHub Issue or Discussion, or somehow try to comprehend the source code.
With all of this in mind, I've applied gamma correction to both font renderers by hijacking the ImFontAtlasBuildMultiplyCalcLookupTable function. This function conveniently applies a post-processing effect to the glyph bitmaps, which is exactly what I need to apply my gamma correction. Now, this function both applies brightening/darkening, and gamma correction. This does technically mean that glyph post-processing is now always applied, even when cfg.RasterizerMultiply is 1.0f, so I've removed the
multiply_table == NULL
checks in the FreeType renderer that were made redundant by this change.The FreeType Group explains the needs for gamma correction here.
Here's a comparison of font renderers with and without gamma correction (the font is Karla Regular at 22px):
FreeType w/out:
stb_truetype w/out:
FreeType w/:
stb_truetype w/:
At native resolution, stb_truetype with gamma correction appears to produce a more pleasing result, suggesting that the default appearance without gamma correction is incorrect:
FreeType w/out:
stb_truetype w/out:
FreeType w/:
stb_truetype w/: