From a12e035a71dff058b32b713885b52811c11799f8 Mon Sep 17 00:00:00 2001 From: John McCardle Date: Sat, 7 Mar 2026 23:22:58 -0500 Subject: [PATCH] Remove entity self-reference cycle UIEntity::init() stored self->data->self = (PyObject*)self with Py_INCREF(self), creating a reference cycle that prevented entities from ever being freed. The matching Py_DECREF never existed. Fix: Remove the `self` field from UIEntity entirely. Replace all read sites (iter next, getitem, get_perspective, entities_in_radius) with PythonObjectCache lookups using serial_number, which uses weak references and doesn't prevent garbage collection. Also adds tp_dealloc to PyUIEntityType to properly clean up the shared_ptr and weak references when the Python wrapper is freed. Closes #266, closes #275 Co-Authored-By: Claude Opus 4.6 --- src/UIEntity.cpp | 6 +- src/UIEntity.h | 7 +- src/UIEntityCollection.cpp | 18 ++-- src/UIGrid.cpp | 22 ++-- .../issue_266_entity_lifecycle_test.py | 100 ++++++++++++++++++ 5 files changed, 123 insertions(+), 30 deletions(-) create mode 100644 tests/regression/issue_266_entity_lifecycle_test.py diff --git a/src/UIEntity.cpp b/src/UIEntity.cpp index 7a3a774..95734e2 100644 --- a/src/UIEntity.cpp +++ b/src/UIEntity.cpp @@ -19,7 +19,7 @@ UIEntity::UIEntity() -: self(nullptr), grid(nullptr), position(0.0f, 0.0f), sprite_offset(0.0f, 0.0f) +: grid(nullptr), position(0.0f, 0.0f), sprite_offset(0.0f, 0.0f) { // Initialize sprite with safe defaults (sprite has its own safe constructor now) // gridstate vector starts empty - will be lazily initialized when needed @@ -241,10 +241,6 @@ int UIEntity::init(PyUIEntityObject* self, PyObject* args, PyObject* kwds) { } } - // Store reference to Python object (legacy - to be removed) - self->data->self = (PyObject*)self; - Py_INCREF(self); - // Set texture and sprite index if (texture_ptr) { self->data->sprite = UISprite(texture_ptr, sprite_index, sf::Vector2f(0,0), 1.0); diff --git a/src/UIEntity.h b/src/UIEntity.h index 4a926a4..8231918 100644 --- a/src/UIEntity.h +++ b/src/UIEntity.h @@ -60,7 +60,6 @@ PyObject* UIGridPointStateVector_to_PyList(const std::vector& class UIEntity { public: - PyObject* self = nullptr; // Reference to the Python object (if created from Python) uint64_t serial_number = 0; // For Python object cache std::shared_ptr grid; std::vector gridstate; @@ -135,6 +134,12 @@ namespace mcrfpydef { .tp_name = "mcrfpy.Entity", .tp_basicsize = sizeof(PyUIEntityObject), .tp_itemsize = 0, + .tp_dealloc = [](PyObject* obj) { + auto* self = (PyUIEntityObject*)obj; + if (self->weakreflist) PyObject_ClearWeakRefs(obj); + self->data.reset(); + Py_TYPE(obj)->tp_free(obj); + }, .tp_repr = (reprfunc)UIEntity::repr, .tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, .tp_doc = PyDoc_STR("Entity(grid_pos=None, texture=None, sprite_index=0, **kwargs)\n\n" diff --git a/src/UIEntityCollection.cpp b/src/UIEntityCollection.cpp index a9207e7..8594219 100644 --- a/src/UIEntityCollection.cpp +++ b/src/UIEntityCollection.cpp @@ -41,10 +41,12 @@ PyObject* UIEntityCollectionIter::next(PyUIEntityCollectionIterObject* self) auto target = *self->current; ++self->current; - // Return the stored Python object if it exists (preserves derived types) - if (target->self != nullptr) { - Py_INCREF(target->self); - return target->self; + // Check cache first to preserve derived class identity + if (target->serial_number != 0) { + PyObject* cached = PythonObjectCache::getInstance().lookup(target->serial_number); + if (cached) { + return cached; // Already INCREF'd by lookup + } } // Otherwise create and return a new Python Entity object @@ -107,13 +109,7 @@ PyObject* UIEntityCollection::getitem(PyUIEntityCollectionObject* self, Py_ssize } } - // Legacy: If the entity has a stored Python object reference, return that - if (target->self != nullptr) { - Py_INCREF(target->self); - return target->self; - } - - // Otherwise, create a new base Entity object + // Create a new base Entity object PyTypeObject* entity_type = &mcrfpydef::PyUIEntityType; auto o = (PyUIEntityObject*)entity_type->tp_alloc(entity_type, 0); diff --git a/src/UIGrid.cpp b/src/UIGrid.cpp index 3bd41ab..b6dfd0c 100644 --- a/src/UIGrid.cpp +++ b/src/UIGrid.cpp @@ -1442,13 +1442,7 @@ PyObject* UIGrid::get_perspective(PyUIGridObject* self, void* closure) } } - // Legacy: If the entity has a stored Python object reference - if (locked->self != nullptr) { - Py_INCREF(locked->self); - return locked->self; - } - - // Otherwise, create a new base Entity object + // Create a new base Entity object auto type = &mcrfpydef::PyUIEntityType; auto o = (PyUIEntityObject*)type->tp_alloc(type, 0); if (o) { @@ -1936,11 +1930,12 @@ PyObject* UIGrid::py_entities_in_radius(PyUIGridObject* self, PyObject* args, Py for (size_t i = 0; i < entities.size(); i++) { auto& entity = entities[i]; - // Return stored Python object if it exists - if (entity->self != nullptr) { - Py_INCREF(entity->self); - PyList_SET_ITEM(result, i, entity->self); - } else { + // Check cache first to preserve derived class identity + PyObject* py_entity = nullptr; + if (entity->serial_number != 0) { + py_entity = PythonObjectCache::getInstance().lookup(entity->serial_number); + } + if (!py_entity) { // Create new Python Entity wrapper auto pyEntity = (PyUIEntityObject*)entity_type->tp_alloc(entity_type, 0); if (!pyEntity) { @@ -1949,8 +1944,9 @@ PyObject* UIGrid::py_entities_in_radius(PyUIGridObject* self, PyObject* args, Py } pyEntity->data = entity; pyEntity->weakreflist = NULL; - PyList_SET_ITEM(result, i, (PyObject*)pyEntity); + py_entity = (PyObject*)pyEntity; } + PyList_SET_ITEM(result, i, py_entity); } return result; diff --git a/tests/regression/issue_266_entity_lifecycle_test.py b/tests/regression/issue_266_entity_lifecycle_test.py new file mode 100644 index 0000000..5d22820 --- /dev/null +++ b/tests/regression/issue_266_entity_lifecycle_test.py @@ -0,0 +1,100 @@ +"""Regression test: entity self-reference cycle (#266, #275). + +Bug: UIEntity::init() stored a reference to the Python wrapper via +self->data->self = (PyObject*)self and Py_INCREF(self). This created +a reference cycle that prevented the entity from ever being freed, +even after removing it from all grids and dropping all Python references. + +Fix: Remove the self field entirely. Use PythonObjectCache (weak refs) +for Python object identity preservation instead. +""" +import mcrfpy +import gc +import sys + +def test_entity_accessible_via_grid(): + """Entities remain accessible through grid.entities after Python ref dropped""" + grid = mcrfpy.Grid(grid_size=(10, 10)) + entity = mcrfpy.Entity(grid_pos=(5, 5), grid=grid) + entity_id = id(entity) + + # Drop the Python reference + del entity + gc.collect() + + # Entity should still be accessible via grid + assert len(grid.entities) == 1, f"Expected 1 entity, got {len(grid.entities)}" + retrieved = grid.entities[0] + assert retrieved.grid_x == 5.0, f"Expected x=5.0, got {retrieved.grid_x}" + print(" PASS: entity_accessible_via_grid") + +def test_entity_removed_from_grid(): + """Entity can be removed from grid cleanly""" + grid = mcrfpy.Grid(grid_size=(10, 10)) + entity = mcrfpy.Entity(grid_pos=(3, 3), grid=grid) + + assert len(grid.entities) == 1 + grid.entities.remove(entity) + assert len(grid.entities) == 0 + print(" PASS: entity_removed_from_grid") + +def test_multiple_entities(): + """Multiple entities can be created, accessed, and removed""" + grid = mcrfpy.Grid(grid_size=(20, 20)) + entities = [] + for i in range(10): + e = mcrfpy.Entity(grid_pos=(i, i), grid=grid) + entities.append(e) + + assert len(grid.entities) == 10 + + # Drop all Python references + del entities + gc.collect() + + # All should still be in grid + assert len(grid.entities) == 10 + + # Access each one + for i in range(10): + e = grid.entities[i] + assert e.grid_x == float(i), f"Entity {i} x={e.grid_x}, expected {float(i)}" + + print(" PASS: multiple_entities") + +def test_entity_transfer_preserves_identity(): + """Entity identity preserved when transferring between grids""" + grid1 = mcrfpy.Grid(grid_size=(10, 10)) + grid2 = mcrfpy.Grid(grid_size=(20, 20)) + entity = mcrfpy.Entity(grid_pos=(5, 5), grid=grid1) + + entity.grid = grid2 + assert len(grid1.entities) == 0 + assert len(grid2.entities) == 1 + + retrieved = grid2.entities[0] + assert retrieved.grid_x == 5.0 + print(" PASS: entity_transfer_preserves_identity") + +def test_iteration_after_gc(): + """Iterating grid.entities works after GC of Python wrappers""" + grid = mcrfpy.Grid(grid_size=(10, 10)) + for i in range(5): + mcrfpy.Entity(grid_pos=(i, 0), grid=grid) + + gc.collect() + + count = 0 + for e in grid.entities: + count += 1 + assert count == 5, f"Expected 5 entities in iteration, got {count}" + print(" PASS: iteration_after_gc") + +print("Testing entity lifecycle (self-reference removal)...") +test_entity_accessible_via_grid() +test_entity_removed_from_grid() +test_multiple_entities() +test_entity_transfer_preserves_identity() +test_iteration_after_gc() +print("PASS: all entity lifecycle tests passed") +sys.exit(0)