-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Approximate indirect specular occlusion #11152
Approximate indirect specular occlusion #11152
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like how it looks especially considering how small the change is but since it breaks the Physically Based part we should probably make this configurable.
The suggestion in discord to do indirect_light += environment_light.diffuse * occlusion + environment_light.specular * specular_occlusion
would make that pretty easy.
The comment should maybe be shortened a bit, maybe don't mention the wheel well example? It seems a bit too specific and over explained to me.
I think this is debatable. Not accounting for specular occlusion is also not physical, but it requires raytracing to simulate accurately. This gets us closer to the ground truth by having something for the specular occlusion term. The next most accurate would be using SSR for the occlusion check, followed by full path tracing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree that it looks better when occluding the specular as well. The Fresnel defeats pretty much the purpose of the occlusion.
I followed closely the discussion on discord I think this is worth merging.
I've yet to see a scene where it looks worse, so I think it should be merged, since it's not even a tradeoff, it's just straight up a positive change.
Of note is that occlusion
is not strictly defined by SSAO, but also by the occlusion texture. But looking at the glTF2 spec I don't see any details about whether this should affect environment light specular reflection or not, so it's fine as-is.
Regarding the suggestion of having it parametrable. I think it should be part of a different PR. It's a different level of controversy, as it includes more tradeoffs.
It seems @DGriffin91 has a more accurate implementation of specular occlusion, but until its ready, I think it's fine to merge this.
Specular ambient occlusion is included in both reference implementation that bevy uses for GTAO (XeGTAO), and is also specified in original paper: [Jimenez et al., 2016] (pg. 3) Specular occlusion from [Lagarde et al., 2014] (pg. 76) is also specified in Filament, the primary reference PBR implementation that bevy uses: https://google.github.io/filament/Filament.md.html#lighting/occlusion/specularocclusion (XeGTAO implements both the simpler Lagarde14 method as well as the improved Jimenez16 one) Using the diffuse ambient occlusion term directly for specular occlusion will result in over occlusion in areas that would receive less ambient diffuse light but still have a path for specular light, while also significantly under occluding in most cases. We should continue implementing GTAO to include GTSO. And then also provide a SSR oriented method for more accurate specular occlusion that also better captures more distant occlusion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a migration guide and changelog for the diffuse_occlusion naming change.
Approving this PR, assuming that ignoring baked occlusion makes sense.
7577a5d
to
e44a67a
Compare
e44a67a
to
5c11ded
Compare
Note, I implemented a |
Objective
Solution
Changelog
Migration Guide
PbrInput::occlusion
todiffuse_occlusion
, and addedspecular_occlusion
.