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

Give access to body set in broad phase filter #278

Merged
merged 6 commits into from
Oct 26, 2020

Conversation

neachdainn
Copy link
Contributor

@neachdainn neachdainn commented Aug 26, 2020

Meant to be paired with ncollide#359.

This utilizes the new formulation of the BroadPhasePairFilter trait to supply both the BodySet and ColliderSet to the broad phase pair filter. This allows for the filter to take into account information that exists in the body, such as multibody link ancestry, velocity, etc.

From the user's perspective, this removes GeometricalWorld::set_broad_phase_pair_filter and provides a MechanicalWorld::step_with_filter(.., impl BroadPhasePairFilter) where the filter has access to the body set.

@neachdainn
Copy link
Contributor Author

A quick rehash of my notes from Discord:

The BroadPhasePairFilter trait modification was the cleanest almost-not-breaking change I could come up with to allow for non-collision information to be passed into the filter. However, removing the Any bound from the trait seemed like a significantly more serious change. Unfortunately, this bound requires that any filter be 'static, meaning that the user filter can't be borrowed, resulting in either:

  1. The GeometricalWorld must own the user filter and become constrained by the collision set type.
  2. The user must pass in an owned version of the filter every step.

The first version seemed less than ideal, considering that the mechanical world is also currently not constrained by the set types. It seemed good to keep that, however it might provide a better API. Additionally, it seems likely that any filter will actually be a ZST meaning creating the filter every frame will be a no-op. That worked out well once it became clear how many times the filter would need to be passed into the geometrical world - adding a Copy bound to it reinforces the ZST assumption. This is a breaking change for the old BP filter. However, the old version was largely crippled by the inability to access body information.

@sebcrozet sebcrozet added breaking change Fixing this issue is likely to require breaking changes enhancement labels Sep 2, 2020
@neachdainn
Copy link
Contributor Author

I should find time to get this fully finished in the next few days.

@neachdainn
Copy link
Contributor Author

neachdainn commented Sep 18, 2020

While making these updates, I found a situation that I had not considered as it wasn't relevant to my use-case: needing dynamic filters. The physics testbed, for example, should have dynamic filters in order to enable filtering while not parameterizing the testbed on the filter. The current state of this pull request is such that dynamic filters cannot exist in any form due to the Copy + 'static requirements imposed by my changes and BroadPhasePairFilter respectively.

An alternative that seems appealing to me would be to remove the Any bound on BroadPhasePairFilter *. It removes the ability to downcast the filter, which is unfortunate, but it would be much less broken overall. I do not know how large of an impact that is on a purely-ncollide application.

* Also BroadPhase. And maybe others - I'm still working in this.

New branch because I didn't want this to update the pull request. Please
don't include this commit in the history, Nate.
@neachdainn
Copy link
Contributor Author

neachdainn commented Sep 23, 2020

So this is finally in a working state. A history of how it got to this form can be found here but basically I removed the Any bound from the filter trait and made the filter something that is passed into each call to MechanicalWorld::step. Through some generous use of magical higher-ranked trait bounds, you can have run-time determined filters.

A few things to note. First, this requires ncolide#361 to be merged. Second, right now I define a type BroadPhaseCollisionSet that exists in nphysics::world. Going by name, it probably should exist in nphysics::object, however it is entirely used for the geometric world and its filter which is why it is there now. It might also be better named BroadPhase{Pair}FilterSet and an alias should probably be made to make the fully qualified for shorter for default sets. Finally, in order to get this to build a large number of changes were made to version numbers specified in the Cargo.toml - this shouldn't be merged without first looking through those.

EDIT: It might be worth making the filter take mutable references for self and the set. I'm trying to debug some funky behavior and not being able to modify the user information and not being able to make changes in the filter (e.g., record contacts) is a bit of a pain.

@neachdainn
Copy link
Contributor Author

I have modified my work code to use this branch and I am pretty satisfied with the results. Currently, some vague knowledge of HRTBs is required to have run-time determined filters but the addition of a few type aliases might mostly solve that problem:

pub type FilterObject<N, Bodies, Colliders> = dyn for<'a> BroadPhasePairFilter<N, BroadPhaseCollisionSet<'a, N, Bodies, Colliders>>;
pub type BoxedFilter<N, Bodies, Colliders> = Box<FilterObject<N, Bodies, Colliders>>;

The library consumer will still have to understand HRTBs in order to use generics with the filter but using a BoxedFilter instead will probably suffice most of the time.

I haven't pushed those type aliases yet because they're probably worth discussing (or at least coming up with a better name) and I'm not sure what module they should go in.

Copy link
Member

@sebcrozet sebcrozet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR!
Could you please bump the dependencies versions to use kiss3d v0.26, and ncollide v0.25?

Cargo.toml Outdated
Comment on lines 11 to 13
ncollide2d = { path = "../ncollide/build/ncollide2d" }
ncollide3d = { path = "../ncollide/build/ncollide3d" }
kiss3d = { path = "../kiss3d" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be left commented.

As per the comment on the Github pull request.
@neachdainn
Copy link
Contributor Author

I believe I got all of them. Any thoughts on the names and type aliases?

@sebcrozet
Copy link
Member

Thanks! Regarding the naming I think it would make more sense if:

  • BroadPhaseCollisionSet was called BroadPhasePairFilterSets instead since they are only used for the pair filtering.
  • nphysics should expose an alias pub type DefaultBroadPhasePairFilterSets<'a, N> = BroadPhasePairFilterSets<'a, N, DefaultBodySet<N>, DefaultColliderSet<N>> to nphysics. In your PR this alias exists, but in the testbed only.

@sebcrozet sebcrozet merged commit 550bc70 into dimforge:master Oct 26, 2020
@sebcrozet
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Fixing this issue is likely to require breaking changes enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants