Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Oct 12, 2021
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a31f3bc
[XR][URP] Fix issue with vignette location in XR
maloyer-unity Aug 27, 2021
118a617
Updated changelog
maloyer-unity Aug 30, 2021
263cb31
Merge branch 'master' into urp/xr/fix_vignette_xr_center
maloyer-unity Aug 30, 2021
348e23b
Extracted comon XRView code into ComputeEyeCenterUV & Allow more than…
maloyer-unity Aug 31, 2021
9364b7d
Fix vignette movement direction for XR when whe change the vignette "…
maloyer-unity Aug 31, 2021
323a155
Returning to 2 views only for XR vignette.
maloyer-unity Sep 1, 2021
3b89c75
Use "Vector4.zero" instead of "new Vector4(0.0f, 0.0f, 0.0f, 0.0f)"
maloyer-unity Sep 1, 2021
890ee1f
Only set v3 if we are using single pass
maloyer-unity Sep 1, 2021
3c100ed
More improvements
maloyer-unity Sep 9, 2021
875610e
Merge branch 'master' into urp/xr/fix_vignette_xr_center
maloyer-unity Sep 9, 2021
958d05c
Adding a comment to describe what ApplyXRViewCenterOffset does
maloyer-unity Sep 9, 2021
85c3375
Quick fix a comment that is not right anymore after moving the code
maloyer-unity Sep 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[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.
  • Loading branch information
@maloyer-unity
maloyer-unity committed Aug 30, 2021
commit a31f3bc20ef6e3df13b54f8e999695a91e267814
31 changes: 29 additions & 2 deletions com.unity.render-pipelines.universal/Runtime/Passes/PostProcessPass.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ void Swap(ref ScriptableRenderer r)
// Setup other effects constants
SetupLensDistortion(m_Materials.uber, isSceneViewCamera);
SetupChromaticAberration(m_Materials.uber);
SetupVignette(m_Materials.uber);
SetupVignette(m_Materials.uber, cameraData.xr);
SetupColorGrading(cmd, ref renderingData, m_Materials.uber);

// Only apply dithering & grain if there isn't a final pass.
Expand Down Expand Up @@ -1235,7 +1235,7 @@ void SetupChromaticAberration(Material material)

#region Vignette

void SetupVignette(Material material)
void SetupVignette(Material material, XRPass xrPass)
{
var color = m_Vignette.color.value;
var center = m_Vignette.center.value;
Expand All @@ -1250,9 +1250,35 @@ void SetupVignette(Material material)
m_Vignette.intensity.value * 3f,
m_Vignette.smoothness.value * 5f
);
#if ENABLE_VR && ENABLE_XR_MODULE
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

var v3 = new Vector4(0.0f, 0.0f, 0.0f, 0.0f);
if (xrPass != null && xrPass.enabled)
{
float centerDeltaX = 0.5f - center.x;
float centerDeltaY = 0.5f - center.y;

XRView view0 = xrPass.views[0];
v2.x = view0.eyeCenterUV.x + centerDeltaX;
v2.y = view0.eyeCenterUV.y + centerDeltaY;
if (xrPass.views.Count == 1)
{
v3.z = v2.x;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

v3.w = v2.y;
}
else
{
XRView view1 = xrPass.views[1];
v3.x = view1.eyeCenterUV.x + centerDeltaX;
v3.y = view1.eyeCenterUV.y + centerDeltaY;
}
}
#endif

material.SetVector(ShaderConstants._Vignette_Params1, v1);
material.SetVector(ShaderConstants._Vignette_Params2, v2);
#if ENABLE_VR && ENABLE_XR_MODULE
material.SetVector(ShaderConstants._Vignette_Params3, v3);
#endif
}

#endregion
Expand Down Expand Up @@ -1502,6 +1528,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");
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can do that

public static readonly int _Lut_Params = Shader.PropertyToID("_Lut_Params");
public static readonly int _UserLut_Params = Shader.PropertyToID("_UserLut_Params");
public static readonly int _InternalLut = Shader.PropertyToID("_InternalLut");
Expand Down
15 changes: 15 additions & 0 deletions com.unity.render-pipelines.universal/Runtime/XR/XRPass.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ internal struct XRView
internal readonly Rect viewport;
internal readonly Mesh occlusionMesh;
internal readonly int textureArraySlice;
internal readonly Vector2 eyeCenterUV;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


internal XRView(Matrix4x4 proj, Matrix4x4 view, Rect vp, int dstSlice)
{
Expand All @@ -46,6 +47,13 @@ internal XRView(Matrix4x4 proj, Matrix4x4 view, Rect vp, int dstSlice)
viewport = vp;
occlusionMesh = null;
textureArraySlice = dstSlice;

var projectionParameters = proj.decomposeProjection;
float left = Math.Abs(projectionParameters.left);
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));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we sure can !

}

internal XRView(XRDisplaySubsystem.XRRenderPass renderPass, XRDisplaySubsystem.XRRenderParameter renderParameter)
Expand All @@ -56,6 +64,13 @@ internal XRView(XRDisplaySubsystem.XRRenderPass renderPass, XRDisplaySubsystem.X
occlusionMesh = renderParameter.occlusionMesh;
textureArraySlice = renderParameter.textureArraySlice;

var projectionParameters = projMatrix.decomposeProjection;
float left = Math.Abs(projectionParameters.left);
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));

// Convert viewport from normalized to screen space
viewport.x *= renderPass.renderTargetDesc.width;
viewport.width *= renderPass.renderTargetDesc.width;
Expand Down
14 changes: 14 additions & 0 deletions com.unity.render-pipelines.universal/Shaders/PostProcessing/UberPost.shader
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ Shader "Hidden/Universal Render Pipeline/UberPost"
float _Chroma_Params;
half4 _Vignette_Params1;
float4 _Vignette_Params2;
#ifdef USING_STEREO_MATRICES
float4 _Vignette_Params3;
#endif
float2 _Grain_Params;
float4 _Grain_TilingParams;
float4 _Bloom_Texture_TexelSize;
Expand All @@ -68,7 +71,14 @@ Shader "Hidden/Universal Render Pipeline/UberPost"
#define LensDirtIntensity _LensDirt_Intensity.x

#define VignetteColor _Vignette_Params1.xyz
#ifdef USING_STEREO_MATRICES
// With XR, the views can use asymmetric FOV which will have the center of
// each eyes to be at a different location
#define VignetteCenterEye0 _Vignette_Params2.xy
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

#define VignetteCenterEye1 _Vignette_Params3.xy
#else
#define VignetteCenter _Vignette_Params2.xy
#endif
#define VignetteIntensity _Vignette_Params2.z
#define VignetteSmoothness _Vignette_Params2.w
#define VignetteRoundness _Vignette_Params1.w
Expand Down Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 !

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, the points sound reasonable.

#endif

color = ApplyVignette(color, uvDistorted, VignetteCenter, VignetteIntensity, VignetteRoundness, VignetteSmoothness, VignetteColor);
}

Expand Down