Input Updates #1028
Input Updates #1028
Conversation
| { | ||
| CurrentDisplayType = DisplayType.Transparent; | ||
| ApplySettingsForTransparentDisplay(); | ||
| if (OnDisplayDetected != null) | ||
| { | ||
| OnDisplayDetected(DisplayType.Transparent); | ||
| return; ; |
| } | ||
|
|
||
| /// <summary> | ||
| /// Sends the events for hand visibility changes & controller connect/disocnnect. | ||
| /// Sends the events for hand visibility changes & controller connect/disconnect. |
There was a problem hiding this comment.
nit: this ampersand is highlighted red for me on GitHub. I guess change it to an "and" to make it happy?
There was a problem hiding this comment.
I see it too, but I don't think it's anything to worry about. Just some formatting thing on Githubs side.
There was a problem hiding this comment.
@keveleigh I figured out why it's marked red. The & should be escaped to &
|
|
||
| namespace HoloToolkit.Unity.InputModule | ||
| { | ||
| public enum InteractionSourcePressInfo |
There was a problem hiding this comment.
I'm not yet sure how I feel about recreating Unity's enum here. It'll add code maintenance whenever the platform changes, and it's a breaking change for anybody using the existing event data classes. What do we gain over wrapping all references with UNITY_WSA?
There was a problem hiding this comment.
I'm not yet sure how I feel about recreating Unity's enum here.
It's not the first time the Input System does this.
it's a breaking change for anybody using the existing event data classes.
Well that's why it's a dev branch right? I'm more worried about forwards compatibility and how the rest of the community is going to have to deal with the new changes, because as it stands it's not a compatible change.
What do we gain over wrapping all references with UNITY_WSA?
All previous projects on the master branch gaining forward compatibility, especially ones where people use the input system in cross platform apps.
There was a problem hiding this comment.
I don't believe SupportedInputInfo is a Unity enum.
Yeah, forward compatibility makes sense for cross platform, though all apps will still need to add a press type to their existing events anyway.
Well that's why it's a dev branch right?
Now we're talking! Haha 😜
There was a problem hiding this comment.
😂😂😂
I don't believe SupportedInputInfo is a Unity enum.
Hmm, could be another I was thinking of?
all apps will still need to add a press type to their existing events anyway.
Why? Isn't there a default type that automatically works?
[Note]
My first attempt actually removed the press type from all events other than the interaction events that were specifically from the Interaction Manager.
I think you're right @DDReaper. That's def something we need to look into. |
…e it an object array.
|
@mrbobbybobberson could I get a review from you please sir? Esp for the tag update. |
| { | ||
| // Create input event | ||
| sourceClickedEventData.Initialize(source, sourceId, tag, pressType, tapCount); | ||
| sourceClickedEventData.Initialize(source, sourceId, tags, pressType, tapCount); |
There was a problem hiding this comment.
Should we move the tags parameter to the end of the statements that use it and make it null by default as well to keep it compatible with the master branch?
…truction of helper scripts if in Editor and occluded.
…e it an object array.
…truction of helper scripts if in Editor and occluded.
…enHodgson/HoloToolkit-Unity into MRTK-Dev2017.2-InputUpdate
|
@keveleigh did you have any other feedback for this PR? |
| #if !UNITY_EDITOR | ||
| Destroy(gameObject); | ||
| #else | ||
| if (Application.isEditor) |
There was a problem hiding this comment.
This check is backwards. This is the editor input script, so we want it in the editor
| } | ||
|
|
||
| /// <summary> | ||
| /// Sends the events for hand visibility changes & controller connect/disocnnect. | ||
| /// Sends the events for hand visibility changes & controller connect/disconnect. |
There was a problem hiding this comment.
Is & actually what you wanted here?
There was a problem hiding this comment.
Yeah #1028 (comment)
| @@ -3,7 +3,6 @@ | |||
|
|
|||
| using System; | |||
| using UnityEngine; | |||
| using UnityEngine.XR.WSA.Input; | |||
|
|
|||
| namespace HoloToolkit.Unity.InputModule | |||
There was a problem hiding this comment.
(Comment on the folder change) Interactions seems like it could be a base-level Input folder instead of inside Utilities. They're a pretty important part of input.
There was a problem hiding this comment.
I think scripts like this should really belong in the Examples or separate from the input system.
Don't get me wrong, they're important, but not part of the "Core" system. More of an implementation of it.
There was a problem hiding this comment.
I agree with them being separate from the input system (like has been mentioned in #1032), but I disagree with them going into Examples. Yeah, it's not quite the "core" system, but it's still a collection of generic scripts that easily add functionality to an app.
There was a problem hiding this comment.
That's why I moved it into Utilities
There was a problem hiding this comment.
I'm down for moving this again when we do the reorg for MRDL
|
@DDReaper You had a review from a while back, I'll wait til you can get a fresh look at this PR before merging. (Unless someone else beats you to a review.) |
|
Hey @StephenHodgson, sorry I didn't get a chance to take a look before the merge. I was out for a bit and still catching up. Yep, the tags change looks fine. I would have probably left it as just "object tag" and let the caller decide whether they wanted to put an objects array in or not, but it works fine as you changed it as well. If others end up wanting to use a single object, they can always add back another override that takes a single object to avoid having to allocate the array. Thanks for working on getting these changes and master to parity :). |
Fixes #875
Fixes #940
Fixes #1044
This PR is the first step to address the compatibility issues with the input system on the master branch, and gets the system into a state where we can start to back port some of the changes into master to help give developers way forward.