Added SpeechInputSource for integration of the KeywordRecognizer in the InputManager #354

aalmada · 2016-11-30T16:11:18Z

KeywordManager implementation as an input source. This makes it coherent with the new InputManager infrastructure.

The KeywordManager test scene was changed to show 3 spheres. Only the sphere with focus changes color. A global "reset all" voice command changes all spheres to grey even with no sphere in focus.

…event pipeline.

…object and also a global listener.

…ada/HoloToolkit-Unity into KeywordManagerAsInputSource # Conflicts: # Assets/HoloToolkit/Input/Tests/Scenes/KeywordManager.unity

…HoloToolkit-Unity into KeywordManagerAsInputSource

msftclas · 2016-11-30T16:11:26Z

Hi @aalmada, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

aalmada · 2016-12-01T12:51:59Z

@maxouellet This is the way I found to extend InputManager although I expressed before some reservations on having to change IInputSource and InputManager every time we integrate a new form of input. Is this what you had in mind?

maxouellet · 2016-12-01T17:28:00Z

Assets/HoloToolkit/Input/Scripts/InputSources/InputSourceEventArgs.cs

+            SemanticMeanings = semanticMeanings;
+            Text = text;
+        }
+    }


The PhraseRecognizedEventArgs class should be in its own file.

maxouellet · 2016-12-01T17:30:18Z

Assets/HoloToolkit/Input/Scripts/Voice/KeywordManager.cs

@@ -19,17 +21,15 @@ namespace HoloToolkit.Unity.InputModule
    /// Edit -> Project Settings -> Player -> Settings for Windows Store -> Publishing Settings -> Capabilities
    /// or in your Visual Studio Package.appxmanifest capabilities.
    /// </summary>
-    public partial class KeywordManager : MonoBehaviour
+    public partial class KeywordManager : BaseInputSource


Can we create a new class (SpeechInputSource) instead of modifying the existing Keyword manager? Then, if it works well, we could simply eventually delete the old KeywordManager.

maxouellet · 2016-12-01T17:30:46Z

Assets/HoloToolkit/Input/Scripts/Voice/KeywordManager.cs

-                // This helps easily link the keyword recognized to the UnityEvent to be invoked.
-                responses = KeywordsAndResponses.ToDictionary(keywordAndResponse => keywordAndResponse.Keyword,
-                                                              keywordAndResponse => keywordAndResponse.Response);
+                return SupportedInputEvents.SourceUpAndDown;


You probably need to add a new type of SupportedInputEvents (SpeechKeyword?) and change this accordingly

maxouellet · 2016-12-01T17:33:55Z

Assets/HoloToolkit/Input/Scripts/Voice/KeywordManager.cs

@@ -130,5 +132,31 @@ public void StopKeywordRecognizer()
                keywordRecognizer.Stop();
            }
        }
