Rename speech-related subsystems and add a sample scene for dictation #11348
Conversation
MaxWang-MS
requested review from
david-c-kline,
keveleigh,
maluoi,
RogPodge and
Zee2
as code owners
|
Missing a |
UnityProjects/MRTKDevTemplate/Assets/Scripts/SpeechRecognitionHandler.cs
Outdated
Show resolved
Hide resolved
|
The naming is starting to confuse me. If "SpeechRecognition" is actually speech-to-text, or dictation/transcription (instead of event-based recognitions like see-it-say-it) can we call it Dictation or something? |
| /// events fired by SpeechRecognitionSubsystem. | ||
| /// </summary> | ||
| [AddComponentMenu("MRTK/Examples/Speech Recognition Handler")] | ||
| public class SpeechRecognitionHandler : MonoBehaviour |
There was a problem hiding this comment.
Can we make it internal just so folks don't end up taking a dep on it?
There was a problem hiding this comment.
This script is a sample script (i.e. in Assembly-CSharp) so not sure if internaling it would do much.
|
Mysterious threading error when exiting Play mode |
|
Huge lagspike when starting recognition
The |
|
Click the button to stop transcribing doesn't work, it just keeps going! I can take a video if you want, but clicking the button to stop it doesn't seem to have any effect at all |
|
@Zee2 I looked into the spike and tried to move the instantiate of the Unity |
|
Fortunately, the spike only appears the first time the Unity recognizer gets created. I will be calling the recognizer constructor once in the subsystem constructor / Start() so that hopefully the hit is only taken at app launch if the user enables the subsystem via the profile. |
MaxWang-MS
changed the title
Upon further investigation dummy initialization at start does not work. Will file a ticket to Unity regarding the time spike. |
Now fixed! |
Fixed as well. |
| { | ||
| return XRSubsystemHelpers.GetFirstRunningSubsystem<PhraseRecognitionSubsystem>() as PhraseRecognitionSubsystem; | ||
| return XRSubsystemHelpers.GetFirstRunningSubsystem<KeywordRecognitionSubsystem>() as KeywordRecognitionSubsystem; |
There was a problem hiding this comment.
nit: is the as KeywordRecognitionSubsystem part needed? I thought this helper returned the same type as the passed-in T
There was a problem hiding this comment.
I also wonder if we want to move this to the new pattern @Zee2 is introducing for the hands aggregator in #11333: https://github.com/microsoft/MixedRealityToolkit-Unity/pull/11333/files#diff-6bb0070ef5fe1b4e52210a345ab2f2c2f693e0b9cff69e18e05456aef8969376R160
Basically, deprecating the old helper from HandsUtils and putting it into XRSubsystemHelpers directly
| /// <summary> | ||
| /// Event data associated with the result of dictation. | ||
| /// </summary> | ||
| public class DictationResultEventArgs |
There was a problem hiding this comment.
are these event args reused or one-time-use? if the latter, could they be rewritten as readonly structs to more clearly express the design intent?
There was a problem hiding this comment.
Now rewritten as readonly structs!
…microsoft#11348) * Add a sample scene for SpeechRecognitionSubsystem * Feedback * Update WindowsSpeechRecognitionSubsystem.cs * Interface and subsystem rename part 1 * Interface and subsystem rename part 2 * Interface and subsystem rename part 3 * Rename configs * More renaming * Rename sample scene * Update DictationExample.unity * Feedback * Add private setters to expose properties in the inspector * Add sample scene hand menu * Better display of error message Co-authored-by: Finn Sinclair <finnnorth@gmail.com>
Overview