[Feature] Skinned motion vectors for HDRP Hybrid #3224

he-ax · 2021-01-21T15:40:32Z

Purpose of this PR

Adding support for deformed motion vectors for High-Definition RP Hybrid. Universal RP does not currently support motion vectors, but changes relating to Compute Deformation Node has been applied there as well.

Before (TAA + Motion Blur)

After (TAA + Motion Blur)

Changes

Compute Deformation Node has been removed

Skinned meshes will now use compute skinning path as default when Hybrid Renderer is used and ENABLE_COMPUTE_DEFORMATION define is set in the project.
This means meshes will automatically get the Vertex ID property, and that default HDRP Shaders (like Lit) now supports skinning without the need to add a custom node.

Linear Blend Skinning Node does not support skinned motion vectors.

These changes are dependent on following changes for Hybrid Renderer in DOTS repo: https://github.com/Unity-Technologies/dots/pull/7101

Testing status

Tested with HDRP Example project in DOTS Animation samples.
Tested with HybrdDHRPSamples project.
Tested with HybridURPSamples project.

Notes for testing:

Everything should compile with/without ENABLE_COMPUTE_DEFORMATION define.
Graphics packages should compile without linked Hybrid Renderer changes (even with Hybrid Renderer package included), but Compute Deformations will not work.
Verify that adding VertexID by default when DOTS_INSTANCING define is enabled does not break Shader Graph functionality.

Comments to reviewers

This adds a variable to global HDRP cbuffer.
https://github.com/Unity-Technologies/Graphics/pull/3224/files#diff-b237e8d61dae81c57d2ccfaee3b722f7d75a872b97368e3ba79ecfb6f79182e4
It would be preferable if this variable could be in the same place for both Universal and HDRP, any suggestions on where that would be is welcome.

- Add separate shader for skinning - Set DOTS_SKINNING define for all passes when hybrid v2 is enabled - Add new builtin property that determines if skinning should be applied

…s frame.

…ing the object itself.

…ctors

com.unity.render-pipelines.high-definition/Editor/Material/ShaderGraph/HDTarget.cs

com.unity.shadergraph/Editor/Generation/Descriptors/PragmaDescriptor.cs

com.unity.render-pipelines.high-definition/Editor/Material/ShaderGraph/HDShaderPasses.cs

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

he-ax · 2021-01-27T14:34:49Z

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

 #define PackVaryingsType PackVaryingsToPS
 #endif

+#include "Packages/com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/ShaderPass/DotsDeformation.hlsl"


surround with dots instancing on define?

…ctors

… vertices from compute buffer in ShaderVariablesFunctions

…ctors

…ward/GBuffer, Depth/DepthNormal, ShadowCaster, Unlit, PostProcessing, ScreenSpaceShadows, SimpleLit-Forward/GBiffer, PreviewPass

TomasKiniulis

Tested with and without dots repo PR.

Same review as here: https://github.com/Unity-Technologies/dots/pull/7101#pullrequestreview-589699280
Please take a look at the few issues mentioned in test doc: https://confluence.unity3d.com/display/HDRP/%5BHybrid%5D+Skinned+motion+vectors+for+HDRP+Hybrid

sebastienlagarde · 2021-02-12T18:13:32Z

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

                m_ShaderVariablesGlobalCB._SpecularOcclusionBlend = 1.0f;
            }
+
+            m_ShaderVariablesGlobalCB._HybridDeformedVertexStreamIndex = UnityEngine.Time.frameCount & 1;


I don't think we want to use
UnityEngine.Time.frameCount here. we have get so many issue with it (#3173)

We have a FrameCount per Camera. So I guess here we should use camera.GetCameraFrameCount();
cc @adrien-de-tocqueville / @JulienIgnace-Unity for confirmation.

Indeed, UnityEngine.Time.frameCount is not always incremented in the editor

sebastienlagarde · 2021-02-12T18:14:50Z

...nity.render-pipelines.high-definition/Runtime/RenderPipeline/ShaderPass/DotsDeformation.hlsl

+uniform StructuredBuffer<DeformedVertexData> _PreviousFrameDeformedMeshData;
+
+// Reads vertex data for compute skinned meshes in Hybdrid Renderer
+void FetchComputeVertexData(inout AttributesMesh input)


isn't this almost a duplication of this: https://github.com/Unity-Technologies/Graphics/pull/3224/files#diff-a6dc0267c305f8c7397a906b7ccdb070d168bd45350cac9b7b75ceaef24f2e44R14 ? Can't we share more code there?

same below FetchComputeVertexPosition. Or maybe the question is more, why do we have code in Core package if it need to be rewrite per pipeline anyway?

Ah now I read more code I udnerstand that the Core version is the URP version. Core shouldn't be use as URP code, Core is also use for custom SRP. So the code specific to pipeline must in in pipeline.

sebastienlagarde · 2021-02-12T18:21:31Z

...nity.render-pipelines.high-definition/Runtime/RenderPipeline/ShaderPass/DotsDeformation.hlsl

+        const int prevMeshStart = deformProperty[prevStreamIndex];
+
+        if(prevMeshStart == -1)
+            prevPos = _DeformedMeshData[prevMeshStart + vertexID].Position;


Suggetsion / Remark. It could be more efficient to have a single StructuredBuffer with both the previous and current position and dealing with and offset instead of branching on buffer read like this which waste VGPR.

i.e

StructuredBuffer _FrameDeformedMeshData

int offset = (prevMeshStart == -1) ? 0 : nextStart;
offset += vertexID;
prevPos = _FrameDeformedMeshData[offset].Position;

=> no branch, less sgpr/vgpr use to store the two uniform structureBuffer.

thought?

hmm currently I am confuse by the code, it looks like you alrady do that. with prevMeshStart and currMeshStart , so why is there a different _PreviousFrameDeformedMeshData structure buffer?

sorry if my question is stupid I don't know the underlaying of DOTS skinning

sebastienlagarde · 2021-02-12T18:29:53Z

...nity.render-pipelines.high-definition/Runtime/RenderPipeline/ShaderPass/DotsDeformation.hlsl

+        currPos = _DeformedMeshData[currMeshStart + vertexID].Position;
+    }
+
+    const int skinMotionVec = deformProperty.w;


shouldn't deformProperty.w; be dependent on deformProperty.z; ?

i.e we can have skinMotionVector only if computeSkin is true. Right now the code allow to have skin motion vector without skin which seems weird.

sebastienlagarde · 2021-02-12T18:33:41Z

...elines.high-definition/Runtime/RenderPipeline/ShaderPass/MotionVectorVertexShaderCommon.hlsl

+        float3 deformedPrevPos = inputPass.previousPositionOS;
+
+#if defined(DOTS_INSTANCING_ON)
+        FetchComputeVertexPosition(inputMesh.positionOS, deformedPrevPos, inputMesh.vertexID);


Question: is dots skinning cumulative with regular skinning?

inputPass.previousPositionOS contain the previous skinned mesh if any otherwise it is empty, so I am curious.

Also not all DOTS instancing object have skin right? And in this case there is no need to call FetchComputeVertexPosition?

I have the feeling that we should rather have a code like this:

#if defined(DOTS_INSTANCING_ON)
bool hasDeformation = asint(unity_DOTSDeformationParams).z; // DOTS skinning
if (hasDeformation)
FetchComputeVertexPosition(inputMesh.positionOS, inputPass.previousPositionOS, inputMesh.vertexID);
#else
bool hasDeformation = unity_MotionVectorsParams.x > 0.0; // Skin or morph target
#endif

float3 effectivePositionOS = (hasDeformation ? inputPass.previousPositionOS: inputMesh.positionOS);

Thought?

sebastienlagarde · 2021-02-12T18:35:46Z

...nity.render-pipelines.high-definition/Runtime/RenderPipeline/ShaderPass/DotsDeformation.hlsl

+
+// Reads vertex position for compute skinned meshes in Hybdrid Renderer
+// and also previous frame position if skinned motion vectors are used
+void FetchComputeVertexPosition(inout float3 currPos, inout float3 prevPos, uint vertexID)


Why inout float3 currPos, inout float3 prevPos are currently inout? they should be out only no ?

sebastienlagarde · 2021-02-12T18:44:54Z

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

 #endif
-
+#ifdef DOTS_INSTANCING_ON
+    uint vertexID        : SV_VertexID;


Note: be sure this compile on DX12 / PS4 / Xbox. We have get various surprise with system sematic ordering on those platform (with double sided, instance id etc...)

also we need to use VERTEXID_SEMANTIC and not SV_VertexID

Question: Guess for now Vertex_ID is only used with skinned mesh right?
so it should not be define when DOTS_INSTANCING_ON is on, but when DOTS_SKINNING_INSTANCING_ON ? (which doesnt exist but you get the idea). Just wondering :)

ok reading more code it seems that we can only know dynamically if we use skinning or not, so fine.

sebastienlagarde

Overall the PR seems good, I have just various question and suggestion but it could be related to my non knowledge of dots intancing code.

…enerated unique GUID. Fixes GUID conflict with the HDRP version of DotsDeformation.hlsl.meta

sebastienlagarde · 2021-06-16T17:46:53Z

Hey, is there any new about this PR, is it going to be merge?

sebastienlagarde · 2022-05-30T12:44:24Z

@hedvigaxelsson guess this PR have landed in private repo? can you close this one then? thanks

hedvig and others added 10 commits January 18, 2021 17:23

Compute Skinning is applied in VertMesh through defines

f5d9db1

- Add separate shader for skinning - Set DOTS_SKINNING define for all passes when hybrid v2 is enabled - Add new builtin property that determines if skinning should be applied

Add front buffer index to access deformed vertices of current/previou…

58cbddd

…s frame.

Actually get the the vertex position of the current frame when render…

48b1ecf

…ing the object itself.

d3d debug pragma for HDTarget

a60bccc

Remove skin setting component

b6a4805

Apply skinning in motion vec pass, rename function

b7dd69d

Add motion vectors to unity instancing properties

81d0788

Remove compute deformation node

0d6a8dd

Merge branch '10.x.x/release' into 10.x.x/dots-deformations/motion-ve…

860362d

…ctors

Update changelog for HRDP and SG

4e921d8

github-actions bot added HDRP Shader Graph labels Jan 21, 2021

he-ax commented Jan 21, 2021

View reviewed changes

com.unity.render-pipelines.high-definition/Editor/Material/ShaderGraph/HDTarget.cs Outdated Show resolved Hide resolved

he-ax commented Jan 21, 2021

View reviewed changes

com.unity.render-pipelines.high-definition/Editor/Material/ShaderGraph/HDTarget.cs Outdated Show resolved Hide resolved

he-ax commented Jan 21, 2021

View reviewed changes

com.unity.shadergraph/Editor/Generation/Descriptors/PragmaDescriptor.cs Outdated Show resolved Hide resolved

he-ax commented Jan 21, 2021

View reviewed changes

com.unity.render-pipelines.high-definition/Editor/Material/ShaderGraph/HDShaderPasses.cs Outdated Show resolved Hide resolved

he-ax commented Jan 22, 2021

View reviewed changes

com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/ShaderPass/Skinning.hlsl Outdated Show resolved Hide resolved

hedvig added 8 commits January 27, 2021 11:05

Enable vertexID when DOTS_INSTANCING_ON is defined

82ffcd8

Remove manual adding of vertex id and defines in HD shader passes

028e908

Remove skinning include and define

80c2dcd

Remove compute mesh index property when dots instancing is not defined

19846d9

Shader cleanup for dots deformations

8ee28d1

Rename shader file for dots skinning functions

0145e4b

Remove changes from HDShaderPasses

cf88c73

Remove formatting issues

ce95d4c

he-ax commented Jan 27, 2021

View reviewed changes

he-ax changed the title ~~[Feature] Skinned motion vectors for HDRP Hybrid DRAFT~~ [Feature] Skinned motion vectors for HDRP Hybrid Jan 27, 2021

hedvig added 3 commits January 27, 2021 16:22

Remove include define

fe9468e

Merge branch '10.x.x/release' into 10.x.x/dots-deformations/motion-ve…

80199e9

…ctors

Remove all changes from HDShaderPasses

34db42a

hedvig added 13 commits January 27, 2021 17:00

Remove duplicated define

2825bd2

Add compute mesh index to URP shader variables. Add function to fetch…

69e7eae

… vertices from compute buffer in ShaderVariablesFunctions

Rename some functions, move fetching to different file

98d986e

Rename deformation function for HDRP

690298f

Merge branch '10.x.x/release' into 10.x.x/dots-deformations/motion-ve…

8716688

…ctors

Keep vertex ID as optional when hybrid v2 is not used

b84ffcf

Remove formatting from HDTarget

6f3cbf1

Updated changelogs to be more descriptive.

aa24bcf

Rename material property

7ac5ee9

Add dots skinning function to URP passes PBR-Forward/GBuffer, Lit-For…

3c2a52c

…ward/GBuffer, Depth/DepthNormal, ShadowCaster, Unlit, PostProcessing, ScreenSpaceShadows, SimpleLit-Forward/GBiffer, PreviewPass

Remove modulus from deform vertex function

808bf70

Remove modulus, change to uints

a3c3a8e

Remove documentation for Compute Deformation Node

b70a82a

nigeljw-unity self-requested a review February 12, 2021 10:23

cinight self-requested a review February 12, 2021 10:24

TomasKiniulis self-requested a review February 12, 2021 10:50

sebastienlagarde self-requested a review February 12, 2021 17:43

TomasKiniulis reviewed Feb 12, 2021

View reviewed changes

sebastienlagarde reviewed Feb 12, 2021

View reviewed changes

Update Core/ShaderLibrary/DotsDeformation.hlsl.meta file with newly g…

5d3abda

…enerated unique GUID. Fixes GUID conflict with the HDRP version of DotsDeformation.hlsl.meta

he-ax closed this May 30, 2022

[Feature] Skinned motion vectors for HDRP Hybrid #3224

[Feature] Skinned motion vectors for HDRP Hybrid #3224

Uh oh!

Conversation

he-ax commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of this PR

Changes

Testing status

Comments to reviewers

Uh oh!

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!

TomasKiniulis 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!

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 Feb 12, 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!

sebastienlagarde Feb 12, 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!

Choose a reason for hiding this comment

Uh oh!

sebastienlagarde left a comment

Choose a reason for hiding this comment

Uh oh!

sebastienlagarde commented Jun 16, 2021

Uh oh!

sebastienlagarde commented May 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

he-ax commented Jan 21, 2021 •

edited

Loading

sebastienlagarde Feb 12, 2021 •

edited

Loading

sebastienlagarde Feb 12, 2021 •

edited

Loading