Skip to content
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

PPV2 - Fix VR/XR detection when using XR plugins #5698

Merged
merged 6 commits into from
Dec 15, 2021

Conversation

maloyer-unity
Copy link
Contributor

@maloyer-unity maloyer-unity commented Sep 16, 2021

This detection was only working at run-time or with a pre-2020.1 Editor... So any recent Editor that fully rely on XR system will not work.
With XR extensions, we can query the XR Management system to see if there is any active XR extension.


Purpose of this PR

Fix for : https://fogbugz.unity3d.com/f/cases/1302489

Warnings were not showing up when XR plugins are used (was working with old VR module).

  • Lens distortion
  • Motion blur
  • Bloom's "dirt texture"

This PR fix the detection of VR/XR in the editor (run-time detection was already working).


Testing status

Checked that the warnings appear when XR is active and have a XR plugin selected, if no XR plugin is selected or XR manager is not installed, the warnings will not show.

This detection was only working at run-time or with a pre-2020.1 Editor... So any recent Editor that fully rely on XR system will not work.
With XR extensions, we can query the XR Management system to see if there is any active XR extension.
@maloyer-unity maloyer-unity changed the title Fix VR/XR detection when using XR plugins PPV2 - Fix VR/XR detection when using XR plugins Sep 16, 2021
@maloyer-unity
Copy link
Contributor Author

Not sure if the failed tests are valid or if the test even work at all... My code does not change any rendering features, only Editor code was changed.

@maloyer-unity maloyer-unity marked this pull request as ready for review September 17, 2021 19:23
Copy link
Contributor

@fcoulombe fcoulombe left a comment

Choose a reason for hiding this comment

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

LGTM

"references": [
"Unity.Postprocessing.Runtime"
"Unity.Postprocessing.Runtime",
"Unity.XR.Management",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need these new deps here ? Can we avoid them ?

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'll double check if we can avoid new dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need these references, otherwise the code that query the XR Management module can't work.

But it does not add a strict dependency to XR Management extension, it will only use it if the package is added to the project by the user (detected in code via the preproc #if ENABLE_XR_MODULE && XR_MANAGEMENT_4_0_1_OR_NEWER), and if its not there, the part of the code that require it is disabled.

@mikesfbay mikesfbay self-requested a review October 8, 2021 08:23
@mikesfbay
Copy link
Contributor

@maloyer-unity Have you tested this with 2018.4 LTS or 2019.4 LTS using old VR module? How far back will this be backported?
We need to add QA to the loop to test with both 19.4 and 2020+.

# Conflicts:
#	com.unity.postprocessing/CHANGELOG.md
The warning was not appearing with XR module, but was there with old VR extension.
@maloyer-unity
Copy link
Contributor Author

@mikesfbay, I re-tested the fix with 2019.4.29f1, 2020.3.24f1 and 2022.1.0a17(trunk).
On 2020.x and higher the code will only try to use the XR Management package if the package is installed by the user.
On 2019.4 it will use XR Management package if the package is installed by the user, otherwise it will try via the old VR extension (both works as expected now).
I could not test with 2018.x, but it will use the same path as 2019.4 with the old VR extension code (which was the code used before my change, so I expect it will keep on working)

@mikesfbay
Copy link
Contributor

@maloyer-unity Sounds good, looping in QA @ryanhy-unity on postfx v2.

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.

Changes are editor only and seem pretty low risk and isolated to post processing. Verified fix with user project and saw the proper errors displayed after updating to package with these changes.

@maloyer-unity maloyer-unity merged commit f9b266d into master Dec 15, 2021
@maloyer-unity maloyer-unity deleted the ppv2/xr/fix_xr_detection_in_editor branch December 15, 2021 22:27
@mikesfbay
Copy link
Contributor

@maloyer-unity Fyi, we should let Sebastian's team do the final merge to master since he has ownership of the package and is making the releases.

@maloyer-unity
Copy link
Contributor Author

@mikesfbay, good point, I'll do that next time I have changes for this package

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants