[Transformations] New Transformations system which replaces the old Solvers+Constraints system #11323

RogPodge · 2022-12-26T05:43:42Z

Overview

This PR introduces the new Transformations system which aims to simply and improve on the previous spatial manipulation system which used a combination of solvers + constraints.

This PR includes a new ObjectManipulator which operates within the context of the new system, as well as tests.

The PlacementHub(name pending) is now the central hub for managing and applying transformations which are applied to an object within a virtual space.

Example showing how the new placement hub holds the information for both a min max constraint and the process of "solving" for the object manipulation:

Changes

Fixes: Refactor smoothing to be less "bad" #10807 Fixing issues with constraints and off-center ObjManip host transforms #11086 and more...

Zee2 · 2022-12-28T01:51:33Z

com.microsoft.mrtk.spatialmanipulation/ObjectManipulator/NewObjectManipulator.cs

+            rotateManipulation = new RotateTransformation(this);
+            scaleManipulation = new ScaleTransformation(this);
+
+            moveManipulation.logic = Activator.CreateInstance(ManipulationLogicTypes.moveLogicType) as ManipulationLogic<Vector3>;


Are these MoveLogics going to be Transformations at some point?

I think for sake of back compatibility we shouldn't remove the MoveLogics code into the Transformations class, at best we duplicate the code, but I'm a bit unsure of the best way to handle using a new interface while the code we need is still embedded in the older API.

@keveleigh for some perspective?

com.microsoft.mrtk.spatialmanipulation/Tests/Runtime/ObjectManipulatorTests.cs

com.microsoft.mrtk.spatialmanipulation/ObjectManipulator/ObjectManipulator.cs

…tManipulator.cs Co-authored-by: Kurtis <[email protected]>

com.microsoft.mrtk.spatialmanipulation/PlacementHub.cs

…alityToolkit-Unity into SolverRefactor2

Zee2 · 2023-01-04T21:20:25Z

com.microsoft.mrtk.spatialmanipulation/ObjectManipulator/NewObjectManipulator.cs

+    /// </remarks>
+    [RequireComponent(typeof(PlacementHub))]
+    [AddComponentMenu("MRTK/Spatial Manipulation/New Object Manipulator")]
+    public class NewObjectManipulator : StatefulInteractable


Mark internal, probably. I don't think we should stay with this naming. If we need to have the old and new objmanips side by side (I hope not) we should name it something new, like PlacementManipulator. Just want to avoid having public API surface that's explicitly named "NewThing"

I think after this naming hump, I want the "NewObjectManipulator" to be public, much like our previous one was. We've had prior examples of people extending our base manipulator, and I don't want to buck that expectation, since our other UI components are also marked as public currently.

Zee2 · 2023-01-04T21:20:46Z

com.microsoft.mrtk.spatialmanipulation/ObjectManipulator/NewObjectManipulator.cs

+        /// <remarks>
+        /// Far smoothing is enabled by default.
+        /// </remarks>
+        public bool SmoothingFar


I thought you said you moved Smoothing into the placement hub?

I left these controls in here in this draft because the original object Manipulator interface allows for different smoothing behavior for far/near or near interactions. This wasn't quite collapsible back down to the placement hub (which is not an interactable), so these bools are surfaced here and control the placementhub in the background.

The reasons for why it was separated out can be found here, basically, near smoothing didn't feel as good:
#8204
MicrosoftDocs/mixed-reality#291

Zee2 · 2023-01-04T21:22:20Z

com.microsoft.mrtk.spatialmanipulation/ObjectManipulator/NewObjectManipulator.cs

+                rotateManipulation.logic.Setup(interactorsSelecting, this, initialTransform);
+                scaleManipulation.logic.Setup(interactorsSelecting, this, initialTransform);
+
+                placementHub.Transformations.Add(scaleManipulation);


Do we setup and add all of these transformations, regardless of the allowedManipulations (Scale|Rotate|Move) bitflags?

Good catch, I'll gate these transformations working behind the bitflags

Zee2 · 2023-01-04T21:22:57Z

com.microsoft.mrtk.spatialmanipulation/ObjectManipulator/NewObjectManipulator.cs

+        }
+
+
+        public class MoveTransformation : ITransformation


Different files for these classes would be nice.

Zee2 · 2023-01-04T21:23:07Z

com.microsoft.mrtk.spatialmanipulation/ObjectManipulator/NewObjectManipulator.cs

+        }
+
+
+        public class MoveTransformation : ITransformation


Also, these should all be internal for now, right?

Zee2 · 2023-01-04T21:23:28Z

com.microsoft.mrtk.spatialmanipulation/PlacementHub.cs

+
+namespace Microsoft.MixedReality.Toolkit.SpatialManipulation
+{
+    public class PlacementHub : MonoBehaviour


internal?

After some additional considerations, I think after this naming hump, I want the "PlacementHub" to be public, as it contains a lot of the smoothing code from the ObjectManipulator, and we let that behavior be extensible in the past.

Zee2 · 2023-01-04T21:24:08Z

com.microsoft.mrtk.spatialmanipulation/PlacementHub.cs

+        /// <summary>
+        /// Enter amount representing amount of smoothing to apply to the scale. Smoothing of 0 means no smoothing. Max value means no change to value.
+        /// </summary>
+        public float ScaleLerpTime


See comment about smoothing; looks like we have smoothing in two different places now

Addressed in : #11323 (comment)

RogPodge · 2023-01-09T22:38:15Z

Improved the in-editor usability by a bit

keveleigh · 2023-01-11T01:12:49Z

com.microsoft.mrtk.spatialmanipulation/ObjectManipulator/NewObjectManipulator.cs

+            [SerializeField]
+            [Tooltip("The concrete type of ManipulationLogic<Vector3> to use for moving.")]
+            [Extends(typeof(ManipulationLogic<Vector3>), TypeGrouping.ByNamespaceFlat)]
+            /// <summary>
+            /// The concrete type of <see cref="ManipulationLogic"/> to use for moving.
+            /// </summary>
+            public SystemType moveLogicType;


I wonder if these could use @maluoi's new (to MRTK at least) pattern of [SerializeReference, InterfaceSelector]. I know this extends a non-interface, but I think that pattern should still work? Would save us the hassle of the Activator stuff too, potentially?

Because ManipulationLogic is a Generic Inflated Type, it cannot be serialized with this attribute unfortunately :(

keveleigh · 2023-01-11T01:23:03Z

com.microsoft.mrtk.core/Editor/Inspectors/PropertyDrawers/DrawIfPropertyDrawer.cs

@@ -33,7 +33,7 @@ public override float GetPropertyHeight(SerializedProperty property, GUIContent
                return 0f;
            }

-            return base.GetPropertyHeight(property, label);


Is not calling into the base implementation okay here?

Yeah, base will only return the height of the property without accounting for the children (i.e, a single line + field for most assets), while using the static method provides options for also including the children.

…kit-Unity into SolverRefactor2

AMollis · 2023-03-02T22:15:58Z

@srinjoym Can you take a look at this PR please

AMollis · 2024-01-13T01:09:31Z

We appreciate your contribution and thank you for the pull request.

Microsoft Mixed Reality Toolkit version 2 (MRTK2) is currently in limited support. This means that Microsoft is only fixing high priority issues. Unfortunately, this pull request does not meet the necessary priority and will be closed. If you strongly feel that this change deserves more attention, please open a new pull request and explain why it is important.

Microsoft recommends that all new HoloLens 2 Unity applications use MRTK3 instead of MRTK2.

Please note that MRTK3 was released in August 2023. It features an all-new architecture for developing rich mixed reality experiences and has a minimum requirement of Unity 2021.3 LTS. For more information about MRTK3, please visit https://www.mixedrealitytoolkit.org.

Thank you for your continued support of the Mixed Reality Toolkit!

keveleigh · 2024-01-13T01:11:52Z

@AMollis also an MRTK3 PR.......

Draft of new object manipulator implementation along with tests

50c58f0

RogPodge requested a review from Zee2 as a code owner December 26, 2022 05:43

github-actions bot added the MRTK3 label Dec 26, 2022

RogPodge added this to the MRTK3 GA milestone Dec 26, 2022

RogPodge marked this pull request as draft December 26, 2022 05:44

RogPodge added 2 commits December 26, 2022 21:27

fixed bugged object manipulator test

68a4bc6

fixing NewObjectManipulator and tests to pass

c18c3d5

Zee2 reviewed Dec 28, 2022

View reviewed changes

Fixing unit tests

d6346bb

keveleigh reviewed Jan 4, 2023

View reviewed changes

com.microsoft.mrtk.spatialmanipulation/Tests/Runtime/ObjectManipulatorTests.cs Show resolved Hide resolved

keveleigh reviewed Jan 4, 2023

View reviewed changes

com.microsoft.mrtk.spatialmanipulation/ObjectManipulator/ObjectManipulator.cs Outdated Show resolved Hide resolved

Update com.microsoft.mrtk.spatialmanipulation/ObjectManipulator/Objec…

5860c89

…tManipulator.cs Co-authored-by: Kurtis <[email protected]>

keveleigh reviewed Jan 4, 2023

View reviewed changes

RogPodge added 2 commits January 4, 2023 12:24

addressing PR asks

6c90dc7

Merge branch 'SolverRefactor2' of https://github.com/RogPodge/MixedRe…

d21d80c

…alityToolkit-Unity into SolverRefactor2

Zee2 reviewed Jan 4, 2023

View reviewed changes

RogPodge added 3 commits January 9, 2023 14:50

Refactoring the placementhub to improve usability

c4a12ae

Adjusted ITransformation to use MixedRealityTransform

ec8d0a2

revamping the new Object Manipulator Editor

f49d7fb

keveleigh reviewed Jan 11, 2023

View reviewed changes

RogPodge added 2 commits January 17, 2023 11:37

Merge branch 'mrtk3' of https://github.com/Microsoft/MixedRealityTool…

c9401e6

…kit-Unity into SolverRefactor2

renaming the inspector to editor

c5dfe63

AMollis added the Solvers - General label Feb 24, 2023

AMollis assigned AMollis and srinjoym and unassigned AMollis Mar 2, 2023

AMollis mentioned this pull request Mar 10, 2023

Refactor Solver System to be more extendible and user friendly #10759

Closed

srinjoym mentioned this pull request Apr 14, 2023

Evaluate new solver system in the context of the Object Manipulator #11459

Closed

AMollis added the DO NOT MERGE label Apr 27, 2023

AMollis modified the milestones: MRTK v3.0 GA, MRTK v3.x future Apr 27, 2023

AMollis closed this Jan 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Transformations] New Transformations system which replaces the old Solvers+Constraints system #11323

[Transformations] New Transformations system which replaces the old Solvers+Constraints system #11323

RogPodge commented Dec 26, 2022 •

edited

Loading

Zee2 Dec 28, 2022

RogPodge Dec 28, 2022

Zee2 Jan 4, 2023

RogPodge Jan 17, 2023

Zee2 Jan 4, 2023

RogPodge Jan 4, 2023

Zee2 Jan 4, 2023

RogPodge Jan 17, 2023

Zee2 Jan 4, 2023

Zee2 Jan 4, 2023

Zee2 Jan 4, 2023

RogPodge Jan 4, 2023

RogPodge Jan 17, 2023

Zee2 Jan 4, 2023

RogPodge Jan 17, 2023

RogPodge commented Jan 9, 2023

keveleigh Jan 11, 2023

RogPodge Jan 17, 2023

keveleigh Jan 11, 2023

RogPodge Jan 17, 2023

AMollis commented Mar 2, 2023

AMollis commented Jan 13, 2024

keveleigh commented Jan 13, 2024

[Transformations] New Transformations system which replaces the old Solvers+Constraints system #11323

[Transformations] New Transformations system which replaces the old Solvers+Constraints system #11323

Conversation

RogPodge commented Dec 26, 2022 • edited Loading

Overview

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RogPodge commented Jan 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AMollis commented Mar 2, 2023

AMollis commented Jan 13, 2024

keveleigh commented Jan 13, 2024

RogPodge commented Dec 26, 2022 •

edited

Loading