Minimal enclosing sphere fix #4436

nigeljw-unity · 2021-05-04T20:13:18Z

Chicken bit for the minimal enclosing sphere frustum cascade culling stabilization fix, and also user control over the number of iterations for the iterative method for finding the minimal enclosing sphere from the 8 frustum cascade points. This fix makes it so that the culling spheres appropriately fully enclose the cascade. Since this is an iterative method, it is conservative. This fixes URP so there are not false positives when culling shadows.

https://jira.unity3d.com/browse/URP-782

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 bug where shadows disappear in the frustum corners from false positives in the shadow culling.
https://fogbugz.unity3d.com/f/cases/1153151

Also fixes inherent bug with shadow fade where the shadows will be culled with a hard line across the fade which is due to the same problem.

Testing status

Tested with URP default template and custom cascade positioning.

Comments to reviewers

This PR is dependent on the following change to trunk:
https://github.cds.internal.unity3d.com/unity/unity/pull/2418

Design doc:
https://docs.google.com/document/d/10YxFKdA8YFS7QS8Owg2dvnYaM-t9tsLXWQLtiEXZQvQ

…universal/debug-cull-features

github-actions · 2021-05-04T20:13:56Z

It appears that you made a non-draft PR!
Please convert your PR to draft (button on the right side of the page)
and cancel any jobs that started on Yamato.
See the PR template for more information.
Thank you!

…universal/debug-cull-features

…universal/tight_enclosing_sphere

nigeljw-unity · 2021-07-07T13:13:34Z

@Unity-Technologies/gfx-qa-urp could I get a performance analysis of this change? I am curious if this potentially improves the rendering time of shadows due to the cascade culling sphere overlap removal.

erikabar

URP QA got re-added again, approving

nigeljw-unity · 2021-07-23T12:55:39Z

Adding the test results doc:
https://docs.google.com/document/d/1Sa5mLkJ8_NyBT9dh8hMlJnaVri1sQMtxKsbpxlv6no4

…universal/tight_enclosing_sphere

github-actions · 2021-08-19T14:03:45Z

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://yamato.cds.internal.unity3d.com/jobs/902-Graphics
Search for your PR branch using the sidebar on the left, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

URP
/.yamato%252Fall-urp.yml%2523PR_URP_2021.2

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

nigeljw-unity · 2021-08-19T14:06:45Z

@simon-engelbrecht-soerensen @erikabar would either of you be able to do a quick sanity test of this latest PR? None of the logic changed so no need to test in depth, but there was a big merge conflict that I had to resolve.

Could you also verify that creating a new project has this enabled, and when upgrading old project, this is disabled?

nigeljw-unity · 2021-08-19T15:01:05Z

com.unity.render-pipelines.universal/Runtime/Data/UniversalRenderPipelineAsset.cs

            instance.m_EditorResourcesAsset = instance.editorResources;

+            // Only enable for new URP assets by default
+            instance.m_ConservativeEnclosingSphere = true;


@Verasl @phi-lira @eh-unity This is how I am enabling it only for new projects. Let me know if this is the right approach.

…universal/tight_enclosing_sphere

eh-unity

lgtm.

oleks-k · 2021-09-17T07:30:27Z

...pelines.universal/Editor/UniversalRenderPipelineAsset/UniversalRenderPipelineAssetUI.Skin.cs

            public static GUIContent shadowDepthBias = EditorGUIUtility.TrTextContent("Depth Bias", "Controls the distance at which the shadows will be pushed away from the light. Useful for avoiding false self-shadowing artifacts.");
            public static GUIContent shadowNormalBias = EditorGUIUtility.TrTextContent("Normal Bias", "Controls distance at which the shadow casting surfaces will be shrunk along the surface normal. Useful for avoiding false self-shadowing artifacts.");
            public static GUIContent supportsSoftShadows = EditorGUIUtility.TrTextContent("Soft Shadows", "If enabled pipeline will perform shadow filtering. Otherwise all lights that cast shadows will fallback to perform a single shadow sample.");
+            public static GUIContent conservativeEnclosingSphere = EditorGUIUtility.TrTextContent("Conservative Enclosing Sphere", "If enabled, a conservative enclosing sphere is used. If disabled, the sphere will be smaller than the cascade size, and shadows will be culled in the corners of the cascades.");


My suggestion:
EditorGUIUtility.TrTextContent("Conservative Enclosing Sphere", "Enable this option to improve shadow frustum culling. Enabling this option prevents Unity from excessively culling shadows in the corners of the shadow cascades. Disable this option only for compatibility purposes of projects created in previous Unity versions.");

…universal/tight_enclosing_sphere

…echnologies/Graphics into universal/tight_enclosing_sphere

oleks-k

Looks good!

Backport of minimal enclosing sphere solution for shadow frustum culling. #4436 Co-authored-by: Erik Hakala <erik.hakala@unity3d.com> Co-authored-by: Oleksandr Kokoshyn <oleksandr.kokoshyn@unity3d.com>

sandy-carter-unity · 2021-09-23T17:33:16Z

com.unity.render-pipelines.universal/Runtime/Data/UniversalRenderPipelineAsset.cs

+            set { m_ConservativeEnclosingSphere = value; }
+        }
+
+        public int numItertionsEnclosingSphere


A bit late to the party but there's a small spelling mistake here:

numItertionsEnclosingSphere -> numIterationsEnclosingSphere

eh-unity and others added 15 commits October 19, 2020 21:17

Debug culling render feature

19a8055

Very basic UI for toggling debug features.

615edb8

Add support for shadow cascade debug and spotlights.

159958d

Merge branch 'master' into universal/debug-cull-features

b16ce6b

Added main camera culling debug viewer.

a7f9504

Merge branch 'master' into universal/debug-cull-features

ca3041c

Fix indexing bug when no direct lights.

f784053

Merge branch 'master' into universal/debug-cull-features

61b7bbe

Refactor

ba8be04

Merge branch 'master' into universal/debug-cull-features

f1b1156

Basic frustum debug support for point light shadows.

8bbd148

Merge branch 'master' of github.com:Unity-Technologies/Graphics into …

318f9f0

…universal/debug-cull-features

Merge branch 'master' of github.com:Unity-Technologies/Graphics into …

a50048a

…universal/debug-cull-features

Merge branch 'master' of github.com:Unity-Technologies/Graphics into …

e66520f

…universal/debug-cull-features

Added minimal enclosing sphere chicken bit and also iteration .

2ec5e04

nigeljw-unity requested a review from a team as a code owner May 4, 2021 20:13

nigeljw-unity changed the base branch from universal/debug-cull-features to master May 4, 2021 20:13

nigeljw-unity requested review from a team as code owners May 4, 2021 20:13

github-actions bot added the sdet label May 4, 2021

nigeljw-unity changed the title ~~Debug culling render feature~~ Minimal enclosing sphere fix May 4, 2021

nigeljw-unity removed the request for review from a team May 4, 2021 21:11

This comment has been minimized.

Sign in to view

nigeljw-unity removed the sdet label May 5, 2021

nigeljw-unity and others added 3 commits May 6, 2021 15:21

Added user control for number of iterations

839062b

Merge branch 'master' of github.com:Unity-Technologies/Graphics into …

69219b9

…universal/debug-cull-features

Merge branch 'master' into universal/debug-cull-features

0cbf27d

nigeljw-unity changed the base branch from master to universal/debug-cull-features May 7, 2021 08:01

Merge branch 'master' of github.com:Unity-Technologies/Graphics into …

35fc302

…universal/tight_enclosing_sphere

nigeljw-unity requested a review from a team July 7, 2021 13:11

erikabar approved these changes Jul 13, 2021

View reviewed changes

