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

Fix collision detection by calculating positive penetration depth. #949

Closed
wants to merge 1 commit into from
Closed

Conversation

ak-1
Copy link
Contributor

@ak-1 ak-1 commented Nov 29, 2020

I had an issue with collision detection not working properly. Changing in collide_aabb the calculation of the penetration depth (making it a positive value) fixed it for me.

@ak-1
Copy link
Contributor Author

ak-1 commented Nov 29, 2020

To reproduce the issue play breakout with the following change applied:

--- a/examples/game/breakout.rs
+++ b/examples/game/breakout.rs
@@ -59,7 +59,7 @@ fn setup(
         .spawn(SpriteBundle {
             material: materials.add(Color::rgb(1.0, 0.5, 0.5).into()),
             transform: Transform::from_translation(Vec3::new(0.0, -50.0, 1.0)),
-            sprite: Sprite::new(Vec2::new(30.0, 30.0)),
+            sprite: Sprite::new(Vec2::new(100.0, 100.0)),
             ..Default::default()
         })
         .with(Ball {

Here is a demo: https://ibb.co/bHr8LHq

@karroffel karroffel added C-Bug An unexpected or incorrect behavior A-Core Common functionality for all bevy apps labels Nov 29, 2020
@cart
Copy link
Member

cart commented Nov 30, 2020

The fix in this pr definitely solves the issue in question. I can reproduce the error shown in the video and the pr resolves it, but I think flipping the sign just shifts the problem elsewhere (in a way that is more favorable for the breakout example). I think the core issue is that the penetration depth is a signed comparison, when it should really be absolute.

Applying this change instead both resolves the bug in the demo video and resolves a bunch of other collision jank. Can we adjust this pr to do that instead?

image

@ak-1
Copy link
Contributor Author

ak-1 commented Dec 1, 2020

Looks good to me. I wanted to update this PR but closed it accidentaly and was not able to re-open it. #966 is the followup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants