diff --git a/CHANGELOG.md b/CHANGELOG.md index d7eb0d2..d2f8d6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Component reads and tag cleanup no longer retain unused entities through empty registry rows. +- Failed component removals no longer create empty rows, and clearing removes existing empty rows. - Replacing component relations from an inherited view now preserves values which depend on the destination. - Removing a relation tag or component now preserves query membership when the other relation still exists. - Queries collecting component rows now resolve inherited components instead of raising `KeyError` for prefab instances. diff --git a/tcod/ecs/entity.py b/tcod/ecs/entity.py index c498c45..08a548d 100644 --- a/tcod/ecs/entity.py +++ b/tcod/ecs/entity.py @@ -420,9 +420,12 @@ def __getitem__(self, key: ComponentKey[T]) -> T: assert self.__assert_key(key) _components_by_entity = self.entity.registry._components_by_entity for entity in _traverse_entities(self.entity, self.traverse): + components = _components_by_entity.get(entity) + if components is None: + continue try: - return _components_by_entity[entity][key] # type: ignore[no-any-return] - except KeyError: # noqa: PERF203 + return components[key] # type: ignore[no-any-return] + except KeyError: pass raise KeyError(key) @@ -444,10 +447,13 @@ def __delitem__(self, key: TypeForm[object] | tuple[object, TypeForm[object]]) - """Delete a directly held component from an entity.""" assert self.__assert_key(key) - old_value = self.entity.registry._components_by_entity[self.entity].get(key) + components = self.entity.registry._components_by_entity.get(self.entity) + if components is None: + raise KeyError(key) + old_value = components.get(key) - del self.entity.registry._components_by_entity[self.entity][key] - if not self.entity.registry._components_by_entity[self.entity]: + del components[key] + if not components: del self.entity.registry._components_by_entity[self.entity] del self.entity.registry._components_by_type[key][self.entity] @@ -589,8 +595,12 @@ def pop( def clear(self) -> None: """Remove any components stored directly in this entity.""" if self.traverse: - return self(traverse=()).clear() - return super().clear() + self(traverse=()).clear() + return + super().clear() + # MutableMapping.clear() only calls __delitem__ for existing components, + # so it leaves an already empty registry row behind. + self.entity.registry._components_by_entity.pop(self.entity, None) @attrs.define(eq=False, frozen=True, weakref_slot=False) @@ -626,7 +636,7 @@ def add(self, tag: object) -> None: def discard(self, tag: object) -> None: """Discard a tag directly held by an entity.""" - if tag not in self.entity.registry._tags_by_entity[self.entity]: + if tag not in self.entity.registry._tags_by_entity.get(self.entity, ()): return # Already doesn't have tag tcod.ecs.query._touch_tag(self.entity.registry, tag) # Tag removed @@ -648,8 +658,9 @@ def remove(self, tag: object) -> None: def clear(self) -> None: """Remove all tags directly in this entity.""" _tags_by_entity = self.entity.registry._tags_by_entity - while _tags_by_entity[self.entity]: - self.discard(next(iter(_tags_by_entity[self.entity]))) + while tags := _tags_by_entity.get(self.entity): + self.discard(next(iter(tags))) + _tags_by_entity.pop(self.entity, None) def __contains__(self, x: object) -> bool: """Return True if this entity has the given tag.""" @@ -996,10 +1007,16 @@ def __setitem__(self, target: Entity, component: T) -> None: def __delitem__(self, target: Entity) -> None: """Delete a component assigned to the target entity.""" registry = self.entity.registry - del registry._relation_components_by_entity[self.entity][self.key][target] - if not registry._relation_components_by_entity[self.entity][self.key]: - del registry._relation_components_by_entity[self.entity][self.key] - if not registry._relation_components_by_entity[self.entity]: + by_entity = registry._relation_components_by_entity.get(self.entity) + if by_entity is None: + raise KeyError(target) + by_key = by_entity.get(self.key) + if by_key is None: + raise KeyError(target) + del by_key[target] + if not by_key: + del by_entity[self.key] + if not by_entity: del registry._relation_components_by_entity[self.entity] _relations_lookup_discard(registry, self.entity, self.key, target) diff --git a/tests/test_entity_lifecycle.py b/tests/test_entity_lifecycle.py new file mode 100644 index 0000000..3d48f75 --- /dev/null +++ b/tests/test_entity_lifecycle.py @@ -0,0 +1,83 @@ +"""Regression tests for unreferenced entity lifetimes.""" + +from __future__ import annotations + +import gc +import weakref + +import pytest + +from tcod.ecs import Registry + + +@pytest.mark.parametrize( + "operation", + [ + "clear", + "clear_empty", + "clear_tags", + "discard_missing_tag", + "read_missing_component", + "read_inherited_component", + "delete_missing_component", + "delete_missing_relation_component", + "delete_missing_relation_component_type", + ], +) +def test_unused_entity_is_released(operation: str) -> None: + """Reads and cleanup must leave no registry reference to an unused entity.""" + registry = Registry() + prefab = registry["prefab"] + prefab_value = 7 + prefab.components[int] = prefab_value + entity = prefab.instantiate() if operation == "read_inherited_component" else registry.new_entity() + if operation == "clear": + entity.components[int] = 3 + entity.tags.add("inventory") + entity.clear() + elif operation == "clear_empty": + entity.clear() + elif operation == "clear_tags": + entity.tags.add("inventory") + entity.tags.clear() + elif operation == "discard_missing_tag": + entity.tags.discard("inventory") + elif operation == "read_missing_component": + assert entity.components.get(str) is None + elif operation == "read_inherited_component": + assert entity.components[int] == prefab_value + entity.relation_tags_many.clear() + elif operation == "delete_missing_component": + with pytest.raises(KeyError, match="str"): + del entity.components[str] + elif operation == "delete_missing_relation_component": + with pytest.raises(KeyError, match="target"): + del entity.relation_components[str][registry["target"]] + else: + assert operation == "delete_missing_relation_component_type" + entity.relation_components[int][prefab] = 3 + with pytest.raises(KeyError, match="target"): + del entity.relation_components[str][registry["target"]] + assert entity.relation_components[int] == {prefab: 3} + del entity.relation_components[int][prefab] + reference = weakref.ref(entity) + del entity + gc.collect() + assert reference() is None + assert registry["prefab"] is prefab + assert prefab.components[int] == prefab_value + + +def test_incoming_relation_retains_cleared_entity() -> None: + """An incoming relation intentionally keeps its target alive until removed.""" + registry = Registry() + owner, target = registry["owner"], registry.new_entity() + owner.relation_tag["target"] = target + reference = weakref.ref(target) + target.clear() + del target + gc.collect() + assert reference() is not None + del owner.relation_tag["target"] + gc.collect() + assert reference() is None