Improve Environment adjustments (favor performance).#110630
Conversation
e74abc8 to
9219c54
Compare
Environment adjustments consistent between rendering configurationsEnvironment adjustments consistent between rendering configurations
69984ac to
c201743
Compare
|
This PR is now ready for review and merging after #110671 has been merged. |
d7f033e to
3590115
Compare
|
For reasons described in this comment, I have pushed a change that removes the "Legacy mode" project setting. |
Calinou
left a comment
There was a problem hiding this comment.
Tested locally on all renderers, it works as expected. Code looks good to me.
Note that zero and negative contrast/saturation are now allowed to be set in the inspector (using or_less), but this was already possible in master using a script (and looked correct in all renderers). It still works in all renderers, so it makes sense to expose in case you want to go for silly effects.
Thanks as always for the review! I got the idea of allowing negative values in the editor from #106962 -- I can't imagine any problems coming from this behaviour, and the editor's primary range for the slider is still limited to non-negative values which implies that non-negative values are the "normal" range for these adjustments. |
Environment adjustments consistent between rendering configurationsEnvironment adjustments.
|
In response to the discussion in last night's rendering meeting, I've updated this PR's text and commit text to better describe what problems this PR solves. |
3590115 to
a20fce5
Compare
a20fce5 to
3e42af8
Compare
|
@allenwp To clarify: was this approved at the meeting? |
Not approved yet because the content of the PR has not been reviewed by anyone except Calinou. In the meeting we only discussed the messaging around this PR, which is why I updated the PR description and commit message text. |
| if (bool(params.flags & FLAG_CONVERT_TO_SRGB) || bool(params.flags & FLAG_USE_COLOR_CORRECTION)) { | ||
| color.rgb = linear_to_srgb(color.rgb); | ||
| } |
There was a problem hiding this comment.
My feeling is that we should just convert to sRGB before applying BCS. Then convert from sRGB back to linear after applying color correction. That would maintain compatibility completely and would require way less complexity here.
That comment below color correction about the color correction texture doing the srgb->linear conversion is bogus. That would only be the case for users who specifically bake srgb->linear into the color conversion texture. Which as of Godot 4.5 is going to be zero because it is currently totally in sRGB space.
There was a problem hiding this comment.
That comment below color correction about the color correction texture doing the srgb->linear conversion is bogus. That would only be the case for users who specifically bake srgb->linear into the color conversion texture. Which as of Godot 4.5 is going to be zero because it is currently totally in sRGB space.
This comment is not bogus. See the implementation here, which uses a trick on the texture setting to circumvent the need for one of these two necessary conversions: https://github.com/godotengine/godot/pull/107782/files#diff-a1edd0a9251f7120d8b7dded07217bf5dfbf748bf3bd4c8abed63e577e2bde32R678
(If the link doesn't work, it's the changes in #107782 to RendererSceneRenderRD::_render_buffers_post_process_and_tonemap)
There was a problem hiding this comment.
My feeling is that we should just convert to sRGB before applying BCS. Then convert from sRGB back to linear after applying color correction. That would maintain compatibility completely and would require way less complexity here.
I'll open up a new PR with an approach like this that favours maintaining compatibility with HDR 2D disabled!
3e42af8 to
31cf82c
Compare
This commit changes adjustments to behave as follows for all rendering configurations: - Apply brightness to linear-encoded values, preventing hue and saturation from being affected. - Apply contrast to linear-encoded values, allowing for highest performance in all rendering configurations. - Apply saturation with luminance weights, preventing brightness from changing with certain colors. Adjustments are applied after glow and tonemapping to match existing behavior.
31cf82c to
16fb53c
Compare
Environment adjustments.Environment adjustments (favor performance).
|
From #110630 (comment):
I've implemented this idea that maintains compatibility in a separate PR for comparison: #111897 I've also implemented a followup PR that can be considered entirely separately relating to changes the saturation adjustment: #111898 |
|
Closing as superseded by #111897 |
Environmentadjustments. godot-proposals#13195This PR supersedes:
This commit changes adjustments to behave as follows for all rendering configurations:
Adjustments are applied after glow and tonemapping to match existing behavior.
Summary
This PR is an alternative approach to the following:
Environmentadjustments (favor old behavior and quality). #111897Rationale and screenshots
Rationale and screenshots for this PR can be found in godotengine/godot-proposals#13195.
Changes and improvements
The following is a summary of the key points that are further detailed in the proposal.
Brightness
HDR 2D Disabled
Problem: When HDR 2D is disabled in Godot 4.5 and earlier, the brightness adjustment is applied as a scaling of nonlinear sRGB-encoded values. This is incorrect in terms of CIE colorimetry and results in a change in saturation, hue, and contrast when brightness is adjusted.
To correct this issue, scaling is applied to linear-encoded RGB values, which results in relative luminance being scaled directly without affecting hue or saturation:
This PR also removes clipping from the conversion to nonlinear sRGB encoding, which may have impact on some scenes with bright values. A future PR could add back in explicit user-controlled clipping of bright values before applying adjustments, but I believe that the new behaviour in this PR is generally preferred:
HDR 2D Enabled
Problem: When HDR 2D is enabled in Godot 4.3 through 4.5, the brightness adjustment is not perceptually uniform, which makes it difficult to use as a "fade to black" effect.
In this video I demonstrate the two approaches to the brightness adjustment: brightness fades in and out with the sRGB transfer function and then without (cycling back and forth between the two). Note that without the sRGB transfer function, the image fully darkens very suddenly, making for a more abrupt transition:
brightess-scale.mp4
This PR corrects this issue by making the brightness adjustment more perceptually uniform when HDR 2D is enabled by using the nonlinear sRGB transfer function. The brightness adjustment is still correctly applied to linear-encoded RGB values.
Contrast
Problem: The contrast adjustment heavily darkens the image when HDR 2D is enabled and is very difficult to use and fine-tune. This problem happens because the contrast pivot is at a linear value of
0.5instead of a more common value of around0.18(middle grey) or0.214041140482232(sRGB 50%).This PR corrects this problem by changing the contrast pivot point to be
0.18, which better matches industry standards.Note: In order to avoid an additional round trip to and from nonlinear sRGB encoding, the contrast adjustment behaves differently with this PR compared to Godot 4.5 when HDR 2D is disabled.
Saturation
Problem: Low-luminance colours change in apparent brightness when saturation is changed. A prime example is deep blue colours which appear to brighten when saturation is decreased and darken when saturation is increased.
The solution to this problem is to simply apply saturation to linear-encoded values using CIE luminance weights for the Rec. BT.709 primaries that Godot uses:
Docs
I suggest to use
tonemap_exposureinstead of scene brightness becausecamera_attributesexposure does not affect scenes without a camera that target a "canvas" background mode. But currently,tonemap_exposureis just not implemented at all for the Compatibility renderer.Notes to reviewers