Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix for New spatial mapping drops frames every update period #266

Merged
merged 9 commits into from Oct 14, 2016

Conversation

darax
Copy link
Member

@darax darax commented Oct 6, 2016

Fixes #264
Fixes #207

@izatar
Copy link

izatar commented Oct 7, 2016

I just tried this out, and the flickering issue is fixed.
However, I did notice two new things I have not seen before.
First, the button panel that is put up is under the finalized mesh, so I can't see it. It appears to be too close the mesh plane and the z-ordering is inaccurate.
The second is that the finalized mesh is not 'water-tight' anymore, there are gaps that have no polygons.

UPDATE: I just tried this with my geometry-shader free material, and I am now seeing up to 60fps on the finalized mesh. This is the good news. The bad news is that meshes don't match up, have holes in them, and float around relative to each other when the anchor drifts.

@izatar
Copy link

izatar commented Oct 7, 2016

20161006_231535_hololens

The attached image shows an example of the gaps in the finalized mesh. It looks worse in holo-reality.

@izatar
Copy link

izatar commented Oct 7, 2016

After some false starts (like normal calculation), I did find the change that seems to trigger this problem:

 if (GetComponent<WorldAnchor>() == null)
            {
                gameObject.AddComponent<WorldAnchor>();
            }

I don't understand why. But, there it is. Wasn't easy to discover.

having just one world anchor on the parent of the spatial understanding meshes resulted in awkward geometry on the seams between meshes.
@darax
Copy link
Member Author

darax commented Oct 7, 2016

I reverted to having an anchor per mesh rather than one anchor. That seems to have fixed the seams.

@darax
Copy link
Member Author

darax commented Oct 7, 2016

I've also added a change to get rid of the unreachable code warning.

@izatar
Copy link

izatar commented Oct 8, 2016

This is better, but there are still some seams.
Converting:
AddSurfaceObject(null, string.Format("SurfaceUnderstanding Mesh-{0}", meshSectorsIndex), transform).AddComponent<WorldAnchor>();

to
AddSurfaceObject(null, string.Format("SurfaceUnderstanding Mesh-{0}", meshSectorsIndex), transform);

seems to remedy this situation. That is to say, just don't do the worldanchor at all. It works without them so I'm not sure if they are necessary?

EDIT:
Unfortunately, even with this fix, there is still a problem with seams when you use a different material from the wireframe, like anything that relies on normals for lighting which is most materials.

It turns out my investigation into normal calculation was not a false start.
MeshObject.RecalculateNormals();
is setting the normals incorrectly at edges, which makes a tiny seam.

The right answer is to manually set the normals correctly. If you merely want parity with the previous code base, remove this line of code and don't put in the normals at all. Doing this will force shaders to do more work, and will limit some effects they can do however. However this is still better than incorrect normals!

If you really want to do it right, UVs could be set too.

@darax
Copy link
Member Author

darax commented Oct 10, 2016

We are ignoring the normals that come from the native layer. I can refactor to use them to see if it helps.

Regarding the anchor, if you don't have an anchor and lose tracking the meshes will likely be off the surfaces after tracking is restored.

@darax
Copy link
Member Author

darax commented Oct 10, 2016

The normals from the native layer seem to be all zero. However, in doing so I was forced to remove an optimization where a single vert could be reused multiple times, and I think that will fix the normal problem. Let me clean this up and I'll push it so that you can try it.

Thank you very much for the time you spent on this!

@darax
Copy link
Member Author

darax commented Oct 10, 2016

I think I got it. :)

I tested with a shader that rendered raw normals, and they look consistent, even at the edges (I saw how they weren't before). I had to stick with a single world anchor or the seams would appear when rendering with wireframes.

I still do see some artifacts when using the wireframe.

@izatar
Copy link

izatar commented Oct 12, 2016

This is looking good to me. I did not notice artifacts on the wire mesh, but I didnt spend much time at it.

Copy link
Contributor

@StephenHodgson StephenHodgson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one question about the need of Linq (totally understandable if it actually does help) and just a single space nit.

@@ -207,6 +236,7 @@ public IEnumerator Import_UnderstandingMesh()
(meshIndices != null) &&
(meshIndices.Length > 0))
{

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: extra space

@@ -6,6 +6,7 @@
using System;
using System.Collections.Generic;
using UnityEngine.VR.WSA;
using System.Linq;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we actually using Linq in this class? If so, do we need to?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Nothing I saw in this change seemed Linq related.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better namespace to get the ElementAt extension from?
// Get the next MeshData.
MeshData meshData = meshSectors.Values.ElementAt(meshSectorsIndex);

@darax
Copy link
Member Author

darax commented Oct 14, 2016

I'm going to merge this now.

@darax darax merged commit 28fc008 into microsoft:master Oct 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants