Add additional APV debug modes #6169

tlaedre · 2021-10-28T21:57:53Z

Purpose of this PR

This PR adds a visualization mode for probe placement virtual offset.

An additional toggle has been added for drawing offsets, along with a size slider. The culling distance control has been moved to the bottom and is applied to both probes and offsets visualization. It's also always visible now, so you can reduce the culling distance before enabling drawing.

Offsets are visualized as arrows starting at the probe's grid position and ending at the virtual position of the pushed out probe.

The PR also adds probe volume debug visualizers for displaying probe L0 only and L0L1. This is useful for visualizing the ambient component and for comparing L1 and L2 probes data.

Testing status

Tested that it all works in editor.

Tested that it doesn't break anything in standalone, however since the probe volume debug visualizers already don't work in standalone in master, actually testing the functionality in standalone isn't possible atm.

Comments to reviewers

Storing the virtual offset data with the rest of the baked data bloats the runtime data even more. There is a follow up PR coming that separates runtime data from debug data.

There are inline comments on the commit pointing out various motivations.

github-actions · 2021-10-28T21:58:06Z

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://unity-ci.cds.internal.unity3d.com/project/902/
Search for your PR branch using the search bar at the top, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/jobDefinition/.yamato%2Fall-hdrp.yml%23PR_HDRP_trunk
With changes to HDRP packages, you should also run
/jobDefinition/.yamato%2Fall-lightmapping.yml%23PR_Lightmapping_trunk

SRP Core
You could run ABV on your branch before merging your PR, but it will start A LOT of jobs. Please be responsible about it and run it only when you feel the PR is ready:
/jobDefinition/.yamato%252F_abv.yml%2523all_project_ci_trunk
Be aware that any modifications to the Core package impacts everyone in the Graphics repo so please discuss the PR with your lead.

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.

tlaedre · 2021-10-28T22:00:04Z

com.unity.render-pipelines.core/Editor/Lighting/ProbeVolume/ProbeGIBaking.VirtualOffset.cs

+            }
+        }
+
+        static void ApplyVirtualOffsetsSingleThreaded(Vector3[] positions, out Vector3[] offsets, VirtualOffsetSettings voSettings)


This and ApplyVirtualOffsets above are kinda stop-gap changes as the current approach is very slow in complex scenes.

There's a separate PR coming that moves this work into bursted jobs while also fixing some offsetting bugs (currently probes are actually pushed inside geometry in certain circumstances), so I've separated them as much as possible.

tlaedre · 2021-10-28T22:02:22Z

com.unity.render-pipelines.core/Editor/Lighting/ProbeVolume/ProbeGIBaking.VirtualOffset.cs

                    foreach (MeshRenderer mr in renderComponents)
                    {
-                        if (!mr.gameObject.GetComponent<MeshCollider>() && (GameObjectUtility.GetStaticEditorFlags(mr.gameObject).HasFlag(StaticEditorFlags.ContributeGI)))
+                        if ((GameObjectUtility.GetStaticEditorFlags(mr.gameObject) & StaticEditorFlags.ContributeGI) != 0 && !mr.gameObject.TryGetComponent<MeshCollider>(out _))


Changed to remove dual allocations from boxing.

tlaedre · 2021-10-28T22:03:24Z

com.unity.render-pipelines.core/Editor/Lighting/ProbeVolume/ProbeGIBaking.cs

-                }
-                CleanupOccluders();
-            }
+            ApplyVirtualOffsets(positions, out m_BakingBatch.virtualOffsets);


This call adds occluders, calculates (and returns) offsets, and cleans up occluders if virtual offset is enabled.

tlaedre · 2021-10-28T22:04:03Z

com.unity.render-pipelines.core/Editor/Lighting/ProbeVolume/ProbeGIBaking.VirtualOffset.cs

        static List<MeshRenderer> addedOccluders;

-        private static void AddOccluders()
+        static void ApplyVirtualOffsets(Vector3[] positions, out Vector3[] offsets)


