Ensure Areas and Bodies only interact with Areas with layers in their mask

[madmiraal]

Currently, when interacting with an Area, the collision_layer-collision_mask combination is checked both ways. This results in interactions that are not wanted:

• If an Area's mask bit is disabled, it should not respond to CollisionObjects with that layer bit set.
• If a CollisionObject's layer bit is disabled, an Area with that mask bit set, should not respond to the CollisionObject.

However, in both cases, if the other CollisionObject's mask matches one of the Area's layer bits, the Area will incorrectly respond to the other CollisionObject.

This PR ensures that Areas only respond to CollisionObjects that have layer bits that match the Area's mask bits. It fixes both 2D and 3D (Godot and Bullet) physics.

Fixes #7644.

[AndreaCatania]

This changes how the layer and mask check works. It may be good or bad depending the situations.

For example, the bullet default check make sure that layer and mask matches in both ways. It allows a grater range of combination but in the past was considered too difficult to get right, so the Godot approach was kept.

I think we need a proposal to change this behaviour, and in case the check should behave the same also for RigidBodies, otherwise would be too confusing.

[capnm]

@madmiraal I looked at the relevant issue-tests (take a look at the attached collection) on the 3.2 branch and noticed the following with the PR:

• SpatialAreaEnterLayer works as intended.
• AreaEnterLayer is still broken, one area triggers the exited event only.
• Area2DMonitor: The issue fixed by [3.x] Fix Area monitorable in 2D and 3D Godot physics. #41699 ocurrs again (but I may made a mistake by merging the PRs)

test-pr42268.zip

I think we need a proposal to change this behaviour, and in case the check should behave the same also for RigidBodies, otherwise would be too confusing.

Someone's 4yr old comment »I intuitively expect that an object would live in it's layers, and see stuff in it's mask.«
sounds as very good proposal ;)

[madmiraal]

@AndreaCatania This PR fixes an old issue (#7644). I understand that when issues are old, people get used to things not working and then are surprised when they are fixed and things behave differently. But I don't think that there is an argument for not fixing an issue simply because it is old.
Edit: I agree that the same should be done for RigidBodies and all PhysicsBodies including KinematicBodies i.e. #15243, but I'll do that in a separate PR to make reviewing easier.

@capnm Thanks for testing. This PR specifically addresses the issues of Area's collision_layer and collision_mask not working properly; as described in #7644. The issues you see are due to the problems with monitorable not working properly, which are fixed with #41698 (and its 3.2 version #41699). #41698 and #41699 haven't been merged yet; so the problems with monitorable remain with this PR too. As you noticed, these two PRs will create merge conflicts, which will need to be addressed when rebasing depending on the order these two PRs are merged.

[madmiraal]

@capnm

AreaEnterLayer is still broken, one area triggers the exited event only.

The example project doesn't have the area_entered signal connected, it has the body_entered signal connected; therefore only the "area" exited event is triggered. The project works as expected when the area_entered signal is connected.

[capnm]

As you noticed, these two PRs will create merge conflicts...

In case someone has time to take a quick look at the merge (pr41699 + pr42268 ;)
0001-Ensure-Areas-and-Bodies-only-interact-with-Areas-wit.patch.gz

[AndreaCatania]

It works following a decision made time ago, so it's working as intended. If you think that such decision should be changed (and I'm not against this idea), I think we should discuss it in a proposal. Also, I think that such change cannot be back ported to 3.2 since it's breaking change.

[Zireael07]

@AndreaCatania, source for your statement that it is following a deliberate decision made in the past? To me it looks more like a bug that is being fixed now after being overlooked for years.

[AndreaCatania]

Godot physics engine was working in that way because reduz thought it was the easiest thing to have. I'm not against changing it, rather in the past I proposed to stick with bullet standard that allows more combinations (similar to the following change).

Since it's working as was designed, I'm saying this is not a bug, because it's already working as intended.

I think we need a proposal because this is a change in how the engine behave, and I think we should discuss how it should be. Especially because I think that RigidBody and Area should behave exactly the same, so we need a common agreement.

Which is not strange, since the current workflow to change the engine behavior is to open a proposal and discuss it.

[madmiraal]

#42641 is the sister PR for KinematicBodies.

[madmiraal]

Rebased following #42639.

[madmiraal]

Rebased following merge of #46917 and #46937.

[pouleyKetchoupp]

The documentation only states that it's checked both ways:
https://docs.godotengine.org/en/stable/classes/class_physicsbody.html#class-physicsbody-property-collision-layer
Without any mention of a specific behavior for areas in the documentation, and given that it has worked like that for years, this change can be considered compatibility breaking.

If we ignore that, we're just going to end up with as many issues the other way around from users who expect the original behavior to be correct.

Godot 4.0 would be a great time to make this change though, and I'm personally all for it.

So I agree with @AndreaCatania that a proposal is the best route. It can be very short, but it's just to summarize what collision layer/mask rules should be in different cases (including kinematic bodies as well, and maybe some other cases). This proposal can also be used to discuss how to update the documentation, which is needed too.

[p-brighenti]

I recently ran into this issue and I strongly agree with the quote I found on the linked PR:

I intuitively expect that an object would live in it's layers, and see stuff in it's mask.

Having it be bidirectional makes it harder to manage in my opinion. When trying to figure out how something works I defer to the documentation. In this case Area2D's documentation entry for the collision_mask field gives the following explanation:

The physics layers this area scans to determine collision detection. See Collision layers and masks in the documentation for more information.

Given that an area can be scanning no layers (having no layers in it's) mask, then that area should not be detecting any collisions.

As it stands you can't look at the Area2D's layer/mask setup and get the full picture of how the node is interacting with the nodes in your physics layers which feels counter-intuitive and makes debugging harder.
Even after the developer is aware of that fact, it forces them to implement a way to discard the signal, so I can't agree that it being bidirectional makes it easier to use/more beginner friendly than being able to understand which nodes collide with each other through the layers in their masks.

Is there currently any proposal for this change to be included in 4.0?

[sttifer]

I'm having a problem:
When A enter in area B, but B is not in area A, both has signal trigered!

[pouleyKetchoupp]

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

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

[madmiraal]

Updated to include equivalent fix from #50685.

[pouleyKetchoupp]

Superseded by #51801.