Fix specular envmap in deferred #11534

atlv24 · 2024-01-26T05:02:38Z

Objective

Fixes Environment map not working when using deferred rendering #11414

Solution

Add specular occlusion to g-buffer so PbrInput can be properly reconstructed for shading with a non-zero value allowing the spec envmap to be seen

IceSentry · 2024-01-26T05:10:39Z

crates/bevy_pbr/src/deferred/pbr_deferred_functions.wgsl

@@ -80,9 +80,11 @@ fn pbr_input_from_deferred_gbuffer(frag_coord: vec4<f32>, gbuffer: vec4<u32>) ->
    let props = deferred_types::unpack_unorm3x4_plus_unorm_20_(gbuffer.b);
    // Bias to 0.5 since that's the value for almost all materials.
    pbr.material.reflectance = saturate(props.r - 0.03333333333);
+    pbr.specular_occlusion = props.b; // use diffuse occlusion as specular occlusion because we can't fit both in the g-buffer on webgl


Shouldn't we just disable specular occlusion on webgl instead? I don't really know the impact of doing that tbh.

i figured that would be a closer approximation than not occluding specular at all. this was discussed quite extensively around the original specular occlusion PR, but if not occluding specular at all on webGL is preferred then I don't really mind changing it

I don't really have an opinion personally, it was mostly out of curiosity. I guess this is better than nothing yeah.

I'd rather disable specular occlusion personally, and leave diffuse occlusion functional, if it's a choice between the two.

But also, does it have to be a choice? Can't the deferred function just calculate the specular occlusion based on the diffuse occlusion? Or is the needed data no longer available, idr.

As far as I know diffuse occlusion will not be affected by this

What does this mean then?

use diffuse occlusion as specular occlusion because we can't fit both in the g-buffer on webgl

It uses the diffuse occlusion value for specular occlusion, but the diffuse occlusion value is also used for diffuse occlusion. At least, that's my understanding.

That understanding is correct. The needed data for specular occlusion calculation is not present here anymore, you need the non-multibounce raw SSAO output, and a couple other things that im pretty sure we do have, like NdotV type stuff.

Ah I get it now, the wording confused me 😅

It's pretty debatable if it's even worth including occlusion in the deferred gbuffer in the first place. The gbuffer space is pretty precious, and baked AO is quite rare.

crates/bevy_pbr/src/deferred/pbr_deferred_functions.wgsl

Co-authored-by: JMS55 <47158642+JMS55@users.noreply.github.com>

JMS55 · 2024-01-26T20:01:32Z

Oh btw, did we document anywhere that specular occlusion doesn't work on webgl2, and it'll use diffuse instead? If not, please add that to the StandardMaterial docs.

atlv24 · 2024-01-26T20:14:00Z

I dont see why it should be in standard material docs, but I could add it there. There's no specular_occlusion input, the occlusion_texture field only affects diffuse. This is an SSAO/PBR effect, maybe it can be mentioned on the SSAO docs.

JMS55 · 2024-01-26T20:32:52Z

Ah right. Docs on either the PbrInput in the shader or in SSAO, or in deferred would be fine, up to you where you think it's best. I just think we should note this down somewhere.

DGriffin91 · 2024-01-26T22:18:48Z

@JMS55 There shouldn't be any limitation keeping specular occlusion from working in webgl2 vs diffuse with the current impl. The current XeGTAO impl in bevy doesn't work in webgl2 (though it could), but that's a separate issue.

atlv24 · 2024-01-26T23:36:16Z

Yeah it would just mean adding another g-buffer texture on webGL, which could hurt performance. I think that can be done in a follow up with its own perf testing to assess whether its worthwhile. The visual difference will not be that big and we're already monochromizing the diffuse occlusion for gbuffer's sake.

atlv24 · 2024-01-27T02:03:42Z

I just realized this fix is wrong and the correct fix is much simpler, here's why:

the deferred lighting example doesn't have SSAO on
PbrInput::specular_occlusion is only written by SSAO
reconstruction assumes there's data there even though SSAO wrote nothing and reads a zero
spec envmap gets multiplied by zero in deferred without SSAO

this means that a correct fix would just be to initialize specular_occlusion to 1.0 when there's no SSAO. If SSAO is enabled, then we get proper specular occlusion. If its not, then we get spec envmap unoccluded (we have no data with which to occlude it with).

DGriffin91 · 2024-01-27T02:05:46Z

I definitely don't think we should add another texture for baked occlusion. At some point though we may add a color attachment for depth to enable the features the read depth directly in webgl. But it's really important to keep the attachments of the deferred prepass as small as possible. And we really should be removing the normal attachment and copying to the normal prepass texture in a subsequent pass, and then consider if there's a way to further minimize the size of the motion vectors.

