[APV] Cell streaming system #5731

JulienIgnace-Unity · 2021-09-20T15:07:01Z

Purpose of this PR

This PR introduces cell streaming to the APV.

This includes:

Refactor to split properly loading cell from the asset and uploading data to GPU
Renamed things a bit: Add for adding a cell from asset and Load to upload it to GPU
Handled out of memory (both index and texture) more gracefully: Cell isn't uploaded but still available in CPU memory
Cell debug is now red for cells that aren't "loaded" (uploaded to GPU)
Precompute of cell chunk cost
Added name of 3D textures allocated for APV
First version of streaming of cell. Only based on distance to camera for now.
Debug option to "freeze" streaming in order to assess the current state
Removed all GCAlloc for load/unload code as it is now called regularly.
Disabled by default. Added a debug entry to enable it.
Actual data upload is done in "chunks" to avoid allocating for worse case scenario. Currently set at 8 chunks (500KB)
Index GPU Buffer update is now done partially to improve performance.

Still needs to be done:

Handle multiple cameras
Handle fragmentation
Base streaming on more than just camera position.

What has been tested:

Automated tests are green
Baking one/Multiple scenes
Tested add/remove of scenes after baking
Tested that streaming actually worked while under budget (with the help of cell debug)
Tested that being under budget is handled gracefully with streaming disabled (no error, just stops loading after being oom)

Updated Probe Reference debug to use it completely

… into hd/apv-cell-streaming

…enes.

…oaded at the same time.

…nstant buffers

…ebug." This reverts commit 51eac63.

… into hd/apv-msic-fixes

This reverts commit e128c24.

…ad of constant buffers" This reverts commit d381f78.

… into hd/apv-msic-fixes

… into hd/apv-cell-streaming # Conflicts: # com.unity.render-pipelines.core/Runtime/Utilities/MeshGizmo.cs

…gies/Graphics into hd/apv-cell-streaming

… into hd/apv-cell-streaming # Conflicts: # com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeReferenceVolume.cs

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

FrancescoC-unity · 2021-09-24T07:44:12Z

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

+        int m_UpdateMaxIndex = int.MinValue;
+
+        // Static variable required to avoid allocations inside lambda functions
+        static Cell g_Cell = null;


yikes, can we use the cell index instead to do the checks?

We can but I don't think that would make any difference, would it? It's just comparing references.

would we still need to have the global static thing? I guess?

Yeah, the static is mandatory because we use lambda for finding stuff. So it would capture the variable and generate gcalloc without it.
I guess we can make functors and set the ref in it to avoid this but it's much more verbose for not a lot of gain (I don't expect this to be run multi-threaded)

Alternatively, you can replace the FindIndex with an old school for loop, it will avoid GC allocs and the static global field.

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

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

FrancescoC-unity · 2021-09-24T08:03:39Z

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

+
+            // In order not to pre-allocate for the worse case, we update the texture by smaller chunks with a preallocated DataLoc
+            int chunkIndex = 0;
+            while (chunkIndex < cellInfo.chunkList.Count)


This looks correct, but is delicate, have you stress tested this a bit to see if all looks fine?

I tested as much as I could but it could be interesting to test with bigger/more complex scenes as well. As it is, it's hard to force streaming activity in a relevant way :/

FrancescoC-unity · 2021-09-24T08:05:38Z

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

+            public int flatIdxInCellIndices = -1;
+            public bool loaded = false;
+            public ProbeBrickIndex.CellIndexUpdateInfo updateInfo;
+            public int sourceAssetInstanceID;


Any reason for this change? It was very useful to use a string as a ID to debug

Perf reason. I need to check that everytime an asset is streamed in/out to remove existing cells from internal arrays. Comparing strings did not feel good at all.
I might be overthinking this though, for now we don't even have asset streaming so...

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

FrancescoC-unity · 2021-09-24T08:11:32Z

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

-            cells[cell.index] = cell;
+            // The same cell can exist in more than one asset
+            // Need to check existence because we don't want to add cells more than once to streaming structures
+            // TODO: Check perf if relevant?


Kind of the same as above. This is relevant in term of perf when we have asset streaming. Right now it is expected that this happens during loading only so it's probably fine as is.

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

… into hd/apv-cell-streaming-2

…ilation

… into hd/apv-cell-streaming-2

