-
Notifications
You must be signed in to change notification settings - Fork 111
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
Makes non native keyboard touch enabled #655
base: main
Are you sure you want to change the base?
Makes non native keyboard touch enabled #655
Conversation
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.
Thank for you changes. I'm made some change requests. Also, please link to an issue and add unit tests.
@keveleigh and @marlenaklein-msft ... can you please review as well.
org.mixedrealitytoolkit.uxcore/Experimental/NonNativeKeyboard/NonNativeKeyTouchAdapter.cs
Show resolved
Hide resolved
org.mixedrealitytoolkit.uxcore/Experimental/NonNativeKeyboard/NonNativeKeyTouchAdapter.cs
Outdated
Show resolved
Hide resolved
org.mixedrealitytoolkit.uxcore/Experimental/NonNativeKeyboard/NonNativeKeyTouchAdapter.cs
Show resolved
Hide resolved
org.mixedrealitytoolkit.uxcore/Experimental/NonNativeKeyboard/NonNativeKeyTouchAdapter.cs
Show resolved
Hide resolved
org.mixedrealitytoolkit.uxcore/Experimental/NonNativeKeyboard/NonNativeKeyTouchAdapter.cs
Show resolved
Hide resolved
org.mixedrealitytoolkit.spatialmanipulation/Solvers/RadialViewDisplayInitializeHelper.cs
Outdated
Show resolved
Hide resolved
org.mixedrealitytoolkit.spatialmanipulation/Solvers/RadialViewDisplayInitializeHelper.cs
Show resolved
Hide resolved
org.mixedrealitytoolkit.spatialmanipulation/Solvers/RadialViewDisplayInitializeHelper.cs
Show resolved
Hide resolved
...realitytoolkit.uxcomponents/Experimental/NonNativeKeyboard/TouchableNonNativeKeyboard.prefab
Show resolved
Hide resolved
org.mixedrealitytoolkit.uxcore/Experimental/NonNativeKeyboard/NonNativeKeyTouchAdapter.cs
Show resolved
Hide resolved
I don't understand "please link to an issue". There is no issue, this is more or less new functionality. Or actually, functionality that got lost when the NonNativeKeyboard got ported from MRTK2. I thought it was fun to bring it back. :) |
|
||
private void OnEnable() | ||
{ | ||
var distance = (radialView.MinDistance + radialView.MaxDistance) / 2.0f; |
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 wonder if it would make more sense to include this as a configurable behavior on RadialView instead of adding an additional script. I.E. adding a serialized field CenterInitialPosition, and if that is true then making this adjustment.
That would also allow SolverHandler.TransformTarget to be used in place of Camera.main.transform
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.
That is a possibility. I was trying to mess as little as possible with existing code. :)
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.
@AMollis do you have a preference here?
So I think I fixed all I could fix, and left 5 items unresolved because I think they are open to discussion, and I think only you can resolve them (or send me back to the drawing board). What else do you need me to do? |
@AMollis @marlenaklein-msft so I addressed all the points I think should be addressed, and added two unit test, one actually testing a hand pressing the "j" actually resulting in a "j" appearing in the keyboard preview. I hope this satisfies your needs. |
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 thought about this some more. Is building a "touch adapter" the right thing here?
Back when we added this component, we decided to first release a "flat canvas keyboard" similar to the old MRTK2 one, so we had a starting point. We would then later update the keyboard with "pressable buttons".
That is, instead of creating and adding a touch adapter, can we just use XRInteractables instead. Like PressableButton?
Sidenote, the XRI's poke interactor works with UGUI canvas elements. That is, XRI's poke interactor can "touch" UGUI buttons. Ideally MRTK3's poke interactor would also work with UGUI buttons, however doing so requires access to an internal XRI type UIInteractionType
.
/// <summary>
/// Defines the type of interaction expected from the interactor when interacting with UI.
/// </summary>
/// <seealso cref="TrackedDeviceModel.interactionType"/>
internal enum UIInteractionType
{
/// <summary>
/// The UI interaction is a ray interaction.
/// </summary>
Ray,
/// <summary>
/// The UI interaction is a poke interaction.
/// </summary>
Poke,
}
In the next major version, I'd like to get rid of MRTK3's Poke Pointer, and use XRI's. This would unlock UGUI touch interactions
This is a bit confusing to me. It was my intention to create a simple, small add-on to make the existing non-native keyboard work with HoloLens 2 as it is now not very usable, or at least clashing quite heavily with the usage pattern. And I needed it for my own app, so I made it to work and thought it nice to add it so the MRTK3 itself. It seems now that you want to go in a complete different direction, or maybe even two ;) This works now, with very little impact, maybe we can do the complete redesign later - because I suppose that is not high on the prio list. Do you rather want a redesign of the keyboard? Am I correct in understanding that you don't want to merge this at all? I can release it as an add-on package and keep it outside of the MRTK3, if you think that is a better idea. Or people can just pull the code from my blog ;) that happens a lot. |
Two keyboard options.
It sounds like you prefer 1. While I prefer 2. If we wanted to continue with 1, I would much prefer getting the MRTK Poke Interactor to work with all UGUI Buttons, without the need of colliders, similar to how the XRI Poke Interactor works. I've pinged the XRI folks to see how we can make MRTK's poke interactor work with UGUI Buttons. I'll follow-up here.... |
I don't prefer so much one or the other, I prefer something that works, without much work, and that works now. Yes, I can rebuild the keyboard using actual MRTK3 PressableButtons (which are, incidentally child classes of the StatefulInteractable that I use). But then I would have to add all the graphics parts as well - so, in fact, replace all the entries in the keyboard with Action Button prefabs. Manually, as the keys are now no prefabs (like I said before), I would basically have to build the whole keyboard from the ground up. Key by key. Definitely doable, but a lot of work for something that I don't think a lot of people use. So I took a piece of code to do the trick. WAY less work and leaves current functionality intact. And the Action Button, incidentally, uses a BoxCollider as well. Where do you think I got the idea in the first place? Great artists steal ;) Also, apparently the alternative way you suggest requires a change to an external (Unity) package while mine just uses what is readily available. Today. |
Makes non native keyboard touch enabled, as described in https://localjoost.github.io/Using-the-non-native-keyboard-in-touch-scenarios-in-MRTK3/