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

Added triangle adapter. #2

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

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

for (int i = 0; i < iterations; i++) {
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 :)

@@ -79,6 +80,38 @@ public List<BVHNode<T>> Traverse(NodeTraversalTest hitTest)
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.

None yet

2 participants