Extensible input module + new cursor #319
Conversation
… still need to be cleaned up, and headers need to be adjusted. Documentation still needs to be added. Removed the old input module (GestureManager and related scripts) [!] Bug fixes to the build deployment code [+] Added a new 3D cursor. This is a temporary cursor for now: a better one is incoming, but probably as part of a different change. For now, the existing HoloToolkit cursors still exist, to maintain backward compatibility [+] Added a new HoloLensCamera prefab that can be used to have a Holographic camera that is controllable in the scene. This replaces the old "Main Camera" prefab, but the old prefab hasn't been deleted yet [~] Modified a lot of the scripts used for testing and examples so that they use the new input module [~] Cleaned up the existing Singleton, so that it doesn't do a FingByType anymore, and instead uses proper inheritance. Still need to switch some methods in the Singleton from public to protected, will be done as a separate change. [+] Added a few Utility scripts, including a timer class, a few new helpful extension methods
…s still need to be cleaned up, and headers need to be adjusted. Documentation still needs to be added. Removed the old input module (GestureManager and related scripts) [!] Bug fixes to the build deployment code [+] Added a new 3D cursor. This is a temporary cursor for now: a better one is incoming, but probably as part of a different change. For now, the existing HoloToolkit cursors still exist, to maintain backward compatibility [+] Added a new HoloLensCamera prefab that can be used to have a Holographic camera that is controllable in the scene. This replaces the old "Main Camera" prefab, but the old prefab hasn't been deleted yet [~] Modified a lot of the scripts used for testing and examples so that they use the new input module [~] Cleaned up the existing Singleton, so that it doesn't do a FingByType anymore, and instead uses proper inheritance. Still need to switch some methods in the Singleton from public to protected, will be done as a separate change. [+] Added a few Utility scripts, including a timer class, a few new helpful extension methods
[~] Fixed the main test scene for input (the InputManager prefab was broken) [~] Fixed various test scenes
…thods instead of public ones [-] Removed the old ManualCameraControl script and its associated prefab. The "HoloLensCamera" prefab should be used now [~] Fixed up all test scenes
[~] Modified the InputManager so that it doesn't instantiate new event data objects. Instead, it reuses existing events. The downside to this approach is that users should never cache the event data in their local class, as it it could be overwritten by another event
…pport all HoloLens input devices (clicker and hands) [~] Modified IInputSource interface so that it can dynamically specify whether a specific input source instance supports specific info, as opposed to globally
…t properly adjust to their rescale animation
…he example cursor
[~] Removed the reference to the active cursor that InputManager had, and removed the EnableInput / DisableInput methods from the cursor interfaces. Replaced this logic with an event that is triggered by InputManager when input is enabled / disabled, that the cursor can listen to. [~] Replaced delegates with Action in IInputSOurce and BaseInputSource
…/HoloToolkit-Unity into ExtensibleInputModule
…xed in SpatialUnderstanding example.
…instead of using Action. This aligns it with standard C# event practices and also allows us to more easily add additional members to our *EventArgs classes as this input module evolves
…/HoloToolkit-Unity into ExtensibleInputModule
[~] Moved the old BasicCursor that is only necessary for SPatialUnderstanding example to the SpatialUnderstanding example [~] Fixed a few scenes that had broken references
[-] Removed unused CursorFeedback script [~] Moved test prefabs and scenes to the appropriate location
|
@maxouellet |
| @@ -0,0 +1,33 @@ | |||
| using UnityEngine; | |||
There was a problem hiding this comment.
Missing licencing header
| @@ -0,0 +1,345 @@ | |||
| using System.Collections.Generic; | |||
There was a problem hiding this comment.
Missing licensing header
| { | ||
| if (animatorHashes[i].nameHash == deHydrateButtonId) | ||
| { | ||
| ButtonAnimator.SetTrigger(deHydrateButtonId); |
There was a problem hiding this comment.
Does this trigger also call gameObject.setActive(false)?
We may want to deactivate this game object so colliders deactivate as well.
There was a problem hiding this comment.
It does not, but I don't think it should in this case, as this button is . The dehydration animation itself sets the button's renderer to inactive. I modified the animation so that it also sets the button collider to inactive, which I think addresses the issue.
That being said, I'm not convinced this button script is ready for main HoloToolkit yet. As such, I renamed it to TestButton and moved it to the Tests/Scripts location, so that it serves as an example script for the test scene, rather than something that people should start building on. Some design work would be required to make this into a great reusable button script that works across the board.
There was a problem hiding this comment.
I think it's a good idea to let developers figure out how they want to handle buttons and just have an example.
| { | ||
| RaycastHit? minHit = null; | ||
|
|
||
| foreach (RaycastHit h in hits) |
There was a problem hiding this comment.
could be a for loop because this is embedded in the update loop.
| @@ -0,0 +1,237 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
| // TODO - Review this code before uploading to the HoloToolkit | |||
There was a problem hiding this comment.
Fix licencing header
|
@HodgsonSDAS Ahh I finally understand what you mean, there's animations embedded in that cursor FBX. Working on fixing that, I'll just remove the animations that are in the cursor and essentially regenerate the FBX. |
[~] Moved Button class to Tests folder and renamed it to TestButton
…s. Also renamed some components in it so that it's a little more straightforward.
| gazing = false; | ||
| } | ||
|
|
||
| public void OnInputUp(InputEventData eventData) |
There was a problem hiding this comment.
Sorry I should have thought of this in the interface review but just for handling a simple air tap every class with not have these two stub methods that do nothing. Thoughts?
There was a problem hiding this comment.
Yeah, it might make sense. I agree that there's a bunch of cases where you only care about click, and other cases where you only care about up/down (and not click).
We could split the interface in two: IInputHandler (for up/down) and IInputClickHandler (for click). Would it make sense to do this as a separate pull request, or would you prefer I add it to this one?
There was a problem hiding this comment.
I am OK if it's fixed in a separate one. Although, do you think the up/down might make sense in ISourceState?
There was a problem hiding this comment.
It could, but ISourceStateHandler is already used for visibility. I think it'd make sense to do this:
ISourceStateHandler: source up/down
ISourceVisibilityHandler: source detected / lost
I can look at making that change tomorrow, it shouldn't be too bad.
|
@maxouellet Is the InputManagerTest.scene gone? I liked that example with multiple forms of interaction. |
|
It's still there @aalmada HoloToolkit\Input\Tests\Scenes |
|
@maxouellet After doing some tests in the InputManagerTest scene, I've found problems with the pop up menu on the cylinder we grow/shrink. Sometimes either the pop up menu doesn't go away, or it never pops in. There's also some issues with the animations as well, but that's trivial. I'd also like to see a way to disable the debug unless some symbol is defined in the player settings. The frame rate of the test scene is also a concern. |
|
@HodgsonSDAS Sorry but I was misinterpreting the info for this PR. I was already looking in the local ExtensibleInputManager branch but it will be here only after the merge. Thanks. |
|
|
||
| if (previousObject != null) | ||
| { | ||
| ExecuteEvents.ExecuteHierarchy<IFocusable>(previousObject, null, (inputHandler, eventData) => { inputHandler.OnFocusExit(); }); |
There was a problem hiding this comment.
Not so sure I like this line, and those like it down below. I wonder if there's a better way. Seems to be a lot of allocations in this block. Too many anon methods.
Potentially we could create a static delegate that can be reused between methods that use any one of our interface handler types. It would nicer on the GC by reducing the number of delegate allocations.
For Example:
private static readonly ExecuteEvents.EventFunction<ISourceStateHandler> OnSourceDetectedEventHandler =
delegate (ISourceStateHandler handler, BaseEventData eventData)
{
SourceStateEventData casted = ExecuteEvents.ValidateEventData<SourceStateEventData>(eventData);
handler.OnSourceDetected(casted);
};
private void InputSource_SourceDetected(object sender, InputSourceEventArgs e)
{
// Create input event
sourceStateEventData.Initialize(e.InputSource, e.SourceId);
// Handler for execute events
ExecuteEvents.ExecuteHierarchy(GazeManager.Instance.GazeTransform.gameObject, sourceStateEventData, OnSourceDetectedEventHandler);
// Pass handler through HandleEvent to perform modal/fallback logic
HandleEvent(sourceStateEventData, OnSourceDetectedEventHandler);
}
There was a problem hiding this comment.
Agreed that this might save a few allocations. I'd suggest making that optimization at a later time, should it become necessary. It wouldn't be a breaking change, since it's an internal implementation detail of InputManager, and I'd like to get this first version in.
Let me know if you disagree.
There was a problem hiding this comment.
No worries, I wouldn't mind doing this work later on.
|
@HodgsonSDAS I have not been able to repro the cylinder popup issue you are mentioning. What do you mean by disabling the debug? Do you mean the debug output for the events in the test scene? It's a test scene, meant to show concepts, so I think it's fine to have them in there. The debug log statements are in their own debug script, so it's not like they are polluting other scripts that you might want to reuse for your projects. For the framerate on that scene, I think it's mostly because the content in that scene is using the standard shader. I can try to switch it to a different shader and see what it does. Again, not super concerned about it, since it's a test scene that we can improve as needed. |
…to improve its framerate
|
When building on device the debug scripts really impact performance. I understand it's a test scene, and debug logs are good for showing off exactly what's happening, but I think having a way to disable it might be nice. |
|
My guess is that the major debug log perf impact comes from the NavigationUpdated and ManipulationUpdated events being broadcasted many times a second when you're dragging an object. I can disable those behind a boolean (and default them to disabled), I think that'd help. |
…te events are logged. Logging for the *Updated events now defaults to false. This fixes a framerate problem that was occuring when manipulating objects, since this lead to logging multiple times per second.
|
@maxouellet Repro steps for Cylinder:
Valid in Editor and on device. This also affects clicking wait button when modal popup is open. |
|
@HodgsonSDAS If I understand your repro steps well, that's by design. When you open the modal input popup, it disabled all other input until the popup is dismissed. So until you click the "Exit" button that is in the popup, all other input won't do anything. |
|
Ohh, wow, I can't read. XD I think some sort of screen fade would help illustrate this effect more. |
|
Okay, one last thing. If you walk through the scene objects the rendering of the cursor does some crazy stuff until you look at the scene objects again. Hard to describe unless you try it and see what I mean. I tried to record it but it doesn't render the same. I think it has to do with how the camera near clip plane stops rendering geometry but the raycast for the cursor still hits geometry that's no longer being rendered. |
|
Looks like only blocking comment left is what HodgonSDAS reported.
This would defeat the purpose of all this :) |
|
@HodgsonSDAS Just tried this on device. Do you mean that if you approach an object that has a collider, your cursor will eventually disappear if that object gets closer than 0.85 meters (current near clip plane)? And then the cursor reappears as soon as you target an object farther than the near clip plane. If this is what you mean, then this is not a new issue: it already existed with the previous cursor + gaze manager code (just validated by testing with the current HoloToolkit). Given that it is not a new issue, I'd be tempted to leave it as-is for now and fix it as a separate change if needed. I don't know that we really want to make the cursor go through objects that are in the near clip plane: there might be scenarios where that is not desirable. If this is not what you meant, would love to have more details about this. |
|
Agreed with @maxouellet. I understood something else. |
|
No this isn't about the cursor clipping the near plane, it's a rendering issue where all the colors get separated like the GPU is having a hard time rendering everything quickly enough. I see the red green and blue colors separately, and the sequential colors get rendered very far apart. I think this is called Judder? To test walk completely through the panel until you see the cursor again, and then turn around. |
|
@HodgsonSDAS I believe this is a known color separation issue with the device when having a full white cursor. This is compounded when framerate drops and interpolation speed is minimal. A simple test to mitigate this is to shift the cursor color from being full white to #7BCEFFFF or a slight grey #C8C8C8FF. I've also noticed perf issues with the default textmesh shader and backface transparency. You should see it if you go to the back side of the panels and look back to see the 2 panels text overlapping from the reverse side. There has been some additional research at methods to mitigate further the color separation artifacts using some rendering tricks that I can expand on if need be. Also noted at the bottom "Color Separation" |
|
@paseb I can understand if I'm moving my head quickly but this was happening even when standing still, looking away from the panels, after I had walked through them. I'll try to capture on my phone and upload a vid. |
|
Ok, with your repro step, I've repro'ed the issue on my side. Will take a look, this is definitely weird |
|
@HodgsonSDAS I was able to repro as well. @maxouellet it looks like an issue with the StabilizationPlaneModifier when you go past the panels. The plane remains exactly where the camera is and so does the focus point. |
|
@paseb Yeah, I think you're right. |
|
Yep for a quick fix or to validate is to use the Default cursor as the override for the StabilizationPlaneModifier transform to test. @maxouellet is working on a better fix for the issue. Good catch @HodgsonSDAS. |
[!] Fixed StabilizationPlaneModifier so that it uses the default distance when no object is being looked at. Otherwise this could create issues when the hit distance is very close to the user
|
Thanks a lot for finding this @HodgsonSDAS , and thanks @paseb for the help. Pushed a change that fixes the issue by using the default plane distance when no object is hit, as opposed to using the last hit distance. Also fixed a bug in StabilizationPlaneModifier that made it not respect the "Draw Gizmos" option: it now behaves properly. |
|
@maxouellet , @paseb you guys are awesome. Sorry for being such a pain in the ass. |
|
Before we merge this PR, could we sync |
|
@HodgsonSDAS thank you for being so diligent. Rock on! |
These changes add the new HoloToolkit Input Module and the new cursor implementation. Documentation for the new input scripts has been updated, and all existing test scenes and prefabs have been fixed so that their behavior remains the same. This pull request is targeting the ExtensibleInputModule branch, so that we can stabilize in there as necessary and give people some time to take a look at the changes before they go to master.
The main changes are the following:
New extensible input module, as discussed here. The old GesturesManager, HandsManager and GazeManager have been removed, since they are fully replaced by the new scripts. Existing scripts that dependend on those have been modified to use the new input module.
New extensible cursor implementation, as described by @paseb here. This adds a new Cursor prefab (DefaultCursor). The existing cursors prefabs have been modified so that they use the new cursor scripts, and the old cursor scripts have been removed.
Singleton has been modified to be more streamlined and predictable. Existing Singletons have been adjusted accordingly. The main breaking change here is that singleton classes that implement Awake or OnDestroy should override them, and make sure to call base.Awake() and base.OnDestroy().