Fix issue 1354395 where the right eye was shifted in stereo mode #5789

fcoulombe · 2021-09-23T18:07:08Z

Please read the Contributing guide before making a PR.

Read the Graphics repository & Yamato FAQ.

Checklist for PR maker

Have you added a backport label (if needed)? For example, the need-backport-* label. After you backport the PR, the label changes to backported-*.
Have you updated the changelog? Each package has a CHANGELOG.md file.
Have you updated or added the documentation for your PR? When you add a new feature, change a property name, or change the behavior of a feature, it's best practice to include related documentation changes in the same PR. If you do add documentation, make sure to add the relevant Graphics Docs team member as a reviewer of the PR. If you are not sure which person to add, see the Docs team contacts sheet.
Have you added a graphic test for your PR (if needed)? When you add a new feature, or discover a bug that tests don't cover, please add a graphic test.

Purpose of this PR

This commit Unity-Technologies/PostProcessing@08b1319 forced a reset on the projection matrix in PreCull and PostRender to avoid issues when switching on/off the TAA.
This is fine except that it doesn't reset the projection matrix to the device's projection matrix which seems to trickle down somewhere and result in some systems getting the wrong projection matrix.

This fix consist in forcing the device's projection matrix onto the camera to make sure that everyone is using the correct matrix if we happen to be in stereo.
PostRender doesn't need to copy from device because the Render step of the TAA PostProcessLayer performs that action.
PostRender doesn't need to deal with unjittered because that's probably only used in TAA.

Testing status

2020.3.11f1, 2019.4.30f1
https://unity-ci.cds.internal.unity3d.com/job/9147944
https://unity-ci.cds.internal.unity3d.com/job/9147930

Comments to reviewers

Notes for the reviewers you have assigned.

fcoulombe · 2021-09-28T20:08:55Z

I'm missing changelog.md modification

mikesfbay

In general, for a bug fix PR, there's no need to fix all failed automated tests that happen in master branch. For a PR to pass QA, one only needs to prove that the same test failure also occurs in master branch. This also isolates the PR bug fix changes from automated test changes so QA will know one isn't biasing the tests in favor of the PR.

Thus, all automated test changes should be reverted from this PR.

com.unity.postprocessing/PostProcessing/Runtime/PostProcessLayer.cs

fcoulombe · 2021-10-04T19:57:22Z

few things to note with the latest code push:
this commit removed the reset on the camera projection for TAA but did not remove the reset on the stereo!
Unity-Technologies/PostProcessing@806605d

physical camera fix also didn't really care
Unity-Technologies/PostProcessing@7bb85bf

this guy used our if statement to hook his code but has nothing to do with TAA
Unity-Technologies/PostProcessing@4969e03

more info https://docs.google.com/document/d/1u0esuZNxBT_2D-X99M5wLY42zAt-QarMr1LocZaKVJI/edit?usp=sharing

fabien-unity · 2021-10-05T16:28:30Z

I'll let @mikesfbay and @daves-unity review this PR as this code is unfamiliar to me and I don't know the ramifications of this change.

mikesfbay · 2021-10-07T07:25:07Z

@JasonCostanza Can you help with testing to see if this fogbugz case is fixed for multipass, and there's no regressions with other postfx effects during stereo instancing?

JasonCostanza · 2021-10-07T21:30:06Z

Attempting to test this using the original repro project, i've opened it in 21.2 and I'm missing namespaces:

E:\Dev\SRP\com.unity.render-pipelines.universal\Editor\2D\ShaderGraph\Targets\UniversalSpriteCustomLitSubTarget.cs(67,77): error CS0234: The type or namespace name 'EnumField' does not exist in the namespace 'UnityEngine.UIElements' (are you missing an assembly reference?)
E:\Dev\SRP\com.unity.render-pipelines.universal\Editor\2D\ShaderGraph\Targets\UniversalSpriteLitSubTarget.cs(69,77): error CS0234: The type or namespace name 'EnumField' does not exist in the namespace 'UnityEngine.UIElements' (are you missing an assembly reference?)
E:\Dev\SRP\com.unity.render-pipelines.universal\Editor\2D\ShaderGraph\Targets\UniversalSpriteLitSubTarget.cs(69,77): error CS0234: The type or namespace name 'EnumField' does not exist in the namespace 'UnityEngine.UIElements' (are you missing an assembly reference?)

