Open Ralith opened 3 years ago
Hi! I feel like adding a root_dispatcher
arguments to all dispatch methods are a bit too intrusive, and makes it weird to use by the end-user (who has do call something like dispatcher.distance(dispatcher, ...)
.
A more transparent way of doing this is through composition:
DefaultQueryDispatcher
to a:
struct BaseQueryDispatcher<'a, D> {
root_dispatcher: &'a D
}
impl QueryDispatcher for BaseQueryDispatcher { fn distance(...) { // ... if let Some(c1) = shape1.as_composite_shape() { Ok(query::details::distance_composite_shape_shape( self.root_dispatcher, pos12, c1, shape2, )) } // .... } }
2. Redefine a new `DefaultQueryDispatcher` that calls the `BaseQueryDispatcher` methods (to avoid code duplication):
```rust
struct DefaultQueryDispatcher;
impl QueryDispatcher for DefaultQueryDispatcher {
fn distance(...) {
let dispatcher = BaseQueryDispatcher { root_dispatcher: self };
dispatcher.distance(...)
}
}
And then you can write your own dispatcher that passes itself to the BaseQueryDispatcher
.
That way there is no need to change the signature of the dispatcher trait methods.
That does look better. It might take some creativity to make composing dispatchers ergonomic (so that independently implemented custom shapes can be used without the end user writing a ton of boilerplate), but I think it's tractable; I'll play with it.
Consider two libraries that define custom compound shapes, and an application that wants to use both. Ideally the application, which does not itself define any custom shapes, should not have to implement QueryDispatcher
. I don't think the revised proposal supports that, since there's no way for parry to provide for composition of recursive dispatch logic, unless I'm missing something.
If external crates are to composably define shapes that need to dispatch recursively (i.e. compound shapes similar to HeightField), then I don't see any alternatives to the original proposal. However, end-user ergonomics could be preserved by relegating the root_dispatcher
argument to a trait for use only by shape implementers, and turning QueryDispatcher
into a blanket-implemented trait that encapsulates the self.query(self, ...)
dance, and provides a chain
helper for composition. How does that sound?
When processing queries on a compound shape,
QueryDispatcher
methods must be invoked recursively. This makes it infeasible for custom shapes that occur inside compound shapes to be handled. Ideally downstream code could providechain
ableQueryDispatcher
impls that only handle the shapes they introduce and otherwise returnUnsupported
, but in that case a compound shape would fall through to the default dispatcher, which would recurse internally and fail to handle custom shapes it encounters. Similar issues affect any user-defined composite shapes. This could be fixed by adding aroot_dispatcher: &dyn QueryDispatcher
argument to every trait method. The same issue also affectsPersistentQueryDispatcher
.I can bang this out, but it's a bunch of boilerplate updates and given that this logic was not preserved from ncollide I want to be sure it's welcome before proceeding.