Simplify calling code by getting valid offsets output regardless of whether VO is enabled or not.

tlaedre · 2021-10-28T22:05:12Z

com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeReferenceVolume.Debug.cs

        }

-        const int kProbesPerBatch = 1023;
+        const int kProbesPerBatch = 511;


The shaders used for drawing instanced probes / offsets are in-fact only capable of drawing 511 instances in a single batch. Breaking them into batches of 1023 effectively means that each batch generates 3 draw calls, hence this change.

tlaedre · 2021-10-28T22:07:44Z

com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeReferenceVolume.Debug.cs

                        Graphics.DrawMeshInstanced(m_DebugMesh, 0, m_DebugMaterial, probeBuffer, probeBuffer.Length, props, ShadowCastingMode.Off, false, 0, camera, LightProbeUsage.Off, null);
                    }
+
+                    if (debugDisplay.drawVirtualOffsetPush)
+                    {
+                        var offsetBuffer = debug.offsetBuffers[i];
+                        Graphics.DrawMeshInstanced(m_DebugOffsetMesh, 0, m_DebugOffsetMaterial, offsetBuffer, offsetBuffer.Length, props, ShadowCastingMode.Off, false, 0, camera, LightProbeUsage.Off, null);


These two DrawMeshInstanced calls don't actually work for me in standalone builds. I'm not sure why, and I didn't have source handy to debug it at time of testing.

I verified that the behaviour is the same in master as well, the draw calls never make it down to the graphics device.

tlaedre · 2021-10-28T22:08:48Z

com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeReferenceVolume.cs

            public Vector3[] probePositions;
+            public Vector3[] offsetVectors;
            public SphericalHarmonicsL2[] sh;
            public float[] validity;


Most of this data isn't actually needed at runtime. There's a separate PR coming for separating them.

Validity will be used at runtime :-) Some other things too, offset vectors probably can hide away

I know that's the plan, but right now it is not used, and I question whether a full float value would make sense anyway for what it represents. (right now the runtime data is quite phat, I think it makes sense to move things as they actually get used)

FrancescoC-unity · 2021-11-02T10:11:41Z

com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeReferenceVolume.Debug.cs

-            {
-                subdivContainer.children.Add(new DebugUI.FloatField { displayName = "Culling Distance", getter = () => debugDisplay.subdivisionViewCullingDistance, setter = value => debugDisplay.subdivisionViewCullingDistance = value, min = () => 0.0f });
-            }
+            subdivContainer.children.Add(new DebugUI.FloatField { displayName = "Culling Distance", getter = () => debugDisplay.subdivisionViewCullingDistance, setter = value => debugDisplay.subdivisionViewCullingDistance = value, min = () => 0.0f });


Why removing the check here?

Because I want to be able to change the culling distance before enabling any debug drawing to avoid my machine grinding to a halt when it defaults to drawing everything.

FrancescoC-unity

Adding @iM0ve for testing

iM0ve

It doesnt work for me. I simply switch from master to the branch and it stops rendering when I enable probes.

Master
https://user-images.githubusercontent.com/36502659/141807101-8c6b3dd9-3224-4acc-8791-51f6a489ebda.mp4

PR
https://user-images.githubusercontent.com/36502659/141807163-ea7c098d-44d2-4c8e-a4e6-910bcdd9b76b.mp4

