Collision not detected when animating monitorable

noidexe commented 2 years ago

Godot version

3.3.4, 3.4-beta1..3.4.1-rc3

System information

Windows 10, GLES 3, GTX 2060

Issue description

Enabling monitorable on an area2d doesn't immediately make it detectable if it is already overlapping with another Area2D. This worked in 3.3.4 if you also set the position(even if it was the same position it already had). On 3.4+ it doesn't work either way.

Steps to reproduce

Open the project and run the scene:
- There are two overlapping Area2d named "Left" and "Right", and an animator. "Left" has area_entered connected to a callback
- Test A: The animator will play an animation turning Right:monitorable on and off three times
- After the animation finished "Passed" will be printed if "Left" emitted area_entered and area_exited three times each.
- Test B: The animator will play a second animation setting Right:position to the same position it already had and then turning Right:monitorable on and off three times
- After the animation finished "Passed" will be printed if "Left" emitted area_entered and area_exited three times each
Expected:
- When Right:monitorable changes from false to true it should trigger area_entered on "Left"
- When Right:monitorable changes from true to false it should trigger area_exited on "Left" Actual:
- On 3.3.4 Test A fails and Test B works as expected
- On 3.4+ both tests fail

Note: a workaround is, instead of toggling monitoring, toggling the disabled property in the CollisionShape node, which works in all cases for both versions. It could be argued that my expectation is wrong, that since the areas never changed from overlapping to non-overlapping and vice-versa, no area_enter/area_exit signals should be emitted, but in that case the behavior would still be inconsistent. I also think that most users will expect that if an area becomes detectable it should be detected in all cases, even if already overlapping.

Minimal reproduction project

MonitorableBug2.zip

Calinou commented 2 years ago

@noidexe Can you reproduce this in any of the 3.4 betas and RCs to determine when the regression started?

lawnjelly commented 2 years ago

Unfortunately the MRP is too complex to spend long investigating at this stage (just a couple of boxes and prints would be much better, no animations, the "minimum" really is key here). This is likely to be a bug we are aware of, so really we just want to confirm that (or not).

There is currently some questionable behaviour in areas / statics, we are in the process of adjusting this and coming up with a new specification, see #55640 and #17238.

Torguen commented 2 years ago

I'm not sure I understand exactly what happens but it seems like this error can break a lot of projects?

noidexe commented 2 years ago

Unfortunately the MRP is too complex to spend long investigating at this stage (just a couple of boxes and prints would be much better, no animations, the "minimum" really is key here). This is likely to be a bug we are aware of, so really we just want to confirm that (or not).

There is currently some questionable behaviour in areas / statics, we are in the process of adjusting this and coming up with a new specification, see #55640 and #17238.

I've added a much simpler project that I hope explains the problem better.

noidexe commented 2 years ago

@noidexe Can you reproduce this in any of the 3.4 betas and RCs to determine when the regression started?

I can confirm it on 3.4-beta1 already fails both tests

I went all the way back to 3.1 and all versions fail Test A and pass Test B

Interestingly, 3.1 is the only one to show the following error in the debugger

Testing collision with already colliding Area.
No position keyframe
======

ERROR: _area_inout: Condition ' !area_in && !E ' is true.
   At: scene/2d/area_2d.cpp:264
Failed

Testing collision with already colliding Area.
With position keyframe
======

ERROR: _area_inout: Condition ' !area_in && !E ' is true.
   At: scene/2d/area_2d.cpp:264
[Area2D:1131] entered
[Area2D:1131] exited
ERROR: _area_inout: Condition ' !area_in && !E ' is true.
   At: scene/2d/area_2d.cpp:264
[Area2D:1131] entered
[Area2D:1131] exited
ERROR: _area_inout: Condition ' !area_in && !E ' is true.
   At: scene/2d/area_2d.cpp:264
[Area2D:1131] entered
[Area2D:1131] exited
Passed

lawnjelly commented 2 years ago

If you turn off the BVH in physics/2d/use_bvh you'll see the same behaviour as in 3.3.4.

I haven't looked in detail at this project - part of the reason is that in a sense there is no great need yet, because we are currently changing this area significantly, and you are in effect running old code, and there's little point debugging old code that is about to be replaced.

That said it's probably down to the pair / non-pair logic in the two current trees and the pair checking (or lack of) when changing trees, which we know is not acting as desired at the moment. Rather than bugs, the problem was that the way it should work as far as pairing is concerned wasn't clear from the source (it was reverse engineered from the octree, and there was a certain amount of guesswork involved).

In a sense it didn't work "properly" previously (with the octree and hashgrid), so it is no surprise that the new version (bvh pairing) based on this is not acting as desired.

And 2D was actually previously a hashgrid rather than octree, so there may have also been other slight differences in the previous behaviour. Now we have a bit more feedback we are fixing up the behaviour so it matches user expectations a lot better.

This is something we are changing at the moment and generalising for 2D and 3D, once @pouleyKetchoupp has finished testing the template PR #55640 and we get that merged we will be experimenting with changing the number of trees and collision masks (which is actually only a few lines of code, but has big effects), so we can verify are getting the desired behaviour at that stage to make sure this issue and others are working correctly. I'm waiting on his review and can't do anything on this until it is merged.

I did actually originally make the PR so that everything paired with everything (which seemed to be what users wanted) but @pouleyKetchoupp is concerned about performance implications, so we will do this in a second PR, and are currently looking at having 3 trees for dynamics, non-monitorable areas, and statics, and setting which can collide with which to get the best balance of performance and "desired behaviour", based on the bug reports.

godotengine / godot