Fixing a bug the causes FaceOrigin collections to face the wrong direction when collections are rotated (and some bonuses) #2229

Ecnassianer · 2018-06-01T20:15:17Z

This is a pull request for the completion of my proposal:
#2227

@paseb

…face the wrong direction when collecitons are rotated. Incidentally adds Radial collections. Previously collections with FaceOrigin selected as their Orientation Type which were not perfectly vertical would align their elements in crazy ways. This commit fixes this, but slightly changes the meaning of FaceOrigin so elements always face the origin, even if that means angling the element up or down to face the origin. This could be considered a breaking change, since all existing FaceOrigin object collections will change behavior. This is particularly true of Cylinder collections which are FaceOrigin orientation. They can be returned to the old behavior by switching to FaceCenterAxis, which keeps elements aligned with the collection's up vector. FaceCenterAxis also works well with spheres. In the process I also consolidated all the orientation switch statements into a single function that handles the orientation on all objection collections. Bringing these changes over also necessitated bringing in @paseb 's addition of Radial collections, that make cool fan-like things. Bonus feature! Co-Authored-By: Patrick Sebring <[email protected]>

…he motion controller

getting master June18 dev into this fork

Proposal: #2228 In the process, I fixed a few bugs, particularly with the custom inspectors. Solvers can now be switched to other tracked objects while playing in editor or at runtime via script. It also adds an offset and rotation to solver handlers so you can track a virtual point near by/rotated around an actual tracked object.

Update from upstream repo microsoft/MixedRealityToolkit-Unity@june18_dev

msftclas · 2018-06-01T20:15:28Z

All CLA requirements met.

Ecnassianer · 2018-06-01T20:20:04Z

That's strange. I've linked my Github account to my corporate account, and msftclas was cool with me yesterday.

StephenHodgson · 2018-06-01T20:40:57Z

Assets/HoloToolkit/UX/Scripts/Collections/CollectionEnums/OrientTypeEnum.cs

-        FaceForwardReversed,    // Zero rotation + 180 degrees
+        FaceParentFoward,       // Parent Relative Forwards
+        FaceParentBack,         // Parent Relative Backwards
+		FaceParentUp,           // Parent Relative Up


strange formatting

This extra tab doesn't show up in my local copy. Is this a merge artifact that I can fix on github?

It looks like it's because FaceParentUp is preceded with tabs, while the others are spaces.

Aha! Thank you!

StephenHodgson · 2018-06-01T20:41:21Z

Assets/HoloToolkit/UX/Scripts/Collections/ObjectCollection.cs

+            Radius = radius >= 0 ? Radius : radius;
+
+            Vector3 newPos = new Vector3(0f, 0f, (Radius/Rows) * row);
+            float yAngle = _radialCellAngle * (column - (_columns / 2f)) + (_radialCellAngle*.5f);


(_columns * 0.5f)

StephenHodgson · 2018-06-01T21:39:40Z

Assets/HoloToolkit-Examples/UX/Prefabs/Base Wedge.prefab

@@ -0,0 +1,134 @@
+%YAML 1.1
+%TAG !u! tag:unity3d.com,2011:


Could we replace the white space with an underscore for the name of this asset?

StephenHodgson · 2018-06-01T21:39:58Z

Assets/HoloToolkit-Examples/UX/Prefabs/Open Wedge.prefab

@@ -0,0 +1,134 @@
+%YAML 1.1
+%TAG !u! tag:unity3d.com,2011:


same for the name here as well, please.

StephenHodgson · 2018-06-01T21:40:12Z

Assets/HoloToolkit/UX/Scripts/Collections/CollectionEnums/OrientTypeEnum.cs

+        FaceParentBack,         // Parent Relative Backwards
+        FaceParentUp,           // Parent Relative Up
+        FaceParentDown,         // Parent Relative Down
+		FaceCenterAxis,         // Lay flat on the surface, facing in


bad formatting

StephenHodgson · 2018-06-01T21:40:43Z

Assets/HoloToolkit/UX/Scripts/Collections/ObjectCollection.cs

+        /// </summary>
+        [Range(5f, 360f)]
+        [Tooltip("Radial range for radial layout")]
+        public float RadialRange = 180f;


should be private and serialized, if access outside this class is required, then add a public property

Same thing for the rest of the fields below.

StephenHodgson · 2018-06-01T21:42:18Z

Assets/HoloToolkit/UX/Scripts/Collections/ObjectCollection.cs

+        /// <param name="node"></param>
+        /// <param name="orientType"></param>
+        /// <param name="newPos"></param>
+        private void UpdateNodeFacing(CollectionNode node, OrientTypeEnum orientType, Vector3 newPos = new Vector3())


instead of creating a new vector3, let's use Vector3.zero

Vector3.zero isn't a compile-time constant, so it can't be used as a default value. How about:

UpdateNodeFacing(CollectionNode node, OrientTypeEnum orientType, Vector3 newPos = default(Vector3))

Didn't think it would be, but it was worth a shot.

StephenHodgson · 2018-06-01T21:43:50Z

Assets/HoloToolkit/UX/Scripts/Collections/ObjectCollection.cs

+        /// <param name="row">This is a <see cref="int"/> for the radius of the cylinder</param>
+        /// <param name="column">This is a <see cref="int"/> for the radius of the cylinder</param>
+        /// <returns></returns>
+        private Vector3 RadialMapping(Vector3 source, float radius, int row, int column)


Do we not use the source?

Why not just update source with new values instead of creating a new vector3 each time?

Radius = radius >= 0 ? Radius : radius; source.x = 0f; source.y = 0f; source.z = (Radius/Rows) * row; float yAngle = _radialCellAngle * (column - (_columns * 0.5f)) + (_radialCellAngle * .5f); Quaternion rot = Quaternion.Euler(0.0f, yAngle, 0.0f); source = rot * source; return source;

Also, why not just do pure Quaternion math so we get better results without gimbal lock?

@paseb Can you weigh in on this one? I'm not sure what your intention was here.

StephenHodgson · 2018-06-02T18:45:18Z

Assets/HoloToolkit/UX/Scripts/Collections/CollectionEnums/OrientTypeEnum.cs

@@ -11,7 +11,11 @@ public enum OrientTypeEnum
        None,                   // Don't rotate at all
        FaceOrigin,             // Rotate towards the origin
        FaceOriginReversed,     // Rotate towards the origin + 180 degrees
-        FaceFoward,             // Zero rotation


Might want to keep these but mark them obsolete to prevent breaking changes.

Be sure to point people to which enum they should be using.

Unity serializes enums as integers, so as long as they're in the same position in the enum, the existing data reflects the rename. FaceForward and FaceForwardReversed will transition automatically to FaceParentForward and FaceParentBack.

As for references in code, they will be a compile error, but I hope these are easy enough to find and replace. I think this is a fair trade for getting it out of the inspector. I'll add a comment to the new value explaining its old name.

Relevant for upvotes:
https://feedback.unity3d.com/suggestions/enum-members-decorated-with-hideininspector-should-not-appear-in-an-enum-popup

Please make this change:

[Obsolete("Please use FaceParentFoward")] FaceFoward = FaceParentFoward, // Zero rotation [Obsolete("Please use FaceParentBack")] FaceForwardReversed = FaceParentBack, // Zero rotation + 180 degrees FaceParentFoward, // Parent Relative Forwards FaceParentBack, // Parent Relative Backwards FaceParentUp, // Parent Relative Up FaceParentDown, // Parent Relative Down FaceCenterAxis, // Lay flat on the surface, facing in FaceCenterAxisReversed // Lay flat on the surface, facing out

Hmm. I'm not familiar with setting an enum to another value in the enum, and your sample doesn't compile. Can you elaborate on what you're suggesting?

The compile error I'm getting is:
The evaluation of the constant value for 'OrientTypeEnum.FaceForwardReversed' involves a circular definition

Oh, this is because FaceParentBack is defined after FaceForwardReversed. Hold on.

This works:

FaceParentFoward, // Parent Relative Forwards, this used to be called FaceForward FaceParentBack, // Parent Relative Backwards, this used to be called FaceForwardReversed [Obsolete("Please use FaceParentFoward")] FaceFoward = FaceParentFoward, // Zero rotation [Obsolete("Please use FaceParentBack")] FaceForwardReversed = FaceParentBack, // Zero rotation + 180 degrees FaceParentUp, // Parent Relative Up FaceParentDown, // Parent Relative Down

But I almost think it's not worth the rename, since it replaces a confusing name in the inspector with an invalid confusing name and a clear name. Unless there's a way to fix that, I'll just put the names back to the old ones.

I just wrote it real quick, but I think you get the idea.

June18 dev --> solverexamples

…/MixedRealityToolkit-Unity into SolverExampleUpdates

… don't attach to an arbitrary controller before a Handedness is selected. Updated the custom inspector to a select a good default.

Update from upstream repo microsoft/MixedRealityToolkit-Unity@june18_dev

StephenHodgson · 2018-06-04T22:06:49Z

Assets/HoloToolkit/Input/Scripts/Utilities/AttachToController.cs

@@ -91,5 +91,11 @@ private void SetChildrenActive(bool isActive)
                }
            }
        }
+        void Reset()


missing private access modifier.

StephenHodgson · 2018-06-04T22:06:59Z

Assets/HoloToolkit/Input/Scripts/Utilities/AttachToController.cs

