Use half float precision buffer for 3D when HDR2D is enabled#109971
Conversation
|
Does this mean even not targeting 2D, enabling |
That's right! Thank you for pointing that out |
|
If so, this may cause a performance regression in 3D projects with HDR2D enabled, which could be unexpected (the term "HDR 2D" is not accurate anymore, docs need changes). |
That's right. We discussed the performance tradeoff at length in #86098 and in subsequent rendering meetings and decided that it was worth the change. If 2D is using full HDR, then the entire pipeline should use full HDR. That being said, its not necessarily going to be a performance regression because:
Overall, we felt that making this change was worthwhile. |
There was a problem hiding this comment.
Tested locally on many demo projects and https://github.com/Calinou/godot-reflection, it works as expected.
However, the sky appears visibly darker in Truck Town when HDR 2D is enabled in the project settings:
hdr_2d_mobile.mp4
This also carries over to the running project. However, reflection probes are not affected by this issue, even if you force them to update by moving them after changing the setting:
hdr_2d_mobile_reflection_probes.mp4
(The NaN you see in the reflection probe on the sun also occurs in master; I'll open a separate issue. Edit: #110207)
Either way, this has the upside of making viewport debanding functional in Mobile if HDR 2D is also enabled.
It's worth investigating for a future PR if we should also switch over the sky and reflection probes to HDR. This would improve visual consistency with Forward+ and increase the viability of HDR output for the Mobile renderer, but this needs its own proposal.
See the last paragraph in the PR description above
|
This comment was marked as off-topic.
This comment was marked as off-topic.
|
I'm actually not immediately sure why from glancing at the code changes, but this PR breaks debanding in the following configuration:
View these screenshots in a dark viewing environment at 100% scaling:
MRP: mobile-hdr-2d-debanding.zip SMAA causes the first tonemap pass to match the scene rendering buffer format, which is 10-bit when HDR 2D is disabled, so 10-bit debanding in the tonemap shader is required for this configuration. The code that detects what buffer format should be used for debanding makes a lot of assumptions about data formats because the exact format of a given buffer isn't really saved in an accessible way for easy access. It's possible that the debanding code now thinks that this configuration is using a 16 bit buffer for even though it is actually using a 10 bit buffer? Edit: Sorry, some of what I wrote in the first pass of this comment wasn't quite right... I think I've corrected it(?) To be honest, I need to dig deep into this every time I try and think about it because it's a little complicated to keep track of the different buffer formats used through the different passes with each renderer and HDR 2D configuration 😵💫 |
6368ddb to
c09e514
Compare
|
@allenwp I think I may have fixed that issue in the last rebase when I fixed the conflicts. I can't repro that issue right now. Your MRP looks totally fine and matches the behaviour in Godot 4.5 
|
c09e514 to
74d9971
Compare
This is necessary for Environment effects like Glow to work correctly.
74d9971 to
f61ee7b
Compare
|
Thanks! |
Just got around to testing this and reviewing the updated PR that was merged: yes, this looks good! Thanks! |
Fixes: #86098
Fixes: #106282
Closes: godotengine/godot-proposals#7878
This took a bit of refactoring. Originally we selected a 3D framebuffer format based purely on whether we were in the mobile renderer or the Forward+ renderer. For the 2D framebuffer, we selected based on the presence of the HDR2D setting.
This created a strange incompatibility when using glow with HDR2D with the mobile renderer. Your 2D content would be rendered in HDR. Then copied into an LDR framebuffer. Then copied back to HDR with glow applied.
While there is a theoretical benefit to using an LDR framebuffer in 3D (especially when using glow), it just isn't compatiblity with 2D HDR. It is much better quality-wise to keep everything in HDR from beginning to end.
The biggest change is that we now look to the RenderBuffers class to determine if we should use an HDR format and whether we need to use a luminance multiplier. The RenderBuffers are automatically recreated if we switch to HDR, so this ensures that every part of the pipeline uses the same settings.
Reflections and sky still use LDR always since they can be shared between several Viewports and not all viewports necessarily have the same HDR2D setting. In the future we may want to consider making ReflectionProbes and Sky always use HDR. But that decision is out of scope for this PR.