Added visibility bitmask as an alternative SSAO method #13454

dragostis · 2024-05-21T14:35:15Z

Early implementation. I still have to fix the documentation and consider writing a small migration guide.

Questions left to answer:

should thickness be an overridable constant?
is there a better way to implement Eq/Hash for SSAOMethod?
do we want to keep the linear sampler for the depth texture?
is there a better way to separate the logic than preprocessor macros?

Migration guide

SSAO algorithm was changed from GTAO to VBAO (visibility bitmasks). A new field, constant_object_thickness, was added to ScreenSpaceAmbientOcclusion. ScreenSpaceAmbientOcclusion also lost its Eq and Hash implementations.

github-actions · 2024-05-21T14:35:28Z

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

crates/bevy_pbr/src/ssao/mod.rs

JMS55 · 2024-05-21T17:22:16Z

References for reviewers:

dragostis · 2024-05-22T14:28:03Z

I added a few questions to answer before merging this and also added a linear sampler for the depth texture since I noticed it slightly improves some of the ghosting noticeable in small objects that happens due to the low resolution mip that gets selected for far-away samples.

JMS55 · 2024-08-09T18:30:30Z

Another good reference: https://cybereality.com/screen-space-indirect-lighting-with-visibility-bitmask-improvement-to-gtao-ssao-real-time-ambient-occlusion-algorithm-glsl-shader-implementation/

@dragostis to give you an update, I'd still like to get this merged for Bevy 0.15 if you're interested. It's just going to be a very slow process, as reviewer bandwidth is very limited, and we have a large backlog of rendering PRs. I'll try to test this out in a few weeks when I'll have more free time, and help push it forward.

dragostis · 2024-08-12T13:26:04Z

@JMS55, I'm happy to get this merged for 0.15!

In it's current form, it only handles AO. Adding GI as well is quite straightforward algorithm-wise, but would require extra textures and a separate denoise pass to look good.

JMS55 · 2024-08-12T15:19:38Z

Yeah lets not do GI in this PR. Feel free to open another after if you're interested though.

To get this PR merged, I want to test this on some more realistic scenes and check how the perf/quality compares. If it's always better than the existing method, then imo we should remove the existing method and maintain just this one. That's basically the only blocker. Unfortunately I don't have a desktop for the next week or two to be able to run Sponza and other scenes.

There's also the opportunity here to change the denoiser/downscale depth/noise patterns that I copied from XeGTAO, and try what the newer vismask impls are using. Up to you on that, we can also leave the existing ones as-is.

crates/bevy_pbr/src/ssao/mod.rs

JMS55 · 2024-08-28T03:09:24Z

The code behind the vismask paper was open sourced (NOTE: Parts, including the denoiser, are from Unity and are not open-source licensed, be careful which files you reference): https://github.com/cdrinmatane/SSRT3

JMS55 · 2024-08-28T03:10:05Z

I'm still quite busy with meshlets, but I need to find some time sometime soon to try this PR out and see how it compares to GTAO.

dragostis · 2024-08-30T08:53:47Z

@JMS55, sorry the late reply. This month has been a bit busy, but I expect o have a bit more time to push this through in September.

A good scene to test this IMO is Blender's Classroom since it has a lot of thin geometry that showcases VBAO's strengths quite well.

JMS55 · 2024-08-30T17:03:08Z

No worries, like I said I haven't had much time lately either.

Thanks, will try.

JMS55 · 2024-09-11T05:27:08Z

crates/bevy_pbr/src/ssao/ssao.wgsl

    }
+
+#if METHOD == 0


I'd rather not use integers for this. Better to do #ifdef VBAO #else imo.

JMS55 · 2024-09-11T05:27:46Z

crates/bevy_pbr/src/ssao/mod.rs

@@ -189,6 +190,49 @@ impl ScreenSpaceAmbientOcclusionQualityLevel {
    }
 }

+#[derive(Reflect, Clone, Copy, Debug)]
+pub enum ScreenSpaceAmbientOcclusionMethod {


This type and GTAO vs VBAO need docs.

crates/bevy_pbr/src/ssao/mod.rs

JMS55

Sorry for the late review, but I finally had some time to look into this,

I tried it on Sponza and it looked good! Didn't have time to compare quality or performance to GTAO, or check the code thoroughly against the paper. I'll leave that up to other reviewers. VBAO seemed better at lower-sample counts especially though based on my very quick glances.

There's also probably a conversation to be had about if we want to support both methods, or use VBAO only. @dragostis it might help to post some example screenshots of the blender classroom comparing the two methods; I didn't have time to try and get the scene setup in bevy.

Side note to other reviewers - the SSAO docs say that it doesn't work on DirectX12, but I believe the naga issues should be fixed by now? We should test and remove the note if so.

dragostis · 2024-09-17T18:09:32Z

I loaded a textureless classroom into bevy and took a bunch of screenshots with varying thickness levels:

GTAO

VBAO 0.5

GTAO

VBAO 0.5

VBAO 1

GTAO

VBAO 0.25

VBAO 1

JMS55 · 2024-09-17T18:14:31Z

Very nice! I'd be happy to remove GTAO and merge VBAO personally. I don't see a need for both.

alice-i-cecile · 2024-09-23T18:39:31Z

@dragostis is this ready to be reviewed now? If so, please take it out of draft :)

IceSentry

I think I agree with @JMS55 on that one. We can probably just get rid of GTAO and just keep VBAO, it looks better in pretty much every case

JMS55 · 2024-09-29T18:35:29Z

@dragostis the release candidate for bevy 0.15 is in about a week. I'd like to get this in before then. Code is mostly done, I think we just need to remove GTAO, and some other smaller stuff. If you're busy, I'd be happy to finish this PR out for you. Let me know if you have time, otherwise I'll probably do this myself to ensure it makes the release in time.

dragostis · 2024-09-30T16:07:17Z

@JMS55, I'll try to address the feedback and un-draft the PR tomorrow morning. Thanks for the heads up!

JMS55 · 2024-09-30T19:12:59Z

Awesome, thanks! Excited to finally get this in!

Summary of needed changes (iirc, might be missing some):

Remove GTAO
Use a uniform/push constant for visbuffer config, not shaderdef
Test on DX12 (should work), remove the docs about SSAO not being supported on DX12
Write migration guide + changelog

dragostis · 2024-10-01T10:25:10Z

I've crossed everything off the list apart from the changelog. Since Bevy doesn't keep a changelog file, does this mean doing a writeup in the release blog post or is it something completely different?

JMS55 · 2024-10-01T16:53:12Z

Nvm looks like we removed the changelog from our PR template in favor of just the migration guide.

JMS55

Thanks for the work! A few small suggestions, almost ready to go!

crates/bevy_pbr/src/ssao/mod.rs

IceSentry · 2024-10-02T05:00:29Z

I think the conflicts are just caused by the required components PR. Should be trivial to resolve.

Co-authored-by: JMS55 <[email protected]>

This removes the need to use the entire SSAO object as a pipeline key, instead using only the quality level. This removes the need for hand-written Eq/Hash implementations.

alice-i-cecile · 2024-10-02T13:44:30Z

I've crossed everything off the list apart from the changelog. Since Bevy doesn't keep a changelog file, does this mean doing a writeup in the release blog post or is it something completely different?

Yep, this will get a writeup in the blog post. I'll be pinging you over in the bevy-website repo to get your advice on how to write it :)

alice-i-cecile added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels May 21, 2024

dragostis changed the title ~~Added visibility bitmass as an alternative SSAO method~~ Added visibility bitmask as an alternative SSAO method May 21, 2024

JMS55 reviewed May 21, 2024

View reviewed changes

crates/bevy_pbr/src/ssao/mod.rs Outdated Show resolved Hide resolved

JMS55 added this to the 0.15 milestone May 21, 2024

JMS55 requested a review from Elabajaba May 21, 2024 17:08

IceSentry self-requested a review August 14, 2024 01:00

DGriffin91 reviewed Aug 22, 2024

View reviewed changes

crates/bevy_pbr/src/ssao/mod.rs Outdated Show resolved Hide resolved

JMS55 self-requested a review August 28, 2024 03:09

JMS55 closed this Aug 30, 2024

JMS55 reopened this Aug 30, 2024

JMS55 reviewed Sep 11, 2024

View reviewed changes

crates/bevy_pbr/src/ssao/mod.rs Outdated Show resolved Hide resolved

JMS55 suggested changes Sep 11, 2024

View reviewed changes

JMS55 requested a review from alice-i-cecile September 23, 2024 18:33

alice-i-cecile removed the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Sep 23, 2024

alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Sep 23, 2024

IceSentry reviewed Sep 23, 2024

View reviewed changes

Affinator mentioned this pull request Sep 25, 2024

SSAO does not work with orthographic projections #13838

Open

dragostis marked this pull request as ready for review October 1, 2024 08:51

dragostis force-pushed the visibility-bitmasks branch from f9f464d to 0b25789 Compare October 1, 2024 10:21

JMS55 suggested changes Oct 1, 2024

View reviewed changes

crates/bevy_pbr/src/ssao/mod.rs Outdated Show resolved Hide resolved

crates/bevy_pbr/src/ssao/mod.rs Outdated Show resolved Hide resolved

IceSentry approved these changes Oct 1, 2024

View reviewed changes

JMS55 approved these changes Oct 2, 2024

View reviewed changes

JMS55 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 2, 2024

dragostis and others added 2 commits October 2, 2024 10:01

Replaced GTAO with visibility bitmasks.

dcf9c19

Adjust docs.

5f74252

Co-authored-by: JMS55 <[email protected]>

dragostis force-pushed the visibility-bitmasks branch from 6ba8c3e to 5f74252 Compare October 2, 2024 07:02

Reduced key to only use quality level.

6eb3e64

This removes the need to use the entire SSAO object as a pipeline key, instead using only the quality level. This removes the need for hand-written Eq/Hash implementations.

alice-i-cecile added this pull request to the merge queue Oct 2, 2024

alice-i-cecile added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Oct 2, 2024

Merged via the queue into bevyengine:main with commit ba7907c Oct 2, 2024
27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added visibility bitmask as an alternative SSAO method #13454

Added visibility bitmask as an alternative SSAO method #13454

dragostis commented May 21, 2024 •

edited

Loading

github-actions bot commented May 21, 2024

JMS55 commented May 21, 2024

dragostis commented May 22, 2024

JMS55 commented Aug 9, 2024

dragostis commented Aug 12, 2024

JMS55 commented Aug 12, 2024 •

edited

Loading

JMS55 commented Aug 28, 2024

JMS55 commented Aug 28, 2024

dragostis commented Aug 30, 2024

JMS55 commented Aug 30, 2024

JMS55 Sep 11, 2024

JMS55 Sep 11, 2024 •

edited

Loading

JMS55 left a comment

dragostis commented Sep 17, 2024

JMS55 commented Sep 17, 2024

alice-i-cecile commented Sep 23, 2024

IceSentry left a comment

JMS55 commented Sep 29, 2024

dragostis commented Sep 30, 2024

JMS55 commented Sep 30, 2024

dragostis commented Oct 1, 2024

JMS55 commented Oct 1, 2024

JMS55 left a comment

IceSentry commented Oct 2, 2024

alice-i-cecile commented Oct 2, 2024

		}

		#if METHOD == 0

Added visibility bitmask as an alternative SSAO method #13454

Added visibility bitmask as an alternative SSAO method #13454

Conversation

dragostis commented May 21, 2024 • edited Loading

Migration guide

github-actions bot commented May 21, 2024

JMS55 commented May 21, 2024

dragostis commented May 22, 2024

JMS55 commented Aug 9, 2024

dragostis commented Aug 12, 2024

JMS55 commented Aug 12, 2024 • edited Loading

JMS55 commented Aug 28, 2024

JMS55 commented Aug 28, 2024

dragostis commented Aug 30, 2024

JMS55 commented Aug 30, 2024

JMS55 Sep 11, 2024

Choose a reason for hiding this comment

JMS55 Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

JMS55 left a comment

Choose a reason for hiding this comment

dragostis commented Sep 17, 2024

JMS55 commented Sep 17, 2024

alice-i-cecile commented Sep 23, 2024

IceSentry left a comment

Choose a reason for hiding this comment

JMS55 commented Sep 29, 2024

dragostis commented Sep 30, 2024

JMS55 commented Sep 30, 2024

dragostis commented Oct 1, 2024

JMS55 commented Oct 1, 2024

JMS55 left a comment

Choose a reason for hiding this comment

IceSentry commented Oct 2, 2024

alice-i-cecile commented Oct 2, 2024

dragostis commented May 21, 2024 •

edited

Loading

JMS55 commented Aug 12, 2024 •

edited

Loading

JMS55 Sep 11, 2024 •

edited

Loading