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

Bullet: Compound Shape Collision Fix #56801

Merged
merged 1 commit into from
May 19, 2022

Conversation

Riordan-DC
Copy link

@Riordan-DC Riordan-DC commented Jan 14, 2022

Background:
Kinematic body collisions resolved with the bullet physics space add together penetration recovery vectors from all shape collisions. The issue is that multiple shapes with the same penetration recovery vector push a kinematic body multiple times when only one push was required. The issue is outlined nicely in this post to the bullet forum in 2014: https://pybullet.org/Bullet/phpBB3/viewtopic.php?t=10843

image
(taken from the bullet forum post from user: Enhex)

FIX:
This fix is to offset the collision shape by the recovery vector before detecting penetration so that repeat pushes are not detected. The change reverses the lines in this commit which added compound collision support:
6dd65c0#diff-28f7bca21ebbe699cd16802c05d43f4e085331c7d0427c38da0ab3e6d14dc696L1186
6dd65c0#diff-28f7bca21ebbe699cd16802c05d43f4e085331c7d0427c38da0ab3e6d14dc696L1217

I admit i have no idea the consequences for such a change but i assume they should be confined to KinematicBody.

This hopefully fixes a lot of bullet physics jitter issues seen here (from @pouleyKetchoupp bullet issue tracker):
#29392
#34596

Test without fix:

without_fix-converted.mp4

Test with fix:

with_fix-converted.mp4

Bugsquad edit:

@Riordan-DC Riordan-DC requested a review from a team as a code owner January 14, 2022 23:36
@akien-mga akien-mga added bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:physics labels Jan 15, 2022
@akien-mga akien-mga added this to the 4.0 milestone Jan 15, 2022
@akien-mga akien-mga changed the title Compound Shape Collision Fix Bullet: Compound Shape Collision Fix Jan 17, 2022
@akien-mga
Copy link
Member

akien-mga commented Jan 17, 2022

Could you provide a test project that demonstrates the bug and can be used to confirm the fix?
It would then also be interesting to see how it behaves with GodotPhysics.

It would also be good to test this change against https://github.com/godotengine/godot-demo-projects/tree/master/3d/physics_tests (well whichever tests actually use the modified code) to ensure that it performs as expected. I'll try to test it myself but if you want to do some extra testing, that's very welcome.

For the record for potential testers: this is best tested against the 3.x branch (the patch should be easy to apply) since the Bullet module is currently disabled in master.

@Riordan-DC
Copy link
Author

Riordan-DC commented Jan 17, 2022

@akien-mga
This test demonstrates the bug. It walks two kinematic bodies towards two static body walls. One wall has 1 collision shape and the other has 4 shapes. On 3.4.2 with Bullet physics the kinematic body that collides with the 4 shape wall will jitter while the other wont.

This demonstration shows this issue isnt present in GodotPhysics.

Running this demonstration with this patch the bullet behavior should match the correct GodotPhysics behavior.

test.zip

EDIT: I don't have much experience with the 3D Physics Tests but at a glance the Bullet physics behavior for a kinematic body on a moving platform seems identical between 3.4.2 and this patch.

@akien-mga
Copy link
Member

akien-mga commented Mar 26, 2022

Sorry for the delay. We've removed the Bullet backend in master since it was disabled for more than a year, and our aim for 4.0 is to have it provided as a GDExtension plugin instead of built-in.

It's still used in 3.x and this PR seems relevant to merge there, could you redo it against the 3.x branch?

@Riordan-DC
Copy link
Author

Sorry for the delay. We've removed the Bullet backend in master since it was disabled for more than a year, and our aim for 4.0 is to have it provided as a GDExtension plugin instead of built-in.

It's still used in 3.x and this PR seems relevant to merge there, could you redo it against the 3.x branch?

Sure thing.
Does that require making a new PR against 3.x or can I change this one?
Also I'm very glad we are still supporting Bullet for now :)

@Riordan-DC Riordan-DC changed the base branch from master to 3.x May 19, 2022 08:31
@Riordan-DC Riordan-DC requested review from a team as code owners May 19, 2022 08:31
@Riordan-DC Riordan-DC changed the base branch from 3.x to master May 19, 2022 08:31
@akien-mga
Copy link
Member

akien-mga commented May 19, 2022

It might be easiest to make a new PR targeting the 3.x branch. Otherwise you can indeed change the base branch for this PR as you had attempted (which then tries to merge master + your commit on 3.x, hence a ton of commits), and then you need to force push an update with the commit on top of your local 3.x branch.

Otherwise I can do it myself too so we keep the useful discussion above in this PR.

…ection to prevent recovery vector from adding repeat penetration responses.
@akien-mga akien-mga removed request for a team May 19, 2022 08:48
@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label May 19, 2022
@akien-mga akien-mga modified the milestones: 4.0, 3.5 May 19, 2022
@akien-mga
Copy link
Member

Now it should be good to merge, sorry for the delay :)

@Riordan-DC
Copy link
Author

Thanks so much for your help getting this merged. :))

@akien-mga
Copy link
Member

akien-mga commented May 19, 2022

The change reverses the lines in this commit which added compound collision support:
6dd65c0#diff-28f7bca21ebbe699cd16802c05d43f4e085331c7d0427c38da0ab3e6d14dc696L1186
6dd65c0#diff-28f7bca21ebbe699cd16802c05d43f4e085331c7d0427c38da0ab3e6d14dc696L1217

This is worth highlight for review. This change was done in #27415 together with a lot of other fixes, but I don't see it discussed specifically. @madmiraal did identify this PR as the cause for some of the regressions linked below (#34596 (comment)).

This hopefully fixes a lot of bullet physics jitter issues seen here (from @pouleyKetchoupp bullet issue tracker):
#29392
#34596

I tested the MRPs from those two issues respectively in 3.5 RC 1 and this PR and confirmed that this PR fixes them 👍

This also seems to fix the following issues which are linked in the above issues or reference #27415:

There might be more, still testing. I'll update when I've reviewed everything I can find.

Some other related issues which do not seem fixed as of this PR (no change):

These might warrant further debugging and might not be related to the 3.1 and 3.2 regressions discussed above.

@akien-mga akien-mga merged commit 5d9c7ea into godotengine:3.x May 19, 2022
@akien-mga
Copy link
Member

Thanks a lot, this was a long awaited fix!

@akien-mga
Copy link
Member

akien-mga commented May 19, 2022

I found one issue where behavior seems to have regressed with this fix: #36334.

With the MRP in that issue, enabling "Sync To Physics" on the moving KinematicBody platform as suggested, there's now back and forth on is_on_floor when the platform is moving upwards by being animated.

Here's the updated MRP from that issue which prints is_on_floor(): moving_platform.zip


I also noticed a behavior change with the MRP in #45058. It's buggy before, and buggy after but in a weird different way where the player cube disappears.

@elvisish
Copy link

elvisish commented May 31, 2022

#35780 is still not fixed if the KinemeticBody is moving slow enough, it'll get stuck on gaps in gridmaps (both ground and wall colliders). It does not do this with Godot Physics. It only seems to happen if gravity is being applied to ground, or if the KB is walking into a wall while sliding along it.

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

Successfully merging this pull request may close these issues.

4 participants