Make collision reporting opt-in

[janhohenheim]

trafficstars

Per my reasoning here, I personally believe the following to provide the best API:

pub fn plugin(app: &mut App) {
    app.add_systems(Startup, spawn_zone);
    app.observe(on_character_entered_zone);
}

#[derive(Event, CollisionEvent)]
struct CharacterEnteredZone {
    // only define the fields you actually need
    character: OtherEntity,
    collision_data: CollisionData,
}

fn spawn_zone(mut commands: Commands) {
    commands.spawn((
        // …
        Sensor,
        OnCollision::<CharacterEnteredZone>::new(),
    ));
}

fn on_character_entered_zone(trigger: Trigger<CharacterEnteredZone>, …) {
    // …
}

[janhohenheim]

Quoting @Jondolf from Discord.

Some quick thoughts on that design

• I still don't love the extra layer of indirection. Instead of "on collision, do X", it's "on collision, trigger another event, and then systems observing that do X". It also requires adding that separate observer for the second trigger. Imo it should be up to the user if they want to trigger a secondary observer like that. I imagine observer chains might also have a bit of extra overhead.
• I don't like introducing a new derive macro for this. Why would collision events need to be treated so differently from other events?
• Getting only the data you need is cool and all, but imo it feels like over-engineering here, and the macro-powered approach is another new concept/API for users to learn. I don't think this would be accepted as an upstream API in Bevy. That's generally how I try to design how APIs should look.
• The most important events we need to be observable are OnCollisionStarted and OnCollisionEnded. I haven't decided yet if OnCollision should be an observer due to the cost and nature of the event. In general, the API for "observing" collisions should just be a hook into the narrow phase that lets you filter and modify contacts, which observers can't do. Currently, you'd do this by accessing Collisions in PostProcessCollisions, but we might need to introduce other APIs for this as well.

Whatever we do, let us just not trigger a general OnCollision event, as that would mean that the user needs to manually check whether this is actually the specific sensor they care about for this system.

I'm not quite sure what you meant by this. If you .observe(...) an entity, it only listens to events targeting that entity. Unless you of course used a global observer, which I would generally consider to be an antipattern for that use case since you should just use buffered events or Collisions directly

Rapier has physics hooks for that, but it only supports one type per simulation https://github.com/dimforge/bevy_rapier/blob/c2bed72eb944b1cbfe4e90d6e86021fcdceff1e9/src/pipeline/physics_hooks.rs#L84 It can have ECS world access in there because the simulation data is copied in its own physics world, so it doesn't conflict, unlike in our case We could still probably have something similar though

[janhohenheim]

Yeah, I totally forgot observing a single entity was a thing. That changes things, as my approach was based on the belief that that was not possible. I update my position to something like adding callbacks through one-shot system's SystemIds, which have the benefit of being able to query stuff from Bevy, or using a Rapier-like interface for them, which is... I guess simpler? Smells like OOP to me, so I'm a bit skeptical, but others probably consider that to be simpler.

[musjj]

Was there any progress here? Observer-based collision reporting seems like a really nice, no-brainer improvement.

We also don't really need a new trait here right? You can just have the user define a From<Collision>.

[janhohenheim]

@Jondolf you have a branch with this feature using a different API, right?

[Jondolf]

I don't have any up-to-date branch with collision events based on observers or one-shot systems, but on the main branch, CollisionStarted and CollisionEnded events are now opt-in via the CollisionEventsEnabled marker component. There is also the new advanced CollisionHooks API that lets you "hook into" the collision pipeline to filter or modify contact pairs in the broad phase or narrow phase.

But yeah, we are still missing a proper per-entity collision event API using observers (or something similar). On the surface, it seems like an easy no-brainer addition, but there are quite a few things to consider there. Historically, some problems have been that:

1. We don't want to trigger collision events for every contact. Doing so would mean that we get one buffered event and two observable events (one for each entity) per started/ended contact pair, which could add up to a decent amount of overhead.
   • Ideally, we'd be able to detect which entities are being targeted by an observer, and only trigger an event for them. But this info is not easily or efficiently exposed by Bevy. And what about global observers, do we send an event for all entities in that case even though buffered events would be a better fit?
2. Observable events shouldn't use the same event types as buffered events; CollisionStarted stores two entities, but for an observer, you'd want the event to only store one entity, since the Trigger already stores the observed entity.

One option I was considering here is to add OnCollisionStarted and OnCollisionEnded components that are like event listeners and internally use one-shot systems to execute the given logic. This would solve 1 (event data is only sent to entities with this component) and 2 (these are separate from the buffered events). However, we would lose some niceties that come with observers (like event bubbling), and this would be a fully custom API that is different from what users might expect for per-entity event handlers in Bevy. People expect to use observers for that.

Can We Still Use Observers?

As I mentioned, collision events on the main branch are now opt-in via the CollisionEventsEnabled marker component. We could extend this to a potential observer-based API, triggering events only for entities with that component, fixing problem 1.

We would still need separate event types to fix problem 2. They'd probably just be called OnCollisionStart and OnCollisionEnd, matching the observer event naming used by Bevy, like OnAdd, Pointer<Move>, and so on.

#[derive(Event, Clone, Copy, Debug, PartialEq, Eq)]
pub struct OnCollisionStart(pub Entity);

#[derive(Event, Clone, Copy, Debug, PartialEq, Eq)]
pub struct OnCollisionEnd(pub Entity);

The docs for these should cross-link to the corresponding buffered events and vice versa, explaining their differences.

Why do we still need the buffered CollisionStarted and CollisionEnded events? There are a few reasons:

• When you do need to go through all/many events, iterating through buffered events is much faster than executing individual observers. Observers are mostly better for truly entity-specific or "fine-grained" event listeners.
• Oftentimes, you might need to specifically iterate through started/ended contact pairs, not events for a specific entity. Using a global observer for this would quickly become complicated and footgunny, since the event might get triggered for both entities in the contact pair, which could easily lead to logic bugs and performing duplicate actions if you're not careful.

There are a few remaining open questions regarding observable collision events:

1. Do we trigger the observers immediately as part of the narrow phase where the buffered events are sent, or defer them until after the solver? The former is more efficient and semantically more accurate (observers run "immediately"), but it means that contact impulses in the observers are always zero since the solver hasn't run yet.
2. Do we change CollisionEventsEnabled from a marker component to a component storing bitflags (or just booleans) to control buffered events and observable events separately? Or do we just make it so that both buffered and observable events are unconditionally sent for entities with the component?
3. Where does #613 fit into all of this?

For 2, at least initially, I think we should keep things simple and keep CollisionEventsEnabled as a marker component. For 1, I'm not sure; if there is an efficient way to defer triggering the event until after the solver, that could be nice, but otherwise we might just trigger it as part of the narrow phase and document the caveats.

For 3, I'm guessing we would just copy the pattern used for normal collision events, but for hit events, i.e. have a HitEventsEnabled component and both buffered and observable events for those entities (or maybe only an observable OnHit event in that case?)