Setting collision layer/mask after the collision doesn't trigger entered signal

KoBeWi commented 2 years ago

Godot version

244faf5 / 3.x da5c843

System information

W10

Issue description

When you have 2 Area2Ds overlapping with masks/layers that don't match, after you change their mask/layer, the area_entered signal isn't emitted.

Steps to reproduce

Add 2 areas
Make them overlap
Set one area's layer/mask to 0
In a script in _ready(), set the area's layer/mask to 1
Notice that the collision isn't detected

Minimal reproduction project

AreaBug.tscn.txt When you set the collision mask of Area2D in the editor, the collision will be detected.

EDIT: Happens also in 3D. An easy workaround is to toggle monitoring twice.

pouleyKetchoupp commented 2 years ago

Confirmed as regression in BVH broadphase compared to hashgrid (which was fixed in #39894).

Doesn't happen when Project Settings > Physics > 2D > Use BVH is set to disabled.

lawnjelly commented 2 years ago

I just had a look and it looks like the BVH itself is running fine, the layer / mask checks are done externally in the glue broadphase code. So the BVH reports the initial collision, the glue rejects it, then when changing the masks there is no further collision checking because they are already in collision as far as the BVH is concerned.

So a couple of possible options spring to mind:

Maintain a list of collision pairs in the glue code specifically for this
Try and bodge the BVH to re-report the collisions (perhaps by de-activating the object concerned then re-activating). The problem with this is it will re-report a bunch of existing collisions as an unpairing / repairing.

We could also potentially add an optional extra user specified check (callback) for the collisions in the BVH, and use this for the mask checks. Although complexity wise this would be a bit more ugly than handling it in the glue code. Pushing the logical checks to the spatial partitioning looks like the approach taken in #39894. Let me know though, if we want to do this I can help.

To be honest the whole system of pairable_masks and pairable_types in the bvh is pretty ugly, and a historical legacy from the octree. It seems like it could be thought out better .. and indeed is not used in the BVH in Godot 4.

I wonder whether as an alternative we could templatize the whole logical system. Instead of having this fixed pairable_mask and pairable_type (which are very vague and I never know what they are meant to mean), we could instead have a user supplied struct as a template parameter, that contains whatever masks are desired, and a function for testing pairs within the struct that the user can supply. This way all the logic can be supplied by the glue code and managed there, and just treated as a black box by the BVH.

Something like this:

struct PairLogicData
{
bool test_collision(const PairLogicData &o) const
{
// ... logic test here
}
uint32_t type;
uint32_t mask;
};

This way it would be simply to add the layer masks etc to this structure, and it could be customized for the physics and rendering, instead of having some 'one size fits all' approach.

I kind of think that this would be a good idea to do anyway to refactor the BVH. Although I don't think we can do such a change for 3.4 release, as it is too major for RC stage. Although we could backport such a fix in a point release.

pouleyKetchoupp commented 2 years ago

@lawnjelly Thanks for looking into this! Your analysis makes total sense, and the last approach you're proposing does seem like a clean way to handle such cases.

As for the 3.4 release, I'd like to make sure we can find a compromise that doesn't affect common cases too much while fixing the regression in this edge case. An alternative would be to disable the BVH by default for 3.4 and take more time to do the proper changes, but I'd like to try and keep it enabled if we can find an easy enough fix :)

Based on the first two solutions you proposed:

Maintain a list of collision pairs in the glue code specifically for this

This one seems like a possible candidate, although it would mean the glue code would have its own pairing structure to manage logical pairing for all pairs that come from the BVH, which seems a bit overkill.

Try and bodge the BVH to re-report the collisions (perhaps by de-activating the object concerned then re-activating). The problem with this is it will re-report a bunch of existing collisions as an unpairing / repairing.

De-activating an object and re-activating would cause a change in behavior. For example, areas would report exit and enter events again in cases where the collision layer has changed but the objects where already considered colliding.

If it makes sense on the BVH side, maybe as an intermediate solution we could add a specific method to reprocess collision for a given object in the BVH?

This method would just take all existing pairs for this object and call a specific re-pairing callback that takes the previous userdata as an argument (or we could just add an argument to the pairing callback that takes the previous userdata - it would just be null in the normal case).

Then the glue code would still be the one responsible for the logical check, and would know from the previous userdata if it needs to pair, unpair or do nothing (in physics the userdata is the collision pair that is created when the collision layers match).

If you can confirm it makes sense, I will look into implementing it (if I understand correctly, it could be a bit similar to _find_leavers but just using a callback for all existing pairs).

What do you think?

lawnjelly commented 2 years ago

Yes this latter approach could be doable. I've been slightly wary that trying to do a hack-it-in approach just for 3.4 stable might be almost as involved as writing it properly, with just as much potential for bugs.. however the latter approach is probably something you can do easier whereas the template approach might need me to rewrite the templated logic structure, so go for it if you want to try it out in the meantime. It may not be that big a job.

I did start having a go at the template approach this morning, it's a bit involved because we have to convert all call sites at once (the 2d and 3d renderer and physics) and is a bit more major surgery, but should be doable, and having all the logic in one place should be less bug prone in the long run. But I suspect it will require a few betas in case of regressions, so perhaps more suitable for 3.5.

akien-mga commented 2 years ago

Fixed by #54108 (for 3.4).

godotengine / godot