Refactor tap to place and fix a few issues #874

Zod- · 2017-08-17T22:20:58Z

FIrst I only wanted to do some cleanup but then found a few issues in the TapToPlace test scene:

Placing an object on another one will put both in IsBeingPlaced. Fixed this by setting TapToPlace as Modal rather than just a global listener.
When moving a child with PlaceParentOnTap the parent or siblings can still get hit by ray cast because only the child itself gets the IgnoreRayCast layer. Setting the parent recursively instead when it's used.
RoomMesh was missing in the test scene.

I refactored quite a bit of the code as it was very cryptic what it was doing like that nested tertiary operator with a ray cast. I've split the SetLayerRecursively method into 2 parts to make it clear that it does cache and moved it into GameObjectExtensions. There was another SetLayerRecursively in Utils that I've also moved in there and set obsolete.

void SetLayerRecursively(this GameObject root, int layer)
void SetLayerRecursively(this GameObject root, int layer, out Dictionary<GameObject, int> cache)
void ApplyLayerCacheRecursively(this GameObject root, Dictionary<GameObject, int> cache)

I've separated the traverse of the transform hierarchy and moved it into TransformExtensions, using a queue instead of recursive method calls.

IEnumerable<Transform> IterateHierarchy(this Transform root)
IEnumerable<Transform> IterateHierarchy(this Transform root, ICollection<Transform> ignore)
e.g.
foreach (var child in root.transform.IterateHierarchy())

The new extension methods are covered by several integration tests since we need some of those.

There are still a few methods that all use their own way of doing a recursive traverse, could probably clean those up too.

These 3 methods are still in TapToPlace but could be moved somewhere else, if so I wasn't sure where to put them.

static Vector3 GetPlacementPosition(Vector3 headPosition, Vector3 gazeDirection, float defaultGazeDistance)
static bool SpatialMappingRaycast(Vector3 origin, Vector3 direction, out RaycastHit spatialMapHit)
static Vector3 GetGazePlacementPosition(Vector3 headPosition, Vector3 gazeDirection, float defaultGazeDistance)

Rework parent finding and validation. Extract interpolator code into method. Extract start and stop from HandlePlacement.

Move Utils.SetLayerRecursively into GameObjectsExtensions and set it obsolete.

…lkit-Unity into Refactor_TapToPlace

Siblings or the parent itself will still be able to hit by raycasts, constantly moving it into the user.

Also layer cache target name

msftclas · 2017-08-17T22:21:02Z

@Zod-,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

StephenHodgson

Looks great!

Just a few comments.

StephenHodgson · 2017-08-18T02:47:43Z

Assets/HoloToolkit-Tests/Utilities/Scripts/Extensions/Editor/GameObjectExtensionsTests.cs

+using NUnit.Framework;
+using UnityEngine;
+
+namespace HoloToolkit.Unity.Tests


This is awesome. Just one suggestion is to move this into HoloToolkit-UnitTests/Utilities/Extensions folder.

They're technically integration tests since they interact with the unity engine and instantiate several game objects in the tests.

No worries, but that's where the other integration tests are located at the moment.

StephenHodgson · 2017-08-18T02:49:06Z

Assets/HoloToolkit/Input/Scripts/Interactions/TapToPlace.cs

-                {
-                    WorldAnchorManager.Instance.AttachAnchor(gameObject, SavedAnchorFriendlyName);
-                }
+                WorldAnchorManager.Instance.AttachAnchor(gameObject, SavedAnchorFriendlyName);


We should probably still check to make sure WorldAnchorManager.Instance != null. It's optional to have in the scene.

It's implied by the else, here without diff bloat:

if (WorldAnchorManager.Instance == null) { Debug.LogError("This script expects that you have a WorldAnchorManager component in your scene."); } else if (!IsBeingPlaced) {

~~In my PR addressing the updates to the WorldAnchorManager I actually remove this line. The WorldAnchorManger shouldn't be a required dependency.~~

Actually I did not. Looks like I also made the hard dependency. I think it would be nice if we didn't need to require it. The script should work as is.

StephenHodgson · 2017-08-18T02:50:24Z

Assets/HoloToolkit/Input/Scripts/Interactions/TapToPlace.cs

+        {
+            if (!ParentGameObjectToPlace)
+            {
+                Debug.LogWarning("No parent has been set or found.");


Do we really want to spam log warnings if there isn't a parent intentionally setup on purpose?

Good question. I actually had trouble keeping those log messages in from the old code, rather than just throwing them away.

StephenHodgson · 2017-08-18T02:52:04Z

Assets/HoloToolkit/Input/Scripts/Interactions/TapToPlace.cs

-                //Removes existing world anchor if any exist.
-                WorldAnchorManager.Instance.RemoveAnchor(gameObject);
+            //Removes existing world anchor if any exist.
+            WorldAnchorManager.Instance.RemoveAnchor(gameObject);


We should check the instance here too. WorldAnchorManager should be optional.

StephenHodgson · 2017-08-18T02:52:48Z

Assets/HoloToolkit/Input/Scripts/Interactions/TapToPlace.cs

+        {
+            var layerCacheTarget = PlaceParentOnTap ? ParentGameObjectToPlace : gameObject;
+            layerCacheTarget.SetLayerRecursively(IgnoreRaycastLayer, out layerCache);
+            InputManager.Instance.PushModalInputHandler(gameObject);


This is awesome. In fact, we probably don't even have to change the layers of the object or its children because of this change.

Modal shouldn't exclude from raycasts if I understood it correctly, I'll give it a try.

As expected the object will just keep moving towards the player when not setting the layers.

Thanks for checking.

StephenHodgson · 2017-08-18T02:53:43Z

Assets/HoloToolkit/Input/Scripts/Interactions/TapToPlace.cs

-                    ParentGameObjectToPlace = gameObject.transform.parent.gameObject;
-                }
-            }
+            SpatialMappingManager.Instance.DrawVisualMeshes = IsBeingPlaced && AllowMeshVisualizationControl;


SpatialMappingManager is also optional. We should check if we have a valid instance.

StephenHodgson · 2017-08-18T02:54:36Z

Assets/HoloToolkit/Input/Scripts/Interactions/TapToPlace.cs

+        /// <param name="headPosition"></param>
+        /// <param name="gazeDirection"></param>
+        /// <param name="defaultGazeDistance"></param>
+        /// <returns></returns>


We should probably add the descriptions of these parameters and what we're returning.

StephenHodgson · 2017-08-18T02:57:54Z

Assets/HoloToolkit/Utilities/Scripts/Extensions/TransformExtensions.cs

+
+                if (ignore.Contains(parent)) { continue; }
+
+                foreach (Transform child in parent)


suggestion: for loop instead of foreach

For the inner loop?

For any loop if possible. Just a perf optimization.

I guess parent.GetChild(index)? I won't be able to change the while loop as its dependent on the queue that constantly gets filled.

for (int i = 0; i < parent.childCount; i++) { parentsQueue.Enqueue(parent.GetChild(i)); }

Oh I see, parent isn't a Transform Component?

Partly why I avoid var when types are not evident. No worries then.

StephenHodgson · 2017-08-18T02:58:20Z

Assets/HoloToolkit/Utilities/Scripts/Utils.cs

@@ -10,14 +10,10 @@ namespace HoloToolkit.Unity
    /// </summary>
    public static class Utils
    {
+        [System.Obsolete("Use GameObjectExtensions.SetLayerRecursively(gameObject, layer) instead;")]


Awesome. Does this PR also update the core MRTK scripts that use this obsolete reference?

I did not find any references to this method.

Why are we not just deleting this class then?

I'm not sure what the strategy is for making methods obsolete. This is in the public Utils class so people could potentially use it in their projects. Do we just straight up remove methods or set them obsolete and delete them in 1-2 releases?

Like your approach of making it obsolete! Let's use it for now and see how it pans out.

@Zod- yup you're correct, generally we mark as obsolete and remove later after people get a chance to update their references.

Move attach/remove into separate methods.

Remove warnings about parents in TapToPlace Check SpatialMapper for null Added more docs to helper methods Renamed variables in IterateHierarchy Use for loop for children and move null check to the top of the loop with the other continue. Add a test for the null check

Zod- · 2017-08-19T03:18:03Z

Is there any reason there are 2 extensions for getting the full path of an object in a hierarchy?

There is:

TransformExtensions.GetFullPath(this Transform transform, string delimiter = ".", string prefix = "/")
GameObjectExtensions.GetFullPath(this GameObject go)

Each with different results and implementations. We could just link the GameObject one to the more customizable Transform one if we want to keep it.

NeerajW · 2017-08-21T19:09:13Z

Assets/HoloToolkit/Input/Scripts/Interactions/TapToPlace.cs

-
-                objectToSet.gameObject.layer = IgnoreRaycastLayer;
-            }
+            spatialMapHit = new RaycastHit();


Should this not new and instead caller should check for null? So hitInfo.point does not fail?

Like on the physics raycast the caller checks for the return to be true. You can't set the hit info to null since it's non-nullable.

StephenHodgson · 2017-08-22T16:04:05Z

Is there any reason there are 2 extensions for getting the full path of an object in a hierarchy?

I'm okay with removing one of them.

StephenHodgson · 2017-08-22T23:44:29Z

Assets/HoloToolkit/Utilities/Scripts/Extensions/GameObjectExtensions.cs

+            foreach (var child in root.transform.EnumerateHierarchy())
+            {
+                int layer;
+                if (!cache.TryGetValue(child.gameObject, out layer)) continue;


nit: braces

Damn I keep forgetting about those

jessemcculloch

Changes look great. I like all the tests. Well done @Zod-

Zod- · 2017-08-28T12:21:40Z

I've written some more tests in a branch based on this one that doesn't include any changes to classes in the main toolkit.

Zod- added 17 commits August 14, 2017 21:56

Refactor huge conditional

8b32615

Add room mesh to TapToPlace test scene

8a3ea89

Refactor start method

fb70777

Rework parent finding and validation. Extract interpolator code into method. Extract start and stop from HandlePlacement.

Add IterateHierarchy extension to transforms

23abcfb

Add EditorUtils with ClearScene method

d73d54d

Add missing license header

3cb2fee

Move layer caching logic into extensions

b6ac726

Move Utils.SetLayerRecursively into GameObjectsExtensions and set it obsolete.

Shorten null checks

685126d

Rework clear scene method by collecting all roots

0f4d251

Merge branch 'master' of https://github.com/Microsoft/MixedRealityToo…

537de1f

…lkit-Unity into Refactor_TapToPlace

Remove 2 unneeded caches

85c3548

Place Parent on Tap on one of the cubes

7d8986f

Use modal input rather than global

0b2481f

Apply ignore raycast layer to parent

97f8eff

Siblings or the parent itself will still be able to hit by raycasts, constantly moving it into the user.

Refactor spatial mesh update

6c276fc

Also layer cache target name

Move helper methods at the end

2195fe3

Unused variable

8ac47c8

msftclas added the cla-already-signed label Aug 17, 2017

StephenHodgson suggested changes Aug 18, 2017

View reviewed changes

Zod- added 4 commits August 19, 2017 03:35

Remove world anchor requirement and refactor

c533702

Move attach/remove into separate methods.

Move test files to UnitTests Project

260dd8f

Added tests for transform extensions GetFullPath

69ffbe9

StephenHodgson approved these changes Aug 19, 2017

View reviewed changes

Rename iterate hierarchy to enumerate

10311d2

NeerajW reviewed Aug 21, 2017

View reviewed changes

NeerajW approved these changes Aug 21, 2017

View reviewed changes

StephenHodgson reviewed Aug 22, 2017

View reviewed changes

Zod- added 3 commits August 24, 2017 19:24

Add missing braces and meta files

b1a072f

Add tests for GameObjectExtensions.GetFullPath

a8aa583

Mark GameObjectExtensions.GetFullPath obsolete

62edf1f

jessemcculloch approved these changes Aug 27, 2017

View reviewed changes

NeerajW merged commit 8ed1817 into microsoft:master Aug 30, 2017


		if (ignore.Contains(parent)) { continue; }

		foreach (Transform child in parent)

Refactor tap to place and fix a few issues #874

Refactor tap to place and fix a few issues #874

Conversation

Zod- commented Aug 17, 2017

msftclas commented Aug 17, 2017

StephenHodgson left a comment

Choose a reason for hiding this comment

StephenHodgson Aug 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenHodgson Aug 19, 2017 • edited

Choose a reason for hiding this comment

StephenHodgson Aug 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenHodgson Aug 18, 2017 • edited

Choose a reason for hiding this comment

StephenHodgson Aug 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zod- Aug 18, 2017 • edited

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

StephenHodgson Aug 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenHodgson Aug 18, 2017 • edited

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

StephenHodgson Aug 22, 2017 • edited

Choose a reason for hiding this comment

Zod- commented Aug 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenHodgson commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessemcculloch left a comment

Choose a reason for hiding this comment

Zod- commented Aug 28, 2017

StephenHodgson Aug 18, 2017 •

edited

StephenHodgson Aug 19, 2017 •

edited

StephenHodgson Aug 18, 2017 •

edited

StephenHodgson Aug 18, 2017 •

edited

StephenHodgson Aug 18, 2017 •

edited

Zod- Aug 18, 2017 •

edited

StephenHodgson Aug 18, 2017 •

edited

StephenHodgson Aug 18, 2017 •

edited

StephenHodgson Aug 22, 2017 •

edited

Zod- commented Aug 19, 2017 •

edited