Skip to content

[Fix] Motion Vectors: Enable for XR #4707

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.

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

Open
wants to merge 47 commits into
base: master
Choose a base branch
from

Conversation

he-ax
Copy link
Contributor

@he-ax he-ax commented May 28, 2021

Purpose of this PR

Enable Motion Vectors for XR and fix some issues with multi pass and camera motion vectors.
When enabling test for xr I noticed that view matrix did not update properly for multi pass, fixed this by uploading view as well.


Testing status

Enabled Object Motion Vector test for XR.
Added test for Camera Motion Vectors (also enabled for XR).
Renamed test scenes to not have a number so close to SSAO scenes.
Camera Motion Vector tests are disabled on GLES3 due to issues with depth and msaa, but running it locally they look correct:
image


Comments to reviewers

Notes for the reviewers you have assigned.

@phi-lira phi-lira requested a review from thomas-zeng June 2, 2021 14:01
@he-ax he-ax requested review from phi-lira and thomas-zeng June 4, 2021 14:36
@he-ax he-ax marked this pull request as ready for review July 6, 2021 12:27
@he-ax he-ax requested review from a team as code owners July 6, 2021 12:27
@he-ax he-ax marked this pull request as draft July 6, 2021 12:29
@he-ax he-ax removed the Shader Graph label Jul 6, 2021
@he-ax he-ax marked this pull request as ready for review July 6, 2021 12:35
@he-ax he-ax removed the request for review from a team July 6, 2021 12:35
Copy link
Contributor

@phi-lira phi-lira left a comment

Choose a reason for hiding this comment

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

There's also changelog missing.

@@ -0,0 +1,65 @@
Shader "MotionVecDebug"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used for testing only? Maybe it should be under the https://github.com/Unity-Technologies/Graphics/tree/master/com.unity.testing.urp package.

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor naming suggestion

Suggested change
Shader "MotionVecDebug"
Shader "MotionVectorsDebug"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There doesn't seem to be any other test related shaders in that package, why should this one specifically be moved? Other tests have shaders kept in the foundation project.

Comment on lines +177 to +194
- FilteredScene: {fileID: 0}
FilteredScenes:
- {fileID: 102900000, guid: 26800f4b3c38849428d29e93038a6084, type: 3}
ColorSpace: -1
BuildPlatform: -2
GraphicsDevice: 21
XrSdk:
StereoModes: 0
Reason: Needs fix for MSAA+depth
- FilteredScene: {fileID: 0}
FilteredScenes:
- {fileID: 102900000, guid: 26800f4b3c38849428d29e93038a6084, type: 3}
ColorSpace: -1
BuildPlatform: -2
GraphicsDevice: 11
XrSdk:
StereoModes: 0
Reason: Needs fix for MSAA+depth
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the exceptions being added here for which platforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excluding new camera motion vector test since there is a bug on vulkan with msaa and depth

Copy link
Contributor

@thomas-zeng thomas-zeng left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@he-ax he-ax requested a review from a team August 9, 2021 06:38
Copy link

@ryanhy-unity ryanhy-unity left a comment

Choose a reason for hiding this comment

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

This looked good from an XR perspective. I tested Quest using Vulkan and Mock HMD using DX11 for multipass, multiview and instancing and motion vectors all looked proper in both eyes in all cases.

@he-ax he-ax requested a review from a team as a code owner November 25, 2021 09:06
@smitdylan2001
Copy link
Contributor

smitdylan2001 commented Aug 10, 2022

Is this PR still active and is it being worked on to merge?

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.

6 participants