Refactor SolverHandler to fix multiple issues #5416

Troy-Ferrell · 2019-07-24T21:00:50Z

Overview

The solver system has many issues (most of which are posted on GitHub). One of the key limitations for the feature is involved in the SolverHandler class. This change addresses multiple issues:

Cleaner understanding of what kind of target type is active
=> Simpler types => Head, MotionController, HandJoint, & CustomOverride
=> Inspector now shows valid sub-properties based on the target type
Developers can define order of solvers in code (via SetSolvers)
Solvers can now be correctly tracked/instantiated at runtime dynamically
Solvers can now follow any hand or motion controller (defaults to Left first if set to Both/Any)
Removed OnValidate from Solver.cs as this is generally poor practice and give inconsistent results
Removed nonexisting property for InBetween.cs
Added solvertests to tests package (See verification section below for details)
Fixed SurfaceMagnetism
=> Renamed properties to not assume always head tracked transform
=> Created new inspector to make properties easier to understand & group
=> Updated MaxDistance for raycast default from 3m to 50m
=> Updated default Raycast Direction Mode to Tracked Target Forward (ray along head gaze or hand pointer, etc)
Fixed InBetween
=> Updated names to be clearer
=> Updated refresh of second solver to be accurate
=> Updated tooltips to be accurate
=> Modified inspector to extend from SolverInspector

NOTE: This changes property names and will break any existing code that tried to extend Solvers themselves

New Solver Handler Targeting

Hand Switching Feature:

New Editor Inspectors:

New Solver Tests:

Changes

Fixes: SolverHandler: Changing transformTarget does not recalculate the Solver #5141 , Allow manual ordering of Solvers for a SolverHandler #5110 , Solver to follow any hand (not one or the other) #4642 , Solvers (i.e RadialView) cannot be instantiated at runtime #4788 , InBetweenInspector/-Editor serializes non existing field #5143

Verification

Added Solver Tests!

Furthermore, I will follow up this PR with another one to fix the Solver Example scene which is mostly broken.

Test dynamic instantiation of solver (Orbital)
Test switch hands (Orbital)
Test switch target types at runtime (Orbital)
Test SurfaceMagnetism (simple raycast against "wall" tracked against head*)
Test InBetween (between two cubes & modifies PartwayOffset)

Tested basic functionality manually

VR Motion Controllers
Editor Hands Simulation
HL2 Hands on Device
Custom override with transform in scene
Dynamically add component to gameobject (both in scene editor & via c# code)
Modify SolverHandler values in Inspector at runtime

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

Troy-Ferrell · 2019-07-24T21:21:49Z

/azp run

azure-pipelines · 2019-07-24T21:22:05Z

Azure Pipelines successfully started running 2 pipeline(s).

Troy-Ferrell · 2019-07-26T01:51:15Z

/azp run

azure-pipelines · 2019-07-26T01:51:30Z

Azure Pipelines successfully started running 2 pipeline(s).

…or example if TrackedHandedness is both)

Troy-Ferrell · 2019-07-26T19:18:17Z

/azp run

azure-pipelines · 2019-07-26T19:18:33Z

Azure Pipelines successfully started running 2 pipeline(s).

Troy-Ferrell · 2019-07-26T19:30:16Z

@keveleigh / @wiwei are there any additional steps I need to do for a breaking change PR?

I know I need to update documentation but I will do that in the follow up PR. I need to fix the examples scene as well. This PR is already getting pretty big and I don't want to include those two steps here.

Cameron-Micka

Looks great! I merged this into my HandConstrait experimental to release branch and everything worked/merged nicely.

I tried the example scene (which was pretty broken before this PR) and everything behaved as I would expect except the surface magnetism solver. Might be worth a sanity check since I'm not 100% sure how it should behave?

Could you also add a note to:
https://microsoft.github.io/MixedRealityToolkit-Unity/Documentation/UpdatingToGA.html for any breaking changes?

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

Troy-Ferrell · 2019-07-26T19:56:06Z

Looks great! I merged this into my HandConstrait experimental to release branch and everything worked/merged nicely.

I tried the example scene (which was pretty broken before this PR) and everything behaved as I would expect except the surface magnetism solver. Might be worth a sanity check since I'm not 100% sure how it should behave?

Could you also add a note to:
https://microsoft.github.io/MixedRealityToolkit-Unity/Documentation/UpdatingToGA.html for any breaking changes?

@Cameron-Micka
yes, the example scene has been broken. I want to do a follow up PR for both the documentation and the example scene updates. This current PR is just getting very big

…ver-handler-fixes

Assets/MixedRealityToolkit.SDK/Inspectors/Utilities/Solvers/SurfaceMagnetismInspector.cs

Assets/MixedRealityToolkit.Tests/PlayModeTests/SolverTests.cs

wiwei · 2019-07-31T00:39:44Z

Thank you SO much for doing this.

I have a bunch of comments, but I think we can work through them and get them addressed reasonably.

I wanted to call out that this is a canonical example of a well done PR that includes great tests for features that should have tests a long time ago.

Because of this I feel 100% more confident making changes to solvers going forward

Cameron-Micka · 2019-07-31T01:19:25Z

@wiwei just a heads up my upcoming hand constraint pull request depends on this pull request. So, please let me know if you and Troy need help getting this in.

Troy-Ferrell · 2019-07-31T05:02:36Z

@wiwei thanks for the very helpful comments! I know it's a substantial change.

@Cameron-Micka I just skimmed through most of the items and I think this is very doable to cover pretty quickly tomorrow morning. It also actually gave me a potential idea to help people with the migration/breaking change. It's particularly around that target type enum

Troy-Ferrell · 2019-07-31T05:17:10Z

public enum TrackedObjectType
The changes to this enum are a bitttttt scarry because they feel so central to existing solver usage.

Do you know if the existing usages of enums are actually serialized as enum names, or enum values? i.e. if someone had configured to have a solver follow the MotionControllerRight before, when they load up their serialized asset, will they suddenly get HandJoint?

If this is the case I would almost rather keep the old values around deprecated with a warning to use the new ones (we'd have to do some "map old to new helper" in order to make use of them).

It's on me to write up the plan for how we get rid of things that are deprecated (i.e. timeline).

Refers to: Assets/MixedRealityToolkit/Definitions/Utilities/TrackedObjectType.cs:6 in 3aeb246. [](commit_id = 3aeb246, deletion_comment = False)

I'm 99% sure they are serialized as numerical values. I'll double check though. Yes your points give me an idea. We could keep the old values, mark as obsolete, and then add on the new values I listed. Then in either OnValidate or in the inspector/awake, check if they are deprecated and just auto switch to the correct value and modify TrackedHandedness as well

Troy-Ferrell · 2019-07-31T18:46:10Z

/azp run

azure-pipelines · 2019-07-31T18:46:26Z

Azure Pipelines successfully started running 2 pipeline(s).

wiwei

@Troy-Ferrell

## Overview This pull request moves the HandConstraint (and HandConstraintPalmUp) solver from Experimental to Release by adding tests, documentation, and a handful of bug fixes and improvements. Functionality remains pretty much the same from the previous pull request (#4532) and utilizes solver fixes from @Troy-Ferrell's (#5416) The solver has also be genericized to work with motion controllers. Thank you @julenka for guidance on how to write unit tests that utilize hands! ## Changes - Fixes: #5312, #5304, #5257, #5413 ## Commits * Removing global input handler warning. * Wip check-in, new rotation behavior. * Disable hand tracking on hands which are currently focus locked. * Adding additional hand menu features and documentation improvements. * Hand constraint example improvements. * Hacks to get solver to not throw null pointers when instantiated. Should be reverted * Skeleton test for ensuring hand menu follows hand * Comment and example improvements. * autoTransitionBetweenHands is now set via the tracked handness of the solver handler * Adding support for motion controllers. * Updating example to support new solver changes. * Moving hand based menus out of the experimental namespace. * Adding hand constraint documentation. * Addressing pr feedback. * Updating example scene to use "new" toggle buttons. * Improving documentation, especially around IMixedRealityControllers usage. * Fixing broken documentation. * Update Documentation/README_Solver.md

@Troy-Ferrell

…Constraint (and HandConstraintPalmUp) solver from Experimental to Release by adding tests, documentation, and a handful of bug fixes and improvements. Functionality remains pretty much the same from the previous pull request (#4532) and utilizes solver fixes from @Troy-Ferrell's (#5416) The solver has also be genericized to work with motion controllers. Thank you @julenka for guidance on how to write unit tests that utilize hands! ## Changes - Fixes: #5312, #5304, #5257, #5413 ## Commits * Removing global input handler warning. * Wip check-in, new rotation behavior. * Disable hand tracking on hands which are currently focus locked. * Adding additional hand menu features and documentation improvements. * Hand constraint example improvements. * Hacks to get solver to not throw null pointers when instantiated. Should be reverted * Skeleton test for ensuring hand menu follows hand * Comment and example improvements. * autoTransitionBetweenHands is now set via the tracked handness of the solver handler * Adding support for motion controllers. * Updating example to support new solver changes. * Moving hand based menus out of the experimental namespace. * Adding hand constraint documentation. * Addressing pr feedback. * Updating example scene to use new toggle buttons. * Improving documentation, especially around IMixedRealityControllers usage. * Fixing broken documentation. * Update Documentation/README_Solver.md )

@Troy-Ferrell

…oft#5500) ## Overview This pull request moves the HandConstraint (and HandConstraintPalmUp) solver from Experimental to Release by adding tests, documentation, and a handful of bug fixes and improvements. Functionality remains pretty much the same from the previous pull request (microsoft#4532) and utilizes solver fixes from @Troy-Ferrell's (microsoft#5416) The solver has also be genericized to work with motion controllers. Thank you @julenka for guidance on how to write unit tests that utilize hands! ## Changes - Fixes: microsoft#5312, microsoft#5304, microsoft#5257, microsoft#5413 ## Commits * Removing global input handler warning. * Wip check-in, new rotation behavior. * Disable hand tracking on hands which are currently focus locked. * Adding additional hand menu features and documentation improvements. * Hand constraint example improvements. * Hacks to get solver to not throw null pointers when instantiated. Should be reverted * Skeleton test for ensuring hand menu follows hand * Comment and example improvements. * autoTransitionBetweenHands is now set via the tracked handness of the solver handler * Adding support for motion controllers. * Updating example to support new solver changes. * Moving hand based menus out of the experimental namespace. * Adding hand constraint documentation. * Addressing pr feedback. * Updating example scene to use "new" toggle buttons. * Improving documentation, especially around IMixedRealityControllers usage. * Fixing broken documentation. * Update Documentation/README_Solver.md

@Troy-Ferrell

…#5560) * Moving the HandConstraint Solver from Experimental to Release (#5500) ## Overview This pull request moves the HandConstraint (and HandConstraintPalmUp) solver from Experimental to Release by adding tests, documentation, and a handful of bug fixes and improvements. Functionality remains pretty much the same from the previous pull request (#4532) and utilizes solver fixes from @Troy-Ferrell's (#5416) The solver has also be genericized to work with motion controllers. Thank you @julenka for guidance on how to write unit tests that utilize hands! ## Changes - Fixes: #5312, #5304, #5257, #5413 ## Commits * Removing global input handler warning. * Wip check-in, new rotation behavior. * Disable hand tracking on hands which are currently focus locked. * Adding additional hand menu features and documentation improvements. * Hand constraint example improvements. * Hacks to get solver to not throw null pointers when instantiated. Should be reverted * Skeleton test for ensuring hand menu follows hand * Comment and example improvements. * autoTransitionBetweenHands is now set via the tracked handness of the solver handler * Adding support for motion controllers. * Updating example to support new solver changes. * Moving hand based menus out of the experimental namespace. * Adding hand constraint documentation. * Addressing pr feedback. * Updating example scene to use "new" toggle buttons. * Improving documentation, especially around IMixedRealityControllers usage. * Fixing broken documentation. * Update Documentation/README_Solver.md * Update Documentation/README_Solver.md Co-Authored-By: David Kline <davidkl@microsoft.com>

@Troy-Ferrell

…rimental to Release (#5500) ## Overview This pull request moves the HandConstraint (and HandConstraintPalmUp) solver from Experimental to Release by adding tests, documentation, and a handful of bug fixes and improvements. Functionality remains pretty much the same from the previous pull request (#4532) and utilizes solver fixes from @Troy-Ferrell's (#5416) The solver has also be genericized to work with motion controllers. Thank you @julenka for guidance on how to write unit tests that utilize hands! ## Changes - Fixes: #5312, #5304, #5257, #5413 ## Commits * Removing global input handler warning. * Wip check-in, new rotation behavior. * Disable hand tracking on hands which are currently focus locked. * Adding additional hand menu features and documentation improvements. * Hand constraint example improvements. * Hacks to get solver to not throw null pointers when instantiated. Should be reverted * Skeleton test for ensuring hand menu follows hand * Comment and example improvements. * autoTransitionBetweenHands is now set via the tracked handness of the solver handler * Adding support for motion controllers. * Updating example to support new solver changes. * Moving hand based menus out of the experimental namespace. * Adding hand constraint documentation. * Addressing pr feedback. * Updating example scene to use new toggle buttons. * Improving documentation, especially around IMixedRealityControllers usage. * Fixing broken documentation. * Update Documentation/README_Solver.md * Update Documentation/README_Solver.md Co-Authored-By: David Kline <davidkl@microsoft.com> )

Troy-Ferrell added 2 commits July 24, 2019 11:58

Update SolverHandler to fix multiple issues

708bfe4

Fixing code for PR

b9c2d64

Troy-Ferrell requested review from keveleigh, wiwei and Cameron-Micka July 24, 2019 21:00

Troy-Ferrell self-assigned this Jul 24, 2019

Troy-Ferrell added the Solvers - General label Jul 24, 2019

Fix onvalidate

c7b7d84

microsoft deleted a comment from azure-pipelines bot Jul 24, 2019

keveleigh added the BREAKING CHANGE label Jul 24, 2019

Troy-Ferrell added 6 commits July 24, 2019 16:10

Fix InBetweenInspector

ca9489c

Add solver tests for solverhandler behavior

0115692

Merge branch 'mrtk_development' into users/trferrel/solver-handler-fixes

40d3ea9

Add Solver inspector and fix surface magnetism

29673ca

Update solver tests for surface magnetism

2672908

Updating tests for InBetween and polishing InBetween component

8579cb9

Troy-Ferrell added 2 commits July 25, 2019 18:55

Surface what hand is currently being tracked by the solver handler (f…

b5cb15e

…or example if TrackedHandedness is both)

Merge branch 'mrtk_development' into users/trferrel/solver-handler-fixes

90050e7

Troy-Ferrell requested a review from julenka July 26, 2019 19:32

Cameron-Micka approved these changes Jul 26, 2019

View reviewed changes

Assets/MixedRealityToolkit.SDK/Features/Utilities/Solvers/Solver.cs Show resolved Hide resolved

Troy-Ferrell added 2 commits July 26, 2019 12:57

Remove serializedfield for solver.cs

69d2404

Merge branch 'prerelease/2.0.0_stabilization' into users/trferrel/sol…

3aeb246

…ver-handler-fixes

wiwei reviewed Jul 31, 2019

View reviewed changes

Assets/MixedRealityToolkit.SDK/Inspectors/Utilities/Solvers/SurfaceMagnetismInspector.cs Outdated Show resolved Hide resolved

wiwei reviewed Jul 31, 2019

View reviewed changes

Assets/MixedRealityToolkit.Tests/PlayModeTests/SolverTests.cs Show resolved Hide resolved

wiwei reviewed Jul 31, 2019

View reviewed changes

Assets/MixedRealityToolkit.Tests/PlayModeTests/SolverTests.cs Show resolved Hide resolved

wiwei reviewed Jul 31, 2019

View reviewed changes

Assets/MixedRealityToolkit.Tests/PlayModeTests/SolverTests.cs Show resolved Hide resolved

Updating fixes for PR

fe22c51

Troy-Ferrell requested a review from wiwei July 31, 2019 17:53

Fix ampersand in docs

3256b1f

microsoft deleted a comment from azure-pipelines bot Jul 31, 2019

Troy-Ferrell added 2 commits July 31, 2019 11:35

Fix function comparison

1ac5029

Tweak tests timing

5075230

wiwei approved these changes Aug 1, 2019

View reviewed changes

wiwei merged commit a8463be into microsoft:prerelease/2.0.0_stabilization Aug 1, 2019

Cameron-Micka mentioned this pull request Aug 2, 2019

Moving the HandConstraint Solver from Experimental to Release #5500

Merged

SimonDarksideJ mentioned this pull request Aug 3, 2019

Port updated Solvers functionality from MRTK XRTK/com.xrtk.core#260

Closed

Troy-Ferrell deleted the users/trferrel/solver-handler-fixes branch August 8, 2019 19:47

julenka mentioned this pull request Aug 9, 2019

Moving the HandConstraint Solver from Experimental to Release (#5500) #5560

Merged

Troy-Ferrell mentioned this pull request Aug 13, 2019

SolverHandler does not force a view #4932

Closed

julianloehr-kg mentioned this pull request Aug 15, 2019

Fix additional Solver added during runtime not being updated #5662

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor SolverHandler to fix multiple issues #5416

Refactor SolverHandler to fix multiple issues #5416

Troy-Ferrell commented Jul 24, 2019 •

edited

Troy-Ferrell commented Jul 24, 2019

azure-pipelines bot commented Jul 24, 2019

Troy-Ferrell commented Jul 26, 2019

azure-pipelines bot commented Jul 26, 2019

Troy-Ferrell commented Jul 26, 2019

azure-pipelines bot commented Jul 26, 2019

Troy-Ferrell commented Jul 26, 2019 •

edited

Cameron-Micka left a comment

Troy-Ferrell commented Jul 26, 2019 •

edited

wiwei commented Jul 31, 2019

Cameron-Micka commented Jul 31, 2019

Troy-Ferrell commented Jul 31, 2019

Troy-Ferrell commented Jul 31, 2019

Troy-Ferrell commented Jul 31, 2019

azure-pipelines bot commented Jul 31, 2019

wiwei left a comment

Refactor SolverHandler to fix multiple issues #5416

Refactor SolverHandler to fix multiple issues #5416

Conversation

Troy-Ferrell commented Jul 24, 2019 • edited

Overview

Changes

Verification

Troy-Ferrell commented Jul 24, 2019

azure-pipelines bot commented Jul 24, 2019

Troy-Ferrell commented Jul 26, 2019

azure-pipelines bot commented Jul 26, 2019

Troy-Ferrell commented Jul 26, 2019

azure-pipelines bot commented Jul 26, 2019

Troy-Ferrell commented Jul 26, 2019 • edited

Cameron-Micka left a comment

Choose a reason for hiding this comment

Troy-Ferrell commented Jul 26, 2019 • edited

wiwei commented Jul 31, 2019

Cameron-Micka commented Jul 31, 2019

Troy-Ferrell commented Jul 31, 2019

Troy-Ferrell commented Jul 31, 2019

Troy-Ferrell commented Jul 31, 2019

azure-pipelines bot commented Jul 31, 2019

wiwei left a comment

Choose a reason for hiding this comment

Troy-Ferrell commented Jul 24, 2019 •

edited

Troy-Ferrell commented Jul 26, 2019 •

edited

Troy-Ferrell commented Jul 26, 2019 •

edited