Merge branch 'csprance:main' into main

Author: MicroGame0

addons/gecs/ecs/world.gd

@@ -397,15 +397,10 @@ func remove_entity(entity: Entity) -> void:
 
 	for processor in ECS.entity_postprocessors:
 		processor.call(entity)
-	entity_removed.emit(entity)
-	_worldLogger.debug("remove_entity Removing Entity: ", entity)
-	var erase_idx = entities.find(entity)
-	if erase_idx >= 0:
-		entities.remove_at(erase_idx)
-	else:
-		_worldLogger.warning("remove_entity: entity not found in entities array: ", entity)
 
-	# Only disconnect signals if they're actually connected
+	# Disconnect entity signals before notifying observers to prevent re-entrancy:
+	# if an observer's on_component_removed calls entity.remove_component() as a side effect,
+	# the signal must not be connected or it will double-notify observers watching that component.
 	if entity.component_added.is_connected(_on_entity_component_added):
 		entity.component_added.disconnect(_on_entity_component_added)
 	if entity.component_removed.is_connected(_on_entity_component_removed):
@@ -415,6 +410,20 @@ func remove_entity(entity: Entity) -> void:
 	if entity.relationship_removed.is_connected(_on_entity_relationship_removed):
 		entity.relationship_removed.disconnect(_on_entity_relationship_removed)
 
+	# Emit component_removed for each component before teardown
+	# so observers learn about removal when an entity is destroyed
+	for comp in entity.components.values():
+		component_removed.emit(entity, comp)
+		_handle_observer_component_removed(entity, comp)
+
+	entity_removed.emit(entity)
+	_worldLogger.debug("remove_entity Removing Entity: ", entity)
+	var erase_idx = entities.find(entity)
+	if erase_idx >= 0:
+		entities.remove_at(erase_idx)
+	else:
+		_worldLogger.warning("remove_entity: entity not found in entities array: ", entity)
+
 	# Remove from ID registry
 	var entity_id = entity.id
 	if (

addons/gecs/tests/core/test_observers.gd

@@ -390,3 +390,97 @@ func test_observer_with_multiple_entities():
 
 	# Should have detected all 5 changes
 	assert_int(observer.changed_count).is_equal(5)
+
+
+## Test that removing an entity via world.remove_entity triggers on_component_removed
+func test_observer_on_remove_entity_triggers_component_removed():
+	var observer = O_ObserverTest.new()
+	world.add_observer(observer)
+
+	# Create entity with the watched component
+	var entity = Entity.new()
+	var component = C_ObserverTest.new(42, "test")
+	entity.add_component(component)
+	world.add_entity(entity)
+
+	assert_int(observer.added_count).is_equal(1)
+	observer.reset()
+
+	# Remove the entire entity from the world
+	world.remove_entity(entity)
+
+	# Observer should have been notified about the component removal
+	assert_int(observer.removed_count).is_equal(1)
+	assert_object(observer.last_removed_entity).is_equal(entity)
+
+
+## Test that removing an entity with multiple components notifies all relevant observers
+func test_observer_on_remove_entity_multiple_components():
+	var observer = O_ObserverTest.new()
+	var health_observer = O_HealthObserver.new()
+	world.add_observer(observer)
+	world.add_observer(health_observer)
+
+	# Create entity with both watched components
+	var entity = Entity.new()
+	entity.add_component(C_ObserverTest.new())
+	entity.add_component(C_ObserverHealth.new(100))
+	world.add_entity(entity)
+
+	assert_int(observer.added_count).is_equal(1)
+	assert_int(health_observer.health_added_count).is_equal(1)
+	observer.reset()
+	health_observer.reset()
+
+	# Remove the entire entity
+	world.remove_entity(entity)
+
+	# Both observers should have been notified
+	assert_int(observer.removed_count).is_equal(1)
+	assert_int(health_observer.health_removed_count).is_equal(1)
+
+
+## Test that removing multiple entities notifies observer for each
+func test_observer_on_remove_multiple_entities():
+	var observer = O_ObserverTest.new()
+	world.add_observer(observer)
+
+	# Create and add 3 entities
+	var entities_list: Array[Entity] = []
+	for i in range(3):
+		var entity = Entity.new()
+		entity.add_component(C_ObserverTest.new(i))
+		world.add_entity(entity)
+		entities_list.append(entity)
+
+	assert_int(observer.added_count).is_equal(3)
+	observer.reset()
+
+	# Remove all entities
+	for entity in entities_list:
+		world.remove_entity(entity)
+
+	# Observer should have been notified for each removal
+	assert_int(observer.removed_count).is_equal(3)
+
+
+## Regression test: observer side-effects during remove_entity must not cause double notification.
+## O_TestCleanupSideEffect removes C_ObserverHealth when it sees C_ObserverTest removed.
+## If entity signals are still connected during the loop, health_observer fires twice (bug).
+## If signals are disconnected before the loop, health_observer fires exactly once (correct).
+func test_observer_no_double_notification_on_remove_entity():
+	var cleanup_observer = O_TestCleanupSideEffectObserver.new()
+	var health_observer = O_HealthObserver.new()
+	world.add_observer(cleanup_observer)
+	world.add_observer(health_observer)
+
+	var entity = Entity.new()
+	entity.add_component(C_ObserverTest.new())
+	entity.add_component(C_ObserverHealth.new(100))
+	world.add_entity(entity)
+
+	health_observer.reset()
+
+	world.remove_entity(entity)
+
+	assert_int(health_observer.health_removed_count).is_equal(1)

addons/gecs/tests/systems/o_test_cleanup_side_effect_observer.gd

@@ -0,0 +1,15 @@
+## Observer that removes C_ObserverHealth as a side effect when C_ObserverTest is removed.
+## Used to test re-entrancy safety in remove_entity: if signals remain connected during
+## the observer notification loop, health_observer will be notified twice.
+class_name O_TestCleanupSideEffectObserver
+extends Observer
+
+func watch() -> Resource:
+	return C_ObserverTest
+
+func match() -> QueryBuilder:
+	return q.with_all([C_ObserverTest])
+
+func on_component_removed(entity: Entity, component: Resource) -> void:
+	if entity.has_component(C_ObserverHealth):
+		entity.remove_component(C_ObserverHealth)