dimforge / rapier

2D and 3D physics engines focused on performance.
https://rapier.rs
Apache License 2.0
3.91k stars 244 forks source link

Panics in broad_phase_multi_sap #180

Closed vilcans closed 3 years ago

vilcans commented 3 years ago

I have observed these two panics in broad_phase_multi_sap. I suspect they are related, but I don't know how to reproduce the error.

I suspect they started appearing once I implemented a feature that updates the collider of an existing object, to make it grow or shrink while it moves. The code is similar to this:

let capsule = Capsule::new_z(
    (CAPSULE_HEIGHT / 2.0 - CAPSULE_RADIUS) * scale,
    CAPSULE_RADIUS * scale,
)
.transform_by(&Isometry3::translation(
    0.0,
    0.0,
    CAPSULE_CENTER_Z * scale,
));
let body = bodies.get_mut(body_handle).unwrap();
body.set_position(pose.into(), true);
let collider = colliders.get_mut(body.colliders()[0]).unwrap();
*collider.shape_mut().as_capsule_mut().unwrap() = capsule;

FWIW, I can still reproduce it even if I recreate the PhysicsPipeline instance for every tick.

Panic traces

swap_remove

Error is always the same: len == 0, but tries to do swap_remove(0).

thread 'fireman-update' panicked at 'swap_remove index (is 0) should be < len (is 0)', library\alloc\src\vec\mod.rs:1262:13
stack backtrace:
   0: std::panicking::begin_panic_handler
             at /rustc/2fd73fabe469357a12c2c974c140f67e7cdd76d0\/library\std\src\panicking.rs:493
   1: core::panicking::panic_fmt
             at /rustc/2fd73fabe469357a12c2c974c140f67e7cdd76d0\/library\core\src\panicking.rs:92
   2: alloc::vec::{{impl}}::swap_remove::assert_failed
             at /rustc/2fd73fabe469357a12c2c974c140f67e7cdd76d0\/library\alloc\src\vec\mod.rs:1262
   3: rapier3d::geometry::broad_phase_multi_sap::sap_layer::SAPLayer::complete_removals
   4: rapier3d::geometry::broad_phase_multi_sap::broad_phase::BroadPhase::update
   5: rapier3d::pipeline::physics_pipeline::PhysicsPipeline::new
   6: rapier3d::pipeline::physics_pipeline::PhysicsPipeline::step

assertion failed

Here the numbers may vary.

thread 'fireman-update' panicked at 'assertion failed: `(left == right)`
  left: `56`,
 right: `54`', ...\.cargo\registry\src\github.com-1ecc6299db9ec823\rapier3d-0.8.0\src\geometry\broad_phase_multi_sap\sap_layer.rs:161:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
vilcans commented 3 years ago

Seems similar to #177, but that shows another panic.

vilcans commented 3 years ago

I found a way to reliably reproduce the bug, so I've looked more into it. Some findings:

It doesn't matter if I change the collider in place using *collider.shape_mut().as_capsule_mut().unwrap() = capsule; or collider.set_shape(SharedShape(Arc::new(capsule)));. It still panics.

My code changes the position in addition to the collider. It doesn't make a difference if I set the position before or after modifying the collider. Or if I don't update the position at all. It still panics.

vilcans commented 3 years ago

I found a workaround (that I thought I had already tried): Remove the collider and create a new one instead.

        colliders.remove(old_collider_handle, bodies, false);
        colliders.insert(create_collider(), body_handle, bodies);

Then there's no panic anymore.

sebcrozet commented 3 years ago

Thank you for the details about this bug. I managed to reproduce the panic from #177 as well as the swap_remove panic you reported in this issue. I did not reproduce the assertion failure you mentioned, but chances are that fixing that swap_remove panic will also result in the assertion failure being fixed.

Feel free to reopen this issue if you are still having problems after #185 is merged.