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

Fix aliasing artifact in MultiScaleVolumetricObscurance (AmbientOcclusion) #7489

Merged
merged 3 commits into from Nov 30, 2022

Conversation

Scoutydren
Copy link
Contributor

Please read the Contributing guide before making a PR.

Checklist for PR maker

  • Have you added a backport label (if needed)? For example, the need-backport-* label. After you backport the PR, the label changes to backported-*.
  • Have you updated the changelog? Each package has a CHANGELOG.md file.
  • Have you updated or added the documentation for your PR? When you add a new feature, change a property name, or change the behavior of a feature, it's best practice to include related documentation changes in the same PR. If you do add documentation, make sure to add the relevant Graphics Docs team member as a reviewer of the PR. If you are not sure which person to add, see the Docs team contacts sheet.
  • Have you added a graphic test for your PR (if needed)? When you add a new feature, or discover a bug that tests don't cover, please add a graphic test.

Purpose of this PR

Fixes for the FogBugz case in Multi Scale Ambient Occlusion where there exists banding and aliasing artifacts when applying the post-processing. This fix adds a configurable parameter Z Bias in the MSVO UI settings and eliminates the banding artifact.


Testing status

Tested manually against FogBugz case on Windows


Comments to reviewers

@mikesfbay mikesfbay self-requested a review June 14, 2022 23:33

#ifdef MSAA
float2 TestSamplePair(float frontDepth, float2 invRange, uint base, int offset)
{
// "Disocclusion" measures the penetration distance of the depth sample within the sphere.
// Disocclusion < 0 (full occlusion) -> the sample fell in front of the sphere
// Disocclusion > 1 (no occlusion) -> the sample fell behind the sphere
float2 disocclusion1 = DepthSamples[base + offset] * invRange - frontDepth;
float2 disocclusion2 = DepthSamples[base - offset] * invRange - frontDepth;
float2 disocclusion1 = DepthSamples[base + offset] * invRange - (frontDepth - zBias);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

frontDepth is related to invThickness of sphere samples, which isn't what we want. Z bias should be done on the screen sample in Main() before invThisDepth is calculated. Remember, thisIdx in Main() is the index of the screen sample where the sphere samples are centered around.

/// <summary>
/// Modifies the z-bias to the depth buffer. This eliminates the banding aliasing artifact for MSVO.
/// </summary>
[Range(0f, 0.001f), Tooltip("Modifies the bias to the depth buffer for reducing aliasing artifact.")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change wording to "Add a bias distance to sampled depth in AO to reduce self-shadowing aliasing artifacts."

Copy link

@ryanhy-unity ryanhy-unity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified fix with customer repro project. Checked Desktop D3D and Vulkan as well as Android. Did a sanity check on URP PP which didn't exhibit any regresssions. Finally, ran through the xr automated end to end test and manually checked a few random other PP effects. Looks good to me.

@@ -63,5 +63,6 @@ This mode is optimized for consoles and desktop platforms. It has better graphic
| Mode | Select the type of **Ambient Occlusion** to use. |
| Intensity | Adjust the degree of darkness **Ambient Occlusion** produces. |
| Thickness Modifier | Modify the thickness of occluders. This increases dark areas but can introduce dark halos around objects. |
| Z Bias | Modifies the z-bias to the depth buffer. This eliminates the banding aliasing artifact for MSVO. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also plz update the wording in the doc .md file.

PierreGac pushed a commit to PierreGac/PostProcessing that referenced this pull request Sep 12, 2022
@mrtenda
Copy link

mrtenda commented Nov 28, 2022

Hi, I see that this change has been approved for about three months, but hasn't been merged in yet. According to LinkedIn, it seems that the original submitter of the PR was an intern at Unity but currently is no longer at Unity.

What are the next steps for this to get merged in?

This is an important fix as it seems to be related to this bug which is affecting multiple teams (including us) from shipping our games: https://issuetracker.unity3d.com/issues/horizontal-line-artifacts-when-using-ambient-occlusion-with-multi-scale-volumetric-obscurance

Thanks!

@mikesfbay
Copy link
Contributor

@sebastienlagarde Can we merge this PR to master soon (for postfx V2)? This fix will help several teams @mrtenda

@sebastienlagarde
Copy link
Collaborator

Looks like postprocess test are broken :( . Not related to this PR

@sebastienlagarde sebastienlagarde merged commit 27c46fa into Unity-Technologies:master Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants