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.

Sign up for GitHub

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

Jump to bottom

Ensure KinematicBodies only interact with other Bodies with matching mask. #42641

Merged
merged 1 commit into from
Jul 19, 2021
Merged

Ensure KinematicBodies only interact with other Bodies with matching mask. #42641

merged 1 commit into from
Jul 19, 2021

Conversation

madmiraal
Copy link
Contributor

Currently, when a KinematicBody collides with another CollisionObject the collision_layer->collision_mask combination is checked both ways. This results in unintended and unwanted collisions. For example, you may want a bullet to be stopped by a shield, but not a shield to be stopped by a bullet.

This PR ensures that KinematicBodies only collide with CollisionObjects that have one of the KinematicBody's collision_layer bits in the other CollisionObject's collision_mask. It fixes both 2D and 3D (Godot and Bullet) physics.

This PR is the KinematicBody sister to #42268, which fixes collision_layer->collision_mask checks for Areas.

Fixes #15243.

@madmiraal madmiraal added this to the 4.0 milestone Oct 8, 2020
This was referenced Oct 8, 2020
Closed
Closed
@madmiraal
Copy link
Contributor Author

Rebased following merge of #43952.

@madmiraal madmiraal requested a review from a team as a code owner March 21, 2021 11:02
@madmiraal
Copy link
Contributor Author

Rebased following merge of #46917 and #46937.

@pouleyKetchoupp pouleyKetchoupp mentioned this pull request May 26, 2021
Closed
@madmiraal madmiraal mentioned this pull request May 28, 2021
Closed
@pouleyKetchoupp
Copy link
Contributor

Now we have an approved proposal:
godotengine/godot-proposals#2775

So this change is welcome for 4.0, the PR just needs a rebase.

pouleyKetchoupp
pouleyKetchoupp approved these changes Jul 19, 2021
View reviewed changes
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

I haven't tested the changes in Bullet, but they look ok.
Works fine in Godot Physics.

@akien-mga akien-mga merged commit 471aae3 into godotengine:master Jul 19, 2021
@akien-mga
Copy link
Member

Thanks!

pouleyKetchoupp
pouleyKetchoupp reviewed Jul 19, 2021
View reviewed changes
servers/physics_3d/space_3d_sw.cpp
@@ -549,7 +549,7 @@ int Space3DSW::_cull_aabb_for_body(Body3DSW *p_body, const AABB &p_aabb) {
keep = false;
} else if (intersection_query_results[i]->get_type() == CollisionObject3DSW::TYPE_SOFT_BODY) {
keep = false;
} else if ((static_cast<Body3DSW *>(intersection_query_results[i])->test_collision_mask(p_body)) == 0) {
} else if (!p_body->layer_in_mask(static_cast<Body3DSW *>(intersection_query_results[i]))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually after doing more tests I realized this layer check is inverted, a kinematic body should collide with objects it scans in its own mask, not objects which have its own layer in their mask. I'll make a fix in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the correct way around. A KinematicBody should only interact with other Bodies that have a matching mask.

Copy link
Contributor

Choose a reason for hiding this comment

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

The way you implemented things, walls would define what they stop instead of moving bodies what they collide with. That would be a nightmare for users to setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. Its mask is the layers it sees. So it is the other way around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It probably makes more sense then to revert the logic (and the name) of layer_in_mask to mask_has_layer.

@pouleyKetchoupp pouleyKetchoupp mentioned this pull request Jul 20, 2021
Merged
@madmiraal madmiraal deleted the fix-15243 branch July 20, 2021 06:18
@madmiraal madmiraal mentioned this pull request Jul 21, 2021
Closed
@pouleyKetchoupp pouleyKetchoupp mentioned this pull request Aug 11, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pouleyKetchoupp pouleyKetchoupp pouleyKetchoupp approved these changes

@AndreaCatania AndreaCatania Awaiting requested review from AndreaCatania

@reduz reduz Awaiting requested review from reduz

Assignees
No one assigned
Labels
bug topic:physics
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

KinematicBody/2D uses any combination of layer-mask for move_* collision detection
3 participants
@madmiraal @pouleyKetchoupp @akien-mga