-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Distance-based Gaussian weighting for Ambient Occlusion #12316
base: main
Are you sure you want to change the base?
Conversation
The last commit before Prettier v3 was used to reformat the code
This commit reformats all code using prettier v3 relative to the pre-prettier-v3 tag
Thank you for the pull request, @jjhembd! ✅ We can confirm we have a CLA on file for you. |
Thanks @jjhembd! Excited to get this in! Is the code ready for review yet, or should we wait for the uniform changes to go in first? |
The code is now ready for performance testing with different sample counts. There are 2 parameters:
See this local Sandcastle for a demonstration of the parameters.
I think it would make sense to wait until the uniform changes are done. I still have one change to implement: the step size should be automatically computed from the window size ( |
sampleDirection = rotateStep * sampleDirection; | ||
|
||
float localAO = 0.0; | ||
float accumulatedWindowWeights = 0.0; | ||
//vec2 radialStep = stepLength * sampleDirection; |
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.
//vec2 radialStep = stepLength * sampleDirection; |
@@ -51,11 +60,11 @@ void main(void) | |||
} | |||
|
|||
vec3 normalEC = getNormalXEdge(positionEC); | |||
float gaussianVariance = lengthCap * sqrt(-positionEC.z); | |||
// TODO: mix of units. Steps are in pixels; variance is in meters. |
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.
Does czm_metersPerPixel
help at all here?
@jjhembd I'm consistently seeing a band along the top of the screen, any idea what may be causing that? Is it the transition between multiple frustums maybe? |
I'm also seeing some flickering due to occlusion when I look up at the sky. Is that just me? |
@jjhembd What's the plan for blur? I noticed your sandcastle disables it entirely. I agree the look of the blur is not great. Would it make sense to remove it completely? Maybe we could use MSAA to remove "noise" from few samples? |
@ggetz thanks for the initial feedback! I reworked the uniforms to compute the step length from the window size and step count. This value also works as the scalar to account for discrete sampling of the Gaussian, so we don't have to compute normalization factors in the inner loop.
This does look like a frustum boundary. The occlusion should be neglecting this, since it appears in the depth buffer as a huge jump--much bigger than the window length. It's possible we're not correctly handling zero values in the depth buffer. I'm looking at this now.
We discussed using a bilateral filtering at one point, which would better preserve edges. I will take a look today to see how much work that would be. We still need to add some kind of deprecation for the old uniform names. I updated the TODO list in the description. |
@jjhembd I'm seeing some flickering across the entire terrain as I zoom out in the sandcastle. I'm also seeing some banding on the smokestack things but I'm not sure if those are related. The flicker is quick and goes away when the camera settles but is that expected? simplescreenrecorder-2024-11-27_16.01.57.mp4 |
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.
This is definitely a nice change to be able to see the AO from further away. I think the code looks good with my somewhat limited shader knowledge. I just had one small comment about the JS code. Also the bug(?) that I pointed out in my comment above
forcePowerOfTwo = false, | ||
sampleMode = PostProcessStageSampleMode.NEAREST, |
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.
Description
Ambient Occlusion (AO) for geospatial data needs to produce good results at a wide range of scales, without requiring the user to adjust parameters for each zoom level. This PR redefines the weight function for AO as a normalized Gaussian window, with a variance that scales with the distance from the camera.
Before this PR, the AO effect disappears when zoomed out:
After this PR, the AO effect blurs wider, remaining visible (though lower intensity) when far from the camera:
Issue number and link
Fixes #10106.
Testing plan
Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change#TODO
Add fallback with deprecation message to support old uniforms