+
+        private void OnPhraseRecognized(UnityEngine.Windows.Speech.ConfidenceLevel confidence,
+            TimeSpan phraseDuration, DateTime phraseStartTime,


NIT: I suggest putting all input params on the same line, or all on separate line.

Also, why I'd suggest calling this method from KeywordRecognizer_OnPhraseRecognized, so that we only have one entry point to RaisePhraseRecognizedEvent

maxouellet · 2016-12-01T17:37:44Z

Assets/HoloToolkit/Input/Scripts/InputSources/IInputSource.cs

@@ -119,6 +119,11 @@ public interface IInputSource
        event EventHandler<NavigationEventArgs> NavigationCanceled;

        /// <summary>
+        /// Event triggered when a speech phrase is recognized.
+        /// </summary>
+        event EventHandler<PhraseRecognizedEventArgs> PhraseRecognized;


From a naming perspective, I wonder if this should be something like SpeechKeywordRecognizedEventArgs, or something else. I don't have a problem with Phrase, just bringing up the discussion in case somebody has a strong opinion on it.

This is the UnityEngine.Windows.Speech naming but I also don't like it much.

Yeah, I'm OK with leaving like that for now unless someone comes up with a better name. Makes sense to align with existing name for things.

@maxouellet I just noticed that changing to SpeechKeywordRecognizedEventArgs avoids having to use explicit namespaces because of name clashing. I would then change the IInputSource event to SpeechKeywordRecognized. What do you think?

That works for me, I agree that having to use explicit namespaces is annoying.

maxouellet · 2016-12-01T17:46:28Z

@aalmada Yeah, this is similar to what I had in mind. This looks pretty good to me, other than the small comments I added. The main thing from my perspective is that I think we should make a clean break from KeywordManager, and create a new SpeechInputSource. That will prevent breaking people that might be using KeywordManager (and we can phase it out eventually, as we "fix" the various test scenes).

I understand the concern around modifying the interface + InputManager every time you add something new, but then again, I don't think that adding something new is something that will happen very frequently. Once we've nailed most types of inputs, we might add information in the event data classes, but that wouldn't require a change to IInputSource or InputManager. I also don't have a better, cleaner and easy-to-understand solution to the problem.

StephenHodgson · 2016-12-01T19:41:42Z

This should be resubmitted on master, now that we've merged the branches.

StephenHodgson · 2016-12-01T19:43:35Z

Assets/HoloToolkit/Input/Scripts/InputSources/InputSourceEventArgs.cs

+        /// </summary>
+        public string Text { get; private set; }
+
+        public PhraseRecognizedEventArgs(IInputSource inputSource, uint sourceId,


Nit: formatting

StephenHodgson · 2016-12-01T19:44:08Z

Assets/HoloToolkit/Input/Scripts/InputSources/InputSourceEventArgs.cs

+        /// <summary>
+        /// The text that was recognized.
+        /// </summary>
+        public string Text { get; private set; }


Nit: more descriptive name than just Text. Could be ambiguous if used improperly. suggested name: RecognizedText

I replicated the names from PhraseRecognizedEventArgs but I can change it.

StephenHodgson · 2016-12-01T19:45:12Z

Assets/HoloToolkit/Input/Scripts/Voice/KeywordManager.cs

@@ -6,6 +6,8 @@
 using UnityEngine;
 using UnityEngine.Events;
 using UnityEngine.Windows.Speech;
+using UnityEngine.VR.WSA.Input;
+using System;


Nit: System should be first.

StephenHodgson · 2016-12-01T19:47:14Z

Assets/HoloToolkit/Input/Tests/Scripts/DisplayKeywords.cs

@@ -20,7 +20,7 @@ void Start()
            }

            textPanel.text = "Try saying:\n";
-            foreach (KeywordManager.KeywordAndResponse k in keywordManager.KeywordsAndResponses)
+            foreach (KeywordManager.KeywordAndKeyCode k in keywordManager.KeywordsAndKeys)


Nit: I know this is like this in the old version but I think we should rename k to be key

StephenHodgson · 2016-12-01T19:48:01Z

Assets/HoloToolkit/Input/Tests/Scripts/SphereGlobalKeywords.cs

+
+        public void OnPhraseRecognized(PhraseRecognizedEventData eventData)
+        {
+            switch (eventData.Text.ToLower())


Why use switch instead of if statement?

readability. under the covers, switch on a string generates the equivalent IL that if/else-if blocks would up to a "magic number" of cases at which time it uses a hashtable for better performance.

Great explanation Tim!

I used the existing SphereKeyword.cs as a reference and it already uses a switch.

keveleigh · 2016-12-01T19:50:02Z

Assets/HoloToolkit/Input/Scripts/Voice/KeywordManager.cs

        {
            [Tooltip("The keyword to recognize.")]
            public string Keyword;
            [Tooltip("The KeyCode to recognize.")]
            public KeyCode KeyCode;
-            [Tooltip("The UnityEvent to be invoked when the keyword is recognized.")]
-            public UnityEvent Response;


This UnityEvent is basically the original point of the KeywordManager. It allows a developer to assign an action in the Inspector, instead of having to worry about the KeywordRecognizer's implementation in code.

There's another comment in here about leaving KeywordManager as is and creating a new class, which I support as well. If you're able to migrate this without losing functionality, then I'd support using KeywordManager.

The propose of this PR is to use the InputManager handling mechanisms so I thought the UnityEvent wouldn't make sense anymore.
Anyway, I'm going to follow the suggestion of creating a new SpeechInputSource and leave the KeywordManager like it was.

…ada/HoloToolkit-Unity into KeywordManagerAsInputSource # Conflicts: # Assets/HoloToolkit/Input/Tests/Scenes/SpeechInputSource.unity

aalmada · 2016-12-15T20:00:52Z

I'll work on this tomorrow.

StephenHodgson · 2016-12-15T20:01:53Z

No worries, great work so far!

…ut leaks.

aalmada · 2016-12-16T09:49:15Z

@HodgsonSDAS I did what you suggested to the shader and it works great for this case. Note that when using [PerRendererData] the Color property isn’t visible anymore in the material editor – per renderer properties can only be changed by script.
I also looked into the other scripts reported in #236 but there's an issue. SelectedObjectMessageReceiver.cs keeps the original color on Start() but MaterialPropertyBlock doesn't have a GetColor().

StephenHodgson · 2016-12-16T14:07:33Z

@aalmada, Sorry, I didn't realize doing that would cause the color property to disappear from the inspector window, but it is the best way to change colors per renderer without the memory leaks discussed earlier. This may also require a custom material inspector editor script or a Custom property drawer as well to get the color property to show in the inspector again.

It's also important to note that even though all objects share the same material, MaterialPropertyBlocks break dynamic batching.

If you feel like this color stuff is too much or doesn't exactly relate to this PR I'm down for just skipping it for now.

StephenHodgson · 2016-12-16T14:08:32Z

Assets/HoloToolkit/Utilities/Materials/HoloToolkit_Default.mat

@@ -154,7 +154,7 @@ Material:
    m_Colors:
    - first:
        name: _Color
-      second: {r: 0.8897059, g: 0.8897059, b: 0.8897059, a: 1}


We should probably revert this back.

StephenHodgson · 2016-12-16T14:19:59Z

Assets/HoloToolkit/Input/Tests/Scripts/SphereGlobalKeywords.cs

+            switch (eventData.RecognizedText.ToLower())
+            {
+                case "reset all":
+                    foreach (Renderer renderer in GetComponentsInChildren<Renderer>())


It's probably best to cache all the renderers here, and be sure to check/update the cache if children are added/removed. (forgive the brevity)

private MaterialPropertyBlock propertyBlock; private Renderer[] childRenderCache; private void Start() { propertyBlock = new MaterialPropertyBlock(); childrenRenderCache = GetComponentsInChildren<Renderer>() } public void OnSpeechKeywordRecognized(SpeechKeywordRecognixedEventData eventData) { switch(keywords) { case "reset all": for(int i=0; i < childrenRenderCache.Length; i++) { ChangeColor(childrenRenderCache[i], Color.white); { break; } } private void ChangeColor(Renderer, Color color) { // Do Color stuff }

Can this performance tweak be left for after this massive PR is reviewed and merged?

StephenHodgson · 2016-12-16T14:44:17Z

Assets/HoloToolkit/Input/Scripts/Voice/SpeechInputSource.cs

+    /// Edit -> Project Settings -> Player -> Settings for Windows Store -> Publishing Settings -> Capabilities
+    /// or in your Visual Studio Package.appxmanifest capabilities.
+    /// </summary>
+    public partial class SpeechInputSource : BaseInputSource


Will this be replacing KeywordManager.cs? (If so should we be removing it/Depreciating it to give others time to migrate over?)

Or does this only exist on the object that will be executing the logic when the keyword is spoken?

@HodgsonSDAS In the new InputManager architecture, event sources register with it. It then looks for a message handler in the object with the focus. If not there, it goes up the object hierarchy. It also supports global handlers and fallback handlers.
The SpeechInputSource is only the source that could replace the KeywordManager. I currently only support handlers for when the keyword is spoken as scripts. I'm prototyping support for inspector.
If this PR results in a better solution than the KeywordsManager, it would be nice to turn it obsolete as having both can be confusing to users.
It's also my fault but I've been trying to clear all reviews and this PR is loosing some focus.

No worries, yeah I agree we should skip the other review content and get back to it later.
I don't mind trying to take a look at updating the material color changes in a separate PR

I think the only thing blocking this PR is the ability to assign the GameObject and script method to trigger with each of the keywords without hard coding it in a class.

I think the only way to really do this is through a UnityEvent.

keveleigh · 2016-12-16T16:30:18Z

I don't consider Inspector compatibility to be blocking at this point, since that's one of the reasons we requested these changes be added to something new instead of the KeywordManager. I also don't think these material changes (adding custom Inspector components and new scripts just for a test scene, etc) are necessary in this case, as it's a one off test script and, if we have issues because of one extra material instance, well...haha. or, if there's only one sphere in the scene, just use sharedMaterial.

StephenHodgson · 2016-12-16T17:42:08Z

While I agree with skipping the material issue, the inspector compatibility might be more of a sticking point for some. If the SpeechInputSource class is intended to replace the KeywordManger, then we should keep similar functionality, like the assignment of GameObjects and functions they call when the keyword is triggered. This will also help with decoupling code from the toolkit.

keveleigh · 2016-12-16T19:44:05Z

Definitely agreed on that (it's a sticking point for me as far as replacing KeywordManager completely #354 (comment) 😃)! If we want to add that functionality in now, I'm all for it. I'm just not against adding this side-by-side without it for now, with it potentially being added later and completing the KeywordManager replacement.

StephenHodgson · 2016-12-16T20:08:11Z

Ahh, my apologies, I must have missed those comments somehow.

Well I'm down for keeping the KeywordManager for now and marking it obsolete, then removing it altogether once we implement the inspector window assignments in the SpeechInputSource.

keveleigh · 2016-12-16T20:14:41Z

All good! I felt like we were more or less on the same page; just wanted to be sure :)

aalmada · 2016-12-17T14:59:12Z

@HodgsonSDAS @keveleigh Please note that all the InputManager handlers are still scripts. I started working on inspector support for the handler but I'd rather do it in a future PR. It raises more questions than the source itself.
@maxouellet I added a SetGlobalListener.cs. It's a one liner script that registers the GameObject with the InputManager as a global listener. This adds editor support for this operation. It makes sense to be at this level as the GameObject could have multiple handler attached that would try to register it multiple times. It could support registration and unregistration at runtime but maybe in a future PR?
@HodgsonSDAS Should I revert all shader work or should I keep it and you work from there once PR merged?

maxouellet · 2016-12-17T22:35:29Z

@aalmada Sorry I haven't been very responsive lately, we just had a baby and that's taken most of my time. I haven't had a chance to look at this coe much, but overall, my opinion is that the input source itself should only be registering the keywords it needs and routing those to the appropriate destination through the InputManager.

If we want to emulate the behavior of the old KeywordManager, we should create a global listener script (KeywordHandler.cs or something like that?) that implements ISpeechHandler and essentially adds itself as a modal listener to the InputManager. That script could then have the same kind of editor logic that KeywordManager made available (associating keywords to actions).

The one thing I never quite figured out is how to make all of that logic type-safe... I'm annoyed at having to check for strings in order to know which word was said by the user. Ideally, we'd have some kind of enum or equivalent that defines that instead, but not sure how to implement that cleanly without requiring every user to modify an enum when they want to add new keywords. It would sure be nice to be able to dynamically add/remove new keywords dynamically, or at least turn them on/off dynamically at app runtime.

StephenHodgson · 2016-12-19T01:37:35Z

First congratulations @maxouellet!

@aalmada, I want to apologize for making this more difficult than anticipated. I didn't mean to complicate the PR. I think we can revert all changes to the shader and material assignments. I've went ahead and pulled your code so I have a copy as is and can start working on the suggested changes after new year's. I think it's best to only submit the relevant changes right now. You've done fantastic work. Thanks.

aalmada · 2016-12-19T09:07:23Z

@maxouellet Congrats! That's a real lifetime project. ;)
I came up with an idea after reading your comment. We can have a custom editor for the handler that lists on a dropdown, all the registered keywords from all SpeechInputSources (as this is not a singleton). It would look just like as an enum. I still have an issue on how to map the keywords to actions but I'll leave it all for another PR, once this one is approved and merged.
@HodgsonSDAS No problem. I'll revert the shader related code.

…related code

StephenHodgson · 2016-12-19T14:02:54Z

Assets/HoloToolkit/Input/Tests/Scripts/SphereGlobalKeywords.cs

+    {
+        private void Start()
+        {
+        }


We can probably remove if empty.

Removing it makes the script enable checkbox disappear but we aren't handling it anyway and this is just test code...

StephenHodgson · 2016-12-19T14:03:23Z

Assets/HoloToolkit/Utilities/Shaders/BlinnPhongConfigurable.shader

@@ -8,7 +8,7 @@ Shader "HoloToolkit/BlinnPhong Configurable"
    {
        [Header(Main Color)]
        [Toggle] _UseColor("Enabled?", Float) = 0
-        _Color("Main Color", Color) = (1,1,1,1)
+		_Color("Main Color", Color) = (1,1,1,1)


Tab offset.

Damn tabs ;)

StephenHodgson · 2016-12-19T14:04:24Z

Besides the two little things I think this PR is ready.

keveleigh · 2016-12-20T00:18:44Z

Agreed! Thanks so much for all the work that went into this, @aalmada!

aalmada added 8 commits November 23, 2016 14:58

Implemented KeywordManager as a input source. Added all the required …

f989951

…event pipeline.

Added missing meta files

2995aa3

Using the InputManager to handle the events.

286813a

Changed the KeywordManager scene to test keyword handlers on focused …

b44b7f7

…object and also a global listener.

Changed the KeywordManager scene to test keyword handlers on focused …

5f7766d

…object and also a global listener.

Merge branch 'KeywordManagerAsInputSource' of https://github.com/aalm…

17f31fd

…ada/HoloToolkit-Unity into KeywordManagerAsInputSource # Conflicts: # Assets/HoloToolkit/Input/Tests/Scenes/KeywordManager.unity

Merge branch 'ExtensibleInputModule' of https://github.com/Microsoft/…

55064fc

…HoloToolkit-Unity into KeywordManagerAsInputSource

Fixed method inheritance in KeywordManager

52844be

msftclas added the cla-already-signed label Nov 30, 2016

maxouellet reviewed Dec 1, 2016

View reviewed changes

StephenHodgson reviewed Dec 1, 2016

View reviewed changes

keveleigh reviewed Dec 1, 2016

View reviewed changes

aalmada added 2 commits December 2, 2016 09:33

Merge branch 'master' into KeywordManagerAsInputSource

1e61bb8

Made all the changes suggested til this date

3ddc7ce

aalmada changed the base branch from ExtensibleInputModule to master December 2, 2016 13:06

aalmada added 3 commits December 2, 2016 13:14

Fixes to the SpeechInputSource test scene

207937f

Fixes to the SpeechInputSource test scene

f271926

Merge branch 'KeywordManagerAsInputSource' of https://github.com/aalm…

78eff41

…ada/HoloToolkit-Unity into KeywordManagerAsInputSource # Conflicts: # Assets/HoloToolkit/Input/Tests/Scenes/SpeechInputSource.unity

Use [PerRendererData] in the shader to efficiently change color witho…

b16a510

…ut leaks.

aalmada added 3 commits December 16, 2016 10:34

Unity doesn't like init blocks

01310d9

Added SetGlobalListener.cs

7aac7b2

Fixed misspellings

f060d0d

StephenHodgson reviewed Dec 16, 2016

View reviewed changes

Reverted changes to HoloToolkit_Default.mat

05f6e3d

aalmada added 2 commits December 19, 2016 11:15

Reverted addition of [PerRendererData] to BlinnPhongConfigurable and …

b30ebc1

…related code

Use the SetGlobalListener.cs instead

11e119c

StephenHodgson reviewed Dec 19, 2016

View reviewed changes

Latest comments fixes

a583847

keveleigh merged commit b8e74e1 into microsoft:master Dec 20, 2016

StephenHodgson mentioned this pull request Dec 21, 2016

Fixes material memory leaks in various files #424

Merged

Added SpeechInputSource for integration of the KeywordRecognizer in the InputManager #354

Added SpeechInputSource for integration of the KeywordRecognizer in the InputManager #354

Conversation

aalmada commented Nov 30, 2016

msftclas commented Nov 30, 2016

aalmada commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxouellet Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxouellet commented Dec 1, 2016

StephenHodgson commented Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

StephenHodgson Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalmada commented Dec 15, 2016

StephenHodgson commented Dec 15, 2016

aalmada commented Dec 16, 2016

StephenHodgson commented Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

StephenHodgson Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenHodgson Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

aalmada Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

StephenHodgson Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keveleigh commented Dec 16, 2016

StephenHodgson commented Dec 16, 2016 • edited Loading

keveleigh commented Dec 16, 2016

StephenHodgson commented Dec 16, 2016

keveleigh commented Dec 16, 2016

aalmada commented Dec 17, 2016

maxouellet commented Dec 17, 2016

StephenHodgson commented Dec 19, 2016 • edited Loading

aalmada commented Dec 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenHodgson commented Dec 19, 2016

keveleigh commented Dec 20, 2016

maxouellet Dec 1, 2016 •

edited

Loading

StephenHodgson commented Dec 1, 2016 •

edited

Loading

StephenHodgson Dec 1, 2016 •

edited

Loading

StephenHodgson commented Dec 16, 2016 •

edited

Loading

StephenHodgson Dec 16, 2016 •

edited

Loading

StephenHodgson Dec 16, 2016 •

edited

Loading

aalmada Dec 16, 2016 •

edited

Loading

StephenHodgson Dec 16, 2016 •

edited

Loading

StephenHodgson commented Dec 16, 2016 •

edited

Loading

StephenHodgson commented Dec 19, 2016 •

edited

Loading