Need to update collision mesh when committing mesh data #542

trzy · 2017-02-28T05:28:55Z

There appears to be a bug with SpatialUnderstandingCustomMesh: when finalizing the mesh data (with Commit()), the rendering mesh picks up the changes but the collision mesh does not. Presumably this is a quirk of how Unity handles collision meshes.

The result of this is that using meshes produced by the Spatial Understanding system (as opposed to the underlying meshes provided by SpatialMappingManager) doesn't work in scenarios where rigid bodies are expected to collide with the world.

This patch appears to be the minimum required change to fix the problem however it's a bit ugly: I have to set a reference to the MeshCollider in MeshData, which otherwise consists exclusively of readonly members. A proper fix would require modifying CreateSurfaceObject() but that is a bit more of an extensive modification and I think something along the line of my approach is simpler.

msftclas · 2017-02-28T05:28:59Z

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

StephenHodgson · 2017-02-28T14:51:17Z

So you're saying this may fix #228? (if so that would be awesome!)

StephenHodgson · 2017-02-28T14:55:39Z

Assets/HoloToolkit/SpatialUnderstanding/Scripts/SpatialUnderstandingCustomMesh.cs

@@ -98,6 +98,11 @@ private class MeshData
            public readonly Mesh MeshObject = new Mesh();

            /// <summary>
+            /// The MeshCollider with which this mesh is associated.
+            /// </summary>
+            public MeshCollider Collider = null;


I suggest using a more specific name. Collider is a name for a Unity class. I suggest SpatialCollider or something along those lines.

StephenHodgson · 2017-02-28T14:56:08Z

Assets/HoloToolkit/SpatialUnderstanding/Scripts/SpatialUnderstandingCustomMesh.cs

@@ -118,6 +123,11 @@ public void Commit()
                    MeshObject.SetTriangles(tris, 0);
                    MeshObject.RecalculateNormals();
                    MeshObject.RecalculateBounds();
+                    if (Collider)


Nit: if (Collider != null)

I'll rename it to SpatialCollider as suggested above and also make this change. To be honest, I would actually prefer not even performing this test. Ideally, we want to ensure this member is always set and having it blow up with a null pointer exception would quickly draw attention to erroneous usage.

StephenHodgson · 2017-02-28T14:57:50Z

Assets/HoloToolkit/SpatialUnderstanding/Scripts/SpatialUnderstandingCustomMesh.cs

-                    ));
+                SurfaceObject surfaceObject = CreateSurfaceObject(
+                  mesh: nextSectorData.MeshObject,
+                  objectName: string.Format("SurfaceUnderstanding Mesh-{0}", surfaceObjectIndex),


Where do we use the object name?
Is it just for naming the object in the hierarchy of the scene?

As far as I know, that is all the name is used for.

…it to always be set and blow up otherwise)

StephenHodgson · 2017-03-01T01:29:04Z

Assets/HoloToolkit/SpatialUnderstanding/Scripts/SpatialUnderstandingCustomMesh.cs

@@ -118,6 +123,8 @@ public void Commit()
                    MeshObject.SetTriangles(tris, 0);
                    MeshObject.RecalculateNormals();
                    MeshObject.RecalculateBounds();
+                    SpatialCollider.sharedMesh = null;
+                    SpatialCollider.sharedMesh = MeshObject;


Just one last thing. Do we need to set the sharedMesh to null before we set it?

Yup. Take a look at CreateSurfaceObject().

You're correct. I had to look it up. Strange that you'd need to set it null before setting it, but it's the only way to reset the mesh collider apparently.

Mesh colliders need serious pre-processing to function, and are not suitable for updating in real-time.

Would it be feasible to use primitive colliders?

Primitive colliders are not feasible -- mesh colliders are used for spatial mapping, too. Also, note that the code that generates these meshes runs "asynchronously" as a coroutine and periodically yields. Also, it appears to me that Spatial Understanding produces a lot of really tiny meshes, which should be easier to set up.

If you look at where Commit() is called, on line 319, you can see that the routine yields immediately afterwards. This should be fine, especially considering that this is performed only once when finalizing the mesh.

We should definitely add a comment here to clarify this quirk so that someone doesn't accidentally remove the null assignment in the future.

Done and pushed.

trzy · 2017-03-01T01:30:27Z

Ok. I've pushed the changes discussed. Regarding #228, I don't think this is the same issue. The symptom here is that when disabling the spatial meshes produced by SpatialMappingManager and only using the meshes generated by SpatialUnderstanding, there are no collisions at all (all rigid bodies will simply fall through).

I'm guessing this was not necessarily by design but I'm not sure. If Spatial Understanding meshes are intended to be used in place of the raw input spatial meshes (i.e., for occlusion and room collision), then collision should definitely be enabled.

The mesh collider exists because the underlying spatial mapping modules are used to produce the game objects but that doesn't mean it was intended to be used... If spatial understanding is intended only to produce "metadata" for object placement, then maybe this "fix" isn't needed.

StephenHodgson · 2017-03-01T01:56:48Z

I think having the choice of utilizing the generated colliders or not would be beneficial.

trzy · 2017-03-01T02:30:35Z

Agreed about the choice of using colliders. Shall I add that and push here for you to check? Can probably do it tonight or tomorrow.

StephenHodgson · 2017-03-01T03:23:33Z

That's up to you.

