Merge pull request #9973 from Zee2/handlecollider-fix

Partial fix for BoundsControl collider issues and inconsistency

Author: Zee2

Assets/MRTK/SDK/Features/UX/Scripts/BoundsControl/Visuals/PerAxisHandles.cs

@@ -9,7 +9,7 @@
 namespace Microsoft.MixedReality.Toolkit.UI.BoundsControl
 {
     /// <summary>
-    /// Rotation handles for <see cref="BoundsControl"/> that are used for rotating the
+    /// Handles for <see cref="BoundsControl"/> that are used for manipulating the
     /// Gameobject BoundsControl is attached to with near or far interaction
     /// </summary>
     public abstract class PerAxisHandles : HandlesBase
@@ -75,6 +75,9 @@ private void UpdateColliderType()
                 // remove old colliders
                 bool shouldCreateNewCollider = false;
                 var oldBoxCollider = handle.GetComponent<BoxCollider>();
+
+                // Caution, Destroy() will destroy one frame later.
+                // Do not check later for presence this frame!
                 if (oldBoxCollider != null && config.HandlePrefabColliderType == HandlePrefabCollider.Sphere)
                 {
                     shouldCreateNewCollider = true;
@@ -92,7 +95,8 @@ private void UpdateColliderType()
                 {
                     // attach new collider
                     var handleBounds = VisualUtils.GetMaxBounds(GetVisual(handle).gameObject);
-                    var invScale = handleBounds.size.x == 0.0f ? 0.0f : config.HandleSize / handleBounds.size.x;
+                    float maxDim = VisualUtils.GetMaxComponent(handleBounds.size);
+                    float invScale = maxDim == 0.0f ? 0.0f : config.HandleSize / maxDim;
                     Vector3 colliderSizeScaled = handleBounds.size * invScale;
                     Vector3 colliderCenterScaled = handleBounds.center * invScale;
                     if (config.HandlePrefabColliderType == HandlePrefabCollider.Box)
@@ -106,7 +110,7 @@ private void UpdateColliderType()
                     {
                         SphereCollider sphere = handle.gameObject.AddComponent<SphereCollider>();
                         sphere.center = colliderCenterScaled;
-                        sphere.radius = colliderSizeScaled.x * 0.5f;
+                        sphere.radius = VisualUtils.GetMaxComponent(colliderSizeScaled) * 0.5f;
                         sphere.radius += VisualUtils.GetMaxComponent(config.ColliderPadding);
                     }
                 }
@@ -185,10 +189,12 @@ private void CreateHandles(Transform parent)
                 handle.transform.position = HandlePositions[i];
                 handle.transform.parent = parent;
 
-                Bounds midpointBounds = CreateVisual(i, handle);
-                float maxDim = VisualUtils.GetMaxComponent(midpointBounds.size);
+                Bounds handleVisualBounds = CreateVisual(i, handle);
+                float maxDim = VisualUtils.GetMaxComponent(handleVisualBounds.size);
                 float invScale = maxDim == 0.0f ? 0.0f : config.HandleSize / maxDim;
-                VisualUtils.AddComponentsToAffordance(handle, new Bounds(midpointBounds.center * invScale, midpointBounds.size * invScale),
+
+                // TODO: Some subclasses of PerAxisHandles shouldn't use CursorContextInfo.CursorAction.Rotate
+                VisualUtils.AddComponentsToAffordance(handle, new Bounds(handleVisualBounds.center * invScale, handleVisualBounds.size * invScale),
                     config.HandlePrefabColliderType, CursorContextInfo.CursorAction.Rotate, config.ColliderPadding, parent, config.DrawTetherWhenManipulating);
 
                 handles.Add(handle.transform);
@@ -209,11 +215,14 @@ protected override void RecreateVisuals()
                 {
                     // get old child and remove it
                     obsoleteChild.parent = null;
+
+                    // Caution, Destroy() will destroy one frame later.
+                    // Do not check later for presence this frame!
                     Object.Destroy(obsoleteChild.gameObject);
                 }
                 else
                 {
-                    Debug.LogError("couldn't find rotation visual on recreating visuals");
+                    Debug.LogError("Couldn't find handle visual on recreating visuals");
                 }
 
                 // create new visual
@@ -228,7 +237,8 @@ protected override void RecreateVisuals()
 
         protected override void UpdateColliderBounds(Transform handle, Vector3 visualSize)
         {
-            var invScale = visualSize.x == 0.0f ? 0.0f : config.HandleSize / visualSize.x;
+            float maxDim = VisualUtils.GetMaxComponent(visualSize);
+            float invScale = maxDim == 0.0f ? 0.0f : config.HandleSize / maxDim;
             GetVisual(handle).transform.localScale = new Vector3(invScale, invScale, invScale);
             Vector3 colliderSizeScaled = visualSize * invScale;
             if (config.HandlePrefabColliderType == HandlePrefabCollider.Box)
@@ -240,47 +250,53 @@ protected override void UpdateColliderBounds(Transform handle, Vector3 visualSiz
             else
             {
                 SphereCollider collider = handle.gameObject.GetComponent<SphereCollider>();
-                collider.radius = colliderSizeScaled.x * 0.5f;
+                collider.radius = VisualUtils.GetMaxComponent(colliderSizeScaled) * 0.5f;
                 collider.radius += VisualUtils.GetMaxComponent(config.ColliderPadding);
             }
         }
 
         private Bounds CreateVisual(int handleIndex, GameObject parent)
         {
-            GameObject midpointVisual;
+            GameObject handleVisual;
             GameObject prefabType = config.HandlePrefab;
             if (prefabType != null)
             {
-                midpointVisual = Object.Instantiate(prefabType);
+                handleVisual = Object.Instantiate(prefabType);
             }
             else
             {
-                midpointVisual = GameObject.CreatePrimitive(PrimitiveType.Sphere);
-                // deactivate collider on visuals and register for deletion - actual collider
-                // of handle is attached to the handle gameobject, not the visual
-                var collider = midpointVisual.GetComponent<SphereCollider>();
+                handleVisual = GameObject.CreatePrimitive(PrimitiveType.Sphere);
+                // We only want the Primitive sphere mesh, but CreatePrimitive will
+                // give us a sphere collider too. Remove the sphere collider here
+                // so we can manually add our own properly configured collider later.
+                var collider = handleVisual.GetComponent<SphereCollider>();
                 collider.enabled = false;
-                Object.Destroy(collider);
-            }
 
-            Quaternion realignment = GetRotationRealignment(handleIndex);
-            midpointVisual.transform.localRotation = realignment * midpointVisual.transform.localRotation;
+                // Caution, Destroy() will destroy one frame later.
+                // Do not check later for presence this frame!
+                Object.Destroy(collider);
+            }           
 
-            Bounds midpointBounds = VisualUtils.GetMaxBounds(midpointVisual);
-            float maxDim = VisualUtils.GetMaxComponent(midpointBounds.size);
+            // handleVisualBounds are returned in handleVisual-local space.
+            Bounds handleVisualBounds = VisualUtils.GetMaxBounds(handleVisual);
+            float maxDim = VisualUtils.GetMaxComponent(handleVisualBounds.size);
             float invScale = maxDim == 0.0f ? 0.0f : config.HandleSize / maxDim;
 
-            midpointVisual.name = visualsName;
-            midpointVisual.transform.parent = parent.transform;
-            midpointVisual.transform.localScale = new Vector3(invScale, invScale, invScale);
-            midpointVisual.transform.localPosition = Vector3.zero;
+            handleVisual.name = visualsName;
+            handleVisual.transform.parent = parent.transform;
+            handleVisual.transform.localScale = new Vector3(invScale, invScale, invScale);
+            handleVisual.transform.localPosition = Vector3.zero;
+            handleVisual.transform.localRotation = Quaternion.identity;
+
+            Quaternion realignment = GetRotationRealignment(handleIndex);
+            parent.transform.localRotation = realignment;
 
             if (config.HandleMaterial != null)
             {
-                VisualUtils.ApplyMaterialToAllRenderers(midpointVisual, config.HandleMaterial);
+                VisualUtils.ApplyMaterialToAllRenderers(handleVisual, config.HandleMaterial);
             }
 
-            return midpointBounds;
+            return handleVisualBounds;
         }
 
         #region BoundsControlHandlerBase 

Assets/MRTK/SDK/Features/UX/Scripts/BoundsControl/Visuals/ProximityEffect/ProximityEffect.cs

@@ -276,8 +276,9 @@ private void ScaleObject(ProximityState state, Transform scaleVisual, float obje
                     weight = lerp ? config.CloseGrowRate : 1.0f;
                     break;
             }
-
-            float newLocalScale = (scaleVisual.localScale.x * (1.0f - weight)) + (objectSize * targetScale * weight);
+            
+            float maxScaleAxis = VisualUtils.GetMaxComponent(scaleVisual.localScale);
+            float newLocalScale = (maxScaleAxis * (1.0f - weight)) + (objectSize * targetScale * weight);
             scaleVisual.localScale = new Vector3(newLocalScale, newLocalScale, newLocalScale);
         }
 

Assets/MRTK/Tests/PlayModeTests/BoundsControlTests.cs

@@ -2083,6 +2083,91 @@ public IEnumerator PerAxisHandlePrefabTest([ValueSource("perAxisHandleTestData")
             yield return null;
         }
 
+        /// <summary>
+        /// Test for verifying automatically generated handle colliders are properly aligned to the handle visual
+        /// </summary>
+        [UnityTest]
+        public IEnumerator PerAxisHandleAlignmentTest([ValueSource("perAxisHandleTestData")] PerAxisHandleTestData testData)
+        {
+            var boundsControl = InstantiateSceneAndDefaultBoundsControl();
+            yield return VerifyInitialBoundsCorrect(boundsControl);
+
+            // Create an oblong-shaped handle
+            // (cylinder primitive will do, as it is longer than it is wide!)
+            var cylinder = GameObject.CreatePrimitive(PrimitiveType.Cylinder);
+            GameObject.Destroy(cylinder.GetComponent<CapsuleCollider>());
+
+            // Wait for Destroy() to do its thing
+            yield return null;
+
+            // Set the rotation handles to be cylinders!
+            System.Reflection.PropertyInfo propName = boundsControl.GetType().GetProperty(testData.configPropertyName);
+            PerAxisHandlesConfiguration config = (PerAxisHandlesConfiguration)propName.GetValue(boundsControl);
+            config.HandlePrefab = cylinder;
+
+            // Reflection voodoo to retrieve the ColliderPadding value regardless of which
+            // handle configuration subclass we're currently using
+            System.Type configType = config.GetType();
+            var paddingProperty = configType.GetProperty("ColliderPadding");
+            Vector3 padding = (Vector3)paddingProperty.GetValue(config);
+
+            // Wait for BoundsControl to update to new handle prefab/rebuild rig
+            yield return null;
+            yield return new WaitForFixedUpdate();
+
+            // Iterate over all handle transforms
+            foreach(Transform handle in boundsControl.transform.Find("rigRoot"))
+            {   
+                // Is this the handle type we're currently looking for?
+                if(handle.name.StartsWith(testData.handleName))
+                {
+                    BoxCollider handleCollider = handle.GetComponent<BoxCollider>();
+
+                    Vector3[] colliderPoints = new Vector3[8];
+                    Vector3[] globalColliderPoints = new Vector3[8];
+
+                    // Strip the padding off the collider bounds, so that these bounds will match up
+                    // correctly with the visual bounds, if the alignment was properly done.
+                    VisualUtils.GetCornerPositionsFromBounds(new Bounds(handleCollider.center, handleCollider.size - padding), ref colliderPoints);
+
+                    // Perform a local-global transformation on all corners of the local collider bounds.
+                    for(int i = 0; i < colliderPoints.Length; ++i)
+                    {
+                        globalColliderPoints[i] = handle.TransformPoint(colliderPoints[i]);
+                    }
+
+                    Transform visual = handle.GetChild(0);
+                    Bounds handleBounds = VisualUtils.GetMaxBounds(visual.gameObject);
+
+                    Vector3[] visualPoints = new Vector3[8];
+                    Vector3[] globalVisualPoints = new Vector3[8];
+
+                    VisualUtils.GetCornerPositionsFromBounds(handleBounds, ref visualPoints);
+
+                    // Perform a local-global transformation on all corners of the local visual handle bounds.
+                    for(int i = 0; i < visualPoints.Length; ++i)
+                    {
+                        globalVisualPoints[i] = visual.TransformPoint(visualPoints[i]);
+                    }
+
+                    // Make sure all corners/vertices of the bounds are coherent after realignment, in global space
+                    bool flag = true;
+                    for(int i = 0; i < globalColliderPoints.Length; ++i)
+                    {
+                        if(globalColliderPoints[i] != globalVisualPoints[i])
+                        {
+                            flag = false;
+                            Debug.LogError($"Bounds mismatch, collider point: {globalColliderPoints[i].ToString("F3")}, visual point: {globalVisualPoints[i].ToString("F3")}");
+                        }
+                    }
+
+                    Assert.IsTrue(flag, "Handle collider does not match visual bounds, likely incorrect realignment of handle/visual transforms");
+                }
+            }
+
+            yield return null;
+        }
+
         /// <summary>
         /// Test for verifying changing the handle prefabs during runtime 
         /// in regular and flatten mode and making sure the entire rig won't be recreated