-
Notifications
You must be signed in to change notification settings - Fork 328
FIX: Removing dependencies on VRModule #2251
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
base: develop
Are you sure you want to change the base?
Conversation
{ | ||
"name": "com.unity.modules.vr", | ||
"expression": "1.0.0", | ||
"define": "UNITY_INPUT_SYSTEM_ENABLE_VR" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also need to remove code that utilize UNITY_INPUT_SYSTEM_ENABLE_VR
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were no code paths left in the InputSystem or Engine repository that utilized the define UNITY_INPUT_SYSTEM_ENABLE_VR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2251 +/- ##
===========================================
+ Coverage 76.70% 76.73% +0.02%
===========================================
Files 465 465
Lines 87919 87933 +14
===========================================
+ Hits 67442 67475 +33
+ Misses 20477 20458 -19 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
…System into vrmodule-deprecation
|
#pragma warning restore CS0618 | ||
} | ||
#endif | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Can we remove the now empty Awake and OnDestroy as well? Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No because they are protected virtual
and users may have created derived scripts that use them. It would be a breaking change to remove the methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had tried to remove the methods and it got caught against CI. Unfortunately, we have to leave in the empty methods for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fantastic change, especially if we can ship that with Unity 2021.3 (a question). Also, would like to amend changelog a bit as well as remove the now-empty functions that used to hold some VR logic before. Thanks!
Description
The VR Module is obsolete and slated to be removed in a future version of Unity: https://docs.unity3d.com/6000.3/Documentation/ScriptReference/UnityEngine.VRModule.html
This PR removes the dependency of the Input System Package from the VR Module. There is one functional change to the PR, where the AutoXRCameraTracking is disabled during
Awake
andOnDestroy
. This function only works when using the Legacy XR Framework, and does nothing when using the modern XR architecture.Testing status & QA
N/A
Overall Product Risks
Comments to reviewers
Removal of the dependency on the VR Module will help make the full removal of the VR Module easier in the future.
Checklist
Before review:
Changed
,Fixed
,Added
sections.Area_CanDoX
,Area_CanDoX_EvenIfYIsTheCase
,Area_WhenIDoX_AndYHappens_ThisIsTheResult
.During merge:
NEW: ___
.FIX: ___
.DOCS: ___
.CHANGE: ___
.RELEASE: 1.1.0-preview.3
.After merge: