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

Dispatch traits aren't composable #8

Open
Ralith opened this issue Feb 15, 2021 · 4 comments · May be fixed by #218
Open

Dispatch traits aren't composable #8

Ralith opened this issue Feb 15, 2021 · 4 comments · May be fixed by #218

Comments

@Ralith
Copy link
Contributor

Ralith commented Feb 15, 2021

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 provide chainable QueryDispatcher impls that only handle the shapes they introduce and otherwise return Unsupported, 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 a root_dispatcher: &dyn QueryDispatcher argument to every trait method. The same issue also affects PersistentQueryDispatcher.

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.

@sebcrozet
Copy link
Member

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:

  1. Change the existing 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,
            ))
        }
        // ....
    }
}
  1. Redefine a new DefaultQueryDispatcher that calls the BaseQueryDispatcher methods (to avoid code duplication):
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.

@Ralith
Copy link
Contributor Author

Ralith commented Feb 20, 2021

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.

@Ralith
Copy link
Contributor Author

Ralith commented Feb 22, 2021

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.

@Ralith
Copy link
Contributor Author

Ralith commented Mar 5, 2021

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?

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 a pull request may close this issue.

2 participants