Merge pull request #71021 from reduz/prevent-physics-callback-removal

Error when removing a physics node during a physics callback

scene/2d/area_2d.cpp

@@ -175,6 +175,7 @@ void Area2D::_body_inout(int p_status, const RID &p_body, ObjectID p_instance, i
 		return; //does not exist because it was likely removed from the tree
 	}
 
+	lock_callback();
 	locked = true;
 
 	if (body_in) {
@@ -224,6 +225,7 @@ void Area2D::_body_inout(int p_status, const RID &p_body, ObjectID p_instance, i
 	}
 
 	locked = false;
+	unlock_callback();
 }
 
 void Area2D::_area_enter_tree(ObjectID p_id) {
@@ -268,6 +270,8 @@ void Area2D::_area_inout(int p_status, const RID &p_area, ObjectID p_instance, i
 	if (!area_in && !E) {
 		return; //likely removed from the tree
 	}
+
+	lock_callback();
 	locked = true;
 
 	if (area_in) {
@@ -317,6 +321,7 @@ void Area2D::_area_inout(int p_status, const RID &p_area, ObjectID p_instance, i
 	}
 
 	locked = false;
+	unlock_callback();
 }
 
 void Area2D::_clear_monitoring() {

scene/2d/collision_object_2d.cpp

@@ -94,10 +94,14 @@ void CollisionObject2D::_notification(int p_what) {
 			bool disabled = !is_enabled();
 
 			if (!disabled || (disable_mode != DISABLE_MODE_REMOVE)) {
-				if (area) {
-					PhysicsServer2D::get_singleton()->area_set_space(rid, RID());
+				if (callback_lock > 0) {
+					ERR_PRINT("Removing a CollisionObject node during a physics callback is not allowed and will cause undesired behavior. Remove with call_deferred() instead.");
 				} else {
-					PhysicsServer2D::get_singleton()->body_set_space(rid, RID());
+					if (area) {
+						PhysicsServer2D::get_singleton()->area_set_space(rid, RID());
+					} else {
+						PhysicsServer2D::get_singleton()->body_set_space(rid, RID());
+					}
 				}
 			}
 
@@ -225,10 +229,14 @@ void CollisionObject2D::_apply_disabled() {
 	switch (disable_mode) {
 		case DISABLE_MODE_REMOVE: {
 			if (is_inside_tree()) {
-				if (area) {
-					PhysicsServer2D::get_singleton()->area_set_space(rid, RID());
+				if (callback_lock > 0) {
+					ERR_PRINT("Disabling a CollisionObject node during a physics callback is not allowed and will cause undesired behavior. Disable with call_deferred() instead.");
 				} else {
-					PhysicsServer2D::get_singleton()->body_set_space(rid, RID());
+					if (area) {
+						PhysicsServer2D::get_singleton()->area_set_space(rid, RID());
+					} else {
+						PhysicsServer2D::get_singleton()->body_set_space(rid, RID());
+					}
 				}
 			}
 		} break;

scene/2d/collision_object_2d.h

@@ -53,6 +53,7 @@ private:
 
 	bool area = false;
 	RID rid;
+	uint32_t callback_lock = 0;
 	bool pickable = false;
 
 	DisableMode disable_mode = DISABLE_MODE_REMOVE;
@@ -83,6 +84,12 @@ private:
 	void _apply_enabled();
 
 protected:
+	_FORCE_INLINE_ void lock_callback() { callback_lock++; }
+	_FORCE_INLINE_ void unlock_callback() {
+		ERR_FAIL_COND(callback_lock == 0);
+		callback_lock--;
+	}
+
 	CollisionObject2D(RID p_rid, bool p_area);
 
 	void _notification(int p_what);

scene/2d/physics_body_2d.cpp

@@ -434,6 +434,8 @@ struct _RigidBody2DInOut {
 };
 
 void RigidBody2D::_body_state_changed(PhysicsDirectBodyState2D *p_state) {
+	lock_callback();
+
 	set_block_transform_notify(true); // don't want notify (would feedback loop)
 	if (!freeze || freeze_mode != FREEZE_MODE_KINEMATIC) {
 		set_global_transform(p_state->get_transform());
@@ -527,6 +529,8 @@ void RigidBody2D::_body_state_changed(PhysicsDirectBodyState2D *p_state) {
 
 		contact_monitor->locked = false;
 	}
+
+	unlock_callback();
 }
 
 void RigidBody2D::_apply_body_mode() {

scene/3d/area_3d.cpp

@@ -230,6 +230,7 @@ void Area3D::_body_inout(int p_status, const RID &p_body, ObjectID p_instance, i
 		return; //likely removed from the tree
 	}
 
+	lock_callback();
 	locked = true;
 
 	if (body_in) {
@@ -279,6 +280,7 @@ void Area3D::_body_inout(int p_status, const RID &p_body, ObjectID p_instance, i
 	}
 
 	locked = false;
+	unlock_callback();
 }
 
 void Area3D::_clear_monitoring() {
@@ -417,6 +419,7 @@ void Area3D::_area_inout(int p_status, const RID &p_area, ObjectID p_instance, i
 		return; //likely removed from the tree
 	}
 
+	lock_callback();
 	locked = true;
 
 	if (area_in) {
@@ -466,6 +469,7 @@ void Area3D::_area_inout(int p_status, const RID &p_area, ObjectID p_instance, i
 	}
 
 	locked = false;
+	unlock_callback();
 }
 
 bool Area3D::is_monitoring() const {

scene/3d/collision_object_3d.cpp

@@ -100,10 +100,14 @@ void CollisionObject3D::_notification(int p_what) {
 			bool disabled = !is_enabled();
 
 			if (!disabled || (disable_mode != DISABLE_MODE_REMOVE)) {
-				if (area) {
-					PhysicsServer3D::get_singleton()->area_set_space(rid, RID());
+				if (callback_lock > 0) {
+					ERR_PRINT("Removing a CollisionObject node during a physics callback is not allowed and will cause undesired behavior. Remove with call_deferred() instead.");
 				} else {
-					PhysicsServer3D::get_singleton()->body_set_space(rid, RID());
+					if (area) {
+						PhysicsServer3D::get_singleton()->area_set_space(rid, RID());
+					} else {
+						PhysicsServer3D::get_singleton()->body_set_space(rid, RID());
+					}
 				}
 			}
 
@@ -223,10 +227,14 @@ void CollisionObject3D::_apply_disabled() {
 	switch (disable_mode) {
 		case DISABLE_MODE_REMOVE: {
 			if (is_inside_tree()) {
-				if (area) {
-					PhysicsServer3D::get_singleton()->area_set_space(rid, RID());
+				if (callback_lock > 0) {
+					ERR_PRINT("Disabling a CollisionObject node during a physics callback is not allowed and will cause undesired behavior. Disable with call_deferred() instead.");
 				} else {
-					PhysicsServer3D::get_singleton()->body_set_space(rid, RID());
+					if (area) {
+						PhysicsServer3D::get_singleton()->area_set_space(rid, RID());
+					} else {
+						PhysicsServer3D::get_singleton()->body_set_space(rid, RID());
+					}
 				}
 			}
 		} break;

scene/3d/collision_object_3d.h

@@ -52,6 +52,7 @@ private:
 	bool area = false;
 
 	RID rid;
+	uint32_t callback_lock = 0;
 
 	DisableMode disable_mode = DISABLE_MODE_REMOVE;
 
@@ -97,6 +98,12 @@ private:
 protected:
 	CollisionObject3D(RID p_rid, bool p_area);
 
+	_FORCE_INLINE_ void lock_callback() { callback_lock++; }
+	_FORCE_INLINE_ void unlock_callback() {
+		ERR_FAIL_COND(callback_lock == 0);
+		callback_lock--;
+	}
+
 	void _notification(int p_what);
 	static void _bind_methods();
 

scene/3d/physics_body_3d.cpp

@@ -484,6 +484,8 @@ struct _RigidBodyInOut {
 };
 
 void RigidBody3D::_body_state_changed(PhysicsDirectBodyState3D *p_state) {
+	lock_callback();
+
 	set_ignore_transform_notification(true);
 	set_global_transform(p_state->get_transform());
 
@@ -578,6 +580,8 @@ void RigidBody3D::_body_state_changed(PhysicsDirectBodyState3D *p_state) {
 
 		contact_monitor->locked = false;
 	}
+
+	unlock_callback();
 }
 
 void RigidBody3D::_notification(int p_what) {

scene/main/node.cpp

@@ -279,7 +279,10 @@ void Node::_propagate_exit_tree() {
 	//block while removing children
 
 #ifdef DEBUG_ENABLED
-	SceneDebugger::remove_from_cache(data.scene_file_path, this);
+	if (!data.scene_file_path.is_empty()) {
+		// Only remove if file path is set (optimization).
+		SceneDebugger::remove_from_cache(data.scene_file_path, this);
+	}
 #endif
 	data.blocked++;
 
@@ -305,7 +308,6 @@ void Node::_propagate_exit_tree() {
 	}
 
 	// exit groups
-
 	for (KeyValue<StringName, GroupData> &E : data.grouped) {
 		data.tree->remove_from_group(E.key, this);
 		E.value.group = nullptr;
@@ -1166,7 +1168,7 @@ void Node::add_sibling(Node *p_sibling, bool p_force_readable_name) {
 
 void Node::remove_child(Node *p_child) {
 	ERR_FAIL_NULL(p_child);
-	ERR_FAIL_COND_MSG(data.blocked > 0, "Parent node is busy setting up children, `remove_child()` failed. Consider using `remove_child.call_deferred(child)` instead.");
+	ERR_FAIL_COND_MSG(data.blocked > 0, "Parent node is busy adding/removing children, `remove_child()` can't be called at this time. Consider using `remove_child.call_deferred(child)` instead.");
 
 	int child_count = data.children.size();
 	Node **children = data.children.ptrw();
@@ -1197,11 +1199,13 @@ void Node::remove_child(Node *p_child) {
 		data.internal_children_back--;
 	}
 
+	data.blocked++;
 	p_child->_set_tree(nullptr);
 	//}
 
 	remove_child_notify(p_child);
 	p_child->notification(NOTIFICATION_UNPARENTED);
+	data.blocked--;
 
 	data.children.remove_at(idx);