Hide tonemap_exposure from editor when using Compatibility with BG_CANVAS.#113863
Hide tonemap_exposure from editor when using Compatibility with BG_CANVAS.#113863allenwp wants to merge 1 commit intogodotengine:masterfrom
tonemap_exposure from editor when using Compatibility with BG_CANVAS.#113863Conversation
allenwp
added
cherrypick:4.5
allenwp
force-pushed
the
compat-editor-fix-tonemap-exposure
branch
from
68b4b05 to
8e0e45d
Compare
allenwp
added
the
cherrypick:4.6
allenwp
force-pushed
the
compat-editor-fix-tonemap-exposure
branch
from
8e0e45d to
8d93f18
Compare
allenwp
force-pushed
the
compat-editor-fix-tonemap-exposure
branch
from
8d93f18 to
c02c568
Compare
There was a problem hiding this comment.
I'm not too sure about this, since the exposure still gets applied to 3D objects, it's just the canvas background that doesn't handle it.
Edit:
I've been looking into a solution and came up with this: https://github.com/blueskythlikesclouds/godot/tree/compatibility-canvas-background-exposure-fix
When exposure is enabled, we can force the intermediary buffer, and copy it with the exposure as the multiplier (alongside the luminance multiplier).
Converting the exposure to sRGB is an approximation. It might be more accurate to do this in the copy shader instead:
color = srgb_to_linear(color)
color *= exposure
color = linear_to_srgb(color)
color *= luminance_multiplierThis forces the intermediary buffer to exist even with glow off, but it might be better than the feature just not working at all.
|
Thanks! Of course, making exposure work is definitely a better solution than simply noting that exposure is not available for canvas background mode. I think I’ll need to look deeper into your proposed solution sometime later because there’s a lot of nuance regarding how exposure interacts with the glow effect and the encoding (linear or nonlinear sRGB) that is used during the calculations of the glow effect. Specifically:
To be honest, it will take me a few hours to get my head back into this, so I can’t remember the exact details, besides what I wrote in the PR’s description text. Feel free to open another PR in the meantime for me to review if you’d like, but otherwise I’ll simply look into what you’ve already provided sometime the future. Also, even if we fix this properly in the future, there may still be an option for using this PR as a cherry pick for the old versions. If we run out of time for a proper fix for 4.7, this PR may still be valuable to merge to master because it turns an obvious user-facing bug into a known limitation. Thanks again! |
3 participants
Issue description
Tonemap exposure does not work with the Compatibility renderer when the Environment background mode is set to Canvas:
post.glslto be usedThis issue is reproducible as far back as v4.3.stable.official [77dcf97]. In earlier versions of Godot 4, it is still reproducible, but the Canvas background mode seems to not function at all.
This issue is entirely separate from the issue that is fixed by #113861 as demonstrated by the optional step 4 that forces
post.glslto be used.This PR
This PR simply hides the non-functional tonemap exposure parameter in the specific configuration where it does not function. I have updated the docs to note this limitation.
Additionally, I have removed the old note that I had introduced as a part of #102820 that said:
Values provided to the tonemapper will also be multiplied by 2.0 and 1.8 for [constant TONE_MAPPER_FILMIC] and [constant TONE_MAPPER_ACES] respectively to produce a similar apparent brightness as [constant TONE_MAPPER_LINEAR].At the time I wrote this, I assumed it must be true, but for some scenes this is not true: The extra 2.0 and 1.8 multipliers actually make Filmic and ACES sometimes appear much brighter than Linear. So I really don't know why these extra multipliers exist and should never have added this unhelpful note in the first place.Why not just add support for exposure to Compatibility with
BG_CANVAS?I looked into this option and discovered that it is probably not worth it. Here is the summary of my decision:
post.glslto match the encoding that was used to create the glow textureSo currently, the behaviour is correct and as expected. If I were to add an exposure to
post.glsl, it would not correctly interact with the existing glow functionality's colour encoding.Maybe there is a way to work around this, but honestly I expect that very few people would even need this exposure feature for this specific configuration, so simply hiding it in the editor sounds to me like the correct choice. If there is a strong demand for the feature, we can try and solve this complicated problem then.
Cherry picks
I've added the cherry pick label for 4.4 and 4.5. Another label for 4.3 could be added, but I've chosen to omit this because the docs change will conflict. (It's a trivial conflict, though, so we could even cherry pick it to 4.3 if that's desired.)