Skip to content

Conversation

cdxntchou
Copy link

@cdxntchou cdxntchou commented Sep 30, 2021

Purpose of this PR

Fix for regression introduced by:

Bug https://fogbugz.unity3d.com/f/cases/1369450/

This PR does not include fixes for Built-in or HDRP, but I will create a separate PR for those, want to unblock URP tests first.

The issue is that _ScreenParams is NOT the render target resolution when XR is enabled -- it is the resolution of the current camera render target (i.e. the final combined image) but the render targets we actually render the shaders to are per eye and of a different resolution.

Unfortunately, there doesn't appear to be any way to access the "current render target" resolution (would require a trunk change to add it). So for the time being I've just reverted the code to use the less accurate projection method of calculating screen position when XR is active.

This PR also fixes another bug where the new screen position code was not taking inverted Y projections into account.

See example failure:
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/master/.yamato%252Funiversal_stereo-win.yml%2523Universal_Stereo_Win_Standalone_mono_Linear_trunk/9048767/job/artifacts
Test: 126_SampleDepth is failing because it is not sampling the depth buffer from the correct screen space location

Now passes!
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252F1369450/.yamato%252Funiversal_stereo-win.yml%2523Universal_Stereo_Win_Standalone_mono_Linear_trunk/9108989/job


Testing status

Describe what manual/automated tests were performed for this PR

Tested locally : scene vs. game view, XR Mock HMD, and URP test.

Yamato:

ShaderGraph PR: 🟡 -- same failures as master
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252F1369450/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_trunk/9090840/job/pipeline

master: 3ab2be1
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/master/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_trunk/9089368/job/pipeline

PR URP: 🟡 -- matches master
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252F1369450/.yamato%252Fall-urp.yml%2523PR_URP_trunk/9111636/job/pipeline

master: 3ab2be1
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/master/.yamato%252Fall-urp.yml%2523PR_URP_trunk/9089372/job/pipeline

failures:
Build URP_Foundation on iPhone_Metal_il2cpp_Linear_Standalone_build_Player on version trunk -- non-test-related failure on both 🟡
Build URP_Foundation on Linux_Vulkan_mono_Linear_Standalone_build_Player on version trunk -- non-test-related failure on both 🟡
Build ShaderGraph on Linux_Vulkan_mono_Linear_Standalone_build_Player on version trunk -- non-test-related failure on both 🟡
Build ShaderGraph on iPhone_Metal_il2cpp_Linear_Standalone_build_Player on version trunk -- non-test-related failure on both 🟡

URP_Lighting on Android_Vulkan_Standalone_il2cpp_Linear on version trunk -- green on re-run 🟢
URP_Lighting on Android_OpenGLES3_Standalone_il2cpp_Linear on version trunk -- same 1 test failed 🟡
URP_Terrain on Android_OpenGLES3_Standalone_il2cpp_Linear on version trunk -- non-test-related failure on both 🟡
URP_Foundation on OSX_Metal_playmode_mono_Linear on version trunk -- green on re-run 🟢
URP_Foundation on Android_OpenGLES3_Standalone_il2cpp_Linear on version trunk -- same 2 tests failed 🟡
URP_Foundation on Win_DX12_Standalone_XR_mono_Linear on version trunk -- same 1 test failed 🟡
URP_Foundation on Win_DX11_playmode_XR_mono_Linear on version trunk -- same 1 test failed 🟡
URP_Foundation on Win_DX11_Standalone_XR_mono_Linear on version trunk -- same 1 test failed 🟡
URP_PostPro on Android_Vulkan_Standalone_il2cpp_Linear on version trunk -- same 2 tests failed 🟡
URP_PostPro on Android_OpenGLES3_Standalone_il2cpp_Linear on version trunk -- same 1 test failed 🟡
URP_PostPro on Linux_Vulkan_Standalone_mono_Linear on version trunk -- same 2 tests failed 🟡
URP_PostPro on Win_Vulkan_Standalone_mono_Linear on version trunk -- same 2 tests failed 🟡
URP_PostPro on Win_DX11_playmode_XR_mono_Linear on version trunk -- green on re-run 🟢


Comments to reviewers

Notes for the reviewers you have assigned.

@github-actions
Copy link

github-actions bot commented Sep 30, 2021

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)

URP
/.yamato%252Fall-urp.yml%2523PR_URP_trunk
With changes to URP packages, you should also run
/.yamato%252Fall-lightmapper.yml%2523PR_LightMapper_trunk

Shader Graph
/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_trunk
Depending on your PR, you may also want
/.yamato%252Fall-shadergraph_builtin_foundation.yml%2523PR_ShaderGraph_BuiltIn_Foundation_trunk
/.yamato%252Fall-shadergraph_builtin_lighting.yml%2523PR_ShaderGraph_BuiltIn_Lighting_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.

Copy link
Contributor

@jessebarker jessebarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (should still get URP approval)

@cdxntchou cdxntchou marked this pull request as ready for review September 30, 2021 23:04
@cdxntchou cdxntchou requested review from a team as code owners September 30, 2021 23:04
@cdxntchou cdxntchou changed the title [ShaderGraph] [2022.1] [Ruby] Fix XR regression in screen position [ShaderGraph] [2022.1] [Ruby] Fix regression in screen position when using XR Sep 30, 2021
@cdxntchou
Copy link
Author

This is ready to merge as soon as it gets review approval

Copy link
Contributor

@pbbastian pbbastian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

@cdxntchou cdxntchou requested a review from jessebarker October 5, 2021 18:45
Copy link
Contributor

@jessebarker jessebarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@phi-lira
Copy link
Contributor

phi-lira commented Oct 6, 2021

Are the failures in yamato expected issues?

@jessebarker
Copy link
Contributor

Are the failures in yamato expected issues?

Based on the links and comments in the "Testing Status" section, it looks like all of the failures on this branch are failing in the same way on master on the same base commit. Chris might be able to clarify further.

@phi-lira phi-lira changed the base branch from master to universal/staging October 7, 2021 10:08
@phi-lira phi-lira merged commit 9372e08 into universal/staging Oct 7, 2021
@phi-lira phi-lira deleted the sg/fix/1369450 branch October 7, 2021 10:09
@phi-lira phi-lira mentioned this pull request Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants