First WIP introduction of Adaptive Probe Volumes. by FrancescoC-unity · Pull Request #2953 · Unity-Technologies/Graphics

FrancescoC-unity · 2020-12-18T12:48:45Z

This PR introduce a first iteration over APV, it is not yet in a usable state, just to have some core code in master.
APV stands for Adaptive Probe Volume, it replaces Probe Volumes v1. It works essentially by adaptively placing probes around the scene, subdividing as required.

The sampling code is simpler as it is a single lookup on the indirection structure and finally samples the SH coefficients from a storage.

The PR includes:

Basic workflow (might contain bugs) with asset and profile production.
Basic baking and placement (will see improvements)
Lightloop sampling of the APV structure
Volumetrics sampling.
Material AO support.
L0 and L1 sampling code (everything does L1, volumetrics do L0)
Some debug visualization.
Multiple scene handling for additive loading of different assets.

The loading in player needs still to be handled with user-side script, but possible with the simple asset that gets produced by the baking process.

A lot is meant to be changed in a near future, so the usage is not recommended.
It is of course turned off by default.

…obevolumes-draft. This reorganization lets us work on Adaptive Probe Volumes in context of Env2.0 without having to wait for Env2.0 to update to HDRP master. # Conflicts: # com.unity.render-pipelines.high-definition/Runtime/Lighting/ProbeVolume.meta

…ssed from Env2.0 code. This may not be needed for the final version, but for now we need this.

…ocation. Also fixed a minor issue in the index, but indexing is still not correctly location aware.

…ing.

# Conflicts: # com.unity.render-pipelines.high-definition/Editor/Lighting/ProbeVolume.meta # com.unity.render-pipelines.high-definition/Editor/Lighting/ProbeVolume/ProbeVolumeEditor.cs # com.unity.render-pipelines.high-definition/Editor/Lighting/ProbeVolume/ProbeVolumeEditor.cs.meta # com.unity.render-pipelines.high-definition/Editor/Lighting/ProbeVolume/ProbeVolumeUI.Drawer.cs # com.unity.render-pipelines.high-definition/Editor/Lighting/ProbeVolume/ProbeVolumeUI.Drawer.cs.meta # com.unity.render-pipelines.high-definition/Editor/Lighting/ProbeVolume/ProbeVolumeUI.Skin.cs # com.unity.render-pipelines.high-definition/Editor/Lighting/ProbeVolume/ProbeVolumeUI.Skin.cs.meta # com.unity.render-pipelines.high-definition/Editor/Lighting/ProbeVolume/SerializedProbeVolume.cs # com.unity.render-pipelines.high-definition/Editor/Lighting/ProbeVolume/SerializedProbeVolume.cs.meta # com.unity.render-pipelines.high-definition/Editor/Lighting/VolumetricLighting/VolumetricMenuItem.cs # com.unity.render-pipelines.high-definition/Runtime/Lighting/ProbeVolume/ProbeVolume.cs # com.unity.render-pipelines.high-definition/Runtime/Lighting/ProbeVolume/ProbeVolume.cs.meta

… in.

… probes when evaluating GI influence. Also fixed a few issues with the existing data flow. # Conflicts: # com.unity.render-pipelines.high-definition/Runtime/Lighting/ProbeVolume/ProbeVolume.hlsl # com.unity.render-pipelines.high-definition/Runtime/Lighting/ProbeVolume/ProbeVolume.hlsl.meta # com.unity.render-pipelines.high-definition/Runtime/Lighting/ProbeVolume/ProbeVolumeLighting.cs # com.unity.render-pipelines.high-definition/Runtime/Lighting/ProbeVolume/ProbeVolumeLighting.cs.meta # com.unity.render-pipelines.high-definition/Runtime/Material/BuiltinGIUtilities.hlsl # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.cs # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDStringConstants.cs

…d public.

…ating the ambient probe.

Introduced a new base offset per column which is automatically updated based on the bricks added to the index.

…there are no bricks to subdivide anymore.

…ng up the index based on previous additions.

We saw surprising behaviour when debugging this line in Rider. The variable would increment at unexpected times presumably due to a bug in the Rider debugger.

We decided to not use a ref here because: * This requires the caller to set up a mutable value which is not always ideal. * We might as well follow the convensions established by the Entities package.

…ProbeVolumes

FrancescoC-unity · 2020-12-18T15:42:15Z

All tests pass locally.

sebastienlagarde · 2020-12-21T12:53:41Z


 From 10.x, a new pass ScenePickingPass have been added to all the shader and master node to allow the editor to correctly handle the picking with tesselated objects and backfaced objects.

+From 10.x, if material ambient occlusion needs to be applied to probe volume GI and the material is deferred, the material needs to define `HAS_PAYLOAD_WITH_UNINIT_GI` constant and a function `float GetUninitializedGIPayload(SurfaceData surfaceData)` that returns the AO factor that is desired to be applied. No action is needed for forward only materials or if no material AO needs to be applied to probe volume GI.


you can remove this statement,

it is not the right location (upagre 2020.1 to 2020.2)

we haven't ship the feature, so we don't care.

I now understand why it is there, should be in 2021.1 to 2021.2 upgrading guide (that we haven't created)

sebastienlagarde · 2020-12-21T13:00:39Z

+            #pragma vertex vert
+            #pragma fragment frag
+            #pragma multi_compile_instancing
+            #include "UnityCG.cginc"


We must convert this shader to new style SRP way, i.e don't use UnityCg.cginc. Can be done in next PR

sebastienlagarde · 2020-12-21T13:01:21Z

+
+            float3 evalSH(float3 normal, float4 SHAr, float4 SHAg, float4 SHAb)
+            {
+                float4 normalPadded = float4(normal, 1);


not sure why you don't reuse the functoin provided in EntityLighting.hlsl?

To answer this and the before, @thefranke lifted this code from another project altogether. We indeed need to rewrite it anyway to be more SRP complaint.

This wasn't considered a blocker for opening the PR though.

sebastienlagarde · 2020-12-21T13:02:29Z


+            if (ShaderConfig.s_EnableProbeVolumes == 1)
+            {
+                ProbeReferenceVolume.instance.InitProbeReferenceVolume(1024, m_Asset.currentPlatformRenderPipelineSettings.probeVolumeMemoryBudget, new Vector3Int(1024, 64, 1024));


what are the hardcoded value 1024 and other? (can be fix in next pr)

The first 1024 is just how much the pool allocator uses as size for the brick pool. It is a value that we need to tweak and see on profiling if it makes a difference changing it, realistically we just don't need it as a parameter of the Init function, but would need investigation whether it makes sense for different pipelines (not different projects) to have different values.

Note that the value is not really that hardcoded, as in it is a fixed number in a constructor, but then the value itself is correctly named in the rest of the code.

The second set is again just whatever is the default for the index dimension of the reference volume. It will be overwritten at first load if it mismatches what's on the profile. We can give it a name and store as static somewhere indeed.

sebastienlagarde · 2020-12-21T13:03:39Z

-    // No need to branch internally on _EnableProbeVolumes uniform.
-    if (featureFlags & LIGHTFEATUREFLAGS_PROBE_VOLUME)
+    // Even so, the bound resources might be invalid in some cases, so we still need to check on _EnableProbeVolumes.
+    bool apvEnabled = featureFlags & LIGHTFEATUREFLAGS_PROBE_VOLUME && _EnableProbeVolumes;


use (featureFlags & LIGHTFEATUREFLAGS_PROBE_VOLUME) && _EnableProbeVolumes for clarity

sebastienlagarde · 2020-12-21T13:21:40Z

-    obbExtents = float3(probeVolumeBounds.extentX, probeVolumeBounds.extentY, probeVolumeBounds.extentZ);
-    obbCenter = probeVolumeBounds.center;
+    real4 SHCoefficients[7];
+    SHCoefficients[0] = unity_SHAr;


We should create a function with this one, the exact same code is used in builtinGIUtilities.hlsl (next PR)

This the function :)
I just need to move this to the builtinGIUtilities and use there.

sebastienlagarde · 2020-12-21T13:23:39Z

-        float3 sampleOutgoingRadiance = SHEvalLinearL0L1(normalWS, coefficients.data[0], coefficients.data[1], coefficients.data[2]);
-        return sampleOutgoingRadiance;
+        // no valid brick, fallback to ambient probe
+        bakeDiffuseLighting = EvaluateAmbientProbe(normalWS);


See my comment above, same code is use for both front and back lighting in builtinGIUtilitiers.hlsl

sebastienlagarde · 2020-12-21T13:34:21Z

+    posInput.positionWS = ray.originWS + t * ray.jitterDirWS;
+
+    float3 apvDiffuseGI;
+    EvaluateAdaptiveProbeVolume(GetAbsolutePositionWS(posInput.positionWS), apvDiffuseGI);


EvaluateAdaptiveProbeVolume evaluate the lighting with the lambert cosine convolution coefficient in it. We should remove it to apply on volumetric. Similar to what is done with Ambient probe in SetPreconvolvedAmbientLightProbe() function

sebastienlagarde · 2020-12-21T13:52:20Z

 }

+#define HAS_PAYLOAD_WITH_UNINIT_GI
+float GetUninitializedGIPayload(SurfaceData surfaceData)


I feel bad about this one, we will need to add it to all our lit material (hair, fabric, eye, staacklit...). But guess we have no choice. Problem is very similar to
GetAmbientOcclusionForMicroShadowing().
Let's let it for now for the experimental development, but not sure we have alternative. At least we should change the name to GetAmbientOcclusionFromSurface() so we can reuse it for other purpose

We don't need to add it to everything that's why I added the define, we only need to add it to deferred materials.

The reason why I avoided using AmbientOcclusion in that name is exactly to allow potential reuse for something else.

sebastienlagarde

Some random comment

# Conflicts: # com.unity.render-pipelines.high-definition/Editor/RenderPipeline/HDRenderPipelineUI.Skin.cs # com.unity.render-pipelines.high-definition/Editor/RenderPipeline/HDRenderPipelineUI.cs # com.unity.render-pipelines.high-definition/Runtime/Lighting/LightLoop/GlobalLightLoopSettings.cs # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDStringConstants.cs

urasmus and others added 30 commits November 10, 2020 13:03

Make ProbeReferenceVolume and related public so that they can be acce…

29da6a0

…ssed from Env2.0 code. This may not be needed for the final version, but for now we need this.

Make API accessible to other namespaces

a6524db

Can't sample RGB16SFloat

1dad630

Make API available to other namespaces

36c3e4e

Fixed a few issues with the size calculation of the BrickPool's DataL…

76bb636

…ocation. Also fixed a minor issue in the index, but indexing is still not correctly location aware.

Make RefVol transform publicly accesible, it is needed for debug draw…

bfa65da

…ing.

Add simple bounding box overlap test for renderers

1aad799

Fixed the indexing logic to correctly handle the brick indices passed…

065caf8

… in.

Need access from Env

cf8b9e8

Add utility method for calculating brick volumes, and make some metho…

df26e06

…d public.

Fixed sampling of the brick pool.

f49754d

Replaced debug ambient probe evaluation with the actual code of evalu…

2966ac9

…ating the ambient probe.

Modified index array addressing from zyx to zxy.

f09dc7c

Introduced a new base offset per column which is automatically updated based on the bricks added to the index.

Take max subdivision into account during rasterization.

bfcc1cd

Made a small modification to prevent unnecessary delegate calls when …

e45086d

…there are no bricks to subdivide anymore.

Added assert

8003219

Expose normal bias setting.

e812820

Added unloading to the runtime probe reference volume, including fixi…

5b9af58

…ng up the index based on previous additions.

Added out of bounds index handling when clearing voxels in the index.

9815cb8

Fixed voxels getting cleared multiple times instead of only once.

9f0a7e1

Avoid increment and read in same line.

5792a76

We saw surprising behaviour when debugging this line in Rider. The variable would increment at unexpected times presumably due to a bug in the Rider debugger.

Change ReleaseBricks's argument to be pass-by-copy.

0cc8dc9

We decided to not use a ref here because: * This requires the caller to set up a mutable value which is not always ideal. * We might as well follow the convensions established by the Entities package.

Fix meta files

b2ec1de

Remove PV1 specific UI and pipeline code

163ca3f

Use existing SH1 implementation

ee818d5

Fix merge from files moved to core and add back callbacks for baking …

704a4c8

…ProbeVolumes

Sample probes using material pass for now

0029cd0

github-actions Bot added the SRP label Dec 18, 2020

FrancescoC-unity added 2 commits December 18, 2020 13:50

Merge branch 'master' into HDRP/probe-volume2-lightloop-sampling

52b5c3b

Fix usage of deprecated baking API

956d262

FrancescoC-unity added 2 commits December 18, 2020 19:51

Wrong size for OctahedralDepth array

f961db5

Fix build errors and wanings about editor-only information.

f4c9eb4

sebastienlagarde reviewed Dec 21, 2020

View reviewed changes

Comment thread com.unity.render-pipelines.high-definition/Runtime/Lighting/LightLoop/LightLoop.hlsl

sebastienlagarde reviewed Dec 21, 2020

View reviewed changes

sebastienlagarde approved these changes Dec 21, 2020

View reviewed changes

sebastienlagarde marked this pull request as ready for review December 21, 2020 13:54

FrancescoC-unity added 5 commits December 21, 2020 15:05

Some review comments addressed.

33bf9fb

Give name to constants.

c53bf3e

Add the upgrade guide to 2021.1

52d82f0

Proper naming in upgrade guide

03cba62

sebastienlagarde marked this pull request as draft December 21, 2020 15:30

sebastienlagarde marked this pull request as ready for review December 21, 2020 15:48

Undo inversion of cosine kernel

19ccb6d

sebastienlagarde merged commit c8311fb into master Dec 21, 2020

sebastienlagarde deleted the HDRP/probe-volume2-lightloop-sampling branch December 21, 2020 17:43

sebastienlagarde restored the HDRP/probe-volume2-lightloop-sampling branch December 22, 2020 10:50

sebastienlagarde deleted the HDRP/probe-volume2-lightloop-sampling branch April 26, 2021 10:42


		From 10.x, a new pass ScenePickingPass have been added to all the shader and master node to allow the editor to correctly handle the picking with tesselated objects and backfaced objects.

		From 10.x, if material ambient occlusion needs to be applied to probe volume GI and the material is deferred, the material needs to define `HAS_PAYLOAD_WITH_UNINIT_GI` constant and a function `float GetUninitializedGIPayload(SurfaceData surfaceData)` that returns the AO factor that is desired to be applied. No action is needed for forward only materials or if no material AO needs to be applied to probe volume GI.

Conversation

FrancescoC-unity commented Dec 18, 2020

Uh oh!

FrancescoC-unity commented Dec 18, 2020

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!

Choose a reason for hiding this comment

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!

sebastienlagarde Dec 21, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

sebastienlagarde left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sebastienlagarde Dec 21, 2020 •

edited

Loading