Can you advise on how to fix these errors so I can test using your PR branch of SRP (13.0)?

JasonCostanza · 2021-10-13T18:45:46Z

Updating:
I tried launching the repro project again from the bug on 21.2 and I'm refreshing my above review because the compile errors changed. I now only get one error
E:\Dev\SRP\com.unity.render-pipelines.universal\Runtime\RenderingUtils.cs(462,77): error CS0117: 'SystemInfo' does not contain a definition for 'supportsMultisampleResolveStencil'

I do get this error in master however as well, so, though I cannot review this PR it is not blocked BY this PR either.

JasonCostanza · 2021-10-13T21:40:19Z

Update again:
According to Fabien, 21.2 is not a new enough editor release so now using 22.1 and all compile errors are gone. Apologies for the confusion, nothing to see here!

JasonCostanza

2022.1.0a12.1604
Revision: trunk 816252c3efbb
Built: Wed, 06 Oct 2021 17:58:29 GMT

commit 18dbd2e87c90b4106a990fbe222805a6bf6c717e (HEAD, origin/xr/graphics/fix-camera-shift-taa)

WMR:
Reviewed the PR using a WMR HMD in play mode and following the steps in the original bug steps. No change in right eye was observed toggling the post processing layer as described in the bug.

Mock HMD:
Did the same test on Mock just to be double-sure things looked good as described in the bug

mikesfbay · 2021-10-14T20:21:28Z

com.unity.postprocessing/PostProcessing/Runtime/PostProcessRenderContext.cs

@@ -33,6 +33,12 @@ public Camera camera
                    stereoRenderingMode = StereoRenderingMode.SinglePass;
                    numberOfEyes = 1;

+                    if (XRSettings.stereoRenderingMode == XRSettings.StereoRenderingMode.SinglePassMultiview)
+                    {
+                        stereoRenderingMode = StereoRenderingMode.SinglePassMultiview;


As discussed in your previous multiview PR, we don't support multiview feature for PPv2 and it currently doesn't work due to engine not providing input texture correctly (see[ discussion link](URL #5917 (comment))).
Thus, please remove all instances of multiview from this PR.

mikesfbay · 2021-10-14T20:23:08Z

com.unity.postprocessing/PostProcessing/Runtime/PostProcessLayer.cs

@@ -973,7 +1001,7 @@ public void Render(PostProcessRenderContext context)
                    context.GetScreenSpaceTemporaryRT(cmd, lastTarget, 0, context.sourceFormat);
                    if (context.stereoActive && context.numberOfEyes > 1)
                    {
-                        if (context.stereoRenderingMode == PostProcessRenderContext.StereoRenderingMode.SinglePassInstanced)
+                        if (context.stereoRenderingMode == PostProcessRenderContext.StereoRenderingMode.SinglePassInstanced || context.stereoRenderingMode == PostProcessRenderContext.StereoRenderingMode.SinglePassMultiview)


Please revert this line, see discussion ( #5917 (comment))

mikesfbay · 2021-10-14T20:23:41Z

com.unity.postprocessing/PostProcessing/Runtime/PostProcessLayer.cs

@@ -1334,7 +1362,7 @@ void RenderFinalPass(PostProcessRenderContext context, int releaseTargetAfterUse
                dithering.Render(context);

                ApplyFlip(context, uberSheet.properties);
-                if (context.stereoActive && context.stereoRenderingMode == PostProcessRenderContext.StereoRenderingMode.SinglePassInstanced)
+                if (context.stereoActive && (context.stereoRenderingMode == PostProcessRenderContext.StereoRenderingMode.SinglePassInstanced || context.stereoRenderingMode == PostProcessRenderContext.StereoRenderingMode.SinglePassMultiview))


Same here, please revert this line, see discussion ( #5917 (comment))

the stereo rendering mode single pass multiview was simply not handled as a case. It is meant to be quite similar to the single pass instance that renders to the texture array so this change just adds the single pass multivew check wherever we were checking for SPI and this seems to fix the problem of the missing eye.

fcoulombe force-pushed the xr/graphics/fix-camera-shift-taa branch from 994895e to 45a9946 Compare September 28, 2021 18:29

mikesfbay self-requested a review September 28, 2021 20:52

mikesfbay suggested changes Sep 28, 2021

View reviewed changes

sebastienlagarde added the postprocessing v2 label Sep 29, 2021

mikesfbay requested a review from daves-unity September 29, 2021 22:39

mikesfbay suggested changes Sep 29, 2021

View reviewed changes

com.unity.postprocessing/PostProcessing/Runtime/PostProcessLayer.cs Outdated Show resolved Hide resolved

fcoulombe requested review from mikesfbay and fabien-unity October 5, 2021 13:24

fcoulombe marked this pull request as ready for review October 5, 2021 13:25

fabien-unity removed their request for review October 5, 2021 16:28

mikesfbay requested a review from JasonCostanza October 7, 2021 07:22

fcoulombe force-pushed the xr/graphics/fix-camera-shift-taa branch from 4d44e76 to 18dbd2e Compare October 13, 2021 16:47

JasonCostanza approved these changes Oct 13, 2021

View reviewed changes

mikesfbay suggested changes Oct 14, 2021

View reviewed changes

fcoulombe force-pushed the xr/graphics/fix-camera-shift-taa branch from 18dbd2e to 99e7cfc Compare October 14, 2021 20:29

fcoulombe requested a review from mikesfbay October 14, 2021 20:30

mikesfbay approved these changes Oct 15, 2021

View reviewed changes

fcoulombe merged commit a54b10e into master Oct 15, 2021

fcoulombe deleted the xr/graphics/fix-camera-shift-taa branch October 15, 2021 22:24

fcoulombe added needs-backport-7.x needs-backport-10.x needs-backport-2021.2 labels Jan 21, 2022

sebastienlagarde removed needs-backport-7.x needs-backport-10.x needs-backport-2021.2 labels Jan 25, 2022

BenGraterUnity mentioned this pull request Nov 17, 2023

[PPV2] PPB-33 Fixed transparent objects rendering incorrectly when TAA is disabled #7996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix issue 1354395 where the right eye was shifted in stereo mode #5789

Fix issue 1354395 where the right eye was shifted in stereo mode #5789

Uh oh!

fcoulombe commented Sep 23, 2021 •

edited

Loading

Uh oh!

fcoulombe commented Sep 28, 2021

Uh oh!

mikesfbay left a comment

Uh oh!

Uh oh!

fcoulombe commented Oct 4, 2021 •

edited

Loading

Uh oh!

fabien-unity commented Oct 5, 2021

Uh oh!

mikesfbay commented Oct 7, 2021

Uh oh!

JasonCostanza commented Oct 7, 2021

Uh oh!

JasonCostanza commented Oct 13, 2021

Uh oh!

JasonCostanza commented Oct 13, 2021

Uh oh!

JasonCostanza left a comment

Uh oh!

mikesfbay Oct 14, 2021

Uh oh!

mikesfbay Oct 14, 2021

Uh oh!

mikesfbay Oct 14, 2021

Uh oh!

Uh oh!

Fix issue 1354395 where the right eye was shifted in stereo mode #5789

Fix issue 1354395 where the right eye was shifted in stereo mode #5789

Uh oh!

Conversation

fcoulombe commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Please read the Contributing guide before making a PR.

Checklist for PR maker

Purpose of this PR

Testing status

Comments to reviewers

Uh oh!

fcoulombe commented Sep 28, 2021

Uh oh!

mikesfbay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

fabien-unity commented Oct 5, 2021

Uh oh!

mikesfbay commented Oct 7, 2021

Uh oh!

JasonCostanza commented Oct 7, 2021

Uh oh!

JasonCostanza commented Oct 13, 2021

Uh oh!

JasonCostanza commented Oct 13, 2021

Uh oh!

JasonCostanza left a comment

Choose a reason for hiding this comment

Uh oh!

mikesfbay Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

mikesfbay Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

mikesfbay Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fcoulombe commented Sep 23, 2021 •

edited

Loading

fcoulombe commented Oct 4, 2021 •

edited

Loading