… into hd/apv-cell-streaming-2 # Conflicts: # com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeReferenceVolume.cs

iM0ve · 2021-10-11T09:32:20Z

Testing status: Complete

Whats to be tested:
Real-world use

Upgrading ClassRoom APV project and rebaking

Feature testing

Probe Volume settings
Probe volume override
Probe volume component
HD Asset APV settings
Cell streaming system itself
Baking sets
Loading/unloading scenes
Debug modes

Workflow

Setting up a new APV project. Used Amalienborg as base

No streaming specific issues found but filed a lot for general APV here

Showcase

4EHeTZ9blF.mp4

iM0ve

Updated the review

… into hd/apv-cell-streaming-2

… into hd/apv-cell-streaming-2 # Conflicts: # com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeReferenceVolume.Debug.cs

* master: Add missing shader stages in shader keyword copying (#6008) [Yamato] Add all DX11 job for URP (#6075) [HDRP] Add more performance test coverage (#5814) Fix templates ISO date (#6073) Fix division by 0 when AO is 0 (#6078) Fixed grammar errors (#6077) [HDRP][Path Tracing] Improved robustness of the stacklit material (#6066) [APV] Cell streaming system (#5731) Update revision for URP update test project (#6061) [HDRP][Path Tracing] Camera ray misses now return a null value with Minimum Depth > 1 (#6067) Renable missing test (Lens Flare) (#5456) [HDRP][Path Tracing] Added proper support for interleaved tiling (#5953) Fix to render depth or depth/normal of waving grass (#4097) Remove ScreenSpaceShadowResolvePass (#6002) [HDRP][Docs] Update docs with RendererList related option (#6031) Layer drawer used in ray/path tracing now matches 100% with camera's. (#5956) Fix iridescence tooltip (#5950) [HDRP] Change RenderGraph Begin/Execute function pattern to avoid leaks (#5929) [HDRP] Fix errors when switching build targets in editor (#5918) Generate a material as a subasset for ShaderGraphs (#5795)

JulienIgnace-Unity and others added 30 commits August 5, 2021 15:06

Updated MeshGizmo to handle both traingles and lines.

75c954a

Updated Probe Reference debug to use it completely

Fixed cell and brick debug scale

570493f

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

1bfc541

… into hd/apv-cell-streaming

With a null ref exception when applying virtual offset on unloaded sc…

53f4f3f

…enes.

Fixed an issue causing failure of the bake when multiple scenes are l…

70a3d3d

…oaded at the same time.

Faster new version of debug probes with compute buffers instead of co…

d381f78

…nstant buffers

Fixed debug probes after domain relaod when using L1

00a460e

Fixed compute buffer probe debug

e128c24

Matched cell debug culling with cell culling used for probe debug.

51eac63

Revert "Matched cell debug culling with cell culling used for probe d…

c3160d6

…ebug." This reverts commit 51eac63.

Fixed per probe culling.

31180f1

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

0a7730f

… into hd/apv-msic-fixes

Revert "Fixed compute buffer probe debug"

78596c7

This reverts commit e128c24.

Revert "Faster new version of debug probes with compute buffers inste…

72a75b4

…ad of constant buffers" This reverts commit d381f78.

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

1cbd0b5

… into hd/apv-msic-fixes

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

b44cde7

… into hd/apv-cell-streaming # Conflicts: # com.unity.render-pipelines.core/Runtime/Utilities/MeshGizmo.cs

Merge branch 'hd/apv-msic-fixes' of https://github.com/Unity-Technolo…

f762552

…gies/Graphics into hd/apv-cell-streaming

Cleaning in preparation for streaming

4c7f031

More cleanup + Fix debug upon load/unload of cells

f0d5e5c

Fix GCAlloc

4fdb748

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

efa24b7

… into hd/apv-cell-streaming # Conflicts: # com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeReferenceVolume.cs

Add missing update function

e89c332

Proper error handling on apv texture allocation failure

8a8b47d

Handle APV index allocation failure

c580086

Small cleanup

7a7e08e

Create hidden per scene data

a584b9b

Start using new mode in baking workflow (Need to fix the tmp send data)

87fed75

Remove loading/removal from authoring

922f827

Need to switch branch, move profile data to asset

49da9e8

Almost completely get rid of baking reference volume

7e86ae5