Apply glow and BCS adjustment in linear space

[beicause]

This is a balanced approach for #101497.
Glow is still blended after tonemapping but in linear space instead of sRGB space, which mainly makes blended colors not too bright. When brightness is low, the difference before and after this PR is small.

The left is 4.5dev4, right is this PR:

[beicause]

I think addtive can be applied before tonemapping, while screen and softlight applied after tonemapping.

Edit: This differs significantly from the original additive mode. It's better to retain the original one and introduce a new additive mode that applies before tonemapping, maybe a proposal is needed?

[fire]

In general a proposal is required for enhancements while bugs can be fixed with pull requests.

The difference is a bit blurry sometimes so I'll post a link to https://github.com/godotengine/godot-proposals

[beicause]

test_glow.zip

Agx:

-	Additive	Screen	Softlight	Pre Additive
Before				-
After

Filmic:

-	Additive	Screen	Softlight	Pre Additive
Before				-
After

[beicause]

I'm not sure whether we should stick to performing math operations in linear space. Aside from glow, BCS adjustments and color correction are also done in sRGB space currently. Should we change all of them to linear space ? However, this may change the appearance of existing projects.

Edit: Color correction texture is in sRGB space.

[EaryChow]

Start moving everything to linear is a good start. I am not sure how long it will take for Godot to get a color management system, but a proper color management workflow always unify all input color into a single linearized working space before all the color maths are done, and then convert them to display pixel percentage. The sRGB inverse EOTF encoding curve is for matching the hardware EOTF, the transfer function curve gets canceled out by its inverse version at the hardware, the end light emitted by the screen will always be linear light no matter what.

TL;DR: The sRGB transfer function curve is a technical compression scheme, it will be undone once the signal reaches the hardware, there's nothings special abou the "sRGB space". Unifying all colors in a single linearized working space is a good practice.

[fire]

@allenwp Can you take a look? I think it looks better.

[Calinou]

Tested locally, it works as expected on Forward+ and Mobile. I like the adjusted ranges for sliders in the inspector too.

Using https://github.com/perfoon/Abandoned-Spaceship-Godot-Demo as a test project and AgX tonemapping:

Additive

Before	After

Screen

Before	After

Softlight

Before	After

Mix

No visible difference.

Before	After

No glow (for reference)

[clayjohn]

I think this should be before BCS since BCS assumes sRGB-space inputs

[beicause]

Just corrected the formula for linear space.

[allenwp]

Because this sort of contrast function only works well when applied to perceptually uniform values, it probably makes the most sense to simply always apply BCS on nonlinear sRGB encoded values, regardless of HDR 2D setting.

[allenwp]

@allenwp Can you take a look? I think it looks better.

Thanks for the ping, @fire! I was actually planning to look into glow effects for the purpose of variable / Extended Dynamic Range (EDR) and HDR output! Specifically, I wanted to look into this possible issue with the HDR output PR. It's important that any changes to these sorts of effects are done with EDR in mind. (And just from reading the title of this PR, I suspect this PR would help a lot with EDR output!)

That said, I am always very hesitant to make changes that will affect existing project configurations, so matching existing visual behaviour is important. Judging solely on Calinou's screenshots, it's looking like this PR is taking this point seriously. Thanks!

@beicause you may be interested in reading this comment from my AgX update PR where I discuss why I needed to clip tonemap output values to the output maximum value. My PR is a change in behaviour that could affect how glow effects behave... My suspicion is that this PR will help glow effects behave better with my AgX update PR, but I haven't tested any of this yet.

[allenwp]

I've looked at the change and read the PR description and now understand what this does. It's not applying these effects in linear space (which happens before tonemapping), it's applying them in post-tonemapped space and postponing the inverse EOTF (nonlinear sRGB) transformation. Gotcha.

As I understand it, some effects are better applied in a nonlinear encoding space, such as log base 2 encoding, which approximately matches how humans perceive differences in brightness. Nonlinear sRGB encoding is absolutely the incorrect space to do these effects that would look best when applied in log base 2 encoding, but nonlinear sRGB encoding may actually be a "happy accident" that produces an effect that appears better because it's closer to the "ideal" log base 2 encoding than the linear scene values.

Additionally, some effects are better applied in a linear encoding space before tonemapping.

