CollisionPipeline ergonomics feedback and a bug

[ogapo]

I just got CollisionPipeline working in my project and it works a treat (thanks for the great crate!). Wanted to give back with some feedback about what my stumbling blocks were plus a bug I found. Not sure if the desired "fix" for some of this feedback would be API adjustments (my suggestion) or just some clarifications in the docs, but I'll let maintainers decide. I'm happy to open a PR for any of this if it the direction feels sound.

First off, I want to say I really like how rapier does separation of concerns between the various top level objects. That said, it does result in a lot of passing contextual things in to methods as explicit parameters. This is actually super friendly to the borrow checker (nice) but it does make questions like "ok where am I supposed to get an IslandManager from?" crop up fairly early on.

I think this could be easily remedied without losing any flexibility by doing two things:

1. make a SimplePhysicsPipeline/SimpleCollisionPipeline (example below) and
2. some of the calls ought to take an Option when these won't be used (this is more common for users of CollisionPipeline).

---

Regarding SimplePhysicsPipeline/SimpleCollisionPipeline here's an example of what I made for my project (I was experimenting with swapping between CollisionPipeline and PhysicsPipeline so I made both). These aren't meant to be complicated they're more like living documentation in the spirit of the KinematicCharacterController. There's not much to them so folks can easily copy-paste and start adjusting but it hints at a solid starting point. All the fields are pub so you often just want to pass the whole thing around but it's still separable so you can take references independently when necessary to make the borrow checker happy.

SimplePhysicsPipeline

#[derive(Default)]
pub struct SimplePhysicsPipeline {
    pub gravity: Vector,
    pub integration_parameters: IntegrationParameters,

    pub rigid_body_set: RigidBodySet,
    pub collider_set: ColliderSet,

    pub island_manager: IslandManager,
    pub broad_phase: BroadPhaseMultiSap,
    pub narrow_phase: NarrowPhase,

    pub query_pipeline: QueryPipeline,

    pub impulse_joints: ImpulseJointSet,
    pub multibody_joints: MultibodyJointSet,
    pub ccd_solver: CCDSolver,

    physics_pipeline: PhysicsPipeline,
}
impl SimplePhysicsPipeline {
    pub fn update(&mut self) {
        self.physics_pipeline.step(
            &self.gravity,
            &self.integration_parameters,
            &mut self.island_manager,
            &mut self.broad_phase,
            &mut self.narrow_phase,
            &mut self.rigid_body_set,
            &mut self.collider_set,
            &mut self.impulse_joints,
            &mut self.multibody_joints,
            &mut self.ccd_solver,
            Some(&mut self.query_pipeline),
            &(),
            &(),
        );
    }
}

SimplePhysicsPipeline is pretty straightforward and I think is pretty close to the example usage in some of the docs. I just think it's a lot friendlier of a top-level API than having to make all those objects yourself. Making them isn't hard, it's just it sort of nags the question of "do I need all this stuff or am I doing it wrong?" because it feels like a lot and people not intimately familiar with the innards of a physics system (but who are experienced enough to want to use one) may not know what a CcdSolver is or why they need it.

---

SimpleCollisionPipeline

#[derive(Default)]
pub struct SimpleCollisionPipeline {
    pub collider_set: ColliderSet,

    pub broad_phase: BroadPhaseMultiSap,
    pub narrow_phase: NarrowPhase,

    pub query_pipeline: QueryPipeline,

    collision_pipeline: CollisionPipeline,

    /// we don't use these (but we need them for various calls). Propose simplifying to not need them.
    pub rigid_body_set: RigidBodySet,
    pub island_manager: IslandManager, // awkwardly required for ColliderSet::remove
}
impl SimpleCollisionPipeline {
    pub fn update(&mut self) {
        const PREDICTION_DISTANCE: f32 = 0.002; //= IntegrationParameters::default().prediction_distance()
        self.collision_pipeline.step(
            PREDICTION_DISTANCE, // would prefer IntegrationParameters::DEFAULT_PREDICTION_DISTANCE
            &mut self.broad_phase,
            &mut self.narrow_phase,
            &mut self.rigid_body_set,
            &mut self.collider_set,
            Some(&mut self.query_pipeline),
            &(),
            &(),
        );
    }
}

This one is a little more interesting because it gave me a lot of trouble. You'll notice the note there about the rigid_body_set, and there's also an IslandManager there which isn't even used by update.

CollisionPipeline + IslandManager

The IslandManager is just there because it's basically required in order to use ColliderSet::remove:

pub fn remove(&mut self, handle: ColliderHandle, islands: &mut IslandManager, bodies: &mut RigidBodySet, wake_up: bool) -> Option<Collider>

This is super awkward especially if you're using CollisionPipeline and you haven't even got one around. Of course making a default one to pass in is a simple option, but I don't think that's a pattern you'd want to encourage since randomly doing that to other params (like RigidBodySet) will definitely not produce sound behavior. That all being said, it's only actually used if the wake_up parameter is true so at a minimum I'd propose changing the signature to

pub fn remove(&mut self, handle: ColliderHandle, bodies: &mut RigidBodySet, wake_up: Option<&mut IslandManager>) -> Option<Collider>

bodies could be Option<&mut RigidBodySet> as well if there was an assert that the collider is unparented added for the case when None is passed in. That said, I think a better solve would be to make remove() just take the ColliderHandle and assert!(collider.parent.is_none()) and instead have

/// removes unparented colliders (asserts otherwise)
pub fn remove(&mut self, handle: ColliderHandle) -> Option<Collider>

/// removes parented or unparented colliders
pub fn remove_with_parent(&mut self, handle: ColliderHandle, bodies: &mut RigidBodySet, wake_up: Option<&mut IslandManager>) -> Option<Collider>

This has a nice symmetry with insert/insert_with_parent and I think is fairly intuitive. Could also be remove_unparented and remove if changing the API that much is not desirable.

CollisionPipeline + RigidBodySet

So this is what gave me the majority of the headache. For a variety of reasons it doesn't really look like CollisionPipeline actually handles rigid body changes properly. For my game I have mostly Fixed bodies and a couple of Kinematic bodies. I think this is pretty much the typical case CollisionPipeline was written for.

However, there seems to be a couple of bugs causing the rigid bodies not to be able to update. For one thing CollisionPipeline is calling bodies.take_modified() at the beginning of CollisionPipeline::step but then never doing anything equivalent to PhysicsPipeline::clear_modified_bodies which removes the MODIFIED flag from any bodies in that list. This means once a body is in the modified list, it gets taken once, but then can never be added there again due to a check for MODIFIED in RigidBodySet::get_mut. Personally I think it would probably be cleaner to just reset that bit in take_modified before returning the vector since it has to be done at some point for things to work propertly anyway.

Secondly, CollisionPipeline never calls anything like PhysicsPipeline::advance_to_final_positions which seems to be required for the RigidBody::set_next_kinematic_position family of functions to work. I think this would be pretty trivial to add but...

I actually think a better fix would be to simply have CollisionPipeline not take a rigid_body_set at all, and instead make it clear that the intended use is to simply use colliders if you don't want dynamics. This is what I ended up doing and it's working great. It would be less code to execute and I don't think they really add anything for Collision-Only. There's a few places that require a rigid_body_set but mostly they just pass it along until it's used conditionally if the CollisionShape has a parent.

• ColliderSet::remove requires a rigid_body_set but I discussed alternatives for this above.
• QueryPipeline::cast_shape/intersection_with_shape/cast_ray all require one, but that could be made into an Option very easily since it just passes it down until it's finally used by QueryFilterFlags::test to pull the type of the CollisionShape's parent. I think this actually would be faster if the parent's type was just denormalized onto the shape at creation time. You could then get rid of this parameter entirely and QueryPipeline would be body agnostic as well as a bonus.
• CollisionPipeline::step also takes a rigid body set, but again since it doesn't actually seem necessary to use them to take full advantage of collision, I think having this as part of the API is a little misleading and probably inefficient.

Anyway, i think just removing the concept of rigid bodies from CollisionPipeline might be the best approach. I haven't done a deep dive on the internals to see how hard that is, but I think it'd be reasonable to (assuming it's even necessary) simply assert if any of the CollisionShapes have a parent when they shouldn't (if used with CollisionPipeline). Denormalizing the parent type (defaulting to Fixed or introducing None) might even fix most of that since it seems to mostly be used for further filtering. It certainly would be possible to fix the couple of bugs I've identified but since test_no_rigid_bodies in the code seems to imply this the anticipated usage pattern it feels like it'd be better to just simplify in this case..

If these simplifications were made then the SimpleCollisionPipeline would reduce to:

#[derive(Default)]
pub struct SimpleCollisionPipeline {
    pub collider_set: ColliderSet,

    pub broad_phase: BroadPhaseMultiSap,
    pub narrow_phase: NarrowPhase,

    pub query_pipeline: QueryPipeline,

    collision_pipeline: CollisionPipeline,
}
impl SimpleCollisionPipeline {
    pub fn update(&mut self) {
        self.collision_pipeline.step(
            IntegrationParameters::DEFAULT_PREDICTION_DISTANCE,
            &mut self.broad_phase,
            &mut self.narrow_phase,
            &mut self.collider_set,
            Some(&mut self.query_pipeline),
            &(),
            &(),
        );
    }
}

Isn't that nice?