Occlusion would usually be either real time, or part of baked light maps, so I don't see much value in it being part of the gbuffer at all and only included it because there was a bit of extra space and it meant I wasn't changing as much about the capabilities for the initial deferred PR. I think the moment there is something more worthwhile to put in the gbuffer we should probably ditch it altogether. Also our current form of specular occlusion is pretty poor imo and leads to a lot of both over and under occlusion in a lot of common situations, and a more appropriate solution would not involve having a specular occlusion channel in the gbuffer anyway.

For forward materials, there shouldn't be anything keeping baked occlusion from working in webgl2.

DGriffin91 · 2024-01-27T02:07:50Z

@rodolphito Oh, good catch regarding the default value! I hadn't taken a look at the code. The default for occlusion should definitely be 1.0 or vec3(1.0) depending on the context for the reasons you stated.

atlv24 · 2024-01-27T02:12:47Z

I just tested the new fix, seems to be working.

…to fix-deferred-spec-env

crates/bevy_pbr/src/render/pbr_types.wgsl

IceSentry

Oh yeah, that new fix makes a lot of sense

# Objective - Fixes bevyengine#11414 ## Solution - Add specular occlusion to g-buffer so PbrInput can be properly reconstructed for shading with a non-zero value allowing the spec envmap to be seen ![image](https://github.com/bevyengine/bevy/assets/11307157/84aa8312-7c06-4dc7-92da-5d94b54b133d) --------- Co-authored-by: JMS55 <47158642+JMS55@users.noreply.github.com>

Fix specular envmap in deferred

2328a2d

IceSentry reviewed Jan 26, 2024

View reviewed changes

IceSentry approved these changes Jan 26, 2024

View reviewed changes

IceSentry added A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior labels Jan 26, 2024

JMS55 added this to the 0.13 milestone Jan 26, 2024

JMS55 approved these changes Jan 26, 2024

View reviewed changes

crates/bevy_pbr/src/deferred/pbr_deferred_functions.wgsl Outdated Show resolved Hide resolved

JMS55 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 26, 2024

Update crates/bevy_pbr/src/deferred/pbr_deferred_functions.wgsl

623e9f8

Co-authored-by: JMS55 <47158642+JMS55@users.noreply.github.com>

Merge branch 'main' into fix-deferred-spec-env

ed715c3

Correct fix and docs

cb96a5c

atlv24 requested review from JMS55, IceSentry and DGriffin91 January 27, 2024 02:10

Merge branch 'main' into fix-deferred-spec-env

6afa08f

atlv24 added 2 commits January 26, 2024 18:32

Improve fix as per Griffin's suggestion

34251b8

Merge branch 'fix-deferred-spec-env' of github.com:rodolphito/bevy in…

0598aa0

…to fix-deferred-spec-env

JMS55 reviewed Jan 27, 2024

View reviewed changes

crates/bevy_pbr/src/render/pbr_types.wgsl Outdated Show resolved Hide resolved

JMS55 approved these changes Jan 27, 2024

View reviewed changes

atlv24 added 2 commits January 26, 2024 20:19

Fix comment

ac19c53

Merge branch 'main' into fix-deferred-spec-env

8d03a3a

IceSentry approved these changes Jan 27, 2024

View reviewed changes

DGriffin91 approved these changes Jan 27, 2024

View reviewed changes

alice-i-cecile added this pull request to the merge queue Jan 29, 2024

Merged via the queue into bevyengine:main with commit 45967b0 Jan 29, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix specular envmap in deferred #11534

Fix specular envmap in deferred #11534

atlv24 commented Jan 26, 2024

IceSentry Jan 26, 2024

atlv24 Jan 26, 2024

IceSentry Jan 26, 2024

JMS55 Jan 26, 2024 •

edited

Loading

IceSentry Jan 26, 2024

JMS55 Jan 26, 2024

IceSentry Jan 26, 2024

atlv24 Jan 26, 2024

JMS55 Jan 26, 2024

DGriffin91 Jan 26, 2024

JMS55 commented Jan 26, 2024

atlv24 commented Jan 26, 2024

JMS55 commented Jan 26, 2024

DGriffin91 commented Jan 26, 2024

atlv24 commented Jan 26, 2024

atlv24 commented Jan 27, 2024

DGriffin91 commented Jan 27, 2024

DGriffin91 commented Jan 27, 2024

atlv24 commented Jan 27, 2024

IceSentry left a comment

Fix specular envmap in deferred #11534

Fix specular envmap in deferred #11534

Conversation

atlv24 commented Jan 26, 2024

Objective

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JMS55 Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JMS55 commented Jan 26, 2024

atlv24 commented Jan 26, 2024

JMS55 commented Jan 26, 2024

DGriffin91 commented Jan 26, 2024

atlv24 commented Jan 26, 2024

atlv24 commented Jan 27, 2024

DGriffin91 commented Jan 27, 2024

DGriffin91 commented Jan 27, 2024

atlv24 commented Jan 27, 2024

IceSentry left a comment

Choose a reason for hiding this comment

JMS55 Jan 26, 2024 •

edited

Loading