Fix inconsistent CanvasModulate color in 2D HDR #93802
Conversation
There was a problem hiding this comment.
Tested locally, it works as expected when the root viewport has HDR 2D enabled. However, when the root viewport has HDR 2D disabled, this makes CanvasModulate in SubViewports no longer match each other depending on whether they have HDR 2D enabled.
The SubViewport on the left has HDR 2D disabled, while the one on the right has HDR 2D enabled. Using Forward+.
Testing project: test_pr_93802.zip
HDR 2D enabled on the root Viewport (working correctly in this PR)
| Before | After |
|---|---|
 |
 |
HDR 2D disabled on the root Viewport (not working correctly in this PR)
| Before | After |
|---|---|
 |
 |
|
@Calinou |
| state_buffer.canvas_modulate[1] = p_modulate.g; | ||
| state_buffer.canvas_modulate[2] = p_modulate.b; | ||
| state_buffer.canvas_modulate[3] = p_modulate.a; | ||
| bool use_linear_colors = texture_storage->render_target_is_using_hdr(p_to_render_target) && GLOBAL_GET("rendering/viewport/hdr_2d"); |
There was a problem hiding this comment.
The project setting should likely be checked in the constructor only once, and saved in a member variable, for performance reasons. Reading project settings for each render command is going to be expensive.
That being said I'm not sure this is the correct check. I haven't looked in depth but I believe rendering/viewport/hdr_2d only sets the default value for the root viewport. It can still be changed later on for any viewport with RenderingServer::viewport_set_use_hdr_2d and Viewport::set_use_hdr_2d.
I'm no rendering expert so TIWAGOS1, but it sounds to me like this might need to check the value for the target viewport. No idea how that's done at this stage of the render pipeline, @clayjohn might know.
Footnotes
-
Take It With A Grain Of Salt ↩
There was a problem hiding this comment.
I suspect the original fix was correct. Taking a look now.
It seems like the demo project is treating the linear HDR Viewport's texture as if it is sRGB color texture. If so, that would be the source of the problem.
There was a problem hiding this comment.
Ya, the problem comes from the demo project. When HDR_2D is disabled on the root viewport, the root viewport expects to receive sRGB textures (it doesn't do any color conversion). However, the demo passes a linear HD texture which ends up looking incorrect. Simply doing the linear->sRGB conversion in the shader makes the output correct.
This issue highlights a broader issue which is that mixing HDR_2D and non HDR content is still confusing for users and prone to error. So there is likely stuff we can do to improve the situation.
As for this PR, we should not be checking the project setting, the code should be reverted back to the way it was
akien-mga
force-pushed
the
Fix-inconsistent-CanvasModulate-color-in-2D-HDR
branch
from
181a4db to
6f30df4
Compare
|
Thanks! And congrats for your first merged Godot contribution 🎉 |
Linearize CanvasModulate color if HDR 2D is on.
Fix #92796