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

Collision Filtering and Modification #150

Closed
datael opened this issue Sep 9, 2023 · 3 comments · Fixed by #610
Closed

Collision Filtering and Modification #150

datael opened this issue Sep 9, 2023 · 3 comments · Fixed by #610
Labels
A-Collision Relates to the broad phase, narrow phase, colliders, or other collision functionality C-Enhancement New feature or request

Comments

@datael
Copy link
Contributor

datael commented Sep 9, 2023

Future features in the readme lists Collision Filtering as something not yet implemented and I would like to get the ball rolling if possible.

Topics discussed in the xpbd help channel on Discord

Some topics that were discussed on Discord included:

  1. Global physics hooks that can iterate over all collisions
  2. Per-entity filter components
    • Filtering collisions with entities by EntityId
    • Filtering collisions by predicate
  3. Being able to modify the individual contacts
  4. The possibility of being able to use one-shot systems in the future
  5. The possibility of being able to use entity relations to allow more efficient filtering

Global Physics hooks

Adding a schedule after the SubstepSet::NarrowPhase, and exposing a way to filter collisions from the Collisions resource gives a very open-ended and straightforward way to allow for modifying collisions.
I've got a working example on this fork here.

I'm in two minds about whether it's worth the schedule to have built-in sets (e.g. First, Filter, Modify, Last), or whether that complexity should be left up to the user. I can see a potential want to pre-filter collisions with faster filters before any slower ones execute, for example.

Per-entity filter components

@Jondolf suggested something in the following form:

ContactFilter::new()
        .with_excluded_entities([...])
        .with_predicate(|mut contact: ContactData| {
            // Modify contact (optional)
            contact.penetration = 0.1;
            // Return the contact or use None to ignore contact
            Some(contact)
        }),

It was also suggested that some form of combination rule may be needed to handle when two entities with conflicting filter predicates are present.

Modifying individual contacts

Being able to modify the individual contacts is desirable. Therefore, being able to return Some(contact) from a post-processing step is more useful than only being able to remove or keep collisions.

One-shot systems

@Jondolf showed the following code on Discord as something that it would be nice to be able to do. There was discussion about how this may not be possible with one-shot systems, but using run_system_with* may allow it as long as the overhead was acceptable.

* run_system_with was added to bevy with this merged but unreleased-at-the-time-of-writing pull request.

fn setup(mut commands: Commands, contact_hook_pipeline: ContactHookPipeline) {
    commands.spawn((
        ContactHook::new(
            &contact_hook_pipeline,
            |In(mut contact): In<ContactData>, query: Query<&MyBundle>| {
                // Modify contact, query for entities, do whatever
                // Return new contact data to narrow phase; if None, skip contact
                Some(contact)
            }
        ),
    ));
}

Entity Relations

Something along the lines of (Body1, CollidesWith(Contacts), Body2) was suggested by @Jondolf.

Bevy currently has a tracking issue for entity relations here: bevyengine/bevy#3742

Other notes/questions

Should filtering be treated as a separate issue from modification?

My current personal needs for collision filtering are very simplistic: I just want to implement one-way platforms, which is easily solved by the first bullet point above. I therefore have a very narrow view of what needs exist when it comes to being able to filter/modify collisions, and so would like to get input from other interested parties.

One shortfall of bevy_rapier that I would like to avoid was that it seemed impossible to register more than one set of query params to use with collision hooks because the parameters are registered on the plugin itself as a generic type. Being able to access queries while filtering and modifying collisions seems very important in an ECS.

Where to go from here

I've started work on global filters and entity-entity collision exclusion in a fork here, but am looking for input. I'd be interested in creating a pull request at some point, even if it's only for the global collision hook, as it's a small enough change that gives quite a bit of power from the get-go.

@Jondolf Jondolf added C-Enhancement New feature or request A-Collision Relates to the broad phase, narrow phase, colliders, or other collision functionality labels Sep 9, 2023
@Jondolf
Copy link
Owner

Jondolf commented Sep 9, 2023

Thanks for such a thorough issue!

I would say that we should kick things off in the initial PR with "global physics hooks/filters" and excluding specific entities. This is probably the easiest and least controversial part, and it should already be enough for a lot of use cases.

Below are some of my current thoughts.

