Pausing causes various errors with Area2Ds

[markdibarry]

Godot version

4.0.dev (a534346)

System information

Windows 10

Issue description

If a body is within an Area2D's CollisionShape2D when the scene is paused and resumed two things happen:
If the body is higher in hierarchy (behind the Area2D), all signals are disconnected from the Area2D.

2022-05-25.15-37-33.mp4

If the body is lower in the hierarchy (in front of the Area2D), errors are written to the console.

2022-05-25.15-29-50.mp4

Steps to reproduce

1. Create CharacterBody2D with collision.
2. Create Area2D with collision.
3. Connect BodyEntered signal for Area2D.
4. Pause and Resume game while body is inside Area2D.

Minimal reproduction project

CollisionBugGD.zip
(converted to GDScript project)

[markdibarry]

Still present in f38ea25
Updated MRP:
CollisionBug.zip

[markdibarry]

More oddities found with the new pause system while testing:
Currently, some nodes/controls (ex: Button) will connect and fire signals (ex: on_mouse_entered) whether the scene tree is paused or node is set to PROCESS_MODE_DISABLED.
Other nodes, will not connect signals if the scene tree is paused in the same cycle, even if paused after the connection happens.
This can be unexpected and maybe impossible to work around, especially in common scenarios like menu stacks where only the highest menu should have processing/signals enabled.
These may be unrelated and worth separate tickets, but I'm unfamiliar with the internals, so I can't say one way or another.

[markdibarry]

Still present in 8a98110
Updated MRP:
PauseBug.zip

[hcoura]

I spent a while looking into this one, posting my findings here cause I am not sure what to do to fix:

1. Discrepancy in behaviour depending on who is on top

Whoever is on top of the tree is the one "monitoring" for the other, then, when enabling/disabling the process mode code will follow different paths on _apply_disabled() and _apply_enabled() resulting in different behaviours.

2. Signal's disconnection

There isn't in fact a signal disconnection, but by the way the area2d function monitoring bodies coming in and out, when that is on top of the hierarchy (thus monitoring) and you set PROCESS_MODE_DISABLED it doesn't get a callback saying the object is out. This makes it so this counter doesn't get decremented, and when the object actually leaves the area this is never called.

3. Why is the callback not called when Area2D is the one monitoring but not when it's CharacterBody2D

Back to 1., when _apply_disabled() goes the area route, this function is called, particularly it calls for get_space()->area_remove_from_monitor_query_list(&monitor_query_list); in essence removing itself from the monitor_query_list and making it so the callback set for the leaving of the object to not be called on call_queries().

4. Error log when CharacterBody2D is on top

I haven't really looked much to it, but this function has some comments on the counter that feels weird to me. https://github.com/godotengine/godot/blob/master/scene/2d/physics_body_2d.cpp#L366

---

Possible fix

On theory this would handle the worst reported issue, making sure all callbacks are handled before set_space does it's thing.

void GodotArea2D::set_space(GodotSpace2D *p_space) {
	call_queries();  // make sure to handle any callbacks before clearing the space
	if (get_space()) {
		if (monitor_query_list.in_list()) {
			get_space()->area_remove_from_monitor_query_list(&monitor_query_list);
		}
		if (moved_list.in_list()) {
			get_space()->area_remove_from_moved_list(&moved_list);
		}
	}

	monitored_bodies.clear();
	monitored_areas.clear();

	_set_space(p_space);
}

I am not opening a PR because this looks completely wrong, imo the way the process enable/disable is currently done is quite error prone and invites these quirks for specific cases.

[MetalChair]

The problem is that the Area2D is getting a new _body_inout event on unpause if the object is still in the Area2D. Because the body_map on the Area2D already has that object in it, it iterates the rc element. The iteration is done in line 195:

godot/scene/2d/area_2d.cpp

Line 195 in c151d32

E->value.rc++;

What I'm confused about now is what situation would require us to count the same body multiple times on the same Area2D. A quick-and-dirty solution is to just check if the body is already in the body_map, but that completely eliminates the use of the rc iteration. I made this change locally and tested it and it resolves the issue. We could also just explicitly set the rc to 1 one on line 185 and eliminate line 195:

godot/scene/2d/area_2d.cpp

Line 185 in c151d32

E->value.rc = 0;

IE

if (body_in) {
		if (!E) {
			E = body_map.insert(objid, BodyState());
			E->value.rid = p_body;
			E->value.rc = 1;
			E->value.in_tree = node && node->is_inside_tree();
			if (node) {
				node->connect(SceneStringNames::get_singleton()->tree_entered, callable_mp(this, &Area2D::_body_enter_tree).bind(objid));
				node->connect(SceneStringNames::get_singleton()->tree_exiting, callable_mp(this, &Area2D::_body_exit_tree).bind(objid));
				if (E->value.in_tree) {
					emit_signal(SceneStringNames::get_singleton()->body_entered, node);
				}
			}
		}
		if (node) {
			E->value.shapes.insert(ShapePair(p_body_shape, p_area_shape));
		}

		if (!node || E->value.in_tree) {
			emit_signal(SceneStringNames::get_singleton()->body_shape_entered, p_body, node, p_body_shape, p_area_shape);
		}

I think there's two areas to further investigate:

1. Why does unpausing send a new inout event?
2. Is the rc element of BodyState used for reference counting? If that's the case, then in what instance would rc need to be anything other than a boolean?

[MetalChair]

It looks like this system is supposed to implement reference counting for a scenario where multiple object are referencing the same body_map object, but the implementation of Area2D as it currently exists creates a body_map for each instance of Area2D.

I would need information from someone more knowledgeable about the physics engine as a whole, but it appears the only references to value.rc are within this specific function call.

This is also an issue in Area3D

[hsandt]

Same error as in #70848

However, trigger conditions slightly differed. I the bug above, I could only trigger in timeout callback (re-enabling at any time), or during process/physics process by re-enable node immediately in same frame as disabling.

It seems than manual toggle also triggers the bug in this demo, but I'm not sure of the exact conditions.

[hsandt]

@hcoura Thanks, I see now that your point "2. Signal's disconnection" may be the cause of #76219

[golfinq]

I think I found the function calls related to the bug, it comes from monitor_query_list in GodotSpace2D being added to after an unpause which will erroneously trigger a call to Area2D::_body_inout when GodotPhysicsServer2D::flush_queries is called leading to GodotArea2D::call_queries which leads to Area2D::_body_inout finally leading to the reference count getting incremented incorrectly.

I'll try to copy the call stack which causes it:

godot/main/main.cpp

Lines 3488 to 3490 in 7872594

	PhysicsServer2D::get_singleton()->end_sync();
	PhysicsServer2D::get_singleton()->step(physics_step * time_scale);

->

godot/servers/physics_server_2d_wrap_mt.cpp

Lines 73 to 75 in 7872594

	command_queue.flush_all(); //flush all pending from other threads
	physics_server_2d->step(p_step);
	}

->

godot/servers/physics_2d/godot_physics_server_2d.cpp