@thebanjomatic Good thinking adding that comment btw. I think it's a good one to have there in case someone removes and doesn't understand the necessity of it.

… mesh and enable collider

trzy · 2017-03-02T01:56:45Z

I've added an option, CreateMeshCollider, that is set true by default, which can be unchecked in order to avoid assigning a collision mesh. It'll also outright disable the collider in that case. Let me know if it looks okay to you.

StephenHodgson · 2017-03-02T15:26:21Z

I can't test on the HoloLens to confirm though, so we might need someone with a device to confirm.

StephenHodgson · 2017-03-02T15:29:01Z

I am a bit concerned that CreateMeshColliders and CreateMeshCollider is a bit confusing though.

[edit]
Oh I see, CreateMeshCollider is part of the MeshData class.

thebanjomatic · 2017-03-02T19:11:45Z

Assets/HoloToolkit/SpatialUnderstanding/Scripts/SpatialUnderstandingCustomMesh.cs

+        /// <summary>
+        ///  Whether to create mesh colliders. If unchecked, mesh colliders will be empty and disabled.
+        /// </summary>
+        public bool CreateMeshColliders = true;


The only comment I have at this point is that I like the idea of this being a property following the behavior of DrawProcessedMesh such that setting it to false would disable the Collider for all the existing surfaceObjects

Great idea.

At which point "UseProcessedMeshCollider" or something else might be more appropriate I guess.

There's one problem with that: setting to true would have to enable the collision meshes (potentially generating them for the first time) and this is too slow to do inside of a setter. Kicking off an async task of some sort also violates the principle of least surprise, I think.

Let me know whether we should pursue this suggestion. I'm definitely open to making it work if there's a simple way of doing it that's consistent with the rest of the API.

That's a good point, and it seems like more trouble than its worth. I'm fine with things as they are right now.

Need to update collision mesh when committing mesh data

6a96180

msftclas added the cla-not-required label Feb 28, 2017

StephenHodgson reviewed Feb 28, 2017

View reviewed changes

Renamed Collider to SpatialCollider and removed check for null (want …

2e1decd

…it to always be set and blow up otherwise)

StephenHodgson reviewed Mar 1, 2017

View reviewed changes

StephenHodgson approved these changes Mar 1, 2017

View reviewed changes

Added comment explaining collider mesh null assignment

9f50ef1

Added CreateMeshCollider option (true by default) to create collision…

af03676

… mesh and enable collider

thebanjomatic approved these changes Mar 2, 2017

View reviewed changes

thebanjomatic approved these changes Mar 8, 2017

View reviewed changes

StephenHodgson merged commit 54d087d into microsoft:master Mar 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Need to update collision mesh when committing mesh data #542

Need to update collision mesh when committing mesh data #542

trzy commented Feb 28, 2017 •

edited

msftclas commented Feb 28, 2017

StephenHodgson commented Feb 28, 2017 •

edited

StephenHodgson Feb 28, 2017 •

edited

StephenHodgson Feb 28, 2017

trzy Mar 1, 2017

StephenHodgson Mar 1, 2017

StephenHodgson Feb 28, 2017 •

edited

trzy Mar 1, 2017

StephenHodgson Mar 1, 2017

trzy Mar 1, 2017

StephenHodgson Mar 1, 2017 •

edited

StephenHodgson Mar 1, 2017

trzy Mar 1, 2017 •

edited

thebanjomatic Mar 1, 2017

trzy Mar 1, 2017

trzy commented Mar 1, 2017 •

edited

StephenHodgson commented Mar 1, 2017 •

edited

trzy commented Mar 1, 2017

StephenHodgson commented Mar 1, 2017 •

edited

trzy commented Mar 2, 2017

StephenHodgson commented Mar 2, 2017 •

edited

StephenHodgson commented Mar 2, 2017 •

edited

thebanjomatic Mar 2, 2017

StephenHodgson Mar 2, 2017

thebanjomatic Mar 2, 2017

trzy Mar 8, 2017

thebanjomatic Mar 8, 2017

Need to update collision mesh when committing mesh data #542

Need to update collision mesh when committing mesh data #542

Conversation

trzy commented Feb 28, 2017 • edited

msftclas commented Feb 28, 2017

StephenHodgson commented Feb 28, 2017 • edited

StephenHodgson Feb 28, 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

StephenHodgson Feb 28, 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

StephenHodgson Mar 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trzy Mar 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trzy commented Mar 1, 2017 • edited

StephenHodgson commented Mar 1, 2017 • edited

trzy commented Mar 1, 2017

StephenHodgson commented Mar 1, 2017 • edited

trzy commented Mar 2, 2017

StephenHodgson commented Mar 2, 2017 • edited

StephenHodgson commented Mar 2, 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

trzy commented Feb 28, 2017 •

edited

StephenHodgson commented Feb 28, 2017 •

edited

StephenHodgson Feb 28, 2017 •

edited

StephenHodgson Feb 28, 2017 •

edited

StephenHodgson Mar 1, 2017 •

edited

trzy Mar 1, 2017 •

edited

trzy commented Mar 1, 2017 •

edited

StephenHodgson commented Mar 1, 2017 •

edited

StephenHodgson commented Mar 1, 2017 •

edited

StephenHodgson commented Mar 2, 2017 •

edited

StephenHodgson commented Mar 2, 2017 •

edited