[ShaderGraph] [bugfix 1352662] [2022.1] Use VPOS for fragment stage screenspace calculations #5428

cdxntchou · 2021-08-24T18:48:42Z

Purpose of this PR

Fix for https://fogbugz.unity3d.com/f/cases/1352662/

2022.1:

2021.2 backport:

[ShaderGraph] [backport] [2021.2] Use VPOS for fragment stage screenspace calculations #5817

The issue is that the calculation for screenspace position is calculated from world space position via applying the matrix transforms. However, the calculation is prone to floating point error, that is exacerbated greatly when the camera is far from the origin and the pipeline is not using camera-relative "world" space.

You can see in the examples below that the error can easily grow to be larger than a pixel.

This PR fixes this by changing the calculation in the fragment shader, to be based on the value returned by the VPOS fragment input. This value is not based on the projection matrices, independent of camera position, and much more accurate overall.

Docs ticket: https://jira.unity3d.com/browse/GSG-593
Console Test PR: https://github.cds.internal.unity3d.com/unity/ScriptableRenderPipelinePrivate/pull/239

Test example:
Left sphere visualizes frac(screenspace pixel coordinate) using a gradient (red, black, green) where 0.5 is black.
Right sphere shows a screenspace checkerboard alphatest pattern

Before: (at 20000 distance from origin) -- quite a bit of inaccuracy

After: (at 20000 distance from origin) -- perfect 0.5 centered pixel coordinates everywhere!

Implementation Details:
We introduce two new global values that ShaderGraph can produce, and nodes can request, in addition to ScreenPosition.
The values are defined as:
PixelPosition 2D pixel coordinate of the current fragment or vertex. Pixels are centered at 0.5.
NDCPosition 2D coordinate (0,1) across the viewport; equal to PixelPosition / ScreenResolution.
ScreenPosition (raw) 4D Clip Space homogenous coordinates of the current fragment or vertex.

Both PixelPosition and NDCPosition are calculated from VPOS in the fragment stage, and are calculated from ScreenPosition in other stages.

Testing status

Tested in both HDRP and URP:

Tested that Screen Position node and slots work correctly in both fragment and vertex shader.
Validated that "Raw" mode works identically to previous, returning clip space ZW.
Tested that the other modes return exact values in the fragment shader, based off of the VPOS value.
Tested that VFX screen position works.

Added a Yamato test to validate screen position accuracy in fragment shader. Previously you would have a good amount of error in the screen position from the floating point roundoff on the matrix transform calculations. Now it gives exact pixel coordinate values out to at least 3 fractional digits.

Updated reference images. Note that the new accuracy actually changes the test results for the dither node, because the position is slightly different (now more accurate), and the specific test we use is extremely position sensitive.

Master base branch: 35e38b1

Yamato:
ShaderGraph: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252F1352662/.yamato%252Fall-shadergraph.yml%2523ShaderGraph_2021.2/8408772/job/pipeline
master:
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/master/.yamato%252Fall-shadergraph.yml%2523ShaderGraph_2021.2/8364150/job/pipeline

HDRP: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252F1352662/.yamato%252Fall-hdrp.yml%2523PR_HDRP_trunk/8681927/job/pipeline

URP: 🟡 -- failures match master
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252F1352662/.yamato%252Fall-urp.yml%2523PR_URP_trunk/8702585/job/pipeline
master:
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/master/.yamato%252Fall-urp.yml%2523PR_URP_trunk/8678639/job/pipeline

Failing jobs:
Build URP_Terrain on iPhone_Metal_il2cpp_Linear_Standalone_build_Player on version trunk -- both failed non-test-related 🟡
Build URP_Terrain on Android_Vulkan_il2cpp_Linear_Standalone_build_Player on version trunk -- both failed non-test-related 🟡
Build URP_Terrain on Android_OpenGLES3_il2cpp_Linear_Standalone_build_Player on version trunk -- both failed non-test-related 🟡
Build URP_Terrain on Linux_Vulkan_mono_Linear_Standalone_build_Player on version trunk -- both failed non-test-related 🟡
Build URP_Terrain on Win_Vulkan_mono_Linear_Standalone_build_Player on version trunk -- both failed non-test-related 🟡
Build URP_Terrain on Win_DX12_mono_Linear_Standalone_XR_build_Player on version trunk -- both failed non-test-related 🟡
Build URP_Terrain on Win_DX12_mono_Linear_Standalone_build_Player on version trunk -- both failed non-test-related 🟡
Build URP_Terrain on Win_DX11_mono_Linear_Standalone_XR_build_Player on version trunk -- both failed non-test-related 🟡
Build URP_Terrain on Win_DX11_mono_Linear_Standalone_build_Player on version trunk -- both failed non-test-related 🟡
URP_Foundation on iPhone_Metal_Standalone_il2cpp_Linear on version trunk -- both failed same 4 tests 🟡
URP_Lighting on iPhone_Metal_Standalone_il2cpp_Linear on version trunk -- both failed same 1 test 🟡
URP_Lighting on Android_Vulkan_Standalone_il2cpp_Linear on version trunk -- both failed on re-run (nondeterministic) 🟡
URP_Terrain on OSX_Metal_playmode_mono_Linear on version trunk -- both failed same 19 tests 🟡
URP_Terrain on Win_DX11_playmode_XR_mono_Linear on version trunk -- both failed same 13 tests 🟡
URP_PostPro on Win_DX11_playmode_XR_mono_Linear on version trunk -- both failed on re-run (nondeterministic) 🟡
ShaderGraph on OSX_Metal_playmode_mono_Linear on version trunk -- success on re-run 🟢

Console Tests: 🟡 -- failures match master

Console Test PR: https://github.cds.internal.unity3d.com/unity/ScriptableRenderPipelinePrivate/pull/239

Comments to reviewers

Notes for the reviewers you have assigned.

…ations. This might be incorrect though for anyone who wants to read zw...

github-actions · 2021-08-24T18:48:57Z

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://yamato.cds.internal.unity3d.com/jobs/902-Graphics
Search for your PR branch using the sidebar on the left, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/.yamato%252Fall-hdrp.yml%2523PR_HDRP_2021.2

URP
/.yamato%252Fall-urp.yml%2523PR_URP_2021.2

Shader Graph
/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_2021.2
Depending on your PR, you may also want
/.yamato%252Fall-shadergraph_builtin_foundation.yml%2523PR_ShaderGraph_BuiltIn_Foundation_2021.2
/.yamato%252Fall-shadergraph_builtin_lighting.yml%2523PR_ShaderGraph_BuiltIn_Lighting_2021.2

VFX
/.yamato%252Fall-vfx.yml%2523PR_VFX_2021.2

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

# Conflicts: # com.unity.shadergraph/CHANGELOG.md

cdxntchou · 2021-08-27T20:37:05Z

