[ShaderGraph] [2022.1] [Ruby] Fix screen position behaviors #6040

cdxntchou · 2021-10-15T21:28:59Z

Purpose of this PR

Proper Fix for: https://fogbugz.unity3d.com/f/cases/1369450/

MUST LAND AFTER (and remove changes from merged):

Fixed SetPerCameraShaderVariables function not working as expected. #6114

Original fix PR here (that just unblocked Universal Tests, but didn't properly fix the whole issue): #5867

This addresses a number of additional platforms and render pipelines, adds a standard for pixel position (origin in upper left), and also adds some tests.

This PR ensures that "Default" Screen Position and Pixel Position work identically across:

Render Pipelines
Graphics APIs
Shader Stages
XR / non-XR
projection flipped / normal

Specific changes:

Cannot use $conditional: and #if preprocessor commands on the same lines, as they interfere.
Moved all of the flip calculations to Pixel Position, so that pixel position is now consistent across all platforms and projections.
Standardized Pixel Position to place the origin in the upper left
Copied all of the relevant code to HDRP and Built-in platforms.
Added a node test verifying that Screen Position matches the expected behavior (relative to raw Screen Position / clip space)
Added a node test verifying that Pixel Position matches the expected behavior (relative to raw Screen Position / clip space)
Updated Dither node tests in the Artistic Nodes scene to be more deterministic
Cleaned up XR special case (using fixes from Fixed SetPerCameraShaderVariables function not working as expected. #6114)
Fixed VFX generated code to use the same logic

Console Reference Image PR:
https://github.cds.internal.unity3d.com/unity/ScriptableRenderPipelinePrivate/pull/306

Testing status

Added automatic editor tests to verify that screen position and pixel position produce the expected result relative to raw clip position, in both the pixel and vertex shaders.

Manually tested that ScreenPosition and PixelPosition always have consistent origins in both vertex and fragment stages, and the sample depth and sample scene nodes operate as expected, and that the _ScreenParams return the correct value.

URP - PC with D3D11, Vulkan and OpenGL
URP XR - PC with D3D11, OpenGL
HDRP - PC with D3D11, Vulkan and D3D12
HDRP XR - PC with D3D11, Vulkan, and D3D12
Builtin - PC with D3D11, Vulkan and OpenGL
VFX in HDRP with D3D12
URP renderscale -- this PR does not fix the handling of render scale in URP. This will be fixed in a follow up PR.

Note: The double UV flip introduced in this PR caused the dither node tests (in the Artistic Nodes scene) to change. But it is because those tests were extremely sensitive to screen position (output was heavily dependent on the least significant bits). I updated the Dither node test -- the new tests are more robust, and won't change results based on lowest bit discrepancies (or device/platform vagaries).

Yamato:

Nightly ShaderGraph: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252Fscreen-position2/.yamato%252Fall-shadergraph.yml%2523Nightly_ShaderGraph_trunk/9504802/job/pipeline

PR ShaderGraph (with latest test updates): 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252Fscreen-position2/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_trunk/9576253/job/pipeline

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

PR URP: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252Fscreen-position2/.yamato%252Fall-urp.yml%2523PR_URP_trunk/9598485/job/pipeline

Comments to reviewers

Notes for the reviewers you have assigned.

github-actions · 2021-10-15T21:29:15Z

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://unity-ci.cds.internal.unity3d.com/project/902/
Search for your PR branch using the search bar at the top, 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
/jobDefinition/.yamato%2Fall-hdrp.yml%23PR_HDRP_trunk
With changes to HDRP packages, you should also run
/jobDefinition/.yamato%2Fall-lightmapping.yml%23PR_Lightmapping_trunk

URP
/jobDefinition/.yamato%252Fall-urp.yml%2523PR_URP_trunk
With changes to URP packages, you should also run
/jobDefinition/.yamato%2Fall-lightmapping.yml%23PR_Lightmapping_trunk

Shader Graph
/jobDefinition/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_trunk
Depending on your PR, you may also want
/jobDefinition/.yamato%252Fall-shadergraph_builtin_foundation.yml%2523PR_ShaderGraph_BuiltIn_Foundation_trunk
/jobDefinition/.yamato%252Fall-shadergraph_builtin_lighting.yml%2523PR_ShaderGraph_BuiltIn_Lighting_trunk

VFX
/jobDefinition/.yamato%252Fall-vfx.yml%2523PR_VFX_trunk

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.

… threshold for error

…and reduce screen position test sensitivity by half OSX GL tests fail even at 1/4 sensitivity.. seems to have unusually large error (but still small on the scale of things)

jessebarker

LGTM

jessebarker · 2021-10-21T16:59:57Z

The main ShaderGraph PR job won't run the built-in pipeline tests. You'll need to kick those off manually (I think just the "foundation" one should do for this PR).

cdxntchou · 2021-10-21T17:51:15Z

The main ShaderGraph PR job won't run the built-in pipeline tests. You'll need to kick those off manually (I think just the "foundation" one should do for this PR).

I ran ShaderGraph nightly to make sure I got all of the reference images updated -- looks like it ran built-in foundation too?

jessebarker · 2021-10-21T18:31:34Z

The main ShaderGraph PR job won't run the built-in pipeline tests. You'll need to kick those off manually (I think just the "foundation" one should do for this PR).

I ran ShaderGraph nightly to make sure I got all of the reference images updated -- looks like it ran built-in foundation too?

Ah, cool. That didn't used to trigger automagically.

joshua-davis

Current PR looks good, but it seems like the vertex and tessellation templates also need updating

cdxntchou · 2021-10-26T21:27:47Z

com.unity.render-pipelines.universal/Runtime/ScriptableRenderer.cs


                // Initialize Camera Render State
                ClearRenderingState(cmd);
-                SetPerCameraShaderVariables(cmd, ref cameraData);


Changes to this file are from PR #6114 and will be backed out before this PR is merged.
It was merged locally to test the combined behavior, as we can simplify the logic in this PR using the fixed _ScreenParams behavior in 6114.

VitaVFX

Seems to look good on VFX side. Checked Screen Position behavior and compared VFX with regular primitives that have SG assigned. Tests included:

Vertex Stack, Fragment Stack with various particle outputs (Output Mesh, Quad, etc)
DX11 (HDRP and URP), DX12, Vulkan (URP only)
New and old (URP only) SG integrations
MockHMD (URP)
SG Scene from our VFX QA FTP project, checked if everything remains in the same places (URP)

PaulDemeulenaere

Looks good to me from the VFX side.
Thanks for the update.
N.B.: I think it could appropriate to have some helper function in core to factorize these implementation.

com.unity.visualeffectgraph/Editor/ShaderGraph/VFXShaderGraphParticleOutput.cs

PaulDemeulenaere · 2021-10-29T06:36:37Z

com.unity.render-pipelines.universal/Editor/ShaderGraph/Templates/SharedCode.template.hlsl

+    $SurfaceDescriptionInputs.PixelPosition:                            output.PixelPosition = float2(input.positionCS.x, (_ProjectionParams.x < 0) ? (_ScreenParams.y - input.positionCS.y) : input.positionCS.y);
+    #else
+    $SurfaceDescriptionInputs.PixelPosition:                            output.PixelPosition = float2(input.positionCS.x, (_ProjectionParams.x > 0) ? (_ScreenParams.y - input.positionCS.y) : input.positionCS.y);
+    #endif


Not related to VFX but following the remark from this discussion
Should the _ScreenParams.y reflect the actual render target after renderscale ?
Is it a change of behavior in that case ?

phi-lira

Looked at yamato. Seems good coverage.

JMargevics

I ran included PixelPosition graphs, dithered SGs and checked material samples on HDRP,

Tested on DX11, DX12, Vulkan, PS4
Tested all DRS upscale filters
Thanks @remi-chapelain for checking DLSS

No issues or regressions seen. Looks good to me! 👍✔

…e (target PR was already merged) This reverts commit bc3cd3f.

BogomilP · 2021-12-10T05:08:19Z

Does this, by any chance, offer a fix for this Issue:
https://issuetracker.unity3d.com/is...position-node-does-not-work-when-xr-is-active

My team has been trying to get HDRP VR Render Texture Portals to work in HDRP which both use the ScreenPosition Node.
Even after successfully implementing the changes from this commit we still cant get it to work.

Chris Tchou added 2 commits October 15, 2021 12:41

Fixing ScreenPosition and PixelPosition flips in built-in, Universal

882c6d0

Adding node test for default screen pos

83d8d97

github-actions bot added Shader Graph testing universal labels Oct 15, 2021

Chris Tchou added 8 commits October 19, 2021 13:23

Screen Position test -- adjust comparison to look correct, and reduce…

68e34ee

… threshold for error

Move pixel coordinate origin to upper left, add pixel position test, …

fe6a33e

…and reduce screen position test sensitivity by half OSX GL tests fail even at 1/4 sensitivity.. seems to have unusually large error (but still small on the scale of things)

Dropping sensitivity to 20

2ad25f9

Putting API switches on the sensitivity

0a9ef3a

Fixing nondeterministic Dither tests

50cbdb1

Updated reference images

cdfb3de

Adding XR Screen Position dependency for HD

da7d727

Adding changelog

055f1d3

cdxntchou marked this pull request as ready for review October 21, 2021 16:52

cdxntchou requested review from a team as code owners October 21, 2021 16:52

cdxntchou mentioned this pull request Oct 21, 2021

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

Closed

jessebarker approved these changes Oct 21, 2021

View reviewed changes

joshua-davis suggested changes Oct 21, 2021

View reviewed changes

Chris Tchou added 3 commits October 21, 2021 16:30

Flip Vertex Pixel Position to match the standard (upper left origin)

4eebf6a

Merge of changes from PR #6114 (simplifies this PR)

bc3cd3f

Removing XR special case, Adding VFX support

912f6b4

cdxntchou changed the title ~~[ShaderGraph] Fix screen position behaviors~~ [ShaderGraph] [2022.1] [Ruby] Fix screen position behaviors Oct 25, 2021

cdxntchou mentioned this pull request Oct 25, 2021

[ShaderGraph] [2022.1] [Ruby] Address Screen Position Issues and Tests #6011

Closed

4 tasks

Adding vertex screen position behavior tests

fb0cc25

joshua-davis approved these changes Oct 26, 2021

View reviewed changes

cdxntchou commented Oct 26, 2021

View reviewed changes

cdxntchou requested review from a team October 27, 2021 16:57

VitaVFX requested review from VitaVFX and removed request for a team October 28, 2021 05:43

PaulDemeulenaere self-requested a review October 28, 2021 06:42

ellioman approved these changes Oct 28, 2021

View reviewed changes

VitaVFX approved these changes Oct 28, 2021

View reviewed changes

PaulDemeulenaere approved these changes Oct 28, 2021

View reviewed changes

com.unity.visualeffectgraph/Editor/ShaderGraph/VFXShaderGraphParticleOutput.cs Show resolved Hide resolved

com.unity.visualeffectgraph/Editor/ShaderGraph/VFXShaderGraphParticleOutput.cs Show resolved Hide resolved

PaulDemeulenaere reviewed Oct 29, 2021

View reviewed changes

phi-lira approved these changes Nov 1, 2021

View reviewed changes

JMargevics approved these changes Nov 8, 2021

View reviewed changes

Chris Tchou added 2 commits November 8, 2021 17:15

Revert "Merge of changes from PR #6114 (simplifies this PR)" for merg…

decbcd8

…e (target PR was already merged) This reverts commit bc3cd3f.

Merge branch 'master' into sg/fix/screen-position2

a1682f7

jessebarker merged commit 2c293d7 into master Nov 9, 2021

jessebarker deleted the sg/fix/screen-position2 branch November 9, 2021 03:50

[ShaderGraph] [2022.1] [Ruby] Fix screen position behaviors #6040

[ShaderGraph] [2022.1] [Ruby] Fix screen position behaviors #6040

Uh oh!

Conversation

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

Uh oh!

Purpose of this PR

MUST LAND AFTER (and remove changes from merged):

Testing status

Comments to reviewers

Uh oh!

github-actions bot commented Oct 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jessebarker left a comment

Choose a reason for hiding this comment

Uh oh!

jessebarker commented Oct 21, 2021

Uh oh!

cdxntchou commented Oct 21, 2021

Uh oh!

jessebarker commented Oct 21, 2021

Uh oh!

joshua-davis left a comment

Choose a reason for hiding this comment

Uh oh!

cdxntchou Oct 26, 2021

Choose a reason for hiding this comment

Uh oh!

VitaVFX left a comment

Choose a reason for hiding this comment

Uh oh!

PaulDemeulenaere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PaulDemeulenaere Oct 29, 2021

Choose a reason for hiding this comment

Uh oh!

phi-lira left a comment

Choose a reason for hiding this comment

Uh oh!

JMargevics left a comment

Choose a reason for hiding this comment

Uh oh!

BogomilP commented Dec 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

cdxntchou commented Oct 15, 2021 •

edited

Loading

github-actions bot commented Oct 15, 2021 •

edited

Loading