dimforge / ncollide

2 and 3-dimensional collision detection library in Rust.
https://ncollide.org
Apache License 2.0
921 stars 105 forks source link

First interference with ray #320

Closed MJohnson459 closed 4 years ago

MJohnson459 commented 4 years ago

Summary

This PR adds a new method to the CollisionWorld for detecting the first ray intersection with an object.

It creates a new visitor called RayIntersectionCostFnVisitor which takes in a cost function and applies it for each leaf node in the BVH using the data contained. This visitor is relatively generic and can be used for other cost functions applied to a BroadPhase.

The .first_intersection_with_ray() method passes in the equivalent of a narrow phase as the cost function which finds the smallest toi in the tree.

What's left:

MJohnson459 commented 4 years ago

I'm not sure why the wasm build is failing but it doesn't seem related to this PR.

sebcrozet commented 4 years ago

Thanks @MJohnson459! The WASM build failure is independent from this, so I can merge this anyway.

There are a few TODO's left in comments that mainly involve traits being more restrictive than I would like. It should be possible to make them more open in the future without a major interface change so maybe not blocking?

Yeas, we can leave this for future improvements.

I would like to add a minimum and maximum to the cost function visitor so that it can ignore the values outside the accepted range. I think this should be added as a new members to the visitor struct with default values applied. A new constructor method could be created to set them. Does this actually work without bloating the API too much?

I think this can work yeah. I'll merge this and see if I can make the modification today (because I will release a new version of ncollide today).

How optimal is the implementation here? I did a rough benchmark test using an external repo and it seemed to be slightly more efficient with 100 objects or so but it's hard to quantify.

I think the this implementation is as optimal as it can be. Other things could be improved (like not having to pass the Isometry::identity() matrix for the AABB raycast, but that requires other changes unrelated to this PR. So we can leave this for other PRs.

sebcrozet commented 4 years ago

The changes that add a max bound to the ray is in that PR: https://github.com/rustsim/ncollide/pull/321 There is only a maximum bound instead of a full range as suggested because setting a min bound can already be done by making the ray start at ray.orig + ray.dir * min_bound.