com.unity.shadergraph/Editor/Data/Graphs/VertexColorMaterialSlot.cs

 {
    [Serializable]
-    class VertexColorMaterialSlot : Vector4MaterialSlot, IMayRequireScreenPosition
+    class VertexColorMaterialSlot : Vector4MaterialSlot, IMayRequireVertexColor


this class seems to have mistakenly implemented IMayRequireScreenPosition instead of IMayRequireVertexColor, which would make more sense...

sebastienlagarde · 2021-09-01T10:37:02Z

Hi, is this PR targetting 21.2 ? in this case it will need a backport

com.unity.shadergraph/Editor/Generation/Processors/GenerationUtils.cs

com.unity.shadergraph/Editor/Generation/TargetResources/FieldDependencies.cs

jessebarker

LGTM

RemyUnity

Looking at the description and what has been tested, it seems ok to me.
Thanks for the detailed explanations, screenshots and the list of tests.

# Conflicts: # com.unity.shadergraph/CHANGELOG.md

# Conflicts: # com.unity.shadergraph/CHANGELOG.md # com.unity.shadergraph/Editor/Data/Util/ScreenSpaceType.cs

ernestasKupciunas

Approving without testing. Developer did a great job by testing these changes. New tests added. Good and clear description. Good job!

PaulDemeulenaere · 2021-09-23T11:19:41Z

This changes leads to a new compilation failure in VFX_URP test:
Unhandled log message: '[Error] Shader error in 'Hidden/VFX/SampleScene/System (1)/Output Particle Quad': 'VFXGetPositionRWS': no matching 1 parameter function at line 1647 (on d3d11)
Before merge
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/master/.yamato%252Fvfx_urp-win-dx11.yml%2523VFX_URP_Win_DX11_playmode_mono_Linear_trunk/8938317/job
After merge
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/master/.yamato%252Fvfx_urp-win-dx11.yml%2523VFX_URP_Win_DX11_playmode_mono_Linear_trunk/8938121/job
I'm on it ⏳

Use common function to factorize the requirement test from SG Regression introduced by #5428

* Fix inconsistent NeedsPositionWorldInterpolator Use common function to factorize the requirement test from SG Regression introduced by #5428 * Better fix & Better performance Isolate screen computation from the world dependancy See #5784 (comment) /!\ Remove the VFXTransformPositionWorldToClip evaluated in fragment /!\ GraphicTest are 🟢 locally * 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

PaulDemeulenaere · 2021-09-24T19:06:58Z

⚠️ This change 11417d3 must be backported right after or preferably within the same backport PR. See #5784 ⚠️

…creenspace calculations (#5428) * Using SV_Position to calculate screenspace, instead of projection equations. This might be incorrect though for anyone who wants to read zw... * Working URP ScreenPositions * Adding changelog * Adding "ScreenPosition_Accuracy" to the input nodes tests * Fixes for HDRP target * Updating reference images * Fixing vertex color slot -- was using wrong interface * Fix for HDRP * Fix VFX ScreenPos usage * Convert VFX to use VPOS in fragment shader # Conflicts: # com.unity.shadergraph/CHANGELOG.md # com.unity.shadergraph/Editor/Data/Util/ScreenSpaceType.cs

* Fix inconsistent NeedsPositionWorldInterpolator Use common function to factorize the requirement test from SG Regression introduced by #5428 * Better fix & Better performance Isolate screen computation from the world dependancy See #5784 (comment) /!\ Remove the VFXTransformPositionWorldToClip evaluated in fragment /!\ GraphicTest are 🟢 locally * 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

Chris Tchou added 2 commits August 23, 2021 12:43

Using SV_Position to calculate screenspace, instead of projection equ…

b806dea

…ations. This might be incorrect though for anyone who wants to read zw...

Working URP ScreenPositions

d5afbce

github-actions bot added HDRP Shader Graph universal vfx labels Aug 24, 2021

Adding changelog

bcc6205

cdxntchou requested a review from sharlenet August 25, 2021 18:46

Chris Tchou added 5 commits August 25, 2021 14:36

Adding "ScreenPosition_Accuracy" to the input nodes tests

5ca51d2

Fixes for HDRP target

a80746b

Updating reference images

2c6733c

Fixing vertex color slot -- was using wrong interface

627a857

Merge branch 'master' into sg/fix/1352662

5118e38

# Conflicts: # com.unity.shadergraph/CHANGELOG.md

cdxntchou commented Aug 27, 2021

View reviewed changes

cdxntchou removed the request for review from sharlenet August 30, 2021 16:48

cdxntchou added the needs-backport-2021.2 label Sep 1, 2021

Chris Tchou added 3 commits September 1, 2021 18:26

Fix for HDRP

d9306d4

Fix VFX ScreenPos usage

d74237b

Convert VFX to use VPOS in fragment shader

ff722b9

cdxntchou requested review from a team September 2, 2021 14:59

jessebarker reviewed Sep 2, 2021

View reviewed changes

com.unity.shadergraph/Editor/Generation/Processors/GenerationUtils.cs Show resolved Hide resolved

jessebarker reviewed Sep 2, 2021

View reviewed changes

com.unity.shadergraph/Editor/Generation/TargetResources/FieldDependencies.cs Show resolved Hide resolved

jessebarker approved these changes Sep 2, 2021

View reviewed changes

RemyUnity approved these changes Sep 3, 2021

View reviewed changes

julienf-unity approved these changes Sep 7, 2021

View reviewed changes

cdxntchou requested review from a team and pbbastian September 10, 2021 00:41

Merge branch 'master' into sg/fix/1352662

8bf3e2c

# Conflicts: # com.unity.shadergraph/CHANGELOG.md

cdxntchou requested a review from ellioman September 15, 2021 18:51

cdxntchou marked this pull request as ready for review September 15, 2021 21:52

cdxntchou requested a review from a team as a code owner September 15, 2021 21:52

ellioman approved these changes Sep 16, 2021

View reviewed changes

cdxntchou removed the request for review from pbbastian September 16, 2021 16:30

Merge branch 'master' into sg/fix/1352662

e4a532f

# Conflicts: # com.unity.shadergraph/CHANGELOG.md # com.unity.shadergraph/Editor/Data/Util/ScreenSpaceType.cs

ernestasKupciunas approved these changes Sep 21, 2021

View reviewed changes

Merge branch 'master' into sg/fix/1352662

83e6c53

jessebarker merged commit 75f4b42 into master Sep 21, 2021

jessebarker deleted the sg/fix/1352662 branch September 21, 2021 20:03

PaulDemeulenaere added a commit that referenced this pull request Sep 23, 2021

Fix inconsistent NeedsPositionWorldInterpolator

32c67d7

Use common function to factorize the requirement test from SG Regression introduced by #5428

PaulDemeulenaere mentioned this pull request Sep 23, 2021

[VFX] Fix inconsistent NeedsPositionWorldInterpolator #5784

Merged

cdxntchou mentioned this pull request Sep 27, 2021

[ShaderGraph] [backport] [2021.2] Use VPOS for fragment stage screenspace calculations #5817

Closed

cdxntchou mentioned this pull request Sep 30, 2021

[ShaderGraph] [2022.1] [Ruby] Fix regression in screen position when using XR #5867

Merged

cdxntchou mentioned this pull request Oct 7, 2021

Universal Staging #5943

Merged

sebastienlagarde removed the needs-backport-2021.2 label Dec 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ShaderGraph] [bugfix 1352662] [2022.1] Use VPOS for fragment stage screenspace calculations #5428

[ShaderGraph] [bugfix 1352662] [2022.1] Use VPOS for fragment stage screenspace calculations #5428

Uh oh!

cdxntchou commented Aug 24, 2021 •

edited

Loading

Uh oh!

github-actions bot commented Aug 24, 2021

Uh oh!

cdxntchou Aug 27, 2021

Uh oh!

sebastienlagarde commented Sep 1, 2021

Uh oh!

Uh oh!

Uh oh!

jessebarker left a comment

Uh oh!

RemyUnity left a comment

Uh oh!

ernestasKupciunas left a comment

Uh oh!

PaulDemeulenaere commented Sep 23, 2021

Uh oh!

PaulDemeulenaere commented Sep 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

[ShaderGraph] [bugfix 1352662] [2022.1] Use VPOS for fragment stage screenspace calculations #5428

[ShaderGraph] [bugfix 1352662] [2022.1] Use VPOS for fragment stage screenspace calculations #5428

Uh oh!

Conversation

cdxntchou commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of this PR

Testing status

Comments to reviewers

Uh oh!

github-actions bot commented Aug 24, 2021

Uh oh!

cdxntchou Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

sebastienlagarde commented Sep 1, 2021

Uh oh!

Uh oh!

Uh oh!

jessebarker left a comment

Choose a reason for hiding this comment

Uh oh!

RemyUnity left a comment

Choose a reason for hiding this comment

Uh oh!

ernestasKupciunas left a comment

Choose a reason for hiding this comment

Uh oh!

PaulDemeulenaere commented Sep 23, 2021

Uh oh!

PaulDemeulenaere commented Sep 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

cdxntchou commented Aug 24, 2021 •

edited

Loading