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

3D Keyboard #765

Merged
merged 33 commits into from
Jul 18, 2017
Merged

3D Keyboard #765

merged 33 commits into from
Jul 18, 2017

Conversation

paseb
Copy link

@paseb paseb commented Jul 10, 2017

This is the added 3D keyboard for text entry. The keyboard and KeyboardInputField support different styles of keyboard to be targeted. Currently there is a bug with getting HoloLens Input Module to not create double input (https://forums.hololens.com/discussion/6999/ui-button-on-click-event-firing-twice).

paseb and others added 23 commits April 5, 2017 12:12
[~] Updated SymbolDisableHighlight to also take an image
Merging changes from upstream HTK into fork for keyboard
Merging changes from HoloToolkit master
… carat.

[~] Still need to resolve double input issue and overlap with input modules
[^] Updated scene and moved to under the tests directory
@paseb paseb mentioned this pull request Jul 11, 2017
Copy link
Contributor

@StephenHodgson StephenHodgson left a comment

Choose a reason for hiding this comment

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

Please revert Project/Graphics settings.

@@ -39,9 +39,21 @@ GraphicsSettings:
m_PreloadedShaders: []
m_SpritesDefaultMaterial: {fileID: 10754, guid: 0000000000000000f000000000000000,
type: 0}
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks broken

Copy link
Author

Choose a reason for hiding this comment

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

What's the best way to revert? I thought I had but obviously not. D'oh!

thanks,
-pat

iOSRenderExtraFrameOnPause: 1
>>>>>>> ea6c94daea2cfe3366daef3abbd09d429b0f439e
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

m_EditorVersion: 5.6.2f1
>>>>>>> e884de114b8d267c45b534bab5894651c8ff4d90
Copy link
Contributor

Choose a reason for hiding this comment

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

Broken

@paseb
Copy link
Author

paseb commented Jul 11, 2017

Settings should all now be reverted.

@paseb paseb closed this Jul 11, 2017
@paseb paseb reopened this Jul 11, 2017
Copy link
Contributor

@StephenHodgson StephenHodgson left a comment

Choose a reason for hiding this comment

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

Just a few comments

[HideInInspector]
public Vector3 TargetPoint;

float GetAxis(Vector3 v)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: explicit access modifiers. (Is this public or private?)

/// <summary>
/// Use late update to track the input slider
/// </summary>
public void LateUpdate()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: All the Unity API methods should be private, not public.

/// </summary>
private Keyboard m_Keyboard;


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra line space

private void Start()
{
m_Keyboard = this.GetComponentInParent<Keyboard>();
this.UpdateState();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this. is implied, we can safely remove it.


#if UNITY_EDITOR
[CustomEditor(typeof(KeyboardInputField))]
public class KeyboardInputFieldEditor : Editor
Copy link
Contributor

@StephenHodgson StephenHodgson Jul 11, 2017

Choose a reason for hiding this comment

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

This should be broken out into it's own file, in an Editor folder.

/// The color to switch to when the button is disabled.
/// </summary>
[SerializeField]
private Color m_DisabledColor = Color.grey;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: tab formatting


this.UpdateState();


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra spaces

/// </summary>
public List<RectTransform> Items { get; private set; }

void Awake()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: explicit private access modifier for Unity APIs

float columnHeight = 0.0f;
float maxPanelWidth = 0.0f;

foreach (RectTransform item in Items)
Copy link
Contributor

@StephenHodgson StephenHodgson Jul 11, 2017

Choose a reason for hiding this comment

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

We could switch to a for seeing as this is in an update loop.

// Ensure the anchors and pivot are set properly for positioning in the UICollection
item.anchorMin = new Vector2(0.0f, 1.0f);
item.anchorMax = new Vector2(0.0f, 1.0f);
item.pivot = new Vector2(0.0f, 1.0f);
Copy link
Contributor

Choose a reason for hiding this comment

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

We could cache this vector allocation as well bc of the loop.

/// <summary>
/// Initialize key text, subscribe to the onClick event, and subscribe to keyboard shift event.
/// </summary>
void Start()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: private

UpdateLayout();
}

private void _AddItem(RectTransform item)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this method have an underscore prefix?


protected virtual void UpdateLayout()
{
RectTransform rectTransform = GetComponent<RectTransform>();
Copy link
Contributor

@StephenHodgson StephenHodgson Jul 11, 2017

Choose a reason for hiding this comment

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

Nit: we should cache this component as a private field in awake or start.

UpdateLayout();
}

void Update()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: private

@paseb
Copy link
Author

paseb commented Jul 13, 2017

Hi @StephenHodgson,

I should have addressed all our comments with what's currently committed. Please let me know if you see anything else. Also let me know if you have any input on the keyboard itself if you've had the chance to test it.

thanks,
-pat

@StephenHodgson
Copy link
Contributor

Hey Pat, sorry I won't be able to have the chance to test until later today.

@paseb
Copy link
Author

paseb commented Jul 13, 2017

No problem, I just wanted to make sure I didn't miss anything :). Thanks Stephen!

Copy link

@NeerajW NeerajW left a comment

Choose a reason for hiding this comment

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

This is just awesome :)

{
public enum LayoutType
{
Alpha,
Copy link

Choose a reason for hiding this comment

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

Should these have some description?

Copy link

Choose a reason for hiding this comment

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

Oh I see it in the variable names below.

Items.Add(item);

item.SetParent(transform);
item.transform.localScale = new Vector3(1.0f, 1.0f, 1.0f);
Copy link

Choose a reason for hiding this comment

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

NIT: Vector3.one

@cre8ivepark
Copy link
Contributor

Awesome! Once it is merged, I'll do visual fit & finish pass and look into the possibility of adding Japanese Keyboard too. (#735)

@NeerajW
Copy link

NeerajW commented Jul 17, 2017

@StephenHodgson will wait on you to approve and then merge.

@StephenHodgson StephenHodgson merged commit 9ede366 into microsoft:master Jul 18, 2017
keveleigh pushed a commit to keveleigh/HoloToolkit-Unity that referenced this pull request Aug 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants