[XR][URP] Fix issue with vignette location in XR #5471

maloyer-unity · 2021-08-30T16:43:55Z

Vignette center needs to respect the real view center since in XR, with asymmetric FOV, the center of the render taget is not always the center of the view. We deduce the real center via the projection matrix data and use that to offset the vignette's center.

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-*.

Purpose of this PR

Fix the vignette post-process to respect the XR eye center (which is not the texture center because of AsymmetricFov). This caused the vignette effect to look like its overlapping between the eyes (case 1358336)

Testing status

Tested on

PC with Quest via Oculus Link
Quest 2 AndroidPlayer 64bit IL2CPP

Vignette center needs to respect the real view center since in XR, with asymmetric FOV, the center of the render taget is not always the center of the view. We deduce the real center via the projection matrix data and use that to offset the vignette's center.

github-actions · 2021-08-30T16:44:10Z

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_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.

fabien-unity · 2021-08-31T17:08:24Z

com.unity.render-pipelines.universal/Runtime/XR/XRPass.cs

+            float right = Math.Abs(projectionParameters.right);
+            float top = Math.Abs(projectionParameters.top);
+            float bottom = Math.Abs(projectionParameters.bottom);
+            eyeCenterUV = new Vector2(left / (right + left), top / (top + bottom));


Can we add a method to avoid code duplication : computeEyeCenterUV(Matrix4x4 proj) ?

Yes, we sure can !

fabien-unity · 2021-08-31T17:09:23Z

com.unity.render-pipelines.universal/Shaders/PostProcessing/UberPost.shader

