Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stand-alone CollisionPipeline doesn't account for changes to bodies #815

Open
BrettHemes-3M opened this issue Mar 26, 2025 · 3 comments
Open

Comments

@BrettHemes-3M
Copy link

Related to #191 (Stand-alone QueryPipeline doesn't account for changes to bodies)

I am using the CollisionPipeline with colliders attached to parent bodies (with some positional offsets). Upon stepping the pipeline, the respective modified bodies lists are cleared but the actual changed bits of the bodies in the RigidBodySet are not. This prevents the body from ever getting re-added to the modified list upon subsequent changes (i.e., when I update the parent body's position using get_mut).

The blocking check starts here:

pub fn get_mut(&mut self, handle: RigidBodyHandle) -> Option<&mut RigidBody> {

and the modification addition skipped here:
if !rb.changes.contains(RigidBodyChanges::MODIFIED) {

The if check never passes because the modified bits are already set (from never being cleared).

propagate_modified_body_positions_to_colliders doesn't work either due to the above (uses modification list, not changed bits)

The physics pipeline calls is own helpers, clear_modified_colliders and clear_modified_bodies while the CollisionPipeline only calls a single clear_modified_colliders

Should the CollisionPipeline clear the modified body bits? If not, and/or adding a way to clear the changed bits in my RigidBodySet manually would help (not in public API currently).

Workaround

As of right now, I can get the changes to propagate by manually updating the collider positions wrt their parent (to the same value 🙃) every time I move a body like so:

let collider_handle = self.name_collider_map.get(name).unwrap();
let collider = self.colliders.get_mut(*collider_handle).unwrap();
collider.set_position_wrt_parent(*collider.position_wrt_parent().unwrap());

Awesome library btw, thanks for all the hard work 🎉

@BrettHemes-3M
Copy link
Author

Also to add, this affects all body changes via get_mut... (enable/disable, etc.) so I think some form of solution is required.

@Vrixyz
Copy link
Contributor

Vrixyz commented Mar 27, 2025

I share your concerns about this being error-prone, if you have a minimal reproduction use case, this could be a good base to make a new test and make sure it works in an expected way :)

@BrettHemes-3M
Copy link
Author

(had put this in the wrong issue... moving it here)

The same/similar issue is mentioned in #662

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...

Here removing the rigid body set from the collision pipeline is proposed. I was using them to have colliders offset from some other origin. I could keep track of this manually with the proposed change OR I could probably get the same behavior using a compound shape with only one shape in the vec (with a non-identity position).

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

No branches or pull requests

2 participants