[Transformations] New Transformations system which replaces the old Solvers+Constraints system #11323
Conversation
| rotateManipulation = new RotateTransformation(this); | ||
| scaleManipulation = new ScaleTransformation(this); | ||
|
|
||
| moveManipulation.logic = Activator.CreateInstance(ManipulationLogicTypes.moveLogicType) as ManipulationLogic<Vector3>; |
There was a problem hiding this comment.
Are these MoveLogics going to be Transformations at some point?
There was a problem hiding this comment.
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
Show resolved
Hide resolved
com.microsoft.mrtk.spatialmanipulation/ObjectManipulator/ObjectManipulator.cs
Outdated
Show resolved
Hide resolved
…tManipulator.cs Co-authored-by: Kurtis <kurtie@microsoft.com>
| /// </remarks> | ||
| [RequireComponent(typeof(PlacementHub))] | ||
| [AddComponentMenu("MRTK/Spatial Manipulation/New Object Manipulator")] | ||
| public class NewObjectManipulator : StatefulInteractable |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
| /// <remarks> | ||
| /// Far smoothing is enabled by default. | ||
| /// </remarks> | ||
| public bool SmoothingFar |
There was a problem hiding this comment.
I thought you said you moved Smoothing into the placement hub?
There was a problem hiding this comment.
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
| rotateManipulation.logic.Setup(interactorsSelecting, this, initialTransform); | ||
| scaleManipulation.logic.Setup(interactorsSelecting, this, initialTransform); | ||
|
|
||
| placementHub.Transformations.Add(scaleManipulation); |
There was a problem hiding this comment.
Do we setup and add all of these transformations, regardless of the allowedManipulations (Scale|Rotate|Move) bitflags?
There was a problem hiding this comment.
Good catch, I'll gate these transformations working behind the bitflags
| } | ||
|
|
||
|
|
||
| public class MoveTransformation : ITransformation |
There was a problem hiding this comment.
Different files for these classes would be nice.
| } | ||
|
|
||
|
|
||
| public class MoveTransformation : ITransformation |
There was a problem hiding this comment.
Also, these should all be internal for now, right?
|
|
||
| namespace Microsoft.MixedReality.Toolkit.SpatialManipulation | ||
| { | ||
| public class PlacementHub : MonoBehaviour |
There was a problem hiding this comment.
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.
| /// <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 |
There was a problem hiding this comment.
See comment about smoothing; looks like we have smoothing in two different places now
There was a problem hiding this comment.
Addressed in : #11323 (comment)
|
|
| [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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Because ManipulationLogic is a Generic Inflated Type, it cannot be serialized with this attribute unfortunately :(
| @@ -33,7 +33,7 @@ public override float GetPropertyHeight(SerializedProperty property, GUIContent | |||
| return 0f; | |||
| } | |||
|
|
|||
| return base.GetPropertyHeight(property, label); | |||
There was a problem hiding this comment.
Is not calling into the base implementation okay here?
There was a problem hiding this comment.
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.
|
@srinjoym Can you take a look at this PR please |
|
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! |
|
@AMollis also an MRTK3 PR....... |
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