Error:
NullReferenceException: Object reference not set to an instance of an object
UnityEngine.Experimental.Rendering.ProbeReferenceVolume.CreateInstancedProbes () (at D:/Git/Graphics/com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeReferenceVolume.Debug.cs:295)
UnityEngine.Experimental.Rendering.ProbeReferenceVolume.DrawProbeDebug (UnityEngine.Camera camera) (at D:/Git/Graphics/com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeReferenceVolume.Debug.cs:229)
UnityEngine.Experimental.Rendering.ProbeReferenceVolume.RenderDebug (UnityEngine.Camera camera) (at D:/Git/Graphics/com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeReferenceVolume.Debug.cs:74)
UnityEngine.Rendering.HighDefinition.HDRenderPipeline.TryCull (UnityEngine.Camera camera, UnityEngine.Rendering.HighDefinition.HDCamera hdCamera, UnityEngine.Rendering.ScriptableRenderContext renderContext, UnityEngine.Rendering.HighDefinition.SkyManager skyManager, UnityEngine.Rendering.ScriptableCullingParameters cullingParams, UnityEngine.Rendering.HighDefinition.HDRenderPipelineAsset hdrp, UnityEngine.Rendering.HighDefinition.HDRenderPipeline+HDCullingResults& cullingResults) (at D:/Git/Graphics/com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.cs:2366)
UnityEngine.Rendering.HighDefinition.HDRenderPipeline.Render (UnityEngine.Rendering.ScriptableRenderContext renderContext, System.Collections.Generic.List1[T] cameras) (at D:/Git/Graphics/com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.cs:1311) UnityEngine.Rendering.RenderPipeline.InternalRender (UnityEngine.Rendering.ScriptableRenderContext context, System.Collections.Generic.List1[T] cameras) (at <2325cc54de6040309d03768ffdba64e6>:0)
UnityEngine.Rendering.RenderPipelineManager.DoRenderLoop_Internal (UnityEngine.Rendering.RenderPipelineAsset pipe, System.IntPtr loopPtr, System.Collections.Generic.List`1[T] renderRequests, Unity.Collections.LowLevel.Unsafe.AtomicSafetyHandle safety) (at <2325cc54de6040309d03768ffdba64e6>:0)
UnityEngine.GUIUtility:ProcessEvent(Int32, IntPtr, Boolean&)

tlaedre · 2021-12-08T17:30:43Z

@iM0ve fixed the case where data baked before this change would not correctly display debug data unless it's re-baked.

iM0ve

Testing status: Done

What's tested:

Visual comparison of baked probes master vs PR. Nothing changed as expected
Culling distance in Debug menu still works and is moved
New debug modes

Issues:

FIXED Ease of use. It's currently difficult to use the debug system out of the box. The arrows are not visible with default settings, often because they're inside of an actual probe. It requires a lot of tweaking to be able to see the arrows, even more if you want to have them in the right proportion to the probes. Maybe it would make sense to have the arrows of relative size to the probes or/and have an offset from the center?

GiM0c322QV.mp4

tlaedre · 2021-12-15T15:11:56Z

Testing status: In progress

What's tested:

Visual comparison of baked probes master vs PR. Nothing changed as expected

Culling distance in Debug menu still works and is moved

Issues:

Ease of use. It's currently difficult to use the debug system out of the box. The arrows are not visible with default settings, often because they're inside of an actual probe. It requires a lot of tweaking to be able to see the arrows, even more if you want to have them in the right proportion to the probes. Maybe it would make sense to have the arrows of relative size to the probes or/and have an offset from the center?

GiM0c322QV.mp4

@iM0ve I agree that it's not necessarily super easy to dial things in, for me there are a couple of reasons for this:

The virtual offset as it is in master defaults to fairly small search distance and push offset, and it has a couple of bugs that can often push probes inside instead of away from geometry - this makes some cases all the more confusing.

It's hard to know the density / scale users will work at. For me, the default probe size is always too large by a factor of at least ten. For others the opposite might be true.

I'm open to suggestion of how to improve this, but IMO the length of the arrow needs to be fixed to the actual offset that has taken place, so that doesn't give you a whole lot of remaining options to work with. Maybe we should default to a smaller with so they more obviously look like arrows?

The attached image is how I tend to use them

…bug.

tlaedre · 2021-12-17T15:27:11Z

@iM0ve added some automatic downscale of probe size if probes are enabled when enabling offset.

iM0ve

Tested the new UI change. It works better out of the box, The offsets are visible on enabling and you can still resize the probes afterwards if you wish.

Ki9vnyyuaB.mp4

tlaedre · 2022-01-10T13:12:01Z

@FrancescoC-unity @sebastienlagarde am I supposed to hit the merge button myself, or will some guardian do that? (provided the DXR test gets in line)

# Conflicts: # com.unity.render-pipelines.core/Editor/Lighting/ProbeVolume/ProbeGIBaking.cs # com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeReferenceVolume.Debug.cs # com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeReferenceVolume.cs # com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeVolumePerSceneData.cs

Add probe volume debug modes for l0, l0l1 and placement virtual offset.

c207893

github-actions bot added HDRP SRP labels Oct 28, 2021

tlaedre commented Oct 28, 2021

View reviewed changes

tlaedre requested a review from FrancescoC-unity October 28, 2021 22:09

Tweak defaults and threshold values.

9c7673a

FrancescoC-unity reviewed Nov 2, 2021

View reviewed changes

FrancescoC-unity requested a review from iM0ve November 2, 2021 10:19

Merge remote-tracking branch 'origin/master' into HDRP/apv-debug-modes

accfd20

JulienIgnace-Unity approved these changes Nov 9, 2021

View reviewed changes

iM0ve requested changes Nov 15, 2021

View reviewed changes

iM0ve mentioned this pull request Nov 16, 2021

Improvements to APV Virtual Offset #6174

Merged

tlaedre added 2 commits December 8, 2021 15:39

Merge remote-tracking branch 'origin/master' into HDRP/apv-debug-modes

8201030

Account for null value when loading data baked before this revision.

f61e135

tlaedre marked this pull request as ready for review December 8, 2021 17:29

tlaedre requested a review from iM0ve December 8, 2021 17:29

tlaedre and others added 2 commits December 9, 2021 08:41

Merge remote-tracking branch 'origin/master' into HDRP/apv-debug-modes

1249b66

Apply formatting changes

ca5eab7

iM0ve requested changes Dec 14, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into HDRP/apv-debug-modes

a228b20

Automatically scale down debug probes when enabling virtual offset de…

b676879

…bug.

tlaedre requested a review from iM0ve December 17, 2021 15:26

iM0ve approved these changes Dec 20, 2021

View reviewed changes

Apply formatting changes

30057e0

FrancescoC-unity approved these changes Jan 10, 2022

View reviewed changes

FrancescoC-unity added 2 commits January 12, 2022 19:12

Merge branch 'master' into HDRP/apv-debug-modes

361373e

FrancescoC-unity merged commit 1dd75c6 into master Jan 12, 2022

FrancescoC-unity deleted the HDRP/apv-debug-modes branch January 12, 2022 18:42

RSlysz mentioned this pull request Feb 9, 2022

Better builtin support in ResourceReloader #7069

Merged

Add additional APV debug modes #6169

Add additional APV debug modes #6169

Uh oh!

Conversation

tlaedre commented Oct 28, 2021

Purpose of this PR

Testing status

Comments to reviewers

Uh oh!

github-actions bot commented Oct 28, 2021

Uh oh!

tlaedre Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlaedre Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlaedre Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrancescoC-unity left a comment

Choose a reason for hiding this comment

Uh oh!

iM0ve left a comment

Choose a reason for hiding this comment

Uh oh!

tlaedre commented Dec 8, 2021

Uh oh!

iM0ve left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlaedre commented Dec 15, 2021

Uh oh!

tlaedre commented Dec 17, 2021

Uh oh!

iM0ve left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlaedre commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tlaedre Oct 28, 2021 •

edited

Loading

tlaedre Oct 28, 2021 •

edited

Loading

tlaedre Nov 2, 2021 •

edited

Loading

iM0ve left a comment •

edited

Loading

iM0ve left a comment •

edited

Loading

tlaedre commented Jan 10, 2022 •

edited

Loading