Global physics hooks (edit: done in #155)

For now, I would just add a custom schedule like you're currently doing. In my opinion, the separate system sets are unnecessary, since users can just use .after, .before and .chain to order the systems or create their own set. The schedule is basically empty by default, so there is nothing to order your systems relative to other than your own systems.

The main caveat with the approach of using a custom schedule and iterating over Collisions is that it adds a lot of iteration. We would iterate over all collisions once in the narrow phase, and however many times users iterate over them in their custom filter systems. With Rapier's approach, the filter is applied directly in the narrow phase, so it only iterates through all collisions once per physics frame (+ once in the solver).

The two main solutions I can currently come up with:

  1. Rapier's approach of storing physics hooks in a resource and allowing users to implement the filtering logic. Fast, but not very flexible, ergonomic or ECS-like, and only allows one global filter.
  2. Something like callbacks based on one-shot systems (again). Allows access to the ECS world, and could allow multiple (ordered) filters if the filtering systems are stored in e.g. a vector. Probably has extra overhead, needs to be benchmarked.

For now though, I think the approach of using a custom schedule like you're doing is perfectly fine for the initial implementation. Even if we eventually implemented some other approach, the filtering could still be done in the custom schedule.

Excluding specific entities

For excluding specific entities, I recommend moving the logic into the broad phase, next to the collision layer filtering. It's unnecessary and expensive to compute contacts in the narrow phase between entities that will be ignored later.

In terms of the component name, I'm a bit torn. CollisionFilter looks nice, but if it only contains excluded entities, some other name might make more sense. We also already have CollisionLayers, which is a type of collision filter but is under a separate component, so there might be some confusion.

I would say that it feels more ECS-like to have separate components for different types of filter/hook components, like ExcludedCollisionEntities (for the excluded entities), CollisionHook (for future predicate-based filtering) and ActiveCollisionTypes (for filtering by RigidBody type or whether a collider is a sensor, like Rapier). This would also play nicer with change detection, and if we really wanted to group the filters for querying ergonomics, we could instead have CollisionFilter be a world query. These names are just examples though, I'm open to other names.

Entity-level contact hooks

Component-based contact hooks/filters using something like one-shot systems would definitely be nice to have, and they would allow for ergonomic entity-specific logic without having to iterate over all of Collisions.

However, this will require some experimentation to discover potential caveats and find the best approach. I'm not sure if using callbacks like this is considered good or bad practise in the context of an ECS, as it hasn't really been possible before. I also suspect that one-shot systems would have extra overhead which might be significant if many entities have custom contact hooks.

@datael
Copy link
Contributor Author

datael commented Sep 17, 2023

Apologies I've been a while getting back to you. I had some personal stuff come up so I haven't had much time to look at this.

Of the things above, I think adding a global filter is the one we're in agreement with, so I've created a pull request just implementing that. It includes an example in 2d of using the filter to implement a one-way platform.

Edit:
It would probably help if I linked it: #155

Jondolf added a commit that referenced this issue Sep 25, 2023
# Objective

Create a stage to allow filtering and changing collisions from user-defined systems.

This is the simplest part of #150: the Global Physics hooks via user systems only.

## Solution

- Adds `SubstepSet::PostProcessCollisions`, which is directly after `SubstepSet::NarrowPhase`
- Adds `PostProcessCollisions` schedule, which is run in `SubstepSet::PostProcessCollisions`
- Exposes `retain` from `Collisions` to allow direct modification of the set of collisions

Also includes a simple one-way-platform example.

---------

Co-authored-by: Joona Aalto <[email protected]>
RJ pushed a commit to RJ/avian that referenced this issue Sep 25, 2023
# Objective

Create a stage to allow filtering and changing collisions from user-defined systems.

This is the simplest part of Jondolf#150: the Global Physics hooks via user systems only.

## Solution

- Adds `SubstepSet::PostProcessCollisions`, which is directly after `SubstepSet::NarrowPhase`
- Adds `PostProcessCollisions` schedule, which is run in `SubstepSet::PostProcessCollisions`
- Exposes `retain` from `Collisions` to allow direct modification of the set of collisions

Also includes a simple one-way-platform example.

---------

Co-authored-by: Joona Aalto <[email protected]>
@Jondolf
Copy link
Owner

Jondolf commented Dec 31, 2024

Hi! It's been a while, but a quick update:

I would consider #610 to be the correct solution for contact filtering and modification. Global collision hooks exist in pretty much all engines I know, and should be flexible enough to efficiently handle the vast majority of advanced contact scenarios (one-way platforms, conveyor belts, non-uniform friction...).

Per-entity contact filtering and modification on the other hand is something that I think is just very tricky / impossible to do efficiently, if we want the hooks to have ECS access. To do it without severe performance or usability implications, the hooks must be executed immediately, in a potentially parallel loop. It would require exclusive world access, and just isn't very viable here I think. One-shot systems, observers, etc. wouldn't really work.

We could just do per-entity hooks that only provide the entity IDs, maybe some collider components (since those might be available in the system already), and mutable access to the contacts, but even then it's not problem-free. What if both entities in a collision have hooks? Users might need to take special care to make sure they don't do duplicate work, or to consider the order in which hooks are called, and it quickly becomes a mess. Before doing any work on this, I think I would at least want some really solid practical use cases where per-entity hooks would be useful/necessary, and a reason you couldn't just use global hooks or collision layers for it.

Also FYI, it's pretty likely that PostProcessCollisions will be removed in favor of the collision hooks in #610. Running a whole schedule in between contact logic isn't ideal, and seems to prevent some important optimizations I'm working on. Ultimately, I would put performance ahead of the hyper-flexibility of an entire schedule, especially since the hooks should still be enough for basically all scenarios I can think of. I haven't removed it just yet though.

Anyway, just wanted to give my current thoughts on this over a year later. I think I'll close this issue once #610 is merged. New, more narrowly scoped issues can be made afterwards if there are still open questions.

Jondolf added a commit that referenced this issue Jan 1, 2025
# Objective

Closes #150.

Advanced contact scenarios often require filtering or modifying contacts with custom logic. Use cases include:

- One-way platforms
- Conveyor belts
- Non-uniform friction and restitution for terrain

Physics engines typically handle this with *hooks* or *callbacks* that are called during specific parts of the simulation loop. For example:

- Box2D: `b2CustomFilterFcn()` and `b2PreSolveFcn` (see [docs](https://box2d.org/documentation/md_simulation.html#autotoc_md97))
- Rapier: `PhysicsHooks` trait with `filter_contact_pair`/`filter_intersection_pair` and `modify_solver_contacts` (see [docs](https://rapier.rs/docs/user_guides/bevy_plugin/advanced_collision_detection#physics-hooks))
- Jolt: `ContactListener` with `OnContactValidate`, `OnContactAdded`, `OnContactPersisted`, and `OnContactRemoved` (see [docs](https://jrouwe.github.io/JoltPhysics/class_contact_listener.html))

Currently, we just have a `PostProcessCollisions` schedule where users can freely add systems to operate on collision data before constraints are generated. However:

- It doesn't support filtering broad phase pairs.
- It results in unnecessary iteration. Filtering and modification should happen directly when contacts are being created or added.
- It adds more scheduling overhead.
- It forces us to handle collision events and other collision logic in a sub-optimal way.

`PostProcessCollisions` is generally not a good approach for performance, and it is not enough for our needs, even if it is highly flexible. We need proper collision hooks that are called as part of the simulation loop.

## Solution

Add a `CollisionHooks` trait for types implementing `ReadOnlySystemParam`, with a `filter_pairs` method for filtering broad phase pairs, and a `modify_contacts` method for modifying and filtering contacts computed by the narrow phase.

The system parameter allows ECS access in hooks. Only read-only access is allowed, because contact modification hooks may run in parallel, *but* deferred changes are supported through `Commands` passed to the hooks.

An example implementation to support interaction groups and one-way platforms might look like this:

```rust
use avian2d::prelude::*;
use bevy::{ecs::system::SystemParam, prelude::*};

/// A component that groups entities for interactions. Only entities in the same group can collide.
#[derive(Component)]
struct InteractionGroup(u32);

/// A component that marks an entity as a one-way platform.
#[derive(Component)]
struct OneWayPlatform;

// Define a `SystemParam` for the collision hooks.
#[derive(SystemParam)]
struct MyHooks<'w, 's> {
    interaction_query: Query<'w, 's, &'static InteractionGroup>,
    platform_query: Query<'w, 's, &'static Transform, With<OneWayPlatform>>,
}

// Implement the `CollisionHooks` trait.
impl CollisionHooks for MyHooks<'_, '_> {
    fn filter_pairs(&self, entity1: Entity, entity2: Entity, _commands: &mut Commands) -> bool {
        // Only allow collisions between entities in the same interaction group.
        // This could be a basic solution for "multiple physics worlds" that don't interact.
        let Ok([group1, group2]) = self.interaction_query.get_many([entity1, entity2]) else {
            return true;
        };
        group1.0 == group2.0
    }

    fn modify_contacts(&self, contacts: &mut Contacts, commands: &mut Commands) -> bool {
        // Allow entities to pass through the bottom and sides of one-way platforms.
        // See the `one_way_platform_2d` example for a full implementation.
        let (entity1, entity2) = (contacts.entity1, contacts.entity2);
        !self.is_hitting_top_of_platform(entity1, entity2, &self.platform_query, &contacts, commands)
    }
}
```

The hooks can then be added to the app using `PhysicsPlugins::with_collision_hooks`:

```rust
fn main() {
    App::new()
        .add_plugins((
            DefaultPlugins,
            PhysicsPlugins::default().with_collision_hooks::<MyHooks>(),
        ))
        .run();
}
```

> [!NOTE]
>
> The hooks are passed to the `BroadPhasePlugin` and `NarrowPhasePlugin` with generics. An app can only have one set of hooks defined.
>
> Where are the generics on `PhysicsPlugins` then? `bevy_rapier` requires them on `RapierPhysicsPlugin`, forcing people to specify generics even if hooks aren't used, like `RapierPhysicsPlugin::<()>::default()` (see dimforge/bevy_rapier#501).
>
> Given that this is the first thing users do with the engine, I wanted to avoid forcing unnecessary generics. I'm using a subtle trick to get around them; `PhysicsPlugins` has no generics, but there is a separate `PhysicsPluginsWithHooks` wrapper with a similar API that is returned by `with_collision_hooks`. This abstraction is largely transparent to users, and gets around unnecessary generics in the public API.

It is rare to want hooks to run for every single collision pair. Thus, hooks are *only* called for collisions where at least one entity has the new `ActiveCollisionHooks` component with the corresponding flags set. By default, no hooks are called.

```rust
// Spawn a collider with filtering hooks enabled.
commands.spawn((Collider::capsule(0.5, 1.5), ActiveCollisionHooks::FILTER_PAIRS));

// Spawn a collider with both filtering and contact modification hooks enabled.
commands.spawn((
    Collider::capsule(0.5, 1.5),
    ActiveCollisionHooks::FILTER_PAIRS | ActiveCollisionHooks::MODIFY_CONTACTS
));

// Alternatively, all hooks can be enabled with `ActiveCollisionHooks::all()`.
commands.spawn((Collider::capsule(0.5, 1.5), ActiveCollisionHooks::all()));
```

### Comparison With `bevy_rapier`

The design of the collision hooks is partially inspired by `bevy_rapier`, but with what I think is a slightly friendlier and more flexible API. Some core differences:

- Rapier has ["context views"](https://docs.rs/bevy_rapier3d/0.28.0/bevy_rapier3d/pipeline/struct.ContactModificationContextView.html) for both pair filters and contact modification, with a `raw` property you need to access. It provides read-only access to some internal Rapier data (using Nalgebra types) and a contact manifold, and write-access to a contact normal and "solver contacts". There seems to be no way to queue commands or otherwise perform changes to the ECS, only read-only access.
- My pair filters simply provide the entities and access to `Commands`, while the contact modification hook provides mutable access to the `Contacts` (*not* necessarily just one manifold) between a contact pair, and to `Commands`. Read-only data about the involved entities can be queried with the ECS.

Personally, I think `bevy_rapier`'s hooks introduce a bit too much complexity and new APIs for Bevy users; there are "context views", contact manifolds, solver contacts, a bunch of internal Rapier structures, and everything uses Nalgebra types. I tried to keep it more simple, with the same contact types people already use when accessing the `Collisions` resource, while supporting read-only ECS access using the system parameter and deferred changes using `Commands`. No weird context views or Nalgebra types.

Rapier provides solver contacts, while my implementation provides raw narrow phase contact data. Both have their trade-offs, but using raw contact data introduces less new concepts, *and* it allows earlier termination, since the data for solver contacts doesn't need to be computed (though our implementation there is somewhat different from Rapier anyway).

Currently, my implementation runs hooks per *collision pair* (`Contacts`), not per *manifold* (`ContactManifold`). This provides some more data and allows entire collision pairs to be ignored at once if desired. I'm not 100% sure which is preferable though; many other engines seem to have contact modification be per-manifold. There is a possibility that we change this at some point.

### Other Changes

- Updated the `one_way_platform_2d` example to use collision hooks, and overall improved the example code.
- The broad phase now stores `AabbIntervalFlags` instead of several booleans for AABB intervals.
- `BroadPhasePlugin`, `NarrowPhasePlugin`, and many `NarrowPhase` methods now take generics for `CollisionHooks`.
- Reworked some narrow phase contact logic slightly.
- Updated and improved some docs.

## Follow-Up Work

I have several follow-up PRs in progress:

1. Per-manifold tangent velocities (for e.g. conveyor belts) and friction and restitution (for non-uniform material properties)
2. Rename many contact types and properties for clarity, improve docs and APIs
3. Contact graph
4. Reworked contact pair management

I expect them to be ready in that order. For 3-4, I would like #564 first.

It is also highly likely that we will deprecate the `PostProcessCollisions` schedule in favor of these hooks.

---

## Migration Guide

For custom contact filtering and modification logic, it is now recommended to define `CollisionHooks` instead of manually accessing and modifying `Collisions` in the `PostProcessCollisions` schedule.

The `BroadPhasePlugin`, `NarrowPhasePlugin`, and many `NarrowPhase` methods now take generics for `CollisionHooks`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Collision Relates to the broad phase, narrow phase, colliders, or other collision functionality C-Enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants