MRDL->MRTK [Dev unity 2017.2.0] #1007
Conversation
|
@StephenHodgson thanks for adding the label :). Also I heard you've been changing the namespace from HoloToolkit. What's the new namespace? I want to make sure I'm adding things as I migrate them to the correct namespace. thanks, |
| /// Motion Controller Input source for processing input from motion controllers. For each of these you would | ||
| /// want to specify handedness or not. Based on handedness indicates which controller this source would track | ||
| /// </summary> | ||
| public class MotionControllerInputSource : BaseInputSource |
There was a problem hiding this comment.
Pretty sure we already have a class that does exactly this, but in a more generalized way.
There was a problem hiding this comment.
I didn't see anywhere that had motion controller input. All the other scripts duplicated source delegates rather than using InputSource.
There was a problem hiding this comment.
We have https://github.com/Microsoft/MixedRealityToolkit-Unity/blob/Dev_Unity_2017.2.0/Assets/HoloToolkit/Input/Scripts/InputSources/InteractionSourceInputSource.cs for motion controller input, though this does a couple things not present there.
There was a problem hiding this comment.
I also want to make sure the Motion controllers and Xbox Controllers play nicely.
There was a problem hiding this comment.
Ah, so InteractionSourceInputSource (that's a lot of source!) is for both HoloLens gesture and motion controller then. The only things I'd add is the ability to set handedness to filter for on controllers and accessors for linear and angular velocity.
thoughts?
thanks,
-pat
There was a problem hiding this comment.
I'm in favor of making those awesome changes to the InteractionSourceInputSource class. I like that the class is more generalized and cross platform friendly. Makes things less complex.
(that's a lot of source!)
I laughed way too hard at this.
There was a problem hiding this comment.
Agreed! (Also agreed on "that's a lot of source!" haha! Definitely open to rename suggestions 😄). I added handedness as a variable in the source data, but I don't believe there's any way to actually pass that through yet. I think the two best ways would be to add a TryGetHandedness or to add it to each of the event data classes. Thoughts?
Accessors for velocity would be great as well!
There was a problem hiding this comment.
I think InteractionInputSource is fine lol.
There was a problem hiding this comment.
I just updated it to have accessors for velocities in this fork. I'll look to merge changes you've already made in as well.
thanks,
-pat
| /// </summary> | ||
| /// <param name="obj"> The object that recieved the event. </param> | ||
| /// <param name="eventArgs"> The event arguments sent to that object. </param> | ||
| //private void OnNavigationStartedInternal(GameObject obj, UnityEngine.XR.WSA.Input.InteractionManager.InteractionEventArgs eventArgs) { CheckLockFocus(eventArgs.Focuser); CheckAndSendEvent(OnNavigationStarted, obj, eventArgs); } |
There was a problem hiding this comment.
Why is all this commented out? Should it be removed?
ProjectSettings/ProjectVersion.txt
Outdated
| @@ -1 +1 @@ | |||
| m_EditorVersion: 2017.2.0b11 | |||
| m_EditorVersion: 2017.2.0f1 | |||
There was a problem hiding this comment.
Did we also update the readme?
There was a problem hiding this comment.
Accidentally added the version txt. I can revert it back unless we're going to be recommending an update soon. If so I can also update the readme.
There was a problem hiding this comment.
Revert is easiest. I think we should be using that version though.
Packages/manifest.json
Outdated
| @@ -0,0 +1,4 @@ | |||
| { | |||
There was a problem hiding this comment.
Please remove this and it's meta file.
| { | ||
| if (IsInteractible(obj)) | ||
| { | ||
| // sendEvent(obj, eventArgs); |
There was a problem hiding this comment.
What did this use to do? Why was it removed?
There was a problem hiding this comment.
Sorry I had the wrong version in there. That used to filter and send forward the events after filtering that it's coming from and interactible in the list. Changed this to filter and only fire the protected virtual functions based on the interactible being in the list. I'd like to clean this up on the InputManager side allowing for external subscriptions to specific objects in the future but wanted to not have any changes to core HTK manager scripts for now.
Should be resolved in the last commit.
Haha, I've done no such thing! Just renamed folders. The Namespaces should be the same. |
|
@paseb any plans on doing an initial merge to master? Or is the MRDL only for the latest? Also, is there any way to break this up into smaller pieces to review, or is it all a singular package deal? |
|
@StephenHodgson, First a lot of MRDL is reliant on having a Focuser or Pointer for a lot of the interactions so based on that the Dev_Unity_2017.2 branch is the most suitable to integrate in. In pulling over the elements I'm trying to keep the pieces smaller with just the core functionality. The only larger chunk I did was adding the resources for UI and the basic button classes. I'm open to integrate more pointed sets of functionality one a piece by piece basis. The hope is to do one integration unifying the input and replace HoloToolkit with the updated MRTK within our MRDL projects to make sure everything plays nice together then identify the next set to migrate over. thoughts? thanks, |
I think you're right, I noticed that initially from my first pass at it a few months ago, although, I am interested in slowly transforming the master branch inputs to closer align to the updated interaction scheme so people can easily update their projects without much issue. Sounds like you've got a good plan for migration, so I'll just make sure to follow closely and raise issues as they come up. Keep up the good work! |
| // When selected toggle color change | ||
| protected override void InputDown(GameObject obj, InputEventData args) | ||
| { | ||
| Debug.Log("On tapped in interaction receiver"); |
There was a problem hiding this comment.
What am I missing here on formatting? I've updated the code comments for Input down but the other formatting looks valid to me.
thanks,
-pat
There was a problem hiding this comment.
Could be tabs vs spaces
There was a problem hiding this comment.
Hmm.. they should all be set to spaces. Let me double check. I'm not seeing the usual discrepancies in the GitHub webview I usually do when it's tab/space mangled. Will double check.
thanks,
-pat
| public virtual void RegisterInteractible(GameObject interactible) | ||
| { | ||
| if (interactible == null || Interactibles.Contains(interactible)) | ||
| return; |
| /// </summary> | ||
| public virtual void OnEnable() | ||
| { | ||
| InputManager.Instance.AddGlobalListener(gameObject); |
There was a problem hiding this comment.
This should have been resolved with the last commit. Should have proper spaces.
| ClearInstancedAssets(); | ||
| } | ||
|
|
||
| SetIconName(iconName); |
There was a problem hiding this comment.
I've had issues with this code. When enabling/disabling buttons several times the uv-rects of the icons start breaking. See an example here: https://puu.sh/xIL06/0c20ded28c.webm.
This ended up solving it for me but it's more of a workaround than an actual fix, maybe someone with more time or knowledge of the code can figure it out:
private void OnEnable()
{
if (Application.isPlaying)
{
ClearInstancedAssets();
}
StartCoroutine(RefreshIcon());
}
private IEnumerator RefreshIcon()
{
SetIconName(iconName);
yield return new WaitForEndOfFrame();
SetIconName(iconName); // UVRect of the icon atlas will be broken without this, but why??
}Also a tip for debugging it: You can trigger the fix by selecting the button object in the hierarchy because that triggers setting the icon again from the property drawer.
There was a problem hiding this comment.
@Zod- Thanks for pointing this out. Yeah, this looks like some bad ref issues. I'll look into it. I didn't originally write the code and I know we have a thread about the issue on the MRDL GitHub repo.
There was a problem hiding this comment.
@Zod- I have experienced similar issue. I'll ask Lars who created this.
Thanks!
There was a problem hiding this comment.
This is unfortunately due to a common font glitch in Unity which we've attempted to mitigate (with limited success). Some causes are fairly deterministic but others are mysterious.
We've got an issue on it here with @zod's workaround.
Surprisingly I've yet to see this in Unity's issue tracker. We should try to get a bug submitted.
| /// The filtertag still needs to be implemented on the FocusManager/InputManager side to | ||
| /// Match functionality from MRDL. | ||
| /// </summary> | ||
| public abstract class InteractibleObject : MonoBehaviour, IPointerSpecificFocusable, IInputHandler, IHoldHandler, IManipulationHandler, INavigationHandler, ISourceStateHandler |
There was a problem hiding this comment.
Do we really need this class? Shouldn't the interactibles just implement what they need? We don't need clear references to interfaces in the InputManager since it uses Unitys event system that sends events to gameObjects which will then find the interface references on its own.
There was a problem hiding this comment.
The basic problem has been with the discoverability of interfaces for what interactions an object can consume. This would be better solved on the InputManager front by knowing what event's I can consume as oppose to the guess work of going through all the interfaces especially for global listeners.
The InteractibleObject just says I'm an easy base class that takes all interactions and you can easily find what you want to override by simply extending and override the function.
Currently there's not an easy way to filter object interactions and or provide some base functionality for things you interact with that's common across all of them. I'm open to other alternatives to provide filtering methods and other logic commonly across all interaction objects. Perhaps this should be refactored in InputManager instead and provide better delegate hooks as opposed to just global listeners?
Thoughts?
thanks,
-pat
There was a problem hiding this comment.
The basic problem has been with the discoverability of interfaces for what interactions an object can consume. This would be better solved on the InputManager front by knowing what event's I can consume as oppose to the guess work of going through all the interfaces especially for global listeners.
@paseb I'm not sure if I understand this. What do you mean by "the discoverability of interfaces"? From what I understand the reason why we're implementing the Interfaces was to get away from delegates and events to improve overall system performance.
There was a problem hiding this comment.
@StephenHodgson, That performance is negligible. We have a slider with one side being general usability and discoverability and the other considered optimal performance. I can handwrite a shader or write an app completely in assembly to get the best performance, though it would be crap for anyone to try to take that over or work off of what I've done.
Having just interfaces without the ability to quickly use intellisense to see what I can hook into from events represents the same problem. That's why we need some middle ground and if someone wants to implement interfaces in all their base classes that fine. Also if someone wanted to just extend an InteractibleObject and override an virtual function that should be fine as well.
I prefer delegates or event/actions with names variables and really haven't seen any perf issues. Has anyone done a hard study on the actual perf impacts?
thanks,
-pat
There was a problem hiding this comment.
The Unity event system does come with a overhead because it really just does a GetComponent<Interface> in the background.
Here are some real basic profiler results run in the editor with ~30 and ~100 global listeners.
There was a problem hiding this comment.
I was just looking at Jackson's other articles as well. Crazy sauce.
So in summary:
I'm not sure we actually need InteractibleObject as a base abstract class but I'm not sure there is any other alternative that would provide the same easy override functionality. A lot of the MRDL examples and base functionality uses InteractibleObject as a foundation class to build from. I'm open to refactoring it moving forward though think we need it to integrate other MRDL functionality.
thanks,
-pat
There was a problem hiding this comment.
Whatever event system we will have in the end, an important thing to keep in mind is that inheriting from InteractibleObject has other side-effects than just implementing a single interface.
I've recently run into the issue that adding a gameObject as modal will only absorb the interfaces that it implements and I needed it to lock them all. InteractibleObject will do this out of the box as it implements all interfaces but some may find this unexpected behavior?
There was a problem hiding this comment.
Agreed, let's not forget that Unity plans on doing a whole revamp of the Input System as well in a near future release. That's something we also need to take into consideration.
There was a problem hiding this comment.
At some point devs will need to make the custom logic themselves for their implementations in their own apps. Giving the option and examples of what to use is better than packing it all in without a choice.
I'm not worried about devs as they should be fine using and finding the interfaces. They are not the only ones using the toolkit and currently the toolkit is really only targeting devs. I'm more focused on designers, tech artists, creative coders and anyone else that want get off the ground quickly and have an extensible set of abstract classes to build off of. That's sort of the purpose of MRDL and bringing some functionality over to MRTK.
@Zod- InteractibleObject isn't meant to be the one ring but simply a starting point to extend from. Any dev would most likely start with base classes that just implement the interface they want anyway and not start with InteractibleObject. I'm completely open to other implementations if they meet the same goal for accessibility.
Again awesome input on all fronts!
thanks,
-pat
There was a problem hiding this comment.
Given that the modal issue and that the input system will be changing on the Unity side in the near future I'll remove InteractibleObject in favor of specifically targeted interactibles starting just with the base button class. I think we also need a better way to subscribe to interactions through input manager but can look at that later.
thanks,
-pat
|
URL for the people who are not familiar with MRDL |
|
Although @StephenHodgson , a namespace change might be a good idea with the new name for the toolkit :D |
|
+10 @paseb been looking forward to this PR since you mentioned it was coming |
|
The namespace update is coming, but we're blocked by the main toolkit repo. The namespaces have to be updated there first. |
|
Just tried InteractableObject/Button test scene (copy and pasted from MRDL) and it works nicely without modifying any code! Also works with motion controllers. |
|
Any update @paseb to get the changes in so this can be merged? |
| { | ||
| if (bLockFocus) | ||
| { | ||
| //LockFocus(focuser); |
There was a problem hiding this comment.
Why is this commented out?
There was a problem hiding this comment.
Because Focus locking is not currently part of the FocusManager and Modal isn't we we'd want either. Looking adding focus locking to FocusManager and the ability to have a beam controller for focus as well as cleaning up the interface to just having one for focus. Though for the time being I commented it out till we updated the focus manager side of things.
I talked with @keveleigh about this today as well. I'll have a separate pull request for those changes.
| /// <param name="args"></param> | ||
| protected override void InputDown(GameObject obj, InputEventData args) | ||
| { | ||
| Debug.Log("On tapped in interaction receiver"); |
There was a problem hiding this comment.
Could we make this optional?
There was a problem hiding this comment.
Hi @StephenHodgson,
Which part would you want to be optional? The debug log? This is more a simple example script to show easy usage for InteractionReceiver and the debug log could be removed completely. Do we define anywhere a conditional for logging? Might be something to add in the future to enable/disable log spew.
thanks,
-pat
There was a problem hiding this comment.
Yeah just the debug log.
Logs are pretty expensive.
There was a problem hiding this comment.
Yeah totally agree and usually have a #define ENABLE_LOGGING or something equivalent for toggling on and off.
thanks,
-pat
|
Hello. The MRDL of the MRDL_LunarModule repository has LocalHandInput.(https://github.com/Microsoft/MRDesignLabs_Unity_LunarModule/tree/master/Assets/MRDesignLab/HUX/Scripts/Utility).It is very easy to use and useful.I would appreciate it if you could include it in MRTK. Thank you for your time and consideration. regards, |
|
@paseb did you do a merge or a rebase? A lot of these commits seem to be duplicate. |
|
I rebased to the current version of the branch. At least that was what I believed I was doing. I might just have reset the whole fork and add back my changes. |
|
I don't think it went very well. Had this same issue yesterday with one of my own PRs. It sucks. |
| } | ||
|
|
||
| // Now delete the empty nodes | ||
| foreach (CollectionNode node in emptyNodes) |
| { | ||
| CollectionNode node = new CollectionNode(); | ||
|
|
||
| // TODO MICROSOFT: Initial offset |
There was a problem hiding this comment.
What's this TODO for?
There was a problem hiding this comment.
I'm removing the TODO's that are in there for now. Some are more opinion than actionable todos anyway.
| break; | ||
|
|
||
| case SortTypeEnum.Alphabetical: | ||
| NodeList.Sort(delegate (CollectionNode c1, CollectionNode c2) { return c1.Name.CompareTo(c2.Name); }); |
There was a problem hiding this comment.
should these string comparisons be culture specific?
| break; | ||
| } | ||
|
|
||
| _columns = Mathf.CeilToInt((float)NodeList.Count / (float)Rows); |
There was a problem hiding this comment.
redundant float casts
| _columns = Mathf.CeilToInt((float)NodeList.Count / (float)Rows); | ||
| _width = _columns * CellWidth; | ||
| _height = Rows * CellHeight; | ||
| _halfCell = new Vector2(CellWidth / 2f, CellHeight / 2f); |
There was a problem hiding this comment.
instead of diving we should be multiplying by 0.5f
| private void LayoutChildren() { | ||
|
|
||
| int cellCounter = 0; | ||
| float startOffsetX, startOffsetY; |
There was a problem hiding this comment.
tabs to space, and each variable on it's own line.
| switch (LayoutType) | ||
| { | ||
| case LayoutTypeEnum.ColumnThenRow: | ||
| default: |
There was a problem hiding this comment.
default case should be last and throw new ArgumentOutOfRangeException();
| switch (OrientType) | ||
| { | ||
| case OrientTypeEnum.None: | ||
| default: |
There was a problem hiding this comment.
default case should be last and throw new ArgumentOutOfRangeException();
|
|
||
| case OrientTypeEnum.FaceOriginReversed: | ||
| case OrientTypeEnum.FaceForwardReversed: | ||
| newRot = Vector3.zero; |
There was a problem hiding this comment.
value assigned is never used
| switch (OrientType) | ||
| { | ||
| case OrientTypeEnum.None: | ||
| default: |
There was a problem hiding this comment.
default case should be last and throw new ArgumentOutOfRangeException();
| break; | ||
|
|
||
| case SurfaceTypeEnum.Scatter: | ||
| // TODO fix this rough first commit! Magic numbers galore! |
There was a problem hiding this comment.
We should probably address this TODO
There was a problem hiding this comment.
I believe that's an old comment that is no longer valid as there aren't Magic numbers galore.
| /// <param name="source">The source <see cref="Vector3"/> to be mapped to sphere</param> | ||
| /// <param name="radius">This is a <see cref="float"/> for the radius of the sphere</param> | ||
| /// <returns></returns> | ||
| private Vector3 SphericalMapping(Vector3 source, float radius) |
There was a problem hiding this comment.
radius parameter is never used.
There was a problem hiding this comment.
Is now when passed in and non-zero replaces the collections radius and is applied.
| /// <param name="source">The source <see cref="Vector3"/> to be mapped to cylinder</param> | ||
| /// <param name="radius">This is a <see cref="float"/> for the radius of the cylinder</param> | ||
| /// <returns></returns> | ||
| private Vector3 CylindricalMapping(Vector3 source, float radius) |
There was a problem hiding this comment.
radius parameter is never used.
There was a problem hiding this comment.
Same as above. passed in and replace Radius.
| /// <returns></returns> | ||
| private bool ContainsNode(Transform node) | ||
| { | ||
| foreach (CollectionNode cn in NodeList) |
| { | ||
| if (cn != null) { | ||
| if (cn.transform == node) | ||
| return true; |
There was a problem hiding this comment.
merge if statements and add braces
| return distance1.CompareTo(distance2); | ||
| }); | ||
|
|
||
| Vector3 difference = Vector3.zero; |
There was a problem hiding this comment.
value assigned is never used
|
@TakahiroMiyaura In response to your question above, I've added a simple tutorial hand model example to this branch, but I will wait for the pull request to complete before I push it. (It may also turn into a separate pull request for Dev2017.2 branch.) For some reason, I can't comment on your comment above. |
|
Can we summerise the outstanding changes needed to close down this PR and get it merged? |
|
@paseb mind if we close this PR and open a clean one? Also we can re-target the master branch as well. |
|
@StephenHodgson, yeah I can do that. I'll add it later today. thanks, |
This is the first pull request for integrating functionality from MRDL(MRDesignLabs) into MRTK. This pull request also adds motion controllers as an input source so that the can be used as a direct input source.
Added in this pull request
Next set of things to integrate:
thanks,
-pat