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 53528 53528
===========================================
+ Hits 36285 36291 +6
+ Misses 17243 17237 -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.
There was a problem hiding this comment.
Nice to see simplifications of compile time symbols which this code base already has too many off. I am not sure about the historical reason why XR defies are conditionally compiled at all since this is not done for e.g. Keyboards, Mice, Gamepads, Touchpads or other device types. Maybe you can remind me why we need to conditionally compile these at all? I suspect it is to remove them when no runtime backing them up is present, but generally I wonder if the API should be dynamic like this since it adds complexity and compile time burden.
| /// To give your head tracking an extra update before rendering: | ||
| /// First, enable before render updates on your Device. | ||
| /// | ||
| /// // JSON |
There was a problem hiding this comment.
I see this wasn't introduced on this PR (rather moved), but it's not clear what JSON this is referring to which might not be helpful for users consuming this information. I would recommend (while moving this) to add clarity around what JSON is expected to be modified accordingly.
| [InputControl(noisy = true)] | ||
| public Vector3Control leftEyePosition { get; protected set; } | ||
|
|
||
| /// <summary> | ||
| /// Accessor for left eye rotation. |
There was a problem hiding this comment.
Not sure, but I suspect this might cause package violation due to length (short) and missing code example. Similar for properties below.
| /// // To set up an Action to specifically target | ||
| /// // the left-hand XR controller: | ||
| /// | ||
| /// To set up an Action to specifically target |
There was a problem hiding this comment.
Any reason the example and code tags was removed from this?
| @@ -97,10 +106,13 @@ public class XRController : TrackedDevice | |||
| /// <remarks>If there is no left hand connected, this will be null. This also matches any currently tracked device that contains the 'RightHand' device usage.</remarks> | |||
| public static XRController rightHand => InputSystem.GetDevice<XRController>(CommonUsages.RightHand); | |||
|
|
|||
| /// <summary> | |||
| /// Override for FinishSetup(). | |||
There was a problem hiding this comment.
Its clear from line 112 its an override for FinishSetup. I would suggest:
<inheritdoc />
There was a problem hiding this comment.
...since it doesn't (it cannot) change the contract of the API function it should just inherit the base documentation.
| /// <summary> | ||
| /// Sends an impulse command with the given amplitude and duration. | ||
| /// </summary> | ||
| /// <param name="amplitude"> Amplitude of the impulse.</param> |
There was a problem hiding this comment.
I would recommend specifying expected or valid range for this amplitude to make it easier for users.
| /// Sends an impulse command with the given amplitude and duration. | ||
| /// </summary> | ||
| /// <param name="amplitude"> Amplitude of the impulse.</param> | ||
| /// <param name="duration"> Duration of the impulse.</param> |
There was a problem hiding this comment.
Similar here, what is acceptable, is zero ok? Is negative duration ok?
There was a problem hiding this comment.
Looks like it mightn't be checked before allocating for command and sending down to native
Thanks for your review. The problem is that we should have never wrapped entire classes conditionally because this causes every package that uses the Input System to also implement conditionals unnecessarily. We should only wrap necessary types that depend on the XR Module package or for calls that may try to invoke native functions from the XR Module packages. Regarding the rest of your comments, all the comment changes in this PR were to stop Yamato XML errors that were previously hidden. At this point I would very much like this PR to get merged and the Input Team to come back and update any comments that aren't sufficient |
There was a problem hiding this comment.
I realise the main symbol define is dependent on the presence of the XR module being enabled (as stated in description). Since this is more of a refactor than changing anything (apart from simplifying) I think this is an improvement, thanks for doing it @jdiehlUnity. I will approve these changes but suspect that some documentation generating package verification errors might be present (See specific comments).
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: