Skip to content

Added triangle adapter.#2

Open
MattXV wants to merge 4 commits intorossborchers:masterfrom
MattXV:triangle-adapter
Open

Added triangle adapter.#2
MattXV wants to merge 4 commits intorossborchers:masterfrom
MattXV:triangle-adapter

Conversation

@MattXV
Copy link

@MattXV MattXV commented Jan 27, 2022

Hi,
I've added an implementation of a triangle adapter I needed - figured it might be useful to others. I'm still testing it, but seems to work.
P.S. I'm also suggesting a fix for the render method where it would fail due to too many matrices passed in the DrawMeshInstanced.

Copy link
Collaborator

@RossRH RossRH left a comment

Choose a reason for hiding this comment

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

This looks great! I think a triangle adapter could be quite useful for mesh collisions (I presume that's the use case?)

The only additional thing I would request is adding a unit test for the triangle adapter so there is an example of its usage, and a concrete assertion that its working as intended.

Thanks so much for PRing back into this repo. Sorry it took so long to get to this!

@@ -1,4 +1,4 @@
# Unity Bounding Volume Heirachy
# Unity Bounding Volume Hierachy
Copy link
Collaborator

Choose a reason for hiding this comment

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

😄 Whoops

Graphics.DrawMeshInstanced(mesh, 0, _debugRenderMaterial, matricies.GetRange(0, 1023));
matricies.RemoveRange(0, 1023);
}
Graphics.DrawMeshInstanced(mesh, 0, _debugRenderMaterial, matricies);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this not be removed after adding the split draw or are both calls necessary?

Copy link
Author

Choose a reason for hiding this comment

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

Both calls would be necessary as the for-loop should only execute when the limit is exceeded, which often occurs when each triangle in a scene has its own AABB.

mesh.SetIndices(indices, MeshTopology.Lines, 0);
if(_debugRenderMaterial == null)
{
_debugRenderMaterial = new Material(Shader.Find("Standard"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this compatible with SRP?

Copy link
Author

Choose a reason for hiding this comment

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

I will test this to make sure it is and will come back to you :)

return hits;
}

public SortedList<float, BVHNode<T>> SortedTraverse(NodeTraversalTest hitTest) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please double check the curly braces are following the same convention across the project
As far as I know they should be on a newline

@MattXV
Copy link
Author

MattXV commented Mar 18, 2022

Hi!
Many thanks again for your amazing work!

My current use case for the Triangle Adapter is ray-triangle intersections in a scene, which I'm using for ray tracing.
Mesh collisions, as you said, would be another interesting use case.
Otherwise, I'll make sure to add a unit test and apply all the requested changes over the next week.

I'll keep you updated on this.
Thank you again :)

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.

2 participants