Binary APV data with runtime data separated from debug data #6196

tlaedre · 2021-11-03T17:55:16Z

NOTE: This PR depends on native (git) PR#6458 and will stay in draft until this dependency lands.

Purpose of this PR

This PR addresses a couple of overlapping issues with Probe Volume lighting data storage.

The primary purpose of this PR is to change PV lighting data to use a more performant storage method. The way Unity serialization works, storing large amounts of data (hundreds of MB) in ScriptableObjects adds significant overhead to editor and runtime loading. In a test scene with ~150 MB of baked lighting data, storing the data in ScriptableObjects adds more than 4 seconds of domain reload overhead, whereas storing it in the binary format add no measurable overhead.
(As a more extreme example, having baked data in one of our current production scenes stored as ScriptableObjects adds more than 30 seconds overhead to every domain reload, whereas with the binary method the overhead is negligible).

The second purpose is to separate runtime data from debug data to reduce build size and runtime memory usage. Debug data will be included in builds if the global HDRP settings have the runtime debug shaders checkbox ticked, but otherwise stripped for builds. The Probe Volume debug UI will present options based on which data is available.

Lastly, the probes storage itself is changed in two ways: It no longer always stores three full SH bands but instead stores the format of the actual baked data. The probe coefficients are stored pre-swizzled in the final format they will be uploaded as at runtime (the last bit is up for discussion, it came from a performance investigation that I will open a discussion about separately).

Testing status

Tested manually in test scenes and our production project.
Tested in editor and player builds.

Comments to reviewers

Generally speaking, see comments inline in code.

I suppose one of the more controversial changes here might be the decision to change the swizzled format of the probe data. This originated from the fact that runtime loading of cells is quite slow atm and is an overlapping part of ongoing work investigating loading performance (I will bring this as a discussion to Slack before putting further serious work into it). It's easy enough to undo, but at the very least as things stand now, I think it makes sense to store data as close to the runtime usage as possible.

…ialization slowdown (affects domain reloads). Store shader data pre-swizzled (priming a future loading PR).

…ebug display checked. Hide PV debug options when they are not available (and the entire panel when none are)

github-actions · 2021-11-03T17:55:30Z

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-11-03T17:56:45Z

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


        static Dictionary<Vector3Int, int> m_CellPosToIndex = new Dictionary<Vector3Int, int>();
-        static Dictionary<int, Cell> m_BakedCells = new Dictionary<int, Cell>();
+        static Dictionary<int, BakingCell> m_BakedCells = new Dictionary<int, BakingCell>();


BakingCell is now used throughout the entire baking process, only converted to a runtime Cell at the very end of the baking process.

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

tlaedre · 2021-11-03T17:58:22Z

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

-                                asset.cells.Add(cell);
+                                if (!assetsCommitted.Contains(assetPath))
+                                {
+                                    WritebackModifiedCellsData(asset);


Because of the way data storage is aliased, dilation now writes directly into the backing storage and we just re-commit that to disk.

tlaedre · 2021-11-03T18:00:00Z

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

                }
            }

+            // Convert baking cells to runtime cells


We convert all cells of an asset in a single operation since their data is flattened into shared storage.

tlaedre · 2021-11-03T18:00:57Z

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

+            AssetDatabase.Refresh();
            probeRefVolume.clearAssetsOnVolumeClear = false;

+            m_BakingBatch = null;


Clearing this exposes the fact that we still occasionally have multiple callbacks registered for completion (the second one will encounter this null and bail out with a callstack)

tlaedre · 2021-11-03T18:01:44Z

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

+            cellSupportDataFilename = System.IO.Path.ChangeExtension(sourceFilename, "CellSupportData.bytes");
+        }
+
+        static void WriteBakingCells(ProbeVolumeAsset asset, List<BakingCell> bakingCells)


Flatten all cells into two byte streams: one for runtime data and one for debug data.

Now there are three byte streams (the essential data, the optional data - aka L2, and the support data - aka debug) as discussed.

tlaedre · 2021-11-03T18:02:59Z

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

+                startCounts.Add(cellCounts);
+            }
+
+            // Need to save here because the forced import below discards the changes.


This feels like a bug to me.. I'll ask around Slack for what would cause this.

(did ask, will log a bug for this separately)

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

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

tlaedre · 2021-11-03T18:09:10Z

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

+            if (parameters.supportsRuntimeDebug)
+            {
+                // Cells / Bricks visualization is not implemented in a runtime compatible way atm.
+                if(Application.isEditor)
+                    widgetList.Add(subdivContainer);
+
+                widgetList.Add(probeContainer);
+            }
+
+            if (parameters.supportStreaming)
+            {
+                widgetList.Add(streamingContainer);
+            }

-            m_DebugItems = widgetList.ToArray();
-            var panel = DebugManager.instance.GetPanel("Probe Volume", true);
-            panel.children.Add(m_DebugItems);
+            if (widgetList.Count > 0)
+            {
+                m_DebugItems = widgetList.ToArray();
+                var panel = DebugManager.instance.GetPanel("Probe Volume", true);
+                panel.children.Add(m_DebugItems);
+            }


Hide the options that don't have the required backing data available.

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

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

tlaedre · 2021-11-03T18:14:05Z

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

+            if (assets.TryGetValue(m_CurrentState, out var asset) && asset != null)
            {
-                refVol.AddPendingAssetLoading(assets[m_CurrentState]);
+                asset.ResolveCells();


The source data needed to resolve is propagated in OnAfterDeserialize.

tlaedre · 2021-11-03T18:15:03Z

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

+        public void StripSupportData()
+        {
+            foreach (var asset in assets.Values)
+                asset.cellSupportDataAsset = null;


Since OnBeforeSerialize reads this is serves as clearing out the use of the support data asset thus omitting it from builds.

tlaedre · 2021-11-03T18:15:19Z

com.unity.render-pipelines.high-definition/Editor/Lighting/ProbeVolumeDebugStripper.cs

+                // TODO: Arguably we should just destroy the entire component if 'supportProbeVolumes' is disabled. Otherwise previously baked data
+                //       will still be bundled with players and loaded with the scene even though the entire feature is disabled. Discuss this
+                //       before enabling it, though.


Not sure as AFAIK you can potentially swap global settings asset while the player is running (not sure why you would, but it could happen)

FrancescoC-unity · 2021-11-08T10:39:21Z

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

                for (int i = 0; i < probeCount; ++i)
                {
-                    dilatedProbes[i].FromSphericalHarmonicsL2(cell.sh[i]);
+                    dilatedProbes[i].FromSphericalHarmonicsShaderConstants(cell.shBands, cell.shData, i);


The problem I have with this not dilating the whole 3 bands is that this now makes switching between L1 and L2 at runtime not possible.

Let's assume the data is baked with L1 set as option, then it will be stored as L1 with garbage L2.
Switching then to L2 will require a rebake as otherwise garbage will be read.

I see where you are coming from, but I can also this potentially rising issues and annoyances for users.

Before I go on and look more at the PR we should discuss this as I think is pivotal for the rest.

As discussed, we currently always store all three bands when baking (i.e. ignore current lighting setting for bake). A new toggle should be introduced for controlling how many bands to store / how many bands to bundle with builds. Loading mixed cells already works.

FrancescoC-unity · 2021-11-09T09:49:15Z

Will look at this in few minutes, also adding @JulienIgnace-Unity as this will be likely relevant for streaming

FrancescoC-unity · 2021-11-09T09:53:35Z

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


                cell.sh = new SphericalHarmonicsL2[numProbes];
                cell.validity = new float[numProbes];
+                cell.offsetVectors = new Vector3[0]; // pending PR #6169


Reminder here for me/you to remove this comment when we can

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

FrancescoC-unity

Added some comments here and there in addition to what we discussed via slack already, let's have another pass when that lands!

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

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

FrancescoC-unity · 2021-11-09T10:28:45Z

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

+
+            // shData is swizzled for direct runtime upload
+            //
+            // L0r0, L0g0, L0b0, L1r0


It'd be nice to have a very small function to this PR that moves from a probe index to a SphericalHarmonicsL2 (undoing the swizzling) as I am sure it'll be useful for debug and potentially other use cases.

These exist:
ProbeGIBaking.WriteToShaderCoeffsL0L1/WriteToShaderCoeffsL2/ReadFromShaderCoeffsL0L1/ReadFromShaderCoeffsL2
DilatedProbe.FromSphericalHarmonicsShaderConstants/ToSphericalHarmonicsShaderConstants

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

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

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

FrancescoC-unity · 2021-11-09T10:44:04Z

com.unity.render-pipelines.high-definition/Editor/Lighting/ProbeVolumeDebugStripper.cs

+                // TODO: Arguably we should just destroy the entire component if 'supportProbeVolumes' is disabled. Otherwise previously baked data
+                //       will still be bundled with players and loaded with the scene even though the entire feature is disabled. Discuss this
+                //       before enabling it, though.


Not sure as AFAIK you can potentially swap global settings asset while the player is running (not sure why you would, but it could happen)

FrancescoC-unity · 2021-11-29T15:08:54Z

Any ETA on when (or if) this can go be updated?

@adrien-de-tocqueville is currently working on baking states and we'd need to do some optimization on having just SH data in a separate asset; this PR looks like could provide one way of doing that so but if we do something else might end up conflicting badly with this :-)

Let us know if you need us to take ownership of the PR if you don't have the time!

…me-debug-data

… feedback and discussions.

adrien-de-tocqueville

minor comment, also note that the formatting job is failing

adrien-de-tocqueville · 2021-12-13T12:37:55Z

com.unity.render-pipelines.high-definition/Editor/Lighting/ProbeVolumeDebugStripper.cs

+#endif
+                if (!settings.supportRuntimeDebugDisplay)
+                {
+                    Debug.Log($"Stripping debug data from Probe Volume in scene '{scene.name}'.");


do we want to keep that log here ?

I have no strong opinion tbh. I feel like it's somewhat similar to the debug log for shader stripping, in that it's nice to be able to verify in the log that the debug data was indeed stripped.

i guess people assume debug data are stripped so i would remove it, but not very important

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

tlaedre · 2021-12-13T13:49:10Z

minor comment, also note that the formatting job is failing

thanks for pointing that out. I meant to launch the apply formatting job but forgot :(

iM0ve

Test status: Done

Whats tested:

Compared baked result of classroom scene Master vs PR
Checked Dilation still works
Checked that Virtual Offset still works
Debug modes still display bricks, cells and probes
Global and local volumes still work
Baking from lighting tab and APV tab
Baking with Spherical Harmonics L1 and L2
Creating baking sets and using them
Changing memory budget
Building player to see APV is there

iM0ve

I updated my review. I did quite a few tests but as this feature is already used in production I focused on looking for regressions. Have not found any new issues

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

sebastienlagarde · 2021-12-16T16:01:16Z

Hey, test are failing for standalone now:
https://yamato-artifactviewer.prd.cds.internal.unity3d.com/0c0a856d-ba3b-437b-bf8f-e8fe946bae14%2Flogs%2FTestProjects%2FHDRP_RuntimeTests%2Ftest-results/TestReport.html

FrancescoC-unity · 2021-12-16T16:02:54Z

Argh yeah this is not compatible with old assets, @tlaedre can you please rebake that test and update screenshots if needed?
Let me know if you don't have the time and I can try myself.

tlaedre · 2021-12-16T16:05:16Z

I could, if I had any idea whatsoever how that worked. Is there any docs for how to update test screenshots?

tlaedre · 2021-12-16T16:05:55Z

I guess this update should be part of the PR?

FrancescoC-unity · 2021-12-16T16:06:54Z

Yes, give me 10 min and I can explain in slack

…me-debug-data

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

tlaedre added 3 commits November 2, 2021 20:29

Split runtime and debug data. Store data in binary files to avoid ser…

aaa35cf

…ialization slowdown (affects domain reloads). Store shader data pre-swizzled (priming a future loading PR).

Strip out debug data from builds unless global settings has runtime d…

0addc34

…ebug display checked. Hide PV debug options when they are not available (and the entire panel when none are)

Fold in crp/apv-scenedata since the changes are more or less the same.

09881fc

github-actions bot added HDRP SRP labels Nov 3, 2021

tlaedre commented Nov 3, 2021

View reviewed changes

com.unity.render-pipelines.core/Editor/Lighting/ProbeVolume/ProbeGIBaking.cs Outdated Show resolved Hide resolved

tlaedre commented Nov 3, 2021

View reviewed changes

com.unity.render-pipelines.core/Editor/Lighting/ProbeVolume/ProbeVolumeAssetEditor.cs Show resolved Hide resolved

tlaedre commented Nov 3, 2021

View reviewed changes

com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeBrickPool.cs Outdated Show resolved Hide resolved

tlaedre commented Nov 3, 2021

View reviewed changes

com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeBrickPool.cs Outdated Show resolved Hide resolved

tlaedre commented Nov 3, 2021

View reviewed changes

com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeVolumeAsset.cs Outdated Show resolved Hide resolved

tlaedre commented Nov 3, 2021

View reviewed changes

com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeVolumePerSceneData.cs Show resolved Hide resolved

tlaedre commented Nov 3, 2021

View reviewed changes

tlaedre requested a review from FrancescoC-unity November 3, 2021 18:15

FrancescoC-unity reviewed Nov 8, 2021

View reviewed changes

FrancescoC-unity requested a review from JulienIgnace-Unity November 9, 2021 09:48

FrancescoC-unity reviewed Nov 9, 2021

View reviewed changes

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

FrancescoC-unity reviewed Nov 9, 2021

View reviewed changes

FrancescoC-unity requested a review from adrien-de-tocqueville December 3, 2021 13:10

tlaedre added 3 commits December 8, 2021 18:37

Merge remote-tracking branch 'origin/master' into crp/apv-split-runti…

90af120

…me-debug-data

Merge remote-tracking branch 'origin/master' into crp/apv-split-runti…

879332f

…me-debug-data

Replace placeholder binary asset mocking with the real thing. Address…

0c01ec8

… feedback and discussions.

tlaedre requested a review from FrancescoC-unity December 10, 2021 14:50

FrancescoC-unity requested a review from iM0ve December 13, 2021 08:56

adrien-de-tocqueville reviewed Dec 13, 2021

View reviewed changes

Apply formatting changes

ffdd401

adrien-de-tocqueville approved these changes Dec 14, 2021

View reviewed changes

iM0ve requested a review from a team December 14, 2021 12:25

iM0ve reviewed Dec 15, 2021

View reviewed changes

iM0ve approved these changes Dec 16, 2021

View reviewed changes

sebastienlagarde approved these changes Dec 16, 2021

View reviewed changes

Merge branch 'master' into crp/apv-split-runtime-debug-data

cffe783

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

sebastienlagarde marked this pull request as ready for review December 16, 2021 13:52

Apply formatting changes

396e0f4

tlaedre and others added 3 commits December 17, 2021 10:07

Update test data.

bbf04a7

Merge remote-tracking branch 'origin/master' into crp/apv-split-runti…

f3e55f9

…me-debug-data

Merge branch 'master' into crp/apv-split-runtime-debug-data

92783cf

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

FrancescoC-unity merged commit 12ced9a into master Jan 12, 2022

FrancescoC-unity deleted the crp/apv-split-runtime-debug-data branch January 12, 2022 18:09

Binary APV data with runtime data separated from debug data #6196

Binary APV data with runtime data separated from debug data #6196

Uh oh!

Conversation

tlaedre commented Nov 3, 2021

Purpose of this PR

Testing status

Comments to reviewers

Uh oh!

github-actions bot commented Nov 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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 commented Nov 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FrancescoC-unity left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrancescoC-unity commented Nov 29, 2021

Uh oh!

adrien-de-tocqueville left a comment

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!

iM0ve left a comment •

edited

Loading