CHANGE: Refactor class conditional guards #2183
Conversation
|
|
|
should add @ekcoh |
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2183 +/- ##
===========================================
+ Coverage 67.78% 67.79% +0.01%
===========================================
Files 367 367
Lines 53505 53505
===========================================
+ Hits 36268 36274 +6
+ Misses 17237 17231 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
@Pauliusd01 I'm adding you just to make sure there's no breakage. Going through the tests mentioned in the PR description should be enough |
| @@ -37,3 +37,54 @@ apiRules: | |||
| - exclude: | |||
| uidRegex: ^UnityEngine\.InputSystem\.InputSystem\.runInBackground$ | |||
| type: Member | |||
| - exclude: | |||
There was a problem hiding this comment.
I'm understanding this correctly: this means that these API won't be published? Any reason why? I'm just out of context probably.
There was a problem hiding this comment.
Hmm maybe I need to change this then. Besides the XRHMD devices, I started using this file to stop some xml errors I was getting from the yamato tests. I assumed the older define logic was hiding those same errors.
| // Docs generation is skipped because these are intended to be replaced with the com.unity.xr.oculus package. | ||
| #if UNITY_INPUT_SYSTEM_ENABLE_XR && (ENABLE_VR || UNITY_GAMECORE) && !DISABLE_BUILTIN_INPUT_SYSTEM_OCULUS && !UNITY_FORCE_INPUTSYSTEM_XR_OFF && !PACKAGE_DOCS_GENERATION |
There was a problem hiding this comment.
Thanks for removing this messy define. Are we ok with removing the UNITY_GAMECORE as well? I assume that was something Microsoft related?
There was a problem hiding this comment.
Yes we only added UNITY_GAMECORE to these places so that when you're on the gamecore platform these classes will be compiled. Since we are no longer wrapping entire classes they will always be compiled.
Description
Moves around some conditional guards so that we don't unnecessarily wrap entire classes. This stops downstream packages from having to also wrap Input System objects with similar conditionals.
Only device class definitions that get replaced by specific packages should be entirely wrapped.
This PR is a copy of the internal PR created here: https://github.cds.internal.unity3d.com/unity/com.unity.inputsystem/pull/5
Changes made
Removes
#if UNITY_INPUT_SYSTEM_ENABLE_XR && (ENABLE_VR || UNITY_GAMECORE) && !PACKAGE_DOCS_GENERATIONguards from wrapping entire class files under InputSystem/Plugins/XR/Devices and InputSystem/Plugins/XR/Haptics.
This makes it so any packages that want to use these Input System package objects don't have to also wrap the use of those objects with
#if ENABLE_VRguards. Classes that should ignore Package Generation have been added to the filter.yml file.We now only wrap the use of device descriptors with
#if UNITY_INPUT_SYSTEM_ENABLE_XRto prevent use when the XR Module package is not installed.Testing status & QA
Engines Supported: 6.2, 6.1, 6.0, 2022.3, 2021.3
Test 1:
Create project (or use an existing one)
Install Input System package
Install OpenXR package (this package uses the Input System but currently does not wrap any objects).
Switch platform from Standalone or Android into tvOS or QNX (ENABLE_VR conditional wont be defined on these platforms)
Expected result: No compiler errors
Test 2:
Create project (or use an existing one)
Install Input System package.
Install the com.unity.xr.oculus, com.unity.xr.openvr, and com.unity.xr.windowsmr packages (these packages override certain device class definitions).
Expected result: No compiler errors
Test 3:
Create project (or use an existing one)
Install Input System package.
Implement something that tries to leverage an XRHMD or XRController object
Switch platform to tvOS or QNX (ENABLE_VR conditional wont be defined on these platforms)
Execute code in Editor or in a build for the new platform.
Expected result: No runtime errors
Overall Product Risks
Please rate the potential complexity and halo effect from low to high for the reviewers. Note down potential risks to specific Editor branches if any.
Comments to reviewers
Please describe any additional information such as what to focus on, or historical info for the reviewers.
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.After merge: