dimforge / ncollide

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

Dynamic dispatch helper for time-of-impact queries #342

Closed Ralith closed 4 years ago

Ralith commented 4 years ago

This provides a foundation for nphysics to support CCD on custom shapes, and for downstream crates to conveniently compose dispatchers. This requirement is motivated by my work in planetmap to provide collision detection for streaming spherical terrain, which requires close integration with ncollide queries to be efficient, much like heightfields do.

While this could be merged as-is, there's one question I'd like feedback on: should we fold ContactDispatcher and ProximityDispatcher into this trait? A single trait object is more convenient (particularly for composition), and in general anyone seeking to implement a custom shape will probably want to support everything, or at least be fully conscious of any unsupported operations.

Ralith commented 4 years ago

If the traits shouldn't be combined, a case could also be made for defining this in nphysics rather than ncollide, but I think it makes sense for ncollide to have an abstract interface for operations on pairs of dynamically typed shapes to make it easy for downstream library crates to be extensible in general.

Ralith commented 4 years ago

So at least for this semantic difference, we should not merge this trait with the contact generation and proximity determination dispatcher. Also I find it more clear to keep things separated, at least for a first prototype.

I don't think there's much harm in having one trait where some methods return trait objects and others perform computation directly. As different methods that serve different purposes, their interfaces would differ in more ways than that, after all. I'm concerned that this contributes to a proliferation of different trait objects, all of which must be individually implemented and passed all the way through the stack for a custom shape to be supported in nphysics (or another downstream library). Additionally, the convenience tools for composing dispatchers need to be duplicated across each trait.

All together, keeping the traits separate leads to substantially more boilerplate and increases the likelihood of custom shape implementers forgetting to implement some query, especially if new queries are added in the future. That said, I don't mind revisiting this in the future if you'd nonetheless prefer to keep these changes minimally invasive for now.

I wonder if it would have any value to make this dispatcher more similar to the contact/proximity dispatcher (that is, make it return a struct representing the TOI computation algorithm instead of performing the computation directly).

I waffled on this a bit. The contact/proximity dispatchers benefit from returning a trait object because those objects (usually?) contain mutable state to accelerate repeated queries between the same pair, whereas the current ToI queries are 100% stateless. Given no need for state, performing the computation directly saves an indirection, which is both more efficient and easier for a reader to follow.

It's possible to imagine ToI querying being optimized with the introduction of state in the future. I'm generally disinclined to spend effort/complexity on hypotheticals, but maybe you have concrete plans in this direction?

Ralith commented 4 years ago

Renamed to TOIDispatcher and fixed handling of unsupported shapes and compound shapes.

I didn't move the definition into pipeline because compound shapes require a dispatcher so that they can successfully query against custom shapes which they contain. This also required modifying the compound nonlinear ToI query to use dynamic dispatch for its RigidMotions.

Ralith commented 4 years ago

So as of today I have no idea how these will look like in the future

Ah, that makes sense then.

But why is it necessary to make the motion1 and motion2 parameters of query::nonlinear_time_of_impact dynamic?

Indeed passing a &dyn RigidMotion in works just fine. However, I haven't found a way to go the other way and pass a &(impl RigidMotion<N> + ?Sized) to a &dyn RigidMotion<N> which is needed when recursing into the supplied dispatcher. There might be some sort of trait system trick for this that I missed.

Ralith commented 4 years ago

Fixed tests.

sebcrozet commented 4 years ago

Thanks!