-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Replace current ACES tonemapper with a high quality one #52476
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! The results certainly speak for themselves and the implementation is nice and simple.
Can you please cite your source for the changes in the PR (not in the code)? Its nice for future contributors to find out exactly where the implementation comes from.
I'm going to let @JFonS have the final say here, I know he has been thinking about redoing tonemapping for a while and he may notice something important that I have missed.
@clayjohn I've added the source and some implementation details which might help future contributors to understand what's going on. 🙂 |
I enabled the CICD tests so tests can proceed for any problems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great :)
I've been meaning to combine tonemapping and color grading into a single LUT and use this ACES implementation then, but at this rate, and seeing we left physical light units for 4.1, we might as well postpone the LUT until 4.1 too.
I don't think the new ACES implementation will be much slower than the current one, but it wouldn't hurt to check, especially on mobile.
@JFonS I can hardly wait! Sounds like a really great concept. I wonder how this ACES implementation could be integrated into a LUT workflow. |
@Lauson1ex The plan is to take Filament's implementation as a reference. They provide various tonemapper presets and allow for custom implementations, which they bake into a LUT together with the color grading transformations. Then the post-process shader just applies the Alexa Log C curve and samples the look-up table. The idea is to do this at the same time we change lights to use physical units, and we probably want to add physical properties to the camera (shutter speed, aperture and exposure and sensitivity), although we need to find a good balance between physical correctness and usability + artistic control. |
It'd be nice if the old Edit: issue opened here |
I am so happy that this tonemapper was added. It's a very impotants step towards allowing truly beautiful visuals in Godot. I've made a little test project to debug why does the glow effect produce RGB clipping when using ACES (Fitted) tonemapper (in Godot 4). When testing I have started to wonder - why does the tonemapping function saturate pure blue to violet? It's a bit weird, isn't it? |
Oh yeah, I thought I had my monitor configured wrong! I'm seeing the same blue -> violet change with ACES |
This happens because the color buffer which is passed to the glow post process shader is copied before the tonemapping. Rest assured that this is not a bug; this is intentional, and I'll explain why it has to be that way: tonemapping is a gamut compression technique which aims to remap colors to the visible sRGB range, which means it clips colors. The glow effect is supposed to emulate the signal bleed over a camera sensor surface area when any of its microsensors get oversaturated by a bright light ray. That means the glow post process needs the original, unaltered scene color data to detect when a "sensor" in our conceptual camera gets "oversaturated", that is, when a color is over the 1.0 threshold. Keep in mind that, radiometrically speaking, since the oversaturated signal of a real camera sensor looks more like a gaussian curve, the signal that bleeds over to the surrounding microsensors seldom get over the threshold, which mean that having a colored glow aura is the physically correct behavior:
This is known as the Abney Effect and it's a known issue of using the ACES color space. This has been a topic of debate for over a decade, and many have attempted to develop new perceptually uniform color spaces to work around this issue. This is a blue gradient made in Photoshop using the L* a* b* color space. Notice the hue skew to magenta: This happens because both the CIELAB and ACES color spaces are based on the same color appearance model. Keep in mind that this is how it behaves in Unity and Unreal as well. Taken straight from the Unreal documentation page: One easy workaround is to add a bit of green to the color (0.0, 0.01, 1.0) instead of using pure blue (0.0, 0.0, 1.0). This is sufficient to nullify the hue skew while still looking mostly indistinguishable from pure blue. I'm glad you brought this up because this is something I'm looking into tackling in the future. Instead of adding a hundred tonemappers to |
I'd try to integrate https://github.com/fire/godot/tree/littlecms but it's not in my direct work to load tonemappers from icc. |
Thanks a ton <3 - can this get documented in the page where the tonemappers are mentioned? (Forgot if it's environment or post-processing) |
Thank you for the detaield explanation, I am fully aware of this :D I know tonemapping should be the last step of the process to convert scene-linear color to sRGB color with pleasing results (and ACES tonemapper in Godot 4 si the best one we have). I've been talking to @Calinou on Godot Contributors Chat about why does Glow + ACES still produce RGB clipping (as if ACES wasn't there) and there are some theories about what's going on.
That is fascinating. I wonder how do artists work with this. It's a very strange color distortion that I find quite distracting...
I am thinking of a shader that could add a bit of green to all pure-blue colors to avoid this... Maybe we could add a toggle to apply sich a fix in Godot?
Yes, it's strange to me that Filmic doens't desaturate hilights. The "Filmic" in Blender does that. @Calinou told me that "Filmic" in Godot is the tonemapper from Uncharted 2 that was published under an open license. |
I think this is too specific to warrant a dedicated toggle, not to mention some people may actually desire this subtle purple haze. Also, if you follow general art guidelines, remember that pure red/green/blue colors rarely look good in practice; you always want to add some other color to the mix. See the Tailwind color palette for an example of this.
For Godot 4.0, couldn't we replace the existing Filmic tonemapper with your implementation like we did for ACES? @Lauson1ex Also, what do you think about applying tonemapping only once to both color + glow rendering instead of applying it separately to color and glow in two passes? Right now, this is only done when using the Mix glow mode, but we thought about doing this for all glow modes with unfa. (This is also the reason the Replace glow mode looks slightly different compared to the Mix glow mode with mix intensity = 1.0.) For reference, the code that applies glow is here: godot/servers/rendering/renderer_rd/shaders/tonemap.glsl Lines 405 to 444 in 482cdea
|
No need for a shader-level workaround, just a documentation hint would be enough. |
I guess that for 4.0 we could, it will depend on what the core team thinks of this change, and if users actually prefer this behavior. I don't know if there are any real use cases which require bright saturated colors to clip instead of desaturating. What do you think, @clayjohn and @JFonS?
The current implementation of the "screen" blend mode does this:
I understand the reasoning behind this decision; turns out that the "screen" blend mode in Photoshop requires inputs to be in the [0, 1] range. But doing this:
will certainly look more correct. But the way the glow buffer is |
Why would the Screen process omit the tonemapping on glow? That can't produce "correct" results, since the glow will clip. Why even bother with tonemapping in that process at all? It seems very strange to me.
This process you've described is the only thing that seems correct to me and should produce natural looking glow without clipping. with the glow coronas also desaturating in the center. I am not sure why would anyone want something else, unless it's a very specific stylistic decision, but I don't expect many people would do it if they knew the alternative. |
BTW, I've tested and the magenta skew on pure blue is not that big of a deal as I initially thought. Using any other color is enough to produce expected results with stable hue across different exposures. |
Replaces the current ACES tonemapper implementation with a high quality, industry-standard one. This one better represents how lighting works in the real world. High energy lights or emissive materials will become lighter as they get brighter, eventually becoming white if its color is bright enough to saturate the camera sensor.
Closes #3214 🙂
Old implementation:
New implementation:
Old implementation:
New implementation:
Old implementation:
New implementation:
Additionally, fixes a small bug in the tonemapper that clamped colors even when tonemapping was disabled. Fixes the issue found while reviewing PR #51708. Now the PR should work as expected. 🙂