nigeljw-unity requested a review from oleks-k August 5, 2021 09:38

nigeljw-unity added 2 commits August 19, 2021 14:49

Merge branch 'master' of github.com:Unity-Technologies/Graphics into …

3939d95

…universal/tight_enclosing_sphere

Fixed merge conflight and renamed to conservative enclosing sphere

51979d6

Enable for new projects

3ac5008

nigeljw-unity commented Aug 19, 2021

View reviewed changes

nigeljw-unity added 4 commits September 15, 2021 21:49

Merge branch 'master' of github.com:Unity-Technologies/Graphics into …

e762373

…universal/tight_enclosing_sphere

Addressed UX design feedback

3ef8d83

Removed orphan UI element

1143462

Removed orphan prop

941171c

eh-unity approved these changes Sep 16, 2021

View reviewed changes

oleks-k requested changes Sep 17, 2021

View reviewed changes

nigeljw-unity and others added 4 commits September 17, 2021 11:40

Updated tooltip from feedback from docs team and added changelog

5201d3f

Merge branch 'master' of github.com:Unity-Technologies/Graphics into …

fc17721

…universal/tight_enclosing_sphere

Added the Conservative Enclosing Sphere description to URP Asset page.

9629cf5

Merge branch 'universal/tight_enclosing_sphere' of github.com:Unity-T…

653b014

…echnologies/Graphics into universal/tight_enclosing_sphere

oleks-k approved these changes Sep 17, 2021

View reviewed changes

oleks-k and others added 2 commits September 17, 2021 13:03

Added a note about performance.

3399711

Merge branch 'master' into universal/tight_enclosing_sphere

142ed2e

nigeljw-unity added the needs-backport-2021.2 label Sep 17, 2021

ellioman merged commit 9fd657c into master Sep 17, 2021

ellioman deleted the universal/tight_enclosing_sphere branch September 17, 2021 15:14

nigeljw-unity mentioned this pull request Sep 20, 2021

Minimal enclosing sphere backport to 2021.2 #5722

Merged

4 tasks

sandy-carter-unity reviewed Sep 23, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Minimal enclosing sphere fix #4436

Minimal enclosing sphere fix #4436

Uh oh!

nigeljw-unity commented May 4, 2021 •

edited

Loading

Uh oh!

github-actions bot commented May 4, 2021

Uh oh!

This comment has been minimized.

nigeljw-unity commented Jul 7, 2021

Uh oh!

erikabar left a comment

Uh oh!

nigeljw-unity commented Jul 23, 2021

Uh oh!

github-actions bot commented Aug 19, 2021

Uh oh!

nigeljw-unity commented Aug 19, 2021 •

edited

Loading

Uh oh!

nigeljw-unity Aug 19, 2021

Uh oh!

eh-unity left a comment

Uh oh!

oleks-k Sep 17, 2021

Uh oh!

oleks-k left a comment

Uh oh!

sandy-carter-unity Sep 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Minimal enclosing sphere fix #4436

Minimal enclosing sphere fix #4436

Uh oh!

Conversation

nigeljw-unity commented May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist for PR maker

Purpose of this PR

Testing status

Comments to reviewers

Uh oh!

github-actions bot commented May 4, 2021

Uh oh!

This comment has been minimized.

nigeljw-unity commented Jul 7, 2021

Uh oh!

erikabar left a comment

Choose a reason for hiding this comment

Uh oh!

nigeljw-unity commented Jul 23, 2021

Uh oh!

github-actions bot commented Aug 19, 2021

Uh oh!

nigeljw-unity commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nigeljw-unity Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

eh-unity left a comment

Choose a reason for hiding this comment

Uh oh!

oleks-k Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

oleks-k left a comment

Choose a reason for hiding this comment

Uh oh!

sandy-carter-unity Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

nigeljw-unity commented May 4, 2021 •

edited

Loading

nigeljw-unity commented Aug 19, 2021 •

edited

Loading