From 32c67d7276e6366447716179ab5e0eafe84b8fa3 Mon Sep 17 00:00:00 2001 From: Paul Demeulenaere Date: Thu, 23 Sep 2021 13:52:44 +0200 Subject: [PATCH 1/3] Fix inconsistent NeedsPositionWorldInterpolator Use common function to factorize the requirement test from SG Regression introduced by https://github.com/Unity-Technologies/Graphics/pull/5428 --- .../ShaderGraph/VFXShaderGraphParticleOutput.cs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/com.unity.visualeffectgraph/Editor/ShaderGraph/VFXShaderGraphParticleOutput.cs b/com.unity.visualeffectgraph/Editor/ShaderGraph/VFXShaderGraphParticleOutput.cs index b358cbe0edb..86d3e488578 100644 --- a/com.unity.visualeffectgraph/Editor/ShaderGraph/VFXShaderGraphParticleOutput.cs +++ b/com.unity.visualeffectgraph/Editor/ShaderGraph/VFXShaderGraphParticleOutput.cs @@ -617,9 +617,7 @@ public override IEnumerable additionalDefines if (readsTangent || hasNormalPort) // needs tangent yield return $"SHADERGRAPH_NEEDS_TANGENT_{kvPass.Key.ToUpper(CultureInfo.InvariantCulture)}"; - needsPosWS |= graphCode.requirements.requiresPosition != NeededCoordinateSpace.None || - graphCode.requirements.requiresScreenPosition || - graphCode.requirements.requiresViewDir != NeededCoordinateSpace.None; + needsPosWS |= NeedsPositionWorldInterpolator(graphCode); } // TODO Put that per pass ? @@ -748,6 +746,15 @@ public override void EndCompilation() graphCodes.Clear(); } + private static bool NeedsPositionWorldInterpolator(GraphCode graphCode) + { + return graphCode.requirements.requiresPosition != NeededCoordinateSpace.None + || graphCode.requirements.requiresViewDir != NeededCoordinateSpace.None + || graphCode.requirements.requiresScreenPosition + || graphCode.requirements.requiresNDCPosition + || graphCode.requirements.requiresPixelPosition; + } + public override IEnumerable> additionalReplacements { get @@ -820,9 +827,7 @@ public override IEnumerable> additionalRep callSG.builder.AppendLine("INSG.TangentSpaceBiTangent = float3(0.0f, 1.0f, 0.0f);"); } - if (graphCode.requirements.requiresPosition != NeededCoordinateSpace.None || - graphCode.requirements.requiresViewDir != NeededCoordinateSpace.None || - graphCode.requirements.requiresScreenPosition || graphCode.requirements.requiresNDCPosition || graphCode.requirements.requiresPixelPosition) + if (NeedsPositionWorldInterpolator(graphCode)) { callSG.builder.AppendLine("float3 posRelativeWS = VFXGetPositionRWS(i.VFX_VARYING_POSWS);"); callSG.builder.AppendLine("float3 posAbsoluteWS = VFXGetPositionAWS(i.VFX_VARYING_POSWS);"); From c78ff305c8caf4800881c3a318a6b9e7607fc7f4 Mon Sep 17 00:00:00 2001 From: Paul Demeulenaere Date: Thu, 23 Sep 2021 19:16:59 +0200 Subject: [PATCH 2/3] Better fix & Better performance Isolate screen computation from the world dependancy See https://github.com/Unity-Technologies/Graphics/pull/5784#discussion_r714894355 /!\ Remove the VFXTransformPositionWorldToClip evaluated in fragment /!\ GraphicTest are :green_circle: locally --- .../VFXShaderGraphParticleOutput.cs | 38 ++++++++++--------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/com.unity.visualeffectgraph/Editor/ShaderGraph/VFXShaderGraphParticleOutput.cs b/com.unity.visualeffectgraph/Editor/ShaderGraph/VFXShaderGraphParticleOutput.cs index 86d3e488578..acb1499dd8d 100644 --- a/com.unity.visualeffectgraph/Editor/ShaderGraph/VFXShaderGraphParticleOutput.cs +++ b/com.unity.visualeffectgraph/Editor/ShaderGraph/VFXShaderGraphParticleOutput.cs @@ -749,10 +749,7 @@ public override void EndCompilation() private static bool NeedsPositionWorldInterpolator(GraphCode graphCode) { return graphCode.requirements.requiresPosition != NeededCoordinateSpace.None - || graphCode.requirements.requiresViewDir != NeededCoordinateSpace.None - || graphCode.requirements.requiresScreenPosition - || graphCode.requirements.requiresNDCPosition - || graphCode.requirements.requiresPixelPosition; + || graphCode.requirements.requiresViewDir != NeededCoordinateSpace.None; } public override IEnumerable> additionalReplacements @@ -857,20 +854,6 @@ public override IEnumerable> additionalRep callSG.builder.AppendLine("INSG.AbsoluteWorldSpacePositionPredisplacement = posAbsoluteWS;"); } - callSG.builder.AppendLine("{"); - if (graphCode.requirements.requiresNDCPosition || graphCode.requirements.requiresPixelPosition) - callSG.builder.AppendLine("float2 localPixelPosition = i.VFX_VARYING_POSCS.xy;"); - if (graphCode.requirements.requiresNDCPosition) - callSG.builder.AppendLine("float2 localNDCPosition = localPixelPosition.xy / _ScreenParams.w;"); - - if (graphCode.requirements.requiresScreenPosition) - callSG.builder.AppendLine("INSG.ScreenPosition = ComputeScreenPos(VFXTransformPositionWorldToClip(i.VFX_VARYING_POSWS), _ProjectionParams.x);"); - if (graphCode.requirements.requiresNDCPosition) - callSG.builder.AppendLine("INSG.NDCPosition = localPixelPosition / _ScreenParams.xy;"); - if (graphCode.requirements.requiresPixelPosition) - callSG.builder.AppendLine("INSG.PixelPosition = localPixelPosition;"); - callSG.builder.AppendLine("}"); - if (graphCode.requirements.requiresViewDir != NeededCoordinateSpace.None) { callSG.builder.AppendLine("float3 V = GetWorldSpaceNormalizeViewDir(VFXGetPositionRWS(i.VFX_VARYING_POSWS));"); @@ -885,6 +868,25 @@ public override IEnumerable> additionalRep } } + if (graphCode.requirements.requiresScreenPosition + || graphCode.requirements.requiresNDCPosition + || graphCode.requirements.requiresPixelPosition) + { + callSG.builder.AppendLine("{"); + if (graphCode.requirements.requiresNDCPosition || graphCode.requirements.requiresPixelPosition) + callSG.builder.AppendLine("float2 localPixelPosition = i.VFX_VARYING_POSCS.xy;"); + if (graphCode.requirements.requiresNDCPosition) + callSG.builder.AppendLine("float2 localNDCPosition = localPixelPosition.xy / _ScreenParams.w;"); + + if (graphCode.requirements.requiresScreenPosition) + callSG.builder.AppendLine("INSG.ScreenPosition = ComputeScreenPos(localPixelPosition, _ProjectionParams.x);"); + if (graphCode.requirements.requiresNDCPosition) + callSG.builder.AppendLine("INSG.NDCPosition = localPixelPosition / _ScreenParams.xy;"); + if (graphCode.requirements.requiresPixelPosition) + callSG.builder.AppendLine("INSG.PixelPosition = localPixelPosition;"); + callSG.builder.AppendLine("}"); + } + if (graphCode.requirements.requiresMeshUVs.Contains(UVChannel.UV0)) { callSG.builder.AppendLine("INSG.uv0.xy = i.uv;"); From 1db0279e862a495577f27d9a9f7e79bdedc61048 Mon Sep 17 00:00:00 2001 From: Paul Demeulenaere Date: Fri, 24 Sep 2021 14:10:59 +0200 Subject: [PATCH 3/3] Fix incorrect computation of INSG.ScreenPosition If we want to be consistent with SG implementation, we have to reproduced this code: https://github.com/Unity-Technologies/Graphics/blob/75f4b421fa774d43000d703f2bbf05d7a1ca6606/com.unity.render-pipelines.universal/Editor/ShaderGraph/Templates/SharedCode.template.hlsl#L56 ``` $SurfaceDescriptionInputs.ScreenPosition: output.ScreenPosition = ComputeScreenPos(TransformWorldToHClip(input.positionWS), _ProjectionParams.x); ``` Checked manually the values were consistent with VFX --- .../VFXShaderGraphParticleOutput.cs | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/com.unity.visualeffectgraph/Editor/ShaderGraph/VFXShaderGraphParticleOutput.cs b/com.unity.visualeffectgraph/Editor/ShaderGraph/VFXShaderGraphParticleOutput.cs index acb1499dd8d..6667855fe8d 100644 --- a/com.unity.visualeffectgraph/Editor/ShaderGraph/VFXShaderGraphParticleOutput.cs +++ b/com.unity.visualeffectgraph/Editor/ShaderGraph/VFXShaderGraphParticleOutput.cs @@ -749,7 +749,8 @@ public override void EndCompilation() private static bool NeedsPositionWorldInterpolator(GraphCode graphCode) { return graphCode.requirements.requiresPosition != NeededCoordinateSpace.None - || graphCode.requirements.requiresViewDir != NeededCoordinateSpace.None; + || graphCode.requirements.requiresViewDir != NeededCoordinateSpace.None + || graphCode.requirements.requiresScreenPosition; } public override IEnumerable> additionalReplacements @@ -866,24 +867,22 @@ public override IEnumerable> additionalRep if ((graphCode.requirements.requiresViewDir & NeededCoordinateSpace.Tangent) != 0) callSG.builder.AppendLine("INSG.TangentSpaceViewDirection = mul(tbn, V);"); } + + if (graphCode.requirements.requiresScreenPosition) + { + //ScreenPosition is expected to be the raw screen pos (float4) before the w division in pixel (SharedCode.template.hlsl) + callSG.builder.AppendLine("INSG.ScreenPosition = ComputeScreenPos(VFXTransformPositionWorldToClip(i.VFX_VARYING_POSWS), _ProjectionParams.x);"); + } } - if (graphCode.requirements.requiresScreenPosition - || graphCode.requirements.requiresNDCPosition - || graphCode.requirements.requiresPixelPosition) + if (graphCode.requirements.requiresNDCPosition || graphCode.requirements.requiresPixelPosition) { callSG.builder.AppendLine("{"); - if (graphCode.requirements.requiresNDCPosition || graphCode.requirements.requiresPixelPosition) - callSG.builder.AppendLine("float2 localPixelPosition = i.VFX_VARYING_POSCS.xy;"); - if (graphCode.requirements.requiresNDCPosition) - callSG.builder.AppendLine("float2 localNDCPosition = localPixelPosition.xy / _ScreenParams.w;"); - if (graphCode.requirements.requiresScreenPosition) - callSG.builder.AppendLine("INSG.ScreenPosition = ComputeScreenPos(localPixelPosition, _ProjectionParams.x);"); if (graphCode.requirements.requiresNDCPosition) - callSG.builder.AppendLine("INSG.NDCPosition = localPixelPosition / _ScreenParams.xy;"); + callSG.builder.AppendLine("INSG.NDCPosition = i.VFX_VARYING_POSCS.xy / _ScreenParams.xy;"); if (graphCode.requirements.requiresPixelPosition) - callSG.builder.AppendLine("INSG.PixelPosition = localPixelPosition;"); + callSG.builder.AppendLine("INSG.PixelPosition = i.VFX_VARYING_POSCS.xy;"); callSG.builder.AppendLine("}"); }