Adding touchscreen input source #1456
Conversation
…chscreen functionality, implementing BaseInputSource
…utSource This test scene leverages existing scripts (particularly TapResponder and InputTest) to demonstrate response to events raised from touchscreen interactions.
| /// </summary> | ||
| public class TouchscreenInputSource : BaseInputSource | ||
| { | ||
| const float kContactEpsilon = 2.0f/60.0f; |
There was a problem hiding this comment.
Ha! Coming from several years of VR and now AR dev, my mind tends to get stuck in 60fps mode ;)
| [Tooltip("Time in seconds to determine if the contact registers as a tap or a hold")] | ||
| protected float MaxTapContactTime = 0.5f; | ||
|
|
||
| private List<PersistentTouch> ActiveTouches; |
There was a problem hiding this comment.
you could initialize this field in line and remove the start method all together.
private List<PersistentTouch> ActiveTouches = new List<PersistentTouch>(0);
| foreach (Touch touch in Input.touches) | ||
| { | ||
| // Construct a ray from the current touch coordinates | ||
| Ray ray = Camera.main.ScreenPointToRay(touch.position); |
|
|
||
| #region Event generation logic | ||
|
|
||
| public bool UpdateTouch(Touch t, Ray r) |
There was a problem hiding this comment.
please update parameter names to touch and ray
|
|
||
| public bool UpdateTouch(Touch t, Ray r) | ||
| { | ||
| PersistentTouch knownTouch = (ActiveTouches.Find(item => item.touchData.fingerId == t.fingerId)); |
There was a problem hiding this comment.
Lets optimize this by removing the delegate allocation and just iterating through a for loop
| if (knownTouch != null) | ||
| return knownTouch.touchData; | ||
| else | ||
| return null; |
There was a problem hiding this comment.
nit; missing braces
| public Touch touchData; | ||
| public Ray ray; | ||
| public float lifetime; | ||
| public PersistentTouch(Touch t, Ray r) |
There was a problem hiding this comment.
nit: update names to touch and ray
| using Windows.Media.Capture; | ||
| #endif | ||
|
|
||
| namespace HoloToolkit.Unity |
There was a problem hiding this comment.
Why is this class included in your PR?
There was a problem hiding this comment.
I've changed the target branch for this PR
There was a problem hiding this comment.
Hmm. I only meant to include my two commits (the inputSource script, followed by the example scene). Indeed, I can only see my four files included my view of the PR..?
There was a problem hiding this comment.
No need to update the PR. I've already fixed this.
|
Many thanks for the quick feedback - happy to update all the above shortly. |
|
That's actually a good point. That Do we know when that data is updated? |
replaced list init in Start() with member-level definition and removed Start(); renamed abbreviated params to full names; renamed PersistentTouch.ray as PersistentTouch.screenpointRay for clarity and to avoid a this.ray in the constructor
…a simple for-loop & test
…r request; Updating example scene, which no longer needs its own explicit instance
|
I've now pushed most of the requested changes; I've held off on the change to the foreach-loop though as I thought you night be wavering on your original thought - just let me know if there's another reason to make the change :) |
There was a problem hiding this comment.
Looks great on paper! I'll be pulling this today to take a actual look.
While we're here and changing the input manager prefab, could you also rename the RaycastCamera GameObject under the event system to UIRaycastCamera? It'll fix #1444
|
Done. Cheers! |
|
@timGerken I'd like to get a review from you after your touchscreen work in Microsoft/GalaxyExplorer. |
|
@keveleigh , looks reasonable to me. GE does touch somewhat differently (It hooks gestures and events on the DXSwapChainPanel) and the forwards them onto GE's input router (from a way-way-way-back implementation of HTK). GE, in this regard, could use some love to bring it up to a more current HTK/MRTK implementation and move away from its reliance on starting as a XAML app to get at these events (and the app toolbar)... |
|
I hope that GE isn't going to delay this PR |
|
I can't imagine why you think it might. |
|
@NeerajW why remove label? |
…edRealityToolkit-Unity into Dev_Working_Branch # Conflicts: # Assets/HoloToolkit/Input/Prefabs/InputManager.prefab # # Resolved using 'Theirs'
|
Hi all - I saw a conflict had arisen on InputManager.prefab, so I just refreshed my fork and reapplied the changes made previously. @NeerajW Was there anything in particular that was keeping this PR in limbo? |
| @@ -4,4 +4,7 @@ | |||
| EditorBuildSettings: | |||
| m_ObjectHideFlags: 0 | |||
| serializedVersion: 2 | |||
| m_Scenes: [] | |||
There was a problem hiding this comment.
please revert this file
| @@ -28,11 +28,11 @@ SpriteAtlas: | |||
| enableTightPacking: 0 | |||
| packedHash: | |||
| serializedVersion: 2 | |||
| Hash: b579d9a28ce9cc7d8109d453a20bac11 | |||
| Hash: b7a1c327cf35f1b63159875306d23016 | |||
There was a problem hiding this comment.
please revert this file
|
facepalm |
| - component: {fileID: 4277215840702346} | ||
| - component: {fileID: 114080562701649166} | ||
| m_Layer: 0 | ||
| m_Name: TouchInputControl |
There was a problem hiding this comment.
Is this something we want by default on the InputManager prefab?
Does it / can it remove itself when run on devices without a touch screen?
There was a problem hiding this comment.
Hi @keveleigh - this was requested/added on Nov 29 (see earlier in this thread). I hadn't initially intended it to be present by default but other contributions have added other device handlers such as mouse and xbox controller, though to be fair I think they're event based rather than polling.
Happy to add a check for touchSupported at Start() and self-disable, if that would satisfy?
There was a problem hiding this comment.
Ohh interesting, I must have missed that!
Yeah, a check for touchSupported would be perfect, in my opinion
There was a problem hiding this comment.
If this is the case, should we also add TouchSupported in our SupportedInputInfo enum?
There was a problem hiding this comment.
SupportedInputInfo tells you the features of an input source that you can ask it about. From my understanding of this feature, there aren't any touch-specific features to query, as all events are routed through existing ones.
I see this like the Xbox controller script which routes tap/hold/navigation events through the system, where we didn't add "Xbox controller" to that enum.
There was a problem hiding this comment.
Good point. Maybe we should have?
There was a problem hiding this comment.
I don't think so.
I see "Xbox controller" and "touch screen" in the same category as "motion controller", which are input source types.
I see the SupportedInputInfo enum telling you about features you can query on the source itself, like the state of the menu button or the position of the input. As-is, this new source doesn't add any new features that a source can have, but it does add a new type of source that can be used.
The way I feel the Toolkit's InputManager is made is that it should be input source type agnostic. You can definitely reason about the source type based on the supported features, but you don't necessarily care where the events are coming from.
|
@keveleigh what's holding this up? Our question about if it should be on the input manager prefab? |
|
@StephenHodgson I'm fine with it staying on the prefab, but this seems like something that should remove/disable itself on non-touchscreen devices, so it isn't running unnecessarily. |
If we follow that logic, then shouldn't we make the same changes to the rest of the input sources? |
Maybe! I think it's a fine line to walk though, since this toolkit has been created specifically for Mixed Reality development. We don't want to hinder other platforms (via the |
|
Xbox controller for example |
|
Apologies for the silence; I was checking in infrequently over the vacation but am back at the office now. I'll commit the discussed change - to self-deactivate if unsupported - asap, which should finish off the PR? Updating the other sources could perhaps be discussed in a later issue? |
|
Was reading yesterday and thought we should just keep this in mind. No changes requested at this time.
|
Overview
Adding basic support for touchscreen devices with the addition of a new TouchscreenInputSource class. Includes an Example scene demonstrating functionality via pre-existing scripts.