Skip to content
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

Made NonNativeKeyboard touchable #7611

Merged
merged 4 commits into from Apr 14, 2020
Merged

Made NonNativeKeyboard touchable #7611

merged 4 commits into from Apr 14, 2020

Conversation

LocalJoost
Copy link
Contributor

Overview

This makes the nonnative keyboard prefab respond to touch events

Changes

Added a new behaviour NonNativeKeyboardTouchAssistant to the NonNativeKeyboard that enables touch when articulated hand support is detected. By request of @provencher after reading http://dotnetbyexample.blogspot.com/2020/04/migrating-to-mrtk2-using-non-native.html


namespace Microsoft.MixedReality.Toolkit.Experimental.UI
{
public class NonNativeKeyboardTouchAssistant : MonoBehaviour
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to make this script run at edit time. No need to add these components at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not like this a very heavy procedure and I am not quite a fan of modifying things at edit time. Messing with the internals of the keyboard changes the scene permanently, not sure if I would like that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my preference is to simply permanently modify the prefab, rather than having a modifier that adds it. I understand both perspectives though

Copy link
Contributor

@keveleigh keveleigh Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a downside to just updating the prefab to contain all the relevant NearInteractionTouchableUnityUI and audioclip components? And leaving them there regardless of if touch is supported? Especially if this script is already being added to the prefab by default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might not always want this, and this way it's easy to disable it.

var buttons = GetComponentsInChildren<Button>();
foreach (var button in buttons)
{
var ni = button.gameObject.AddComponent<NearInteractionTouchableUnityUI>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should maybe use EnsureComponent instead of Add. Add makes it so that these things might be duplicated.


private void EnableTouch()
{
_clickSoundPlayer = gameObject.AddComponent<AudioSource>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure component here too

Copy link
Contributor Author

@LocalJoost LocalJoost Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. That might mess up an existing AudioSource component and fill it with my sound file. This is intentional


namespace Microsoft.MixedReality.Toolkit.Experimental.UI
{
public class NonNativeKeyboardTouchAssistant : MonoBehaviour
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a summary and the AddComponentMenu attribute here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary, okay, but it's already on the prefab, as suggested by @provencher in the Slack channel. You are not going to add this thing to anything - at most you are going to disable it. At least, that was my idea

@david-c-kline
Copy link

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@david-c-kline david-c-kline merged commit 77c2e69 into microsoft:mrtk_development Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants