microsoft / MixedRealityToolkit-Unity

This repository is for the legacy Mixed Reality Toolkit (MRTK) v2. For the latest version of the MRTK please visit https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity
https://aka.ms/mrtkdocs
MIT License
6k stars 2.12k forks source link

InteractableRotationTheme is using world rotation as start value #7080

Closed BertrandOustriere closed 4 years ago

BertrandOustriere commented 4 years ago

Describe the bug

InteractableRotationTheme is using a world rotation for the interpolation's start value :

/// <inheritdoc />
public override ThemePropertyValue GetProperty(ThemeStateProperty property)
{
    ThemePropertyValue start = new ThemePropertyValue();
    start.Vector3 = hostTransform.eulerAngles;
    return start;
}

/// <inheritdoc />
public override void SetValue(ThemeStateProperty property, int index, float percentage)
{
    Vector3 lerpTarget = property.Values[index].Vector3;

    bool relative = Properties[0].Value.Bool;
    if (relative)
    {
        lerpTarget = originalRotation + lerpTarget;
    }

    hostTransform.localRotation = Quaternion.Euler(Vector3.Lerp(property.StartValue.Vector3, lerpTarget, percentage));
}

Since the interpolation is setting the local rotation in the SetValue function, this induces weird behaviour in some cases, for exemple when the parent transform of the rotated object has a rotation different from Vector3.zero

By using hostTransform.localEulerAngles in the GetProperty function, the issue seems to be fixed.

To reproduce

Steps to reproduce the behavior:

  1. Open the InteractableExamples scene
  2. Set a random rotation to the object Model_Bucky (for example I used 12,80,67)
  3. Press play and hover the object with your cursor to reach the focus state.

Your setup (please complete the following information)

Target platform (please complete the following information)

wiwei commented 4 years ago

Side note: I updated the PR description text and replaced hostTransform.localEulerAngles with hostTransform.eulerAngles, because I'm assuming that the text was intended to show the code as-is (with the issue that exists today), not the fix that was proposed.

wiwei commented 4 years ago

@thalbern it seems like this is actually just a bug due to the non-mirrored nature of the set/get (i.e. non local in one, local in the other) - it seems like making them be mirrored would be the right thing to do, but I guess it might also be a question of which one is the more 'correct' one. Do you have any background/thoughts on this one?

Otherwise there might be some compat concerns (i.e. people now taking a dependency on the weird state that exists today - which would also be fixable with another property/bit without too much difficulty)