@@ -191,6 +201,10 @@ Shader "Hidden/Universal Render Pipeline/UberPost"
            UNITY_BRANCH
            if (VignetteIntensity > 0)
            {
+            #ifdef USING_STEREO_MATRICES
+                const float2 VignetteCenter = unity_StereoEyeIndex == 0 ? VignetteCenterEye0 : VignetteCenterEye1;


In general, we'd like to be able to support more than 2 views. Can we evaluate moving this code to an array with indexing instead ?

Since this use case is very specific to eyes, I don't know if we would even need that for "more than two" displays.
And, as far as I know, we don't have 3-eyes human, yet ;)

Also, for more than 2 display scenarios, like domes, super panoramic views or stuff like that we would need to change the whole vignette post-process to have a center in world space to be able to sync all views to work together, a screen-space effect might not work well in that case without being fully custom.

Update: I had a chat with Fabien and I will implement "more than 2 views" for XR as there is some use-cases for it with some specific headsets

Update 2 : Returning to 2 views to keep the code simple and we'll add more when we need to.

One of our partners is moving away from asymmetric frustums. We should keep this fix as simple as possible. What about extracting the center from the current stereo projection matrix in either vertex or pixel shader directly? Then there's no need to pass in per eye centers or changing script code.

I don't expect this change to prevent symmetric FOV from being used, it will only allow asymmetric FOV to work correctly. This change will work with both symmetric and asymmetric FOV, the center calculated for symmetric FOV will be (0.5, 0.5) for both eyes since in that case leftFov == rightFov && topFov == bottomFov.

Extracting the data from the shader (Vertex or Fragment) will make this change more complicated than it currently is since this is a post process and uses a generic Vertex Shader which does not have information about the projection. Changing that will introduce code duplication and as a side effect, more possible point of failures.

I believe that the eye center should be calculated on the CPU since it is something that will likely be used by other post process or shaders that rely on knowing where the center of the view is (any type of radial post-process, some screen space effects or more), it is also something that I expect some games will want to know too (I used these values for a few things in StarWars Squadrons VR).

In fact, I was very surprised that the XR view center coordinates was not already available in our XR C# code !

Ok, the points sound reasonable.

fabien-unity · 2021-08-31T17:13:27Z

com.unity.render-pipelines.universal/Runtime/XR/XRPass.cs

@@ -38,6 +38,7 @@ internal struct XRView
        internal readonly Rect viewport;
        internal readonly Mesh occlusionMesh;
        internal readonly int textureArraySlice;
+        internal readonly Vector2 eyeCenterUV;


Quick note : this will create a merge conflict with #3557
Not a huge deal but let's discuss the order of PRs.

… 2 XR views

…center" parameter

This will keep the shader as simple as possible, if we need to support more views in the future, we can restore this code

fabien-unity · 2021-09-01T15:32:55Z

com.unity.render-pipelines.universal/Runtime/Passes/PostProcessPass.cs

+#if ENABLE_VR && ENABLE_XR_MODULE
+            if (xrPass != null && xrPass.enabled)
+            {
+                var v3 = new Vector4(0.0f, 0.0f, 0.0f, 0.0f);


I'm being very picky but you can replace this by Vector4.zero :)

fabien-unity · 2021-09-01T15:34:35Z

com.unity.render-pipelines.universal/Runtime/Passes/PostProcessPass.cs

+                v2.y = view0.eyeCenterUV.y - centerDeltaY;
+                if (xrPass.views.Count == 1)
+                {
+                    v3.z = v2.x;


is this part ever used ? if we're in multipass mode (where views.Count == 1) then the _Vignette_Params3 will never be read in the shader.

fabien-unity · 2021-09-01T15:35:51Z

com.unity.render-pipelines.universal/Runtime/Passes/PostProcessPass.cs

+                    v3.y = view1.eyeCenterUV.y - centerDeltaY;
+                }
+
+                material.SetVector(ShaderConstants._Vignette_Params3, v3);


Even this part should really only be set if we're in multiview/spi situation (viewCount > 1)

Yes, good point, I'll make that part of the code only used in single-pass mode and run some tests.

That variable is only needed if single-pass instancing/multiview is active, so we can simplify the code to only do the asymetric FOV correction when we are in multi-pass mode.

eh-unity

Looks good, but added some improvement suggestions.

eh-unity · 2021-09-07T15:05:07Z

com.unity.render-pipelines.universal/Runtime/Passes/PostProcessPass.cs

@@ -1502,6 +1521,7 @@ static class ShaderConstants
            public static readonly int _Chroma_Params = Shader.PropertyToID("_Chroma_Params");
            public static readonly int _Vignette_Params1 = Shader.PropertyToID("_Vignette_Params1");
            public static readonly int _Vignette_Params2 = Shader.PropertyToID("_Vignette_Params2");
+            public static readonly int _Vignette_Params3 = Shader.PropertyToID("_Vignette_Params3");


Maybe call this "_Vignette_ParamsXR" since that's what it is and it's easier to read later.

Yes, we can do that

eh-unity · 2021-09-07T15:11:37Z

com.unity.render-pipelines.universal/Runtime/Passes/PostProcessPass.cs

@@ -1251,6 +1251,25 @@ void SetupVignette(Material material)
                m_Vignette.smoothness.value * 5f
            );

+#if ENABLE_VR && ENABLE_XR_MODULE


Maybe move the XR specific block into a function, easier to read than bunch of defines.
That function could even be in xrPass.
Something like
vec4 xrPass.applyXREyeOffset(vec2 center); which packs XR eye data into Vec4. Or xrPass.applyEyeOffset( ref vec2 centerAsLeft, out vec2 right);

But just moving this block to SetupVignetteXR() would be tiny bit better IMO.

I think we can extract a good amount of code to the XRPass class, but we still need to do some part of the work in SetupVignette() since when we are rendering in Multi-pass (each eye are rendered completely separately), the center still need to be corrected for each eyes and then placed in v2.xy since AFAIK the shader does not have a variation for "XR enabled, but not single-pass".

I'll see what I can do to reduce the amount of XR specific code in here.

eh-unity · 2021-09-07T15:19:53Z

com.unity.render-pipelines.universal/Shaders/PostProcessing/UberPost.shader

@@ -68,7 +71,12 @@ Shader "Hidden/Universal Render Pipeline/UberPost"
        #define LensDirtIntensity       _LensDirt_Intensity.x

        #define VignetteColor           _Vignette_Params1.xyz
+    #ifdef USING_STEREO_MATRICES
+        #define VignetteCenterEye0      _Vignette_Params2.xy


Since we're already differentiating with center and Eye0, Eye1 we could just pack all the XR Eye data to Params3 (or ParamsXR). I think it would be a tiny bit cleaner as it would avoid this interleaving
params2( standard.xy, XR0.xy), params3( XR1.xy, --/standard.xy? )
pattern, and be
params2( standard.xy, --/standard.xy? ), params3( XR0.xy, XR1.xy ) or paramsXR( XR0.xy, XR1.xy )
as it doesn't matter where the 2 extra floats are.

Sure, I'll get the XR single-pass data to its own variable then to group it all together

ernestasKupciunas

Approving without testing. The developer has already tested his changes on Ques2. I have only Quest2 at home too, so there is no point testing twice on the same platform.

- Renamed _Vignette_Params3 to _Vignette_ParamsXR - Moved all single-pass XR values in _Vignette_ParamsXR - Extracted code to correct eye center to XRPass class

# Conflicts: # com.unity.render-pipelines.universal/CHANGELOG.md

maloyer-unity · 2021-09-16T17:45:07Z

Hi everyone, I would like to close this bug, since there is no more action items on my side, is it possible to resume the review ?

mikesfbay · 2021-09-17T06:25:38Z

Need XR QA to do some regression testing on this PR.

fcoulombe

LGTM

JasonCostanza

Version: 2022.1.0a10.1326
Revision: trunk 48c8cfa2acec
Built: Fri, 17 Sep 2021 07:51:06 GMT

commit 85c3375 (HEAD -> urp/xr/fix_vignette_xr_center, origin/urp/xr/fix_vignette_xr_center)

From what I can tell, this appears to look good to me. I spoke to Marc-Andre on slack and compared notes and I believe what I'm seeing is the desired effect. Basically, the vignette tunnels appear to align in the eyes creating the illusion that only one is being drawn. From my tests, the vignette felt correct and natural and I couldn't spot any sign of misalignment.

maloyer-unity · 2021-10-06T18:38:38Z

@mikesfbay, Ryan was not available to do a QA pass, but Jason was able to have look, is there anything else that is needed before we can close this issue?

@phi-lira, As soon we have the final approval, the fix will be ready to merge into the new URP staging branch, I'm just not sure how we can change the upstream target of a PR.

* [XR][URP] Fix issue with vignette location in XR Vignette center needs to respect the real view center since in XR, with asymmetric FOV, the center of the render taget is not always the center of the view. We deduce the real center via the projection matrix data and use that to offset the vignette's center. * Updated changelog * Extracted comon XRView code into ComputeEyeCenterUV & Allow more than 2 XR views * Fix vignette movement direction for XR when whe change the vignette "center" parameter * Returning to 2 views only for XR vignette. This will keep the shader as simple as possible, if we need to support more views in the future, we can restore this code * Use "Vector4.zero" instead of "new Vector4(0.0f, 0.0f, 0.0f, 0.0f)" * Only set v3 if we are using single pass That variable is only needed if single-pass instancing/multiview is active, so we can simplify the code to only do the asymetric FOV correction when we are in multi-pass mode. * More improvements - Renamed _Vignette_Params3 to _Vignette_ParamsXR - Moved all single-pass XR values in _Vignette_ParamsXR - Extracted code to correct eye center to XRPass class * Adding a comment to describe what ApplyXRViewCenterOffset does * Quick fix a comment that is not right anymore after moving the code

#5471 #5471 commit 85c3375 Author: Marc-Andre Loyer <marcandre.loyer@unity3d.com> Date: Thu Sep 9 15:39:40 2021 -0400 Quick fix a comment that is not right anymore after moving the code commit 958d05c Author: Marc-Andre Loyer <marcandre.loyer@unity3d.com> Date: Thu Sep 9 15:18:35 2021 -0400 Adding a comment to describe what ApplyXRViewCenterOffset does commit 875610e Merge: 3c100ed e28ed8a Author: Marc-Andre Loyer <marcandre.loyer@unity3d.com> Date: Thu Sep 9 11:29:56 2021 -0400 Merge branch 'master' into urp/xr/fix_vignette_xr_center # Conflicts: # com.unity.render-pipelines.universal/CHANGELOG.md commit 3c100ed Author: Marc-Andre Loyer <marcandre.loyer@unity3d.com> Date: Thu Sep 9 11:28:04 2021 -0400 More improvements - Renamed _Vignette_Params3 to _Vignette_ParamsXR - Moved all single-pass XR values in _Vignette_ParamsXR - Extracted code to correct eye center to XRPass class commit 890ee1f Author: Marc-Andre Loyer <marcandre.loyer@unity3d.com> Date: Wed Sep 1 11:52:18 2021 -0400 Only set v3 if we are using single pass That variable is only needed if single-pass instancing/multiview is active, so we can simplify the code to only do the asymetric FOV correction when we are in multi-pass mode. commit 3b89c75 Author: Marc-Andre Loyer <marcandre.loyer@unity3d.com> Date: Wed Sep 1 11:35:26 2021 -0400 Use "Vector4.zero" instead of "new Vector4(0.0f, 0.0f, 0.0f, 0.0f)" commit 323a155 Author: Marc-Andre Loyer <marcandre.loyer@unity3d.com> Date: Wed Sep 1 10:50:36 2021 -0400 Returning to 2 views only for XR vignette. This will keep the shader as simple as possible, if we need to support more views in the future, we can restore this code commit 9364b7d Author: Marc-Andre Loyer <marcandre.loyer@unity3d.com> Date: Tue Aug 31 17:42:49 2021 -0400 Fix vignette movement direction for XR when whe change the vignette "center" parameter commit 348e23b Author: Marc-Andre Loyer <marcandre.loyer@unity3d.com> Date: Tue Aug 31 14:49:32 2021 -0400 Extracted comon XRView code into ComputeEyeCenterUV & Allow more than 2 XR views commit 263cb31 Merge: 118a617 9b13da8 Author: Marc-Andre Loyer <marcandre.loyer@unity3d.com> Date: Mon Aug 30 14:49:39 2021 -0400 Merge branch 'master' into urp/xr/fix_vignette_xr_center commit 118a617 Author: Marc-Andre Loyer <marcandre.loyer@unity3d.com> Date: Mon Aug 30 12:47:58 2021 -0400 Updated changelog commit a31f3bc Author: Marc-Andre Loyer <marcandre.loyer@unity3d.com> Date: Fri Aug 27 19:01:47 2021 -0400 [XR][URP] Fix issue with vignette location in XR Vignette center needs to respect the real view center since in XR, with asymmetric FOV, the center of the render taget is not always the center of the view. We deduce the real center via the projection matrix data and use that to offset the vignette's center.

* master: Enable iris normal for Eye shader (#5880) Update to HDRP Asset analytics (#6060) [HDPR] Update reference screenshots for Linux Vulkan Update 206_Motion_Vectors.png (#6046) Updated code owner for URP (#6052) [URP] Re-enable ios pbr tests #5983 Fix error when using gizmos with camera stack in editor #5913 emitting UI geometry for offscreen cameras too (case 1344969 fix) #5894 Remove deprecated UNITY_USE_NATIVE_HDR keyword from shader code. #5569 [XR][URP] Fix issue with vignette location in XR #5471 #5471

* Fix for specular color plus a first draft of a test scene for it * Minor color updates * Test updated * Adding Directional Light * Adding the new test scene to build settings * Reference Images * Redoing the specular bugfix after various changes happened on Master * Adding a Deferred Test Scene * Fixing Specular on Deferred and adding a test scene for it as well * Same fix but in ShaderGraph shaders * Filtering out the 011 deferred scene for Android Vulkan (Issue: 1340634) and updating three reference images * D3D11 updated reference image. * Set a valid stencil format when creating depthStencil render-target. * D3D12 reference image * D3D11 reference image * DX11 Deferred Reference image * Removing a scene from the testcase filters as the issue has been resolved * Changelog * [XR][URP] Fix issue with vignette location in XR (#5471) * [XR][URP] Fix issue with vignette location in XR Vignette center needs to respect the real view center since in XR, with asymmetric FOV, the center of the render taget is not always the center of the view. We deduce the real center via the projection matrix data and use that to offset the vignette's center. * Updated changelog * Extracted comon XRView code into ComputeEyeCenterUV & Allow more than 2 XR views * Fix vignette movement direction for XR when whe change the vignette "center" parameter * Returning to 2 views only for XR vignette. This will keep the shader as simple as possible, if we need to support more views in the future, we can restore this code * Use "Vector4.zero" instead of "new Vector4(0.0f, 0.0f, 0.0f, 0.0f)" * Only set v3 if we are using single pass That variable is only needed if single-pass instancing/multiview is active, so we can simplify the code to only do the asymetric FOV correction when we are in multi-pass mode. * More improvements - Renamed _Vignette_Params3 to _Vignette_ParamsXR - Moved all single-pass XR values in _Vignette_ParamsXR - Extracted code to correct eye center to XRPass class * Adding a comment to describe what ApplyXRViewCenterOffset does * Quick fix a comment that is not right anymore after moving the code * Remove deprecated UNITY_USE_NATIVE_HDR keyword from shader code. (#5569) * Fix error when using gizmos with camera stack in editor (#5913) * Fix game window gizmos when camera stack is used * Remove Assert check for the last camera in stack * Add lastCameraInStack check * Updating WindowsEditor DX11 reference image * Updating Android Vulkan & iOS Metal reference images. Upping the threshold a tiny bit. * Trying a new reference image for WinPlayer DX11 & DX12 * Disabling XR capability for the new test as it breaks due to the Development Build label. * Updating reference images for VFX due to the changes done with my fix. * Changelog * Updating materials * Updating reference images for VFX OSX Editor on Metal Co-authored-by: Andrem <andrem@unity3d.com> Co-authored-by: Kay Chang <kaychang@unity3d.com> Co-authored-by: Marc-Andre Loyer <82059196+maloyer-unity@users.noreply.github.com> Co-authored-by: Pema Malling <pema99@users.noreply.github.com> Co-authored-by: Tomas Zigmantavičius <30701728+tomzig16@users.noreply.github.com> Co-authored-by: Felipe Lira <felipedrl@gmail.com>

github-actions bot added the universal label Aug 30, 2021

maloyer-unity added 2 commits August 30, 2021 12:47

Updated changelog

118a617

Merge branch 'master' into urp/xr/fix_vignette_xr_center

263cb31

maloyer-unity added the XR label Aug 30, 2021

maloyer-unity requested a review from fabien-unity August 30, 2021 21:36

fabien-unity reviewed Aug 31, 2021

View reviewed changes

maloyer-unity added 2 commits August 31, 2021 15:09

Extracted comon XRView code into ComputeEyeCenterUV & Allow more than…

348e23b

… 2 XR views

Fix vignette movement direction for XR when whe change the vignette "…

9364b7d

…center" parameter

mikesfbay self-requested a review September 1, 2021 06:49

Returning to 2 views only for XR vignette.

323a155

This will keep the shader as simple as possible, if we need to support more views in the future, we can restore this code

fabien-unity reviewed Sep 1, 2021

View reviewed changes

Use "Vector4.zero" instead of "new Vector4(0.0f, 0.0f, 0.0f, 0.0f)"

3b89c75

fabien-unity reviewed Sep 1, 2021

View reviewed changes

Only set v3 if we are using single pass

890ee1f

That variable is only needed if single-pass instancing/multiview is active, so we can simplify the code to only do the asymetric FOV correction when we are in multi-pass mode.

maloyer-unity marked this pull request as ready for review September 1, 2021 15:56

maloyer-unity requested review from a team as code owners September 1, 2021 15:56

eh-unity reviewed Sep 7, 2021

View reviewed changes

ernestasKupciunas approved these changes Sep 9, 2021

View reviewed changes

maloyer-unity added 4 commits September 9, 2021 11:28

More improvements

3c100ed

- Renamed _Vignette_Params3 to _Vignette_ParamsXR - Moved all single-pass XR values in _Vignette_ParamsXR - Extracted code to correct eye center to XRPass class

Merge branch 'master' into urp/xr/fix_vignette_xr_center

875610e

# Conflicts: # com.unity.render-pipelines.universal/CHANGELOG.md

Adding a comment to describe what ApplyXRViewCenterOffset does

958d05c

Quick fix a comment that is not right anymore after moving the code

85c3375

fabien-unity approved these changes Sep 16, 2021

View reviewed changes

mikesfbay requested a review from ryanhy-unity September 17, 2021 06:24

eh-unity approved these changes Sep 23, 2021

View reviewed changes

fcoulombe approved these changes Sep 24, 2021

View reviewed changes

JasonCostanza self-requested a review October 6, 2021 17:15

JasonCostanza approved these changes Oct 6, 2021

View reviewed changes

maloyer-unity removed the request for review from ryanhy-unity October 6, 2021 18:13

mikesfbay approved these changes Oct 7, 2021

View reviewed changes

phi-lira changed the base branch from master to universal/staging October 12, 2021 09:34

phi-lira merged commit 5ea5ce4 into universal/staging Oct 12, 2021

phi-lira deleted the urp/xr/fix_vignette_xr_center branch October 12, 2021 09:35

phi-lira mentioned this pull request Oct 12, 2021

Universal Staging #5998

Closed

phi-lira restored the urp/xr/fix_vignette_xr_center branch October 18, 2021 15:13

phi-lira deleted the urp/xr/fix_vignette_xr_center branch October 18, 2021 15:16

phi-lira mentioned this pull request Oct 18, 2021

Universal Staging #6054

Merged

[XR][URP] Fix issue with vignette location in XR #5471

[XR][URP] Fix issue with vignette location in XR #5471

Uh oh!

Conversation

maloyer-unity commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist for PR maker

Purpose of this PR

Testing status

Uh oh!

github-actions bot commented Aug 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eh-unity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ernestasKupciunas left a comment

Choose a reason for hiding this comment

Uh oh!

maloyer-unity commented Sep 16, 2021

Uh oh!

mikesfbay commented Sep 17, 2021

Uh oh!

fcoulombe left a comment

Choose a reason for hiding this comment

Uh oh!

JasonCostanza left a comment

Choose a reason for hiding this comment

Uh oh!

maloyer-unity commented Oct 6, 2021

Uh oh!

Uh oh!

maloyer-unity commented Aug 30, 2021 •

edited

Loading