+        {
+            // We want the default value of Handedness of Controller finders to be Unknown so it doesn't attach to random object.
+            // But we also want the Editor to start with a useful default, so we set a Left handedness on inspector reset.
+            Handedness = UnityEngine.XR.WSA.Input.InteractionSourceHandedness.Left;


breaks cross platform support.

I haven't dealt with that yet. Do you mean to use an ifdef like this?

#if UNITY_2017_2_OR_NEWER using UnityEngine.XR.WSA.Input; #else using UnityEngine.VR.WSA.Input; #endif

yes, But I believe there's already a platform agnostic handedness enum you can use instead.

StephenHodgson

See comments

…ckwards compatibility in code.

…sn't supposed to go into june18_dev This reverts commit 576f2b9, reversing changes made to 6790fdb.

Ecnassianer · 2018-06-04T22:47:35Z

My work in another PR accidentally got merged into this branch. I'm taking all the ControllerFinder stuff out because it didn't belong here.

…assianer/MixedRealityToolkit-Unity into SolverExampleUpdates" because it never belonged here This reverts commit 9a2bef8, reversing changes made to 7cf6e78.

…ver belonged here This reverts commit 6aac3a8, reversing changes made to cba36c7.

keveleigh · 2018-06-04T23:49:56Z

Something somewhere has gone wrong:

Ecnassianer · 2018-06-05T00:22:33Z

Yes, you can ignore this until I learn enough about git to rescue myself. XD

Eric Carter and others added 8 commits May 30, 2018 19:11

Fixed a merge mistake that put a , in the wrong spot

134251b

Updated the solver example with more complex solvers that attach to t…

d0bbc18

…he motion controller

Merge pull request #3 from Microsoft/june18_dev

2f89074

getting master June18 dev into this fork

Merge pull request #5 from Microsoft/june18_dev

792fa0c

Update from upstream repo microsoft/MixedRealityToolkit-Unity@june18_dev

Updated the collection examples map to use the new orientation types

bc8ecd6

added a rotated cylinder collection

00e3266

keveleigh requested a review from paseb June 1, 2018 20:38

StephenHodgson reviewed Jun 1, 2018

View reviewed changes

Eric Carter added 3 commits June 1, 2018 13:53

Added an example of a radial collection to the object collection scene

a4f03ba

Multiply by half instead of dividing by two

4c2b8bf

tabs--> spaces

a2dcecd

StephenHodgson reviewed Jun 1, 2018

View reviewed changes

Eric Carter added 5 commits June 1, 2018 18:02

fixed extraneous spaces in prefab names

3ca8c06

cleaned up public properties

6eb0cb4

tabs --> spaces

4fea185

updated the radial solver example with new public properties

3afe7f3

new Vector3() --> default(Vector3) and some comment cleanup

23a7c55

StephenHodgson added Ready for Review Input System - General labels Jun 2, 2018

StephenHodgson reviewed Jun 2, 2018

View reviewed changes

Eric Carter and others added 9 commits June 4, 2018 11:21

object collection map improvements

7cf6e78

added some comments explaining the enum rename

cba36c7

Merge branch 'SolverExampleUpdates' into june18_dev

6aac3a8

Merge pull request #9 from Ecnassianer/june18_dev

9d96421

June18 dev --> solverexamples

Merge branch 'SolverExampleUpdates' of https://github.com/Ecnassianer…

9a2bef8

…/MixedRealityToolkit-Unity into SolverExampleUpdates

cleaned up Handedness

ea622c2

Switched the default value of Handedness in ControllerFinders so they…

92b801c

… don't attach to an arbitrary controller before a Handedness is selected. Updated the custom inspector to a select a good default.

Merge pull request #7 from Microsoft/june18_dev

6790fdb

Update from upstream repo microsoft/MixedRealityToolkit-Unity@june18_dev

Merge branch 'SolverExampleUpdates' into june18_dev

576f2b9

Ecnassianer mentioned this pull request Jun 4, 2018

Solver example updates #2245

Merged

StephenHodgson reviewed Jun 4, 2018

View reviewed changes

StephenHodgson suggested changes Jun 4, 2018

View reviewed changes

Eric Carter added 3 commits June 4, 2018 15:18

Added XR input using and a explicit private on the Reset()

e155043

Put the FaceForward and FaceForwardReversed names back to maintain ba…

3108754

…ckwards compatibility in code.

Revert "Merge branch 'SolverExampleUpdates' into june18_dev", this wa…

d0727c1

…sn't supposed to go into june18_dev This reverts commit 576f2b9, reversing changes made to 6790fdb.

Eric Carter added 2 commits June 4, 2018 16:20

Revert "Merge branch 'SolverExampleUpdates' of https://github.com/Ecn…

b7e207e

…assianer/MixedRealityToolkit-Unity into SolverExampleUpdates" because it never belonged here This reverts commit 9a2bef8, reversing changes made to 7cf6e78.

Revert "Merge branch 'SolverExampleUpdates' into june18_dev", this ne…

c16fbd0

…ver belonged here This reverts commit 6aac3a8, reversing changes made to cba36c7.

Ecnassianer closed this Jun 5, 2018

Ecnassianer mentioned this pull request Aug 20, 2018

MRTK SDK UX - ObjectCollection #2606

Merged

Fixing a bug the causes FaceOrigin collections to face the wrong direction when collections are rotated (and some bonuses) #2229

Fixing a bug the causes FaceOrigin collections to face the wrong direction when collections are rotated (and some bonuses) #2229

Conversation

Ecnassianer commented Jun 1, 2018

msftclas commented Jun 1, 2018 • edited Loading

Ecnassianer commented Jun 1, 2018

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

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

Ecnassianer Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenHodgson left a comment

Choose a reason for hiding this comment

Ecnassianer commented Jun 4, 2018 • edited Loading

keveleigh commented Jun 4, 2018 • edited Loading

Ecnassianer commented Jun 5, 2018

msftclas commented Jun 1, 2018 •

edited

Loading

Ecnassianer Jun 4, 2018 •

edited

Loading

Ecnassianer commented Jun 4, 2018 •

edited

Loading

keveleigh commented Jun 4, 2018 •

edited

Loading