Lines 1282 to 1284 in 7872594

	for (const GodotSpace2D *E : active_spaces) {
	stepper->step(const_cast<GodotSpace2D *>(E), p_step);
	island_count += E->get_island_count();

->

godot/servers/physics_2d/godot_step_2d.cpp

Lines 255 to 257 in 7872594

	for (uint32_t island_index = 0; island_index < island_count; ++island_index) {
	_pre_solve_island(constraint_islands[island_index]);
	}

->

godot/servers/physics_2d/godot_step_2d.cpp

Lines 84 to 86 in 7872594

	GodotConstraint2D *constraint = p_constraint_island[constraint_index];
	if (p_constraint_island[constraint_index]->pre_solve(delta)) {
	// Keep this constraint for solving.

->

godot/servers/physics_2d/godot_area_pair_2d.cpp

Lines 72 to 74 in 7872594

	if (area->has_monitor_callback()) {
	area->add_body_to_query(body, body_shape, area_shape);
	}

->

godot/servers/physics_2d/godot_area_2d.h

Lines 162 to 164 in 7872594

	if (!monitor_query_list.in_list()) {
	_queue_monitor_update();
	}

->

godot/servers/physics_2d/godot_area_2d.cpp

Lines 199 to 201 in 7872594

	if (!monitor_query_list.in_list()) {
	get_space()->area_add_to_monitor_query_list(&monitor_query_list);
	}

->

godot/servers/physics_2d/godot_space_2d.cpp

Lines 1099 to 1101 in 7872594

	void GodotSpace2D::area_add_to_monitor_query_list(SelfList<GodotArea2D> *p_area) {
	monitor_query_list.add(p_area);
	}

I am not sure what the solution would be, so I am not opening a PR yet

[golfinq]

It happens when disabled because the monitor_query_list is emptied of the area when both paused and disabled (There is a call to area_add_to_monitor_query_list followed by a call to area_remove_from_monitor_query_list)

[Rindbee]

1. Wrong behaviors caused by pause

It takes two to make a collision. If either is disabled, collision will no longer exist. It should be noted that unpair will cause the area to eventually be added to monitor_query_list, although it may have previously tried to remove from monitor_query_list.

godot/servers/physics_2d/godot_area_2d.cpp

Lines 66 to 68 in ba54c34

	if (monitor_query_list.in_list()) {
	get_space()->area_remove_from_monitor_query_list(&monitor_query_list);
	}

If it is disabled in the order of A-B (Area-Body), the area will eventually be added to the monitor_query_list; if it is disabled in the order of B-A (Body-Area), the area will not be added to the monitor_query_list in the end.

godot/servers/physics_2d/godot_space_2d.cpp

Lines 1126 to 1130 in ba54c34

	while (monitor_query_list.first()) {
	GodotArea2D *a = monitor_query_list.first()->self();
	monitor_query_list.remove(monitor_query_list.first());
	a->call_queries();
	}

That is, in the above code, monitor_query_list.first() is different in the two cases. Area2D::_body_inout() will be called when A-B, but will not be called when B-A.

2. Error message during resumed

ERROR: Condition "p_elem->_root" is true.
   at: add (./core/templates/self_list.h:46)

For the error message during resumed. The body is paired with the area in place and becomes active. This causes the same body to be added to the active_list twice.

godot/servers/physics_2d/godot_area_pair_2d.cpp

Lines 99 to 101 in 4df80b0

	if (p_body->get_mode() == PhysicsServer2D::BODY_MODE_KINEMATIC) { //need to be active to process pair
	p_body->set_active(true);
	}

godot/servers/physics_2d/godot_body_2d.cpp

Lines 410 to 412 in 4df80b0

	if (active) {
	get_space()->body_add_to_active_list(&active_list);
	}

[markdibarry]

Tested the new PR. While it doesn't fix the problem enough to close out this ticket, it's still a big step forward. However, I am curious if it is intentional that pausing/unpausing a scene causes enter/exit signals to retrigger in Area*D's. That is the current behavior, so unrelated to the referenced PR, but it's definitely problematic. My wonder is if it's more or less problematic if it does than if it doesn't.

[aniezurawski]

Tested the new PR. While it doesn't fix the problem enough to close out this ticket, it's still a big step forward. However, I am curious if it is intentional that pausing/unpausing a scene causes enter/exit signals to retrigger in Area*D's. That is the current behavior, so unrelated to the referenced PR, but it's definitely problematic. My wonder is if it's more or less problematic if it does than if it doesn't.

I would expect signals to not be retriggered after unpausing. I imagine pausing as some kind of hole in Node's lifetime. It should not affect its behaviour. After unpausing it should stay at the exact same state it was when it was paused.

EDIT: Current behaviour seems to be especially problemiatic in case you want to know if Node exited Area2D but this Area does not care if the object was paused or not meanwhile. It wants to know if it actually left.