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

Narrow phase incorrect incremental state update upon collider reparenting #734

Open
hpmv opened this issue Sep 20, 2024 · 2 comments
Open

Comments

@hpmv
Copy link

hpmv commented Sep 20, 2024

When reparenting a collider to another rigidbody, the narrow phase code does not handle the reparenting correctly:

  • Two colliders' interactions are ignored if they belong to the same parent rigidbody, which makes sense. However, this is only checked when adding the collider pair to the narrow phase (to either the interaction or contact graph), in add_pair.
  • As a result, if two colliders used to be in the same parent but one is reparented, narrow phase ignores their collisions until some other mechanism notifies it of the pair again. The symptom of this is that if at the moment of reparenting the two colliders are already overlapping, narrow phase completely ignores the penetration until they are moved apart and have a chance to re-collide.
  • On the other hand, if two colliders used to be in different parents but are reparented to the same rigidbody, narrow phase does not correctly ignore their collisions. The symptom of this is that if two objects are already overlapping (via e.g. squeezing each other) when the reparenting occurs, and if the positions of the two colliders in the rigidbody are also overlapping, then narrow phase continues to treat the overlapping as normal collisions and tries to resolve them, resulting in spontaneous accelerations of the parent rigidbody.

To reproduce (sorry don't have time to write a test case right now but here's exactly what to do):

  • First issue: make two overlapping colliders in one rigidbody. Simulate. Then reparent one collider to a new rigidbody while keeping global position. Simulate. The two colliders will remain overlapping.
  • Second issue: make two colliders in two separate rigidbodies that overlap. Simulate but without resolving the overlapping completely. Then reparent one collider to the other's rigidbody, keeping the global position. The new rigidbody will spontaneously accelerate.

To fix, we should handle collider parent change correctly in handle_user_changes_on_colliders in narrow_phase.rs, but I'm not sure if this can tackle the first issue, because when the pair was rejected earlier, narrow phase does not know of the pair. The second case can be correctly tackled this way by removing the pair that is no longer supposed to be in the graphs. But there may be other implications of parent change that I'm not aware of.

As a workaround, instead of reparenting I am removing and re-adding the same collider. This solves both issues.

@Vrixyz
Copy link
Contributor

Vrixyz commented Sep 26, 2024

Thanks for the detailed reproduction steps, I'm reproducing both scenarios in #742

@Vrixyz
Copy link
Contributor

Vrixyz commented Oct 17, 2024

I did spend some time looking at the first scenario of this issue: find below my observations (based on the reproduction pull request) then a few options on how to fix.

Observations

My idea was that https://github.com/Vrixyz/rapier/blob/02fe758da7002d2d3af0a06adc4d3462ab91bd9a/src/geometry/narrow_phase.rs#L463 would need to check on ColliderChanges::PARENT

But In NarrowPhase::handle_user_changes_on_colliders, https://github.com/Vrixyz/rapier/blob/02fe758da7002d2d3af0a06adc4d3462ab91bd9a/src/geometry/narrow_phase.rs#L434 doesn´t connect

Internal fix

🕵️ I tried another angle: when a parent change is present, add the pair with the colliders from its previous parent to narrow_phase.graph_indices
The test passes if I indeed add that manually immediately after the set_parent:

narrow_phase.add_pair(
           &collider_set,
           &ColliderPair {
               collider1: collider_2_handle,
               collider2: collider_1_handle,
           },
       );

But NarrowPhase::add_pair is private.

Proper fix discussion

But now.. To fix, we have multiple options:

  • Passing narrow_phase to set_parent() and call that code there
  • Store something to be able to make that change later:
    • Storing previous parent is the collider, I'm not a fan of adding temporary state to that, but we already have a change state so it's not totally crazy.
    • Add a new data within colliderSet for this very specific change
  • use @hpmv's workaround internally ? (removing and re-adding the collider)
  • No fix: Expose and document the work-around above 🤔

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

No branches or pull requests

2 participants