Resolved rotation issue in TwoHandManipulatable script

[shubhamarora28]

Overview

Issue:
If we change the rotation axis constraint at run time, it does not make any difference in the behaviour.

Fix: Reinitialize the rotate logic with the new rotation axis constraint every time the rotation constraint property is set.

Sample:
I have duplicated the TwoHandManipulationTest scene as TwoHandManipulationTest_Fix scene and have added the following game object into the scene content:

• Model_Bucky_Rotation_Constraints_Test: This has a test script(TestRotationConstraints) attached to it and is intended to depict the solution to the issue.

Changes

Changed TwoHandManipultable.cs
Added TestRotationConstraints.cs
Added TwoHandManipulationTest_Fix.unity

[msftclas]

All CLA requirements met.

[StephenHodgson]

The changes look good and the test scene works nicely, but I'm not sure we need to add the test scene with the fix name in it. You can either rename it as an example and remove the fix suffix, or remove the test scene all together. Up to you.

[StephenHodgson]

nit missing private access modifier.

[david-c-kline]

If not adding a fix specific test scene, I presume this file can be left out of the PR.

[StephenHodgson]

initialize with null

[david-c-kline]

You can either rename it as an example and remove the fix suffix, or remove the test scene all together. Up to you.

I'm good with not adding a new test scene.

[david-c-kline]

I am 100% good with the changes in TwoHandManipulatable.cs and agree with Stephen that we do not really need a specific test scene for this fix.

Please remove the test scene and associated scripts and it should be an easy approve.

[david-c-kline]

If not adding a fix specific test scene, I presume this file can be left out of the PR.

[shubhamarora28]

@davidkline-ms The test scene I had created has a game object(Model_Bucky_Rotation_Constraints_Test) that depicts the capability of TwoHandManipulatable to modify the rotation constraint at run time.
Instead of adding this as a new scene just for this fix, I was thinking if I can replace the original TwoHandManipulationTest scene with the one that I have added. Will that be okay?

[SimonDarksideJ]

Does this fix also need applying to vNext ? @davidkline-ms @StephenHodgson

[StephenHodgson]

We haven't ported it yet.

[david-c-kline]

@shubhamarora28, as long as we don't lose functionality in the existing example scene, I think that sounds fine.

Thanks!

[david-c-kline]

Does this fix also need applying to vNext ? @davidkline-ms @StephenHodgson

We haven't ported it yet.

It should definitely be added to #2465 for tracking.

[shubhamarora28]

@StephenHodgson @davidkline-ms Pushed a commit with all the changes.

new changes