Jondolf / avian

ECS-driven 2D and 3D physics engine for the Bevy game engine.
https://crates.io/crates/avian3d
Apache License 2.0
1.35k stars 108 forks source link

Make collision reporting opt-in #481

Open janhohenheim opened 1 month ago

janhohenheim commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.