And finally, there are some effects that are best applied after tonemapping, but before the inverse EOTF (nonlinear sRGB) is applied. Much care must be taken to ensure that these effects behave in a stable fashion across all of variable extended dynamic range (EDR), SDR, and HDR. The sort of effects that I can imagine that are suitable for application in this post-tonemapping space are "tint" filters or certain artistic post-processing.

It is also reasonable to apply the final colour correction texture LUT in nonlinear sRGB encoding, given that a user will be producing a nonlinear sRGB encoded texture LUT. This simply makes for a good artist workflow and its up to their external tooling to correctly apply adjustments to the nonlinear sRGB encoded colour correction texture LUT.

So this PR, although being "less incorrect" than the existing implementation, may not be the fully correct solution that justifies a change in appearance of existing projects that have been meticulously tuned by their authors around the existing behaviour.

I will continue to look into this over the next few weeks, but you can try out this change with my EDR/HDR tonemapper prototype as well, if you'd like, to ensure that behaviour is stable and as-expected regardless of the output max value.

[beicause]

It's not applying these effects in linear space (which happens before tonemapping), it's applying them in post-tonemapped space and postponing the inverse EOTF (nonlinear sRGB) transformation. Gotcha.

See also #106995, which adds a glow mode that applied before tonemapping.

[beicause]

The blending in #106696 looks incorrect:

https://github.com/godotengine/godot/blob/b67a60ada5970a202a1b61dadb6dccda9d0765f7/servers/rendering/renderer_rd/shaders/effects/tonemap.glsl#L475-L477

After converted to nonlinear sRGB, the max output is no longer output_max_value.

Of course, output_max_value can be convert to nonlinear sRGB to match the max value, but I think blending in nonlinear sRGB may make the effects more difficult to adjust.

I agree this will change existing behavior, but it's unavoidable. Adding an optional setting for it isn't really worthwhile.

[allenwp]

I agree this will change existing behavior, but it's unavoidable. Adding an optional setting for it isn't really worthwhile.

Yes, this is sounding correct and I will look into this myself to verify that I come to the same conclusion.

My comment about how sometimes sRGB encoding is closer to the "correct" log base 2 encoding might be relevant for the brightness, saturation, and contrast adjustments, but I need to look into this more. My intuition is that there is no such thing as a correct encoding space to apply these specific transformations, so I'm not sure they should be changed any more than the minimal change to be stable in EDR...

I will look into this further sometime soon!

[EaryChow]

I believe #106995 is the only glow that works, all other ones are not "correct". That one applies glow to the camera exposure, making the glow independent of the image formation, you shouldn't need to care about whether the image is formed for SDR or HDR, since both image formation cases just convert camera exposure into image for displays, and since glow would be in the exposure data in both cases, it's irrelevant. I would advice dropping these "incorrect" glows in a big number release that can break backwards compatibility, but it's your decision to make of course.

My comment about how sometimes sRGB encoding is closer to the "correct" log base 2 encoding might be relevant for the brightness, saturation, and contrast adjustments, but I need to look into this more.

As I have said before, the Log2 and the sRGB transfer function are just compression schemes, they already abuse the happy accident that they store the computer bits in an interval closer to human visual sensitivity. Basically, they use less bits (or bandwidth, whatever you want to call it) where human vision is less sensitive. And since they are just compression schemes, the sRGB curve will be undone by the display hardware, and a log curve will be also undone into linear state if used in a modern color management workflow (like OpenColorIO) that unifies every color input into the same linear working space. But color maths are still best done in "Linear with respect to the standard observer luminance" state.

Image formation is different though, since it involves applying some curves to the exposure data, and then "interprete" the result to be a certain display transfer function encoding, image formation is about this "interpretation". But color maths in traditional sense should be done in linear state.

Creative color grading can also sometimes abuse different processing space's characteristics, since it's for creative purpose there is no "incorrect" in that sense. That's why OCIO's Look has configurable Processing Space for each look, though OCIO still puts all looks strictly before the view transform. OCIO basically pre-encode the color into the processing space, and undo the processing space back to the scene_linear global working space afterwards.

If you consider this "Image formation" terminology weird, I can explain more, but it's really actually kind of compatible with the widely used "scene referred vs display referred" terminology, since both philosophies recognize the pre-formation and post-formation state data changed the meaning during the process. They call the camera exposure "scene referred", since they kinda ignored the camera, and thought the exposure data is referring to the scene. I would call it "exposure referred" though. And "display referred" is more or less similar to AgX's terminology of "image/picture", they are both describing signals made for specific type of display, it's just that the "image" concept can be extended to drawings and paintings, while "display referred" is stuck with the digital color realm.

I would also like to point out that the "HDR" term now has two different meanings in Godot code, one is the "HDR vs LDR" thing as defined by the terminology of "Tonemapping" (which is an outdated term that I have been avoid using, I have been using "Image Formation" or the OCIO term of "View Transform" instead, and both dropped the "HDR vs LDR" concept. Some people also call it "DRT" for "Display Rendering Transform"), the other one is the "HDR vs SDR" thing as defined by the HDR/EDR display standard. The first "HDR" referrs to the camera exposure, or "Scene Referred" if you will; the second one refers to the image, or "Display Referred" if you will. There are a lot of historical burdens it seems, you folks need to clear these out.

[allenwp]

Yes, I agree with all of this.

Creative color grading can also sometimes abuse different processing space's characteristics, since it's for creative purpose there is no "incorrect" in that sense. That's why OCIO's Look has configurable Processing Space for each look, though OCIO still puts all looks strictly before the view transform. OCIO basically pre-encode the color into the processing space, and undo the processing space back to the scene_linear global working space afterwards.

This is what I was intending to communicate when I said that sometimes log base 2 encoding is the appropriate processing space for a specific artistic transformation. (And similarly, nonlinear sRGB.)

I think we’ll change the term “tonemapper” in Godot to be a different term after the other major game engines like Unreal drop the tonemap term.

[allenwp]

I've started looking into this. And the first thing that I've discovered is that FLAG_CONVERT_TO_SRGB is only set in the tonemapper when 2D HDR is disabled.

So this PR actually makes the tonemapper shader (and its glow and BCS functions) behave the same as simply enabling 2D HDR in previous versions of Godot.

And the real issue here that this PR addresses is that turning on and off 2D HDR in project settings affects the glow and BCS... and maybe even "color correction" in 3D scenes. This sounds like something that someone must have logged at some point. Anyone know of an existing issue?

Brightness:
I'm a little confused at why the original function is written with a mix call. Isn't it just the same as color = color * bcs.x;? Maybe I'm misunderstanding the math here.

Contrast:
If I were to re-write this, I would consider pivoting around 0.1841865 in linear space instead of 0.5, since this is "middle grey" according to CIELAB. But I don't think it's worth changing the existing behaviour. So this should probably just be kept at 0.5. (But if we’re changing behaviour for projects with 2D HDR disabled anyway, then maybe it’s a good time to change this? I haven’t even tested what a middle grey pivot looks like, maybe this formula doesn’t work that way.)

Saturation:
I understand the proposed approach of using CIE luminance as the saturation pivot, and it gives good results when desaturating, but I'm not sure if it gives good results when increasing saturation because it favours increasing saturation of greens over blues, for example. So the existing formula might be better suited, even when applied in linear space.

[beicause]

I've started looking into this. And the first thing that I've discovered is that FLAG_CONVERT_TO_SRGB is only set in the tonemapper when 2D HDR is disabled.
So this PR actually makes the tonemapper shader (and its glow and BCS functions) behave the same as simply enabling 2D HDR in previous versions of Godot.

AFAIK FLAG_CONVERT_TO_SRGB is set when 2D HDR is enabled or in 3D. 2D is not tonemapped and is always srgb .

Brightness:
I'm a little confused at why the original function is written with a mix call. Isn't it just the same as color = color * bcs.x;? Maybe I'm misunderstanding the math here.

They should be equivalent, but I'm not sure if there's any performance difference.

[beicause]

Contrast:
If I were to re-write this, I would consider pivoting around 0.1841865 in linear space instead of 0.5, since this is "middle grey" according to CIELAB. But I don't think it's worth changing the existing behaviour. So this should probably just be kept at 0.5. (But if we’re changing behaviour for projects with 2D HDR disabled anyway, then maybe it’s a good time to change this? I haven’t even tested what a middle grey pivot looks like, maybe this formula doesn’t work that way.)

