Bug when collider has an invalid rotation

[kanerogers]

Hi there! 👋

I stumbled onto the following panic when attempting to set a collider's position with Isometry:

panicked at 'assertion failed: proxy.aabb.maxs[dim] >= self.min_bound' 

build\rapier3d\../../src\geometry\broad_phase_multi_sap\sap_axis.rs:55:13

Tracking the issue down, it looked like the collider's rotation value was invalid (I was parsing a quaternion from another library that was producing a bad value).

I've been able to reproduce this with the following test:

#[test]
    #[cfg(feature = "dim3")]
    fn set_collider_position_with_invalid_quaternion() {
        use crate::prelude::*;
        use na::{vector, Quaternion, Translation3, Unit};
        let mut rigid_body_set = RigidBodySet::new();
        let mut collider_set = ColliderSet::new();
        let mut collision_pipeline = CollisionPipeline::new();
        let physics_hooks = ();
        let integration_parameters = IntegrationParameters::default();
        let mut broad_phase = BroadPhase::new();
        let mut narrow_phase = NarrowPhase::new();

        let rotation = Unit::new_normalize(Quaternion::new(0.0, 0.0, 0.0, 0.0)); // invalid quaternion
        let invalid_position = Isometry {
            translation: Translation3::from(vector![0.0, 0.0, 0.0]),
            rotation,
        };
        let collider = ColliderBuilder::cuboid(1.0, 1.0, 1.0)
            .position(invalid_position)
            .build();
        let _ = collider_set.insert(collider);

        collision_pipeline.step(
            integration_parameters.prediction_distance,
            &mut broad_phase,
            &mut narrow_phase,
            &mut rigid_body_set,
            &mut collider_set,
            &physics_hooks,
            &(),
        );
    }

What should the fix here be?

• Updating parry to check if an AAABB's rotation is valid
• Updating rapier to check if a layer's width is >0 (eg. layer_containing_aabb)
• Updating nalgebra to verify that a UnitQuaternion is valid

I personally think that the UnitQuaternion should always be a valid representation of a rotation, so it should validate is constructor parameters somehow.

Happy to open a pull request for whatever direction you'd prefer, or close this if you think it's a non issue!

[sebcrozet]

It sounds like the actual problem here is a misuse of Unit::new_normalize which assumes that its input is normalizable. There is Unit::try_new if the input is not known to be normalizable in advance. So maybe we should simply improve the documentation of Unit::new_normalize to say explicitly that we assume the input vector to be non-zero. And we could add a debug-assert too.

[kanerogers]

Yep, that's a good point - if UnitQuaternion is guaranteed valid then all these problems go away. ✨

Good plan of attack! Happy to tackle it over in nalgebra. So just to summarise:

• [ ] Update documentation for new_normalize to say that the input vector must be non-zero
• [ ] Add a debug-assert to in new_normalize to verify this.

Closing as it's not a rapier issue.