Skip to content
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

Merged

Conversation

aevyrie
Copy link
Member

@aevyrie aevyrie commented Dec 30, 2023

Objective

  • The current PBR renderer over-brightens indirect specular reflections, which tends to cause objects to appear to glow, because specular occlusion is not accounted for.

Solution

Before After Animation
before bike after bike ezgif-1-fbcbaf272b
classroom before classroom after ezgif-1-0f390edd06

Changelog

  • Ambient occlusion now applies to indirect specular reflections to approximate how objects occlude specular light.

Migration Guide

  • Renamed PbrInput::occlusion to diffuse_occlusion, and added specular_occlusion.

Copy link
Contributor

@IceSentry IceSentry left a 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.

@IceSentry IceSentry added the A-Rendering Drawing game state to the screen label Dec 30, 2023
@aevyrie
Copy link
Member Author

aevyrie commented Dec 30, 2023

it breaks the Physically Based part

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.

Copy link
Contributor

@nicopap nicopap left a 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.

@DGriffin91
Copy link
Contributor

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.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior X-Controversial There is active debate or serious implications around merging this PR labels Jan 2, 2024
@aevyrie
Copy link
Member Author

aevyrie commented Jan 2, 2024

The filament docs and related paper you link are equivalent to what I've done here, for rough materials. For rough materials, AO and specular occlusion are 1:1. I would just need to add the wrapping function to match Filament/Frostbite for roughness values < 0.3.

image

It also looks like XeGTAO also uses this (Legarde) method in the simple case.

https://github.com/GameTechDev/XeGTAO/blob/0b276c0ce820475c2adf6e2f3b696b696c172f43/Source/Rendering/Shaders/Filament/ambient_occlusion.va.fs#L78-L79

@JMS55 JMS55 added this to the 0.13 milestone Jan 3, 2024
Copy link
Contributor

@JMS55 JMS55 left a 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.

crates/bevy_pbr/src/deferred/deferred_lighting.wgsl Outdated Show resolved Hide resolved
crates/bevy_pbr/src/deferred/deferred_lighting.wgsl Outdated Show resolved Hide resolved
crates/bevy_pbr/src/render/pbr_fragment.wgsl Outdated Show resolved Hide resolved
@JMS55 JMS55 requested a review from IceSentry January 3, 2024 08:07
@aevyrie aevyrie force-pushed the approximate-specular-occlusion branch 2 times, most recently from 7577a5d to e44a67a Compare January 3, 2024 09:26
@aevyrie aevyrie force-pushed the approximate-specular-occlusion branch from e44a67a to 5c11ded Compare January 3, 2024 11:18
@aevyrie
Copy link
Member Author

aevyrie commented Jan 4, 2024

Note, I implemented a min(baked_ao, ssao) for specular, but this had no effect because SSAO was always darker. We decided to just ignore the baked AO for specular occlusion.

@aevyrie aevyrie 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 4, 2024
@alice-i-cecile alice-i-cecile removed the X-Controversial There is active debate or serious implications around merging this PR label Jan 14, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 15, 2024
Merged via the queue into bevyengine:main with commit 839d2f8 Jan 15, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants