Skip to content

Fix shape modification not updating graphics in testbed#708

Merged
sebcrozet merged 4 commits intodimforge:masterfrom
agarret7:master
Jan 8, 2025
Merged

Fix shape modification not updating graphics in testbed#708
sebcrozet merged 4 commits intodimforge:masterfrom
agarret7:master

Conversation

@agarret7
Copy link
Copy Markdown
Contributor

@agarret7 agarret7 commented Aug 4, 2024

This PR is for a fix for the GraphicsManager::remove_collider_nodes function in the testbed. The problem was that the entity was despawned but not removed from GraphicsManager::b2sn, creating an invalid state. See this thread for the discussion.

TestbedApp::add_callback still requires a manual refresh with gfx.add_collider if the user wants to see the 3D entity update correctly.

if let Some(gfx) = &mut gfx {
    gfx.remove_collider(ball_coll_handle, &physics.colliders);
    gfx.add_collider(ball_coll_handle, &physics.colliders);
}

debug_shape_modification3 before:

before

debug_shape_modification3 after:

after

Comment thread src_testbed/graphics.rs Outdated
Comment on lines +81 to +92
if let Some(sns) = self.b2sn.remove(&body) {
let mut new_sns = vec![];
for sn in sns.into_iter() {
if let Some(sn_c) = sn.collider {
if sn_c == collider {
commands.entity(sn.entity).despawn();
} else {
new_sns.push(sn);
}
}
}
self.b2sn.insert(body, new_sns);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if let Some(sns) = self.b2sn.remove(&body) {
let mut new_sns = vec![];
for sn in sns.into_iter() {
if let Some(sn_c) = sn.collider {
if sn_c == collider {
commands.entity(sn.entity).despawn();
} else {
new_sns.push(sn);
}
}
}
self.b2sn.insert(body, new_sns);
if let Some(mut sns) = self.b2sn.remove(&body) {
sns = sns
.into_iter()
.filter(|sn| {
let Some(sn_c) = sn.collider else {
return false;
};
if sn_c == collider {
commands.entity(sn.entity).despawn();
return false;
} else {
return true;
}
})
.collect();
self.b2sn.insert(body, sns);
}

Small nitpick, I did that just to help me understand the change, not sure if which is better, don´t worry about it I'll merge one or the other after discussion with seb.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified further with Vec::retain:

        if let Some(sns) = self.b2sn.get_mut(&body) {
            sns.retain(|sn| {
                if let Some(sn_c) = sn.collider {
                    if sn_c == collider {
                        commands.entity(sn.entity).despawn();
                        return false;
                    }
                }

                true
            });
        }

this also fixes the case where we the scene node doesn’t have a collider attached, in which case we don’t want this method to remove it from the list.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I applied the change.)

@ThierryBerger
Copy link
Copy Markdown
Contributor

For the record, I think the best solution is to monitor changes over the shapes, and then modify the mesh handle:

In practice, running these snippets of code:

  • recompute the mesh:

let mesh = prefab_meshs
.get(&shape.shape_type())
.cloned()
.or_else(|| generate_collider_mesh(shape).map(|m| meshes.add(m)));

  • Then reapply the mesh handle on the entity.

But as we're in testbed, I think this solution is good enough :)

@agarret7
Copy link
Copy Markdown
Contributor Author

I had thought that it might be good to factor these sorts of updates into the testbed loop, but wasn't sure how to track incremental computation of the state. Especially since I noticed you have a vector of flags already tracking collider updates in ColliderSet::changes private to the rapier3d crate and that collider.set_shape uses this internally:

/// Sets the shape of this collider.
pub fn set_shape(&mut self, shape: SharedShape) {
self.changes.insert(ColliderChanges::SHAPE);
self.shape = shape;
}

It seems like monitoring those changes from the testbed requires a new function like pub fn get_changes() -> &ColliderChanges? From what I could tell the ColliderChanges otherwise cannot be seen outside the main crate and thus wasn't sure how you might select meshes to be recomputed.

@sebcrozet sebcrozet merged commit 552cfeb into dimforge:master Jan 8, 2025
@sebcrozet
Copy link
Copy Markdown
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants