-
Notifications
You must be signed in to change notification settings - Fork 324
CHANGE: Refactor class conditional guards #2183
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
Changes from all commits
76cefa0
adb406a
99cd2d5
27b2800
bf80417
9c64247
dd2569b
893531c
06af488
1b146a4
87a0c2a
b844571
e482c47
0753c0c
715701f
7ca0212
f7562ef
657edfc
4b05bae
0d7b48f
b44ff31
8519aa3
b6f4d64
a9e7791
d33d4be
32e4e5e
1e30bea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,52 +1,71 @@ | ||
// ENABLE_VR is not defined on Game Core but the assembly is available with limited features when the XR module is enabled. | ||
#if UNITY_INPUT_SYSTEM_ENABLE_XR && (ENABLE_VR || UNITY_GAMECORE) && !UNITY_FORCE_INPUTSYSTEM_XR_OFF || PACKAGE_DOCS_GENERATION | ||
using UnityEngine.InputSystem.Controls; | ||
using UnityEngine.InputSystem.XR.Haptics; | ||
using UnityEngine.InputSystem.Layouts; | ||
using UnityEngine.XR; | ||
|
||
namespace UnityEngine.InputSystem.XR | ||
{ | ||
/// <summary> | ||
/// The base type of all XR head mounted displays. This can help organize shared behaviour across all HMDs. | ||
/// </summary> | ||
/// | ||
/// <remarks> | ||
/// | ||
/// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
/// { | ||
/// "name" : "MyHMD", | ||
/// "extend" : "HMD", | ||
/// "beforeRender" : "Update" | ||
/// } | ||
/// | ||
/// Then, make sure you put extra `StateEvents` for your HMD on the queue right in time before rendering. Also, if your HMD is a combination of non-tracking and tracking controls, you can update just the tracking by sending a delta event instead of a full state event. | ||
/// | ||
/// </remarks> | ||
[InputControlLayout(isGenericTypeOfDevice = true, displayName = "XR HMD", canRunInBackground = true)] | ||
public class XRHMD : TrackedDevice | ||
{ | ||
/// <summary> | ||
/// The base type of all XR head mounted displays. This can help organize shared behaviour across all HMDs. | ||
/// Accessor for left eye position. | ||
/// </summary> | ||
/// | ||
/// <remarks> | ||
/// | ||
/// To give your head tracking an extra update before rendering: | ||
/// First, enable before render updates on your Device. | ||
/// | ||
/// <sample> | ||
/// <code> | ||
/// // JSON | ||
/// { | ||
/// "name" : "MyHMD", | ||
/// "extend" : "HMD", | ||
/// "beforeRender" : "Update" | ||
/// } | ||
/// </code> | ||
/// </sample> | ||
/// | ||
/// Then, make sure you put extra `StateEvents` for your HMD on the queue right in time before rendering. Also, if your HMD is a combination of non-tracking and tracking controls, you can update just the tracking by sending a delta event instead of a full state event. | ||
/// | ||
/// </remarks> | ||
|
||
[InputControl(noisy = true)] | ||
public Vector3Control leftEyePosition { get; protected set; } | ||
|
||
/// <summary> | ||
/// Accessor for left eye rotation. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure, but I suspect this might cause package violation due to length (short) and missing code example. Similar for properties below. |
||
/// </summary> | ||
[InputControl(noisy = true)] | ||
public QuaternionControl leftEyeRotation { get; protected set; } | ||
|
||
/// <summary> | ||
/// Accessor for right eye position. | ||
/// </summary> | ||
[InputControl(noisy = true)] | ||
public Vector3Control rightEyePosition { get; protected set; } | ||
|
||
/// <summary> | ||
/// Accessor for right eye rotation. | ||
/// </summary> | ||
[InputControl(noisy = true)] | ||
public QuaternionControl rightEyeRotation { get; protected set; } | ||
|
||
/// <summary> | ||
/// Accessor for center eye position. | ||
/// </summary> | ||
[InputControl(noisy = true)] | ||
public Vector3Control centerEyePosition { get; protected set; } | ||
|
||
/// <summary> | ||
/// Accessor for center eye rotation. | ||
/// </summary> | ||
[InputControl(noisy = true)] | ||
public QuaternionControl centerEyeRotation { get; protected set; } | ||
|
||
/// <summary> | ||
/// Override for FinishSetup(). | ||
/// </summary> | ||
protected override void FinishSetup() | ||
{ | ||
base.FinishSetup(); | ||
|
@@ -72,22 +91,12 @@ 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 'LeftHand' device usage. | ||
/// <example> | ||
/// <code> | ||
/// // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason the example and code tags was removed from this? |
||
/// the left-hand XR controller: | ||
/// var action = new InputAction(binding: "/<XRController>{leftHand}/position"); | ||
/// </code> | ||
/// </example> | ||
/// | ||
/// <example> | ||
/// <code> | ||
/// // To make the left-hand XR controller behave like the right-hand one | ||
/// To make the left-hand XR controller behave like the right-hand one | ||
/// var controller = XRController.leftHand; | ||
/// InputSystem.SetUsage(controller, CommonUsages.RightHand); | ||
/// </code> | ||
/// </example> | ||
/// </remarks> | ||
public static XRController leftHand => InputSystem.GetDevice<XRController>(CommonUsages.LeftHand); | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its clear from line 112 its an override for FinishSetup. I would suggest:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...since it doesn't (it cannot) change the contract of the API function it should just inherit the base documentation. |
||
/// </summary> | ||
protected override void FinishSetup() | ||
{ | ||
base.FinishSetup(); | ||
|
||
#if UNITY_INPUT_SYSTEM_ENABLE_XR | ||
var capabilities = description.capabilities; | ||
var deviceDescriptor = XRDeviceDescriptor.FromJson(capabilities); | ||
|
||
|
@@ -111,6 +123,7 @@ protected override void FinishSetup() | |
else if ((deviceDescriptor.characteristics & InputDeviceCharacteristics.Right) != 0) | ||
InputSystem.SetDeviceUsage(this, CommonUsages.RightHand); | ||
} | ||
#endif | ||
} | ||
} | ||
|
||
|
@@ -119,11 +132,15 @@ protected override void FinishSetup() | |
/// </summary> | ||
public class XRControllerWithRumble : XRController | ||
{ | ||
/// <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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would recommend specifying expected or valid range for this amplitude to make it easier for users. |
||
/// <param name="duration"> Duration of the impulse.</param> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar here, what is acceptable, is zero ok? Is negative duration ok? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like it mightn't be checked before allocating for command and sending down to native |
||
public void SendImpulse(float amplitude, float duration) | ||
{ | ||
var command = SendHapticImpulseCommand.Create(0, amplitude, duration); | ||
ExecuteCommand(ref command); | ||
} | ||
} | ||
} | ||
#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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.