Fix user changes handling

[ThierryBerger]

Fix #802

Note: Fixing those + adding tests is a good first step but we should thrive for making those mistakes more difficult to make altogether.

[ThierryBerger]

This makes me wonder if we should change ColliderChanges::POSITION (and the likes) from:
const POSITION = 1 << 3
to
const POSITION = 1 << 3 | 1 << 0; (adding ColliderChanges::MODIFIED to it)

This would clarify their relation and make mistakes harder to make.

[ThierryBerger]

This change is probably not possible because MODIFIED is used to check if a collider is part of the modified_colliders list. This seems difficult to maintain, I imagine searching for modified_colliders.push should be able to lead to a MODIFIED bit to be added to that collider, but it's sometimes tricky to assess.

I imagine changing modified_colliders to a Hashmap may be less performant than storing that in the bitfield, but I'm throwing the idea.

[ThierryBerger]

I'm not sure this is correct ; the fix may be more around https://github.com/Vrixyz/rapier/blob/607f0988596d346b6afa70e5cfbc8f0f3519e02c/src/geometry/collider.rs#L271 ; to set ColliderChanges::MODIFIED flag too (see https://github.com/dimforge/rapier/pull/803/files#r1980964215 comment for a longer term fix maybe ?)

And then add then refill self.modified_colliders only when applicable (check changes flag)

[Johannes0021]

This might be a better test because it covers both RigidBodyChanges::POSITION with enable and disable, and it doesn't run for 200 iterations.
What do you think?

use rapier2d::prelude::*;

fn main() {
    // Create other structures necessary for the simulation.
    let mut rigid_body_set = RigidBodySet::new();
    let mut collider_set = ColliderSet::new();
    let gravity = vector![0.0, -9.81];
    let integration_parameters = IntegrationParameters::default();
    let mut physics_pipeline = PhysicsPipeline::new();
    let mut island_manager = IslandManager::new();
    let mut broad_phase = DefaultBroadPhase::new();
    let mut narrow_phase = NarrowPhase::new();
    let mut impulse_joint_set = ImpulseJointSet::new();
    let mut multibody_joint_set = MultibodyJointSet::new();
    let mut ccd_solver = CCDSolver::new();
    let mut query_pipeline = QueryPipeline::new();
    let physics_hooks = ();
    let event_handler = ();


    // Create the ball with default values for translation, position and enabled state
    let rigid_body = RigidBodyBuilder::dynamic()
        .translation(vector![0.0, 0.0])
        .rotation(0.0)
        .build();
    let collider = ColliderBuilder::ball(0.5)
        .enabled(true)
        .build();
    let ball_body_handle = rigid_body_set.insert(rigid_body);
    collider_set.insert_with_parent(collider, ball_body_handle, &mut rigid_body_set);


    physics_pipeline.step(
        &gravity,
        &integration_parameters,
        &mut island_manager,
        &mut broad_phase,
        &mut narrow_phase,
        &mut rigid_body_set,
        &mut collider_set,
        &mut impulse_joint_set,
        &mut multibody_joint_set,
        &mut ccd_solver,
        Some(&mut query_pipeline),
        &physics_hooks,
        &event_handler,
    );


    // This causes the error:

    // Test RigidBodyChanges::POSITION and disable
    {
        let ball_body = &mut rigid_body_set[ball_body_handle];

        // Also, change the translation and rotation to different values
        ball_body.set_translation(vector![1.0, 1.0], true);
        ball_body.set_rotation(nalgebra::UnitComplex::new(1.0), true);

        ball_body.set_enabled(false);
    }

    physics_pipeline.step(
        &gravity,
        &integration_parameters,
        &mut island_manager,
        &mut broad_phase,
        &mut narrow_phase,
        &mut rigid_body_set,
        &mut collider_set,
        &mut impulse_joint_set,
        &mut multibody_joint_set,
        &mut ccd_solver,
        Some(&mut query_pipeline),
        &physics_hooks,
        &event_handler,
    );


    // Test RigidBodyChanges::POSITION and enable
    {
        let ball_body = &mut rigid_body_set[ball_body_handle];

        // Also, change the translation and rotation to different values
        ball_body.set_translation(vector![0.0, 0.0], true);
        ball_body.set_rotation(nalgebra::UnitComplex::new(0.0), true);

        ball_body.set_enabled(true);
    }

    physics_pipeline.step(
        &gravity,
        &integration_parameters,
        &mut island_manager,
        &mut broad_phase,
        &mut narrow_phase,
        &mut rigid_body_set,
        &mut collider_set,
        &mut impulse_joint_set,
        &mut multibody_joint_set,
        &mut ccd_solver,
        Some(&mut query_pipeline),
        &physics_hooks,
        &event_handler,
    );
}

[Johannes0021]

I rewrote it again, set default values at the beginning, and ensured that the values change.

[sebcrozet]

Looking good, thanks! I took it one step further by defining a wrapper around the modified handle Vec. That way we can avoid future mistakes without having to introduce the overhead of a HashSet.