It seems Contrast should pivot around 0.5, otherwise it will break linear interpretation gradient.
Before:

Details

After contrast=2.0 (The left pivots at 0.5, right pivots at 0.184):

Details

Contrast=4.0:

Details

Edit: The result of mixing at 0.184 is very close to mixing in sRGB.
Edit: I think contrast should be applied in sRGB or uses 0.1841865, otherwise it will produce incorrect brightness.

Details

The left is mixing in linear, the right is mixing in sRGB

The left is 0.1841865, the right is mixing in sRGB:

[allenwp]

I've started looking into this. And the first thing that I've discovered is that FLAG_CONVERT_TO_SRGB is only set in the tonemapper when 2D HDR is disabled.
So this PR actually makes the tonemapper shader (and its glow and BCS functions) behave the same as simply enabling 2D HDR in previous versions of Godot.

AFAIK FLAG_CONVERT_TO_SRGB is set when 2D HDR is enabled or in 3D. 2D is not tonemapped and is always srgb .

I've posted a bug to detail exactly what I was referring to: #107682

I also created my own variation on this PR in a work-in-progress branch with you as a co-author, but I'm still looking into how these changes should be made to be best suited for #94496:

https://github.com/allenwp/godot/tree/vulkan-linear-bcs-glow

[allenwp]

Does anyone subscribed to this PR have ideas on why Softlight glow is almost invisible when calculated on linear values? This is seen in both of these comments' screenshots:

#106962 (comment)
#106962 (review)

Of course, any issue that exists here already exists for the HDR 2D mode, but I'd like to get an understanding of why this is. I see there are a lot of "0.5f" used for logic in softlight. This is a weird magic number... For sRGB this would be around a middle grey value (perceptually around 50% lightness)... But for linear, it would be a very bright number (around 75% lightness I think?).

[beicause]

There are three soft light formulas, Godot uses Photoshop's https://en.m.wikipedia.org/wiki/Blend_modes. I didn't test if the other formulas are better.

[allenwp]

There are three soft light formulas, Godot uses Photoshop's https://en.m.wikipedia.org/wiki/Blend_modes. I didn't test if the other formulas are better.

Photoshop's was definitely designed to be performed on nonlinear sRGB values... It might be that this specific softlight function does not work correctly when HDR 2D is enabled... I'll look more into this and open a separate bug report if I find this is the case. I'm not sure what the solution should be just yet...

[allenwp]

To those subscribed to this PR, I am further exploring the approach introduced by this PR in godotengine/godot-proposals#13117

[allenwp]

I spent some time looking into the softglow blend function and have come to the conclusion that the approach used in this PR is the only reasonable approach for making glow behave consistently between HDR 2D enabled and disabled (that is, it's the only reasonable approach for calculating softlight glow on linear encoded values).

After plotting the softglow function, I discovered very quickly that the entire (glow.r <= 0.5f) will never apply because the glow is clamped to ensure that it is non-negative and then has 0.5 added to it before the check happens:

glow.rgb = clamp(glow.rgb, vec3(0.0f), vec3(1.0f));
glow = glow * vec3(0.5f) + vec3(0.5f);

color.r = (glow.r <= 0.5f) ? ...

So this actually simplifies the logic, since 1/3 of the math never applies. With this in mind, it becomes easier to reason about the softlight function: Softlight switches from a increasing glow influence on the image to a decreasing glow influence at 0.051 scene value with HDR 2D disabled and at 0.25 scene value with HDR 2D enabled. For reference, middle perceptual lightness is about 0.184. That leaves the question of which of these two is the appropriate one to use... But it seems that the function is only continuous with the conditional value set at 0.25, as demonstrated by this graph that uses 0.051 instead.

So all that to say, our hands are tied with the current function: it can only work with a conditional at 0.25, regardless of linear or nonlinear input values.

[beicause]

Closing, this PR still applies glow after tonemapping, I think in the long run for physically accurate glow we should blend glow before tonemapping, see godotengine/godot-proposals#12528 (comment) (And I have a working CompositorEffect that does this for me)

As for BCS adjustment, not sure if it worth the effort to change, IMO remaining in nolinear srgb is good enough for now.