Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 12 commits into from

Conversation

RogPodge
Copy link
Contributor

@RogPodge RogPodge commented Dec 26, 2022

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:
image

Changes

@RogPodge RogPodge requested a review from Zee2 as a code owner December 26, 2022 05:43
@github-actions github-actions bot added the MRTK3 label Dec 26, 2022
@RogPodge RogPodge added this to the MRTK3 GA milestone Dec 26, 2022
@RogPodge RogPodge marked this pull request as draft December 26, 2022 05:44
rotateManipulation = new RotateTransformation(this);
scaleManipulation = new ScaleTransformation(this);

moveManipulation.logic = Activator.CreateInstance(ManipulationLogicTypes.moveLogicType) as ManipulationLogic<Vector3>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these MoveLogics going to be Transformations at some point?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}


public class MoveTransformation : ITransformation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Different files for these classes would be nice.

}


public class MoveTransformation : ITransformation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


namespace Microsoft.MixedReality.Toolkit.SpatialManipulation
{
public class PlacementHub : MonoBehaviour
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

internal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in : #11323 (comment)

@RogPodge
Copy link
Contributor Author

RogPodge commented Jan 9, 2023

image

Improved the in-editor usability by a bit

Comment on lines +157 to +163
[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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is not calling into the base implementation okay here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@AMollis AMollis assigned AMollis and srinjoym and unassigned AMollis Mar 2, 2023
@AMollis
Copy link
Member

AMollis commented Mar 2, 2023

@srinjoym Can you take a look at this PR please

@AMollis
Copy link
Member

AMollis commented Jan 13, 2024

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 AMollis closed this Jan 13, 2024
@keveleigh
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants