Fix Solvers tracking pointers as target transforms #7012

Troy-Ferrell · 2020-01-08T20:30:57Z

Overview

Due to pointer cache PR #6822 , pointers no longer necessarily destroy themselves after source lost. SolverHandler though would detach itself from a controller/hand once the transform was destroyed. This is actually incorrect since custom pointers could have had DestroyOnSourceLost false.

This change tracks, for the controller ray tracked target type, the associated line pointer being used for the transform and whether it is still being tracked or not. If it is not null, but not being tracked, then we detach and refresh for a new transform.

Also clean up some of the code and comments around solvers/pointers related files. In particular, ControllerPointerSynchronizer did two incorrect things

It would allow setting of Handedness although really this was always the read value from the assigned controller
It would pre-append the handedness onto the gameobject name. However, since we re-use gameobjects of pointers now (they are just deactivated), the name will always pre-append onto itself making strings such as "Left_Right_Left_Left_DefaultControllerPointer"

Updated SolverTests to validate values on SolverHandler for switching hands and tracked target types.

Changes

Fixes: Solver do not properly switch between controllers/pointers #7029

Verification

As a reviewer, it is possible to check out this change locally by using the following
commands (substituting {PR_ID} with the ID of this pull request):

git fetch origin pull/{PR_ID}/head:name_of_local_branch

git checkout name_of_local_branch

julenka · 2020-01-09T18:47:22Z

Consider merging into mrtk_development instead of stabilization

Troy-Ferrell · 2020-01-09T19:03:19Z

@julenka we will ship with Solvers not working for ControllerRay target type if we target MRTK_Development

julenka · 2020-01-09T23:02:15Z

Consider moving away from HandJointService in SolverHandler. Look at detectedinputsources/controllers and then search for hand or not that way. This way consolidate controller ray and hand joint tracked target types?

Seems like a good idea

julenka · 2020-01-09T23:03:57Z

Can you please createa bug for this and fill in "Fixes #"?

julenka

LGTM, except one worry about removing a field that is technically publicly exposed in an asset. Wanted to make sure we are not breaking people

Assets/MixedRealityToolkit.SDK/Features/Input/Handlers/ControllerPoseSynchronizer.cs

Assets/MixedRealityToolkit.SDK/Features/UX/Scripts/Pointers/BaseMousePointer.cs

Assets/MixedRealityToolkit.SDK/Features/Utilities/Solvers/SolverHandler.cs

Assets/MixedRealityToolkit.SDK/Features/Input/Handlers/ControllerPoseSynchronizer.cs

Assets/MixedRealityToolkit.SDK/Features/UX/Scripts/Pointers/BaseControllerPointer.cs

Troy-Ferrell · 2020-01-10T00:16:31Z

@julenka filed #7030 to track work to simplify SolverHandler tracking code with DetectedControllers. Putting off for now since this is going into stabilization and that would be more code churn than necessary to solve the actual issue here.

Speaking of issue here, filed #7029 to track with PR

@julenka / @keveleigh , see my response to your other comments

julenka

Approving to unblock check in, though I had a suggestion about deprecating handedness field instead of removing it. That might surprise people less.

david-c-kline · 2020-01-10T18:58:22Z

deprecating handedness field instead of removing it

Yes, please! And then be sure to add a note in the change tracking issue.

david-c-kline

Please deprecate Handeness instead of removing. Thanks

Dismissing as i was in error in my requested change

david-c-kline · 2020-01-10T19:01:17Z

Yes, please! And then be sure to add a note in the change tracking issue.

This can be ignored :)

david-c-kline · 2020-01-10T19:01:35Z

/azp run

azure-pipelines · 2020-01-10T19:01:48Z

Azure Pipelines successfully started running 2 pipeline(s).

thalbern

LGTM :)
tests are using PlayModeTestUtilities instead of TestHand - but that was already there before - so no need to change that now

…-solvers

Troy-Ferrell · 2020-01-13T18:06:58Z

/azp run

azure-pipelines · 2020-01-13T18:07:13Z

Azure Pipelines successfully started running 2 pipeline(s).

…-solvers

Troy-Ferrell · 2020-01-13T22:15:41Z

/azp run

azure-pipelines · 2020-01-13T22:15:56Z

Azure Pipelines successfully started running 2 pipeline(s).

Troy-Ferrell · 2020-01-14T00:09:09Z

hmmm build error on UWP x86 .NET
https://dev.azure.com/aipmr/MixedRealityToolkit-Unity-CI/_build/results?buildId=8478&view=logs&j=c1841906-0df4-5516-e3c5-68840a78c082&t=dcf3bde5-faf6-583c-8480-29a621c79462

@keveleigh / @wiwei , try to dive into myself now, but does this look like something that could be impacted by #7053 ? Seems like an odd error for this change

2020-01-13T23:05:25.9706477Z Microsoft (R) Visual C# Compiler version 2.9.1.65535 (9d34608e)
2020-01-13T23:05:25.9708647Z Copyright (C) Microsoft Corporation. All rights reserved.
2020-01-13T23:05:25.9710639Z
2020-01-13T23:05:25.9712893Z error CS2001: Source file 'C:\Windows\SERVIC~~2\NETWOR~~1\AppData\Local\Temp.NETCore,Version=v5.0.AssemblyAttributes.cs' could not be found.
2020-01-13T23:05:25.9714753Z
2020-01-13T23:05:25.9715056Z (Filename: Line: 0)
2020-01-13T23:05:25.9715829Z
2020-01-13T23:05:25.9716878Z DisplayProgressNotification: Build Failed
2020-01-13T23:05:25.9717287Z Error building Player because scripts had compiler errors
2020-01-13T23:05:25.9718312Z
2020-01-13T23:05:26.1728978Z (Filename: Line: -1)

Troy-Ferrell · 2020-01-14T04:01:19Z

@keveleigh / @davidkline-ms / @wiwei / @thalbern , can one of you review/merge this when possible? Thanks!

SimonDarksideJ · 2020-01-14T13:00:00Z

Assets/MixedRealityToolkit.SDK/Features/Input/Handlers/ControllerPoseSynchronizer.cs

@@ -32,8 +27,8 @@ public Handedness Handedness
        /// <inheritdoc />
        public bool DestroyOnSourceLost


Why not just use

public bool DestroyOnSourceLost => destroyOnSourceLost;

If you are not altering either the getter / setter?

Also consider this for other areas this is being changed for

I believe that just creates a getter, no setter

SimonDarksideJ · 2020-01-14T13:00:45Z

Assets/MixedRealityToolkit.SDK/Features/Input/Handlers/ControllerPoseSynchronizer.cs

-                if (controller != null && gameObject != null)
-                {
-                    handedness = value.ControllerHandedness;
-                    gameObject.name = $"{handedness}_{gameObject.name}";


Why remove the setting of the game object name, unless you are now doing this somewhere else?

In the PR description there is an example. Basically constant calling to his makes "Left_Right_Left_BlahPointer" names. Instead of checking the name and doing substrings, I just removed it so the name is left as-is

SimonDarksideJ · 2020-01-14T13:07:14Z

Assets/MixedRealityToolkit.SDK/Features/Input/Handlers/ControllerPoseSynchronizer.cs

+        #pragma warning disable 0414
+        [SerializeField]
+        [HideInInspector]
+        [System.Obsolete("Use the Handedness property instead to get current handedness which is set by Controller attached")]


I would seriously have to question why you are putting an obsolete flag on a PRIVATE field.
Obsolete is for telling consumers of a function or property outside a class is it going to be deprecated. Private fields do not fall in this category because they are private. No need to make them obsolete.

It was originally removed. Please read PR thread
#7012 (comment)

SimonDarksideJ · 2020-01-14T13:09:18Z

Assets/MixedRealityToolkit.SDK/Features/UX/Scripts/Pointers/BaseControllerPointer.cs

@@ -208,7 +208,7 @@ public override IMixedRealityController Controller

                if (base.Controller != null && this != null)
                {
-                    pointerName = gameObject.name;
+                    PointerName = gameObject.name;


As you have removed setting the gameobject name above, is this needed?

wiwei · 2020-01-14T17:26:37Z

/azp run

azure-pipelines · 2020-01-14T17:26:53Z

Azure Pipelines successfully started running 2 pipeline(s).

Troy-Ferrell added 2 commits January 8, 2020 11:29

init work

f79416c

Update mouse pointer

7ffd2ae

Troy-Ferrell added the Solvers - General label Jan 8, 2020

Troy-Ferrell requested review from CDiaz-MS, david-c-kline, julenka, keveleigh, thalbern and wiwei as code owners January 8, 2020 20:30

Troy-Ferrell self-assigned this Jan 8, 2020

Troy-Ferrell added the DO NOT MERGE label Jan 8, 2020

Troy-Ferrell added this to the MRTK 2.3.0 milestone Jan 9, 2020

julenka reviewed Jan 9, 2020

View reviewed changes

keveleigh reviewed Jan 9, 2020

View reviewed changes

Assets/MixedRealityToolkit.SDK/Features/Input/Handlers/ControllerPoseSynchronizer.cs Show resolved Hide resolved

keveleigh reviewed Jan 9, 2020

View reviewed changes

Assets/MixedRealityToolkit.SDK/Features/UX/Scripts/Pointers/BaseControllerPointer.cs Show resolved Hide resolved

update solver tests

fd1e617

Troy-Ferrell requested review from julenka and keveleigh January 9, 2020 23:57

Troy-Ferrell removed the DO NOT MERGE label Jan 10, 2020

julenka approved these changes Jan 10, 2020

View reviewed changes

david-c-kline previously requested changes Jan 10, 2020

View reviewed changes

david-c-kline approved these changes Jan 10, 2020

View reviewed changes

thalbern approved these changes Jan 13, 2020

View reviewed changes

Troy-Ferrell added 2 commits January 13, 2020 09:59

Merge branch 'prerelease/2.3.0_stabilization' into users/trferrel/fix…

935bbbb

…-solvers

Deprecated field

c198018

Troy-Ferrell mentioned this pull request Jan 13, 2020

2.3.0 Release Notes and Breaking Changes #6922

Closed

Merge branch 'prerelease/2.3.0_stabilization' into users/trferrel/fix…

c8696d9

…-solvers

SimonDarksideJ suggested changes Jan 14, 2020

View reviewed changes

wiwei merged commit c17ecee into microsoft:prerelease/2.3.0_stabilization Jan 14, 2020

Troy-Ferrell deleted the users/trferrel/fix-solvers branch January 22, 2020 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Solvers tracking pointers as target transforms #7012

Fix Solvers tracking pointers as target transforms #7012

Troy-Ferrell commented Jan 8, 2020 •

edited

Loading

julenka commented Jan 9, 2020

Troy-Ferrell commented Jan 9, 2020

julenka commented Jan 9, 2020

julenka commented Jan 9, 2020

julenka left a comment

Troy-Ferrell commented Jan 10, 2020

julenka left a comment

david-c-kline commented Jan 10, 2020

david-c-kline left a comment

david-c-kline commented Jan 10, 2020

david-c-kline commented Jan 10, 2020

azure-pipelines bot commented Jan 10, 2020

thalbern left a comment

Troy-Ferrell commented Jan 13, 2020

azure-pipelines bot commented Jan 13, 2020

Troy-Ferrell commented Jan 13, 2020

azure-pipelines bot commented Jan 13, 2020

Troy-Ferrell commented Jan 14, 2020

Troy-Ferrell commented Jan 14, 2020

SimonDarksideJ Jan 14, 2020

Troy-Ferrell Jan 15, 2020

SimonDarksideJ Jan 14, 2020

Troy-Ferrell Jan 15, 2020

SimonDarksideJ Jan 14, 2020

Troy-Ferrell Jan 15, 2020

SimonDarksideJ Jan 14, 2020

wiwei commented Jan 14, 2020

azure-pipelines bot commented Jan 14, 2020

		@@ -32,8 +27,8 @@ public Handedness Handedness
		/// <inheritdoc />
		public bool DestroyOnSourceLost

Fix Solvers tracking pointers as target transforms #7012

Fix Solvers tracking pointers as target transforms #7012

Conversation

Troy-Ferrell commented Jan 8, 2020 • edited Loading

Overview

Changes

Verification

julenka commented Jan 9, 2020

Troy-Ferrell commented Jan 9, 2020

julenka commented Jan 9, 2020

julenka commented Jan 9, 2020

julenka left a comment

Choose a reason for hiding this comment

Troy-Ferrell commented Jan 10, 2020

julenka left a comment

Choose a reason for hiding this comment

david-c-kline commented Jan 10, 2020

david-c-kline left a comment

Choose a reason for hiding this comment

david-c-kline commented Jan 10, 2020

david-c-kline commented Jan 10, 2020

azure-pipelines bot commented Jan 10, 2020

thalbern left a comment

Choose a reason for hiding this comment

Troy-Ferrell commented Jan 13, 2020

azure-pipelines bot commented Jan 13, 2020

Troy-Ferrell commented Jan 13, 2020

azure-pipelines bot commented Jan 13, 2020

Troy-Ferrell commented Jan 14, 2020

Troy-Ferrell commented Jan 14, 2020

SimonDarksideJ Jan 14, 2020

Choose a reason for hiding this comment

Troy-Ferrell Jan 15, 2020

Choose a reason for hiding this comment

SimonDarksideJ Jan 14, 2020

Choose a reason for hiding this comment

Troy-Ferrell Jan 15, 2020

Choose a reason for hiding this comment

SimonDarksideJ Jan 14, 2020

Choose a reason for hiding this comment

Troy-Ferrell Jan 15, 2020

Choose a reason for hiding this comment

SimonDarksideJ Jan 14, 2020

Choose a reason for hiding this comment

wiwei commented Jan 14, 2020

azure-pipelines bot commented Jan 14, 2020

Troy-Ferrell commented Jan 8, 2020 •

edited

Loading