SSR Improvement #1928

skhiat · 2020-09-17T16:39:11Z

SSR Improvement

The improvement:

Uses the VNDF Importance Sampling
Temporal accumulation controllable with the new parameter 'Accumulation Amount' (0 No accumulation, 1 Accumulation uses multiple frames). More accumulation generate more ghosting

Better match:
New SSR:

Pathtracer (Bounce == 2):

Checklist for PR maker

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

SSR will give us a better match with Pathtracer.

Blurriness (blur radius)
With the adjustment of "Accumulation Amount" will be "override" by the code depending of the motion vector (of the original point and the hit point).

Testing status

On the scene add a moving object for example a cylinder and move it, note the motion vector for objects are only available in the GameView:
` public float speed = 5.0f;

// Update is called once per frame
void Update()
{
    transform.Rotate(Vector3.right * speed * Time.deltaTime);
}`

For rough surface, we will have more noise even with large accumulation, on region of this cylinder.
Ref:
https://drive.google.com/file/d/1zyrvVWU-bzVqYOyOERe7OMvnZXtjbxzI/view?usp=sharing

Parameters to consider during a test:
Object thickness, which allow the ray to pass behind thin object:

Correct value: scene dependents

Allow more accumulation with multiple frame:

Yamato: (Select your branch):
https://yamato.prd.cds.internal.unity3d.com/jobs/902-Graphics

Comments to reviewers

Notes for the reviewers you have assigned.

… into HDRP/ssr_improvement

… into HDRP/ssr_improvement # Conflicts: # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.cs

… into HDRP/ssr_improvement # Conflicts: # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDStringConstants.cs

… into HDRP/ssr_improvement

… into HDRP/ssr_improvement # Conflicts: # com.unity.render-pipelines.high-definition/Runtime/Lighting/ScreenSpaceLighting/ScreenSpaceReflections.compute

… into HDRP/ssr_improvement # Conflicts: # com.unity.render-pipelines.high-definition/CHANGELOG.md # com.unity.render-pipelines.high-definition/Runtime/Debug/DebugDisplay.cs.hlsl

… into HDRP/ssr_improvement

anisunity · 2020-09-25T12:20:02Z

...ipelines.high-definition/Runtime/Lighting/ScreenSpaceLighting/ScreenSpaceReflections.compute

+    // Pick which subpixel we will be launching our effects from
+    float subPixelSample = GetBNDSequenceSample(positionSS, globalSampleIndex, 3);
+    int subPixel = clamp((int)(subPixelSample * 4.0), 0, 3);
+    uint2 shift = HalfResIndexToCoordinateShift[subPixel];


This is code picked from RayTracingReflection.compute is meant to handle BSDF data info fetch in half res mode, in the current state SSR doesn't have half res mode. This is a copy paste mistake i guess.

We switched to an algorithm that does temporal accumulation this shift help for quality. I am open to discuss that.

This usage of the code, if on purpose, increases the variance and biases the signal even more, I am sure of that.

That help on the edges & very close hit (corners: Wall vs Floor/Ceilling)

…HDRP/ssr_improvement # Conflicts: # com.unity.render-pipelines.high-definition/CHANGELOG.md

…HDRP/ssr_improvement

…HDRP/ssr_improvement # Conflicts: # com.unity.render-pipelines.high-definition/CHANGELOG.md

sebastienlagarde · 2020-10-24T10:46:18Z

com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/ShaderPass/VertMesh.hlsl

@@ -1,3 +1,6 @@
+#include "Packages/com.unity.render-pipelines.high-definition/Runtime/Debug/DebugDisplay.cs.hlsl"


Why do you need those hearders here? it is not expected that we put such header in VertMesh.hlsl

sebastienlagarde · 2020-10-24T10:47:09Z

com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.cs

+            if (hdCamera.isFirstFrame || hdCamera.cameraFrameCount <= 2)
+                cb._SsrAccumulationAmount = 1.0f;
+            else
+                cb._SsrAccumulationAmount = Mathf.Pow(2, Mathf.Lerp(-0.0f, -7.0f, volumeSettings.accumulationFactor.value));


typo, there is a -0.0 here

…Graphics into HDRP/ssr_improvement

…HDRP/ssr_improvement

sebastienlagarde · 2020-10-25T21:31:50Z

com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.LightLoop.cs

+                        passData.ssrAccum = builder.WriteTexture(ssrAccum);
+                        passData.ssrAccumPrev = builder.WriteTexture(ssrAccumPrev);
+                        passData.lightingTexture = builder.CreateTransientTexture(new TextureDesc(Vector2.one, true, true)
+                        { colorFormat = GraphicsFormat.R16G16B16A16_SFloat, clearBuffer = true, clearColor = Color.clear, enableRandomWrite = true, name = "SSR_Lighting_Texture" });


this must be of color buffer format (i.e 101010f or 16f depends on the option of hdrp asset)

I see it is not related to your PR, I will take care of it separately

sebastienlagarde · 2020-10-25T21:54:43Z

com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.cs

+
+            if (!usePBRAlgo)
+            {
+                CoreUtils.SetKeyword(cmd, "SSR_APPROX", false);


I don't understand why you do this at the end. You must no do this but instead do:

CoreUtils.SetKeyword(cmd, "SSR_APPROX", !usePBRAlgo); at the beginning.

oh unless it is for the transparent case?

…HDRP/ssr_improvement

sebastienlagarde · 2020-10-25T22:06:33Z

com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.cs

                cmd.DispatchCompute(cs, parameters.reprojectionKernel, HDUtils.DivRoundUp(parameters.width, 8), HDUtils.DivRoundUp(parameters.height, 8), parameters.viewCount);
+
+                if (parameters.transparentSSR)
+                    CoreUtils.SetKeyword(cmd, "DEPTH_SOURCE_NOT_FROM_MIP_CHAIN", false);


you don't need to reset keyword at the end (unless you reuse the shader, you should setup

CoreUtils.SetKeyword(cmd, "DEPTH_SOURCE_NOT_FROM_MIP_CHAIN", parameters.transparentSSR); at the beginning

oh unless it is for the transparent case?

* Change reflection direction to match GGX * local tmp * Temp Denoise and Inpainting * Backup remote work * Remove blur step * Add doc * ChangeLog + small fix * Speed dependend convergence * Clean code * Fix multicamera + SSR * Cleanup * Saturate local & target velocity * Fix RenderGraph * Code cleaning + comments * Fix feedback * Support previous & new algorithm * missing file: support both algorithm * fix for default RenderGraph * Simplify UI * Remap parameters for more userfriendly parametrization * fix doc missing * Clean up * New rendergraph fix * temp * Fix alloc issues RenderGraphs * Update default values * Add NaN killer * New strategy for 'first frame' reset * Update doc with clearer explaination * Perf improvement * Move code * Fix GCAlloc issues * Fix SSR & Transparent, both pipe RenderGraph & non-RenderGraph * Add what's new * Regenerate shader includes * Fix API Test * Fix name RayTracingCommon.hlsl * Typo simplification * Update VertMesh.hlsl * Update VertMesh.hlsl * Fix parity RG & Non-RG Co-authored-by: Sebastien Lagarde <sebastien@unity3d.com>

skhiat added 14 commits July 17, 2020 08:39

Change reflection direction to match GGX

211cf56

Merge branch 'HDRP/staging' of github.com:Unity-Technologies/Graphics…

46aebf2

… into HDRP/ssr_improvement

Merge branch 'HDRP/staging' of github.com:Unity-Technologies/Graphics…

f70228c

… into HDRP/ssr_improvement # Conflicts: # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.cs

local tmp

3c30991

Temp Denoise and Inpainting

e4cf5e6

Backup remote work

c28f92d

Merge branch 'HDRP/staging' of github.com:Unity-Technologies/Graphics…

940c02f

… into HDRP/ssr_improvement # Conflicts: # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDStringConstants.cs

Merge branch 'HDRP/staging' of github.com:Unity-Technologies/Graphics…

b59b2d4

… into HDRP/ssr_improvement

Remove blur step

72933ce

Add doc

8f0c4a0

Merge branch 'HDRP/staging' of github.com:Unity-Technologies/Graphics…

d505f55

… into HDRP/ssr_improvement # Conflicts: # com.unity.render-pipelines.high-definition/Runtime/Lighting/ScreenSpaceLighting/ScreenSpaceReflections.compute

ChangeLog + small fix

2b39f2f

Speed dependend convergence

fd0b761

Clean code

8d1f573

skhiat added SRP HDRP Docs labels Sep 17, 2020

skhiat requested review from JordanL8, JulienIgnace-Unity, eturquin and sebastienlagarde September 17, 2020 16:39

skhiat assigned sebastienlagarde Sep 17, 2020

sebastienlagarde requested a review from a team September 17, 2020 17:01

skhiat added 5 commits September 21, 2020 22:24

Fix multicamera + SSR

ffa19bf

Cleanup

4203364

Saturate local & target velocity

9629f42

Merge branch 'HDRP/staging' of github.com:Unity-Technologies/Graphics…

a86298a

… into HDRP/ssr_improvement # Conflicts: # com.unity.render-pipelines.high-definition/CHANGELOG.md # com.unity.render-pipelines.high-definition/Runtime/Debug/DebugDisplay.cs.hlsl

Merge branch 'HDRP/staging' of github.com:Unity-Technologies/Graphics…

800dfb6

… into HDRP/ssr_improvement

anisunity self-requested a review September 25, 2020 12:18

anisunity reviewed Sep 25, 2020

View reviewed changes

skhiat added 8 commits October 20, 2020 17:58

Add what's new

08a24b6

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

c5b8c34

…HDRP/ssr_improvement # Conflicts: # com.unity.render-pipelines.high-definition/CHANGELOG.md

Regenerate shader includes

dd67a28

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

2f36d8a

…HDRP/ssr_improvement

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

b397c7b

…HDRP/ssr_improvement

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

88cab4e

…HDRP/ssr_improvement

Fix API Test

c2ae5d5

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

16280be

…HDRP/ssr_improvement

skhiat marked this pull request as ready for review October 23, 2020 13:20

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

9636bb3

…HDRP/ssr_improvement # Conflicts: # com.unity.render-pipelines.high-definition/CHANGELOG.md

sebastienlagarde reviewed Oct 24, 2020

View reviewed changes

sebastienlagarde and others added 10 commits October 24, 2020 13:22

Fix name RayTracingCommon.hlsl

87f8666

Merge branch 'master' into HDRP/ssr_improvement

39657f1

Typo simplification

c22c128

Merge branch 'HDRP/ssr_improvement' of github.com:Unity-Technologies/…

2341c15

…Graphics into HDRP/ssr_improvement

Merge branch 'master' into HDRP/ssr_improvement

ea6753d

Update VertMesh.hlsl

5be5011

Update VertMesh.hlsl

a6d20a8

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

46c36ad

…HDRP/ssr_improvement

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

d569092

…HDRP/ssr_improvement

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

2b71318

…HDRP/ssr_improvement

sebastienlagarde reviewed Oct 25, 2020

View reviewed changes

skhiat added 2 commits October 25, 2020 22:55

Fix parity RG & Non-RG

5b913e6

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

99ed42d

…HDRP/ssr_improvement

sebastienlagarde reviewed Oct 25, 2020

View reviewed changes

sebastienlagarde merged commit 80042e3 into master Oct 25, 2020

sebastienlagarde deleted the HDRP/ssr_improvement branch October 25, 2020 23:24

		@@ -1,3 +1,6 @@
		#include "Packages/com.unity.render-pipelines.high-definition/Runtime/Debug/DebugDisplay.cs.hlsl"

SSR Improvement #1928

SSR Improvement #1928

Uh oh!

Conversation

skhiat commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SSR Improvement

Checklist for PR maker

Purpose of this PR

Testing status

Comments to reviewers

Uh oh!

anisunity Sep 25, 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!

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 Oct 25, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

skhiat commented Sep 17, 2020 •

edited

Loading

anisunity Sep 25, 2020 •

edited

Loading

sebastienlagarde Oct 25, 2020 •

edited

Loading