diff --git a/src/UIEntity.cpp b/src/UIEntity.cpp index 4a82a37..4a07b2f 100644 --- a/src/UIEntity.cpp +++ b/src/UIEntity.cpp @@ -26,6 +26,7 @@ UIEntity::UIEntity() } UIEntity::~UIEntity() { + releasePyIdentity(); if (serial_number != 0) { PythonObjectCache::getInstance().remove(serial_number); } @@ -230,7 +231,7 @@ int UIEntity::init(PyUIEntityObject* self, PyObject* args, PyObject* kwds) { // Initialize weak reference list self->weakreflist = NULL; - // Register in Python object cache + // Register in Python object cache if (self->data->serial_number == 0) { self->data->serial_number = PythonObjectCache::getInstance().assignSerial(); PyObject* weakref = PyWeakref_NewRef((PyObject*)self, NULL); @@ -239,6 +240,13 @@ int UIEntity::init(PyUIEntityObject* self, PyObject* args, PyObject* kwds) { Py_DECREF(weakref); // Cache owns the reference now } } + + // Hold a strong reference to preserve Python subclass identity. + // Without this, the Python wrapper can be GC'd while the C++ entity + // lives on in a grid, and later access returns a base Entity wrapper + // that lacks subclass methods. Cleared in die() and set_grid(None). + self->data->pyobject = (PyObject*)self; + Py_INCREF(self); // Set texture and sprite index if (texture_ptr) { @@ -660,6 +668,9 @@ int UIEntity::set_grid(PyUIEntityObject* self, PyObject* value, void* closure) entities->erase(it); } self->data->grid.reset(); + + // Release identity strong ref — entity left grid + self->data->releasePyIdentity(); } return 0; } @@ -762,6 +773,9 @@ PyObject* UIEntity::die(PyUIEntityObject* self, PyObject* Py_UNUSED(ignored)) entities->erase(it); // Clear the grid reference self->data->grid.reset(); + + // Release identity strong ref — entity is no longer in a grid + self->data->releasePyIdentity(); } Py_RETURN_NONE; diff --git a/src/UIEntity.h b/src/UIEntity.h index 8231918..ca2ac31 100644 --- a/src/UIEntity.h +++ b/src/UIEntity.h @@ -61,6 +61,7 @@ class UIEntity { public: uint64_t serial_number = 0; // For Python object cache + PyObject* pyobject = nullptr; // Strong ref: preserves Python subclass identity while in grid std::shared_ptr grid; std::vector gridstate; UISprite sprite; @@ -70,7 +71,17 @@ public: UIEntity(); ~UIEntity(); - + + // Release the strong reference that preserves Python subclass identity. + // Called when entity leaves a grid (die, set_grid, collection removal). + void releasePyIdentity() { + if (pyobject) { + PyObject* tmp = pyobject; + pyobject = nullptr; + Py_DECREF(tmp); + } + } + // Visibility methods void ensureGridstate(); // Resize gridstate to match current grid dimensions void updateVisibility(); // Update gridstate from current FOV @@ -136,6 +147,8 @@ namespace mcrfpydef { .tp_itemsize = 0, .tp_dealloc = [](PyObject* obj) { auto* self = (PyUIEntityObject*)obj; + // Clear the identity ref without DECREF - we ARE this object + if (self->data) self->data->pyobject = nullptr; if (self->weakreflist) PyObject_ClearWeakRefs(obj); self->data.reset(); Py_TYPE(obj)->tp_free(obj); diff --git a/src/UIEntityCollection.cpp b/src/UIEntityCollection.cpp index 8594219..240287a 100644 --- a/src/UIEntityCollection.cpp +++ b/src/UIEntityCollection.cpp @@ -156,6 +156,7 @@ int UIEntityCollection::setitem(PyUIEntityCollectionObject* self, Py_ssize_t ind if (self->grid) { self->grid->spatial_hash.remove(*it); } + (*it)->releasePyIdentity(); (*it)->grid = nullptr; list->erase(it); return 0; @@ -181,6 +182,7 @@ int UIEntityCollection::setitem(PyUIEntityCollectionObject* self, Py_ssize_t ind } // Clear grid reference from the old entity + (*it)->releasePyIdentity(); (*it)->grid = nullptr; // Replace the element and set grid reference @@ -409,6 +411,7 @@ int UIEntityCollection::ass_subscript(PyUIEntityCollectionObject* self, PyObject if (self->grid) { self->grid->spatial_hash.remove(*it); } + (*it)->releasePyIdentity(); (*it)->grid = nullptr; } self->data->erase(start_it, stop_it); @@ -426,6 +429,7 @@ int UIEntityCollection::ass_subscript(PyUIEntityCollectionObject* self, PyObject if (self->grid) { self->grid->spatial_hash.remove(*it); } + (*it)->releasePyIdentity(); (*it)->grid = nullptr; self->data->erase(it); } @@ -479,6 +483,7 @@ int UIEntityCollection::ass_subscript(PyUIEntityCollectionObject* self, PyObject if (self->grid) { self->grid->spatial_hash.remove(*it); } + (*it)->releasePyIdentity(); (*it)->grid = nullptr; } @@ -610,6 +615,7 @@ PyObject* UIEntityCollection::remove(PyUIEntityCollectionObject* self, PyObject* if (self->grid) { self->grid->spatial_hash.remove(*it); } + (*it)->releasePyIdentity(); (*it)->grid = nullptr; list->erase(it); Py_RETURN_NONE; @@ -727,6 +733,20 @@ PyObject* UIEntityCollection::pop(PyUIEntityCollectionObject* self, PyObject* ar entity->grid = nullptr; list->erase(it); + // Return cached Python object if available (preserves subclass identity) + if (entity->serial_number != 0) { + PyObject* cached = PythonObjectCache::getInstance().lookup(entity->serial_number); + if (cached) { + // Release identity ref — entity is leaving the grid + // The caller now holds a strong ref via 'cached' + entity->releasePyIdentity(); + return cached; + } + } + + // Release identity ref (no cached object to return) + entity->releasePyIdentity(); + // Create Python object for the entity PyTypeObject* entity_type = &mcrfpydef::PyUIEntityType; diff --git a/tests/regression/issue_266_subclass_identity_test.py b/tests/regression/issue_266_subclass_identity_test.py new file mode 100644 index 0000000..7170aae --- /dev/null +++ b/tests/regression/issue_266_subclass_identity_test.py @@ -0,0 +1,242 @@ +"""Regression test: Python subclass identity preservation in grid.entities. + +Tests that Entity subclasses retain their Python type and custom methods +when accessed via grid.entities iteration, indexing, and pop(). This +pattern is critical for games like Liber Noster (7DRL 2026) where +subclasses like GameEntity, ZoneExit, Combatant add custom behavior. + +Related issues: #266 (self-reference cycle), #275 (tp_dealloc) +""" +import mcrfpy +import gc +import sys + +PASS = 0 +FAIL = 0 + +def test(name, condition): + global PASS, FAIL + if condition: + PASS += 1 + print(f" PASS: {name}") + else: + FAIL += 1 + print(f" FAIL: {name}") + + +# --- Define subclass hierarchy mimicking Liber Noster --- + +class GameEntity(mcrfpy.Entity): + """Base game entity with custom methods.""" + def __init__(self, **kwargs): + super().__init__(**kwargs) + self.entity_name = kwargs.get("name", "unnamed") + self.description = "A game entity" + + def tooltip(self): + return f"{self.entity_name}: {self.description}" + + +class ZoneExit(GameEntity): + """Teleportation entity.""" + def __init__(self, target_zone="unknown", **kwargs): + super().__init__(**kwargs) + self.target_zone = target_zone + self.description = f"Exit to {target_zone}" + + def send(self, target_entity): + return f"Sending {target_entity} to {self.target_zone}" + + +class AnimatedEntity(GameEntity): + """Entity with animation state.""" + def __init__(self, **kwargs): + super().__init__(**kwargs) + self.moving = False + self.facing_dir = 0 + + def anim(self, direction): + self.facing_dir = direction + return f"Animating {self.entity_name} dir={direction}" + + +class Combatant: + """Mixin class for combat (not an Entity subclass).""" + def __init_combatant__(self, hp=10, atk=3, dfn=1): + self.hp = hp + self.atk = atk + self.dfn = dfn + self.alive = True + + def bump(self, target): + damage = max(0, self.atk - target.dfn) + target.hp -= damage + if target.hp <= 0: + target.alive = False + return {"damage": damage, "defeated": not target.alive} + + +class Enemy(AnimatedEntity, Combatant): + """Combines animation and combat.""" + def __init__(self, hp=10, atk=3, dfn=1, **kwargs): + AnimatedEntity.__init__(self, **kwargs) + self.__init_combatant__(hp=hp, atk=atk, dfn=dfn) + + +# --- Tests --- + +print("Testing Entity subclass identity preservation...") + +# Create a grid and scene +scene = mcrfpy.Scene("test_identity") +grid = mcrfpy.Grid(grid_size=(20, 20), pos=(0, 0), size=(400, 400)) +scene.children.append(grid) +mcrfpy.current_scene = scene + +# Test 1: Basic subclass creation and grid addition +zexit = ZoneExit(target_zone="dungeon", grid_pos=(5, 5)) +grid.entities.append(zexit) +test("ZoneExit added to grid", len(grid.entities) == 1) + +# Test 2: Access via index preserves type +e = grid.entities[0] +test("grid.entities[0] returns ZoneExit type", type(e).__name__ == "ZoneExit") +test("grid.entities[0] has send() method", hasattr(e, "send")) +test("grid.entities[0].send() works", e.send("player") == "Sending player to dungeon") +test("grid.entities[0].tooltip() works", "dungeon" in e.tooltip()) +del e + +# Test 3: Access via iteration preserves type +for e in grid.entities: + test("iteration returns ZoneExit type", type(e).__name__ == "ZoneExit") + test("iteration has send() method", hasattr(e, "send")) + test("iteration has target_zone attr", hasattr(e, "target_zone")) + +# Test 4: Drop the original Python reference, force GC +del zexit +gc.collect() +gc.collect() + +# This is the critical test: after GC, the subclass should survive +e = grid.entities[0] +test("after GC: type preserved", type(e).__name__ == "ZoneExit") +test("after GC: send() works", e.send("hero") == "Sending hero to dungeon") +test("after GC: target_zone preserved", e.target_zone == "dungeon") +test("after GC: tooltip() works", "dungeon" in e.tooltip()) +del e + +# Test 5: Multiple subclass types in same grid +enemy1 = Enemy(hp=20, atk=5, dfn=2, name="goblin", grid_pos=(3, 3)) +enemy2 = Enemy(hp=15, atk=4, dfn=1, name="skeleton", grid_pos=(7, 7)) +anim = AnimatedEntity(name="npc", grid_pos=(10, 10)) + +grid.entities.append(enemy1) +grid.entities.append(enemy2) +grid.entities.append(anim) + +# Drop Python refs, force GC +del enemy1, enemy2, anim +gc.collect() +gc.collect() + +test("multiple types: 4 entities in grid", len(grid.entities) == 4) + +# Check each entity's type via iteration +types_found = [] +for e in grid.entities: + types_found.append(type(e).__name__) + +test("type list correct", types_found == ["ZoneExit", "Enemy", "Enemy", "AnimatedEntity"]) + +# Test 6: isinstance checks (Liber Noster pattern: find Combatants) +combatants = [] +for e in grid.entities: + if isinstance(e, Combatant) and e.alive: + combatants.append(e) + +test("isinstance(Combatant) finds 2 enemies", len(combatants) == 2) +test("combatant has bump()", hasattr(combatants[0], "bump")) +test("combatant has hp", hasattr(combatants[0], "hp")) + +# Test 7: Combat between entities retrieved from grid +attacker = combatants[0] +defender = combatants[1] +result = attacker.bump(defender) +test("combat returns result dict", "damage" in result) +test("combat deals damage", defender.hp < 15) +del attacker, defender, combatants + +# Test 8: hasattr checks (Liber Noster pattern: tooltip, pickup) +gc.collect() +for e in grid.entities: + if hasattr(e, "tooltip"): + tip = e.tooltip() + test(f"tooltip() on {type(e).__name__}", tip is not None) + break # Just test one + +# Test 9: die() releases identity, allows GC +pre_count = len(grid.entities) +e = grid.entities[0] # ZoneExit +test("before die: is ZoneExit", type(e).__name__ == "ZoneExit") +e.die() +test("after die: entity count decreased", len(grid.entities) == pre_count - 1) +del e +gc.collect() + +# Test 10: Remaining entities still have correct types +types_after_die = [type(e).__name__ for e in grid.entities] +test("types after die()", types_after_die == ["Enemy", "Enemy", "AnimatedEntity"]) + +# Test 11: pop() preserves identity +e = grid.entities.pop(0) +test("pop() returns Enemy", type(e).__name__ == "Enemy") +test("pop() entity has combat attrs", hasattr(e, "hp")) +del e + +# Test 12: Entity created with grid= kwarg +zexit2 = ZoneExit(target_zone="tower", grid_pos=(1, 1), grid=grid) +del zexit2 +gc.collect() +gc.collect() +e = grid.entities[-1] +test("grid= kwarg: type preserved after GC", type(e).__name__ == "ZoneExit") +test("grid= kwarg: send() works", "tower" in e.send("x")) +del e + +# Test 13: remove() releases identity +enemy_ref = None +for e in grid.entities: + if isinstance(e, Enemy): + enemy_ref = e + break +test("found Enemy for remove test", enemy_ref is not None) +if enemy_ref: + grid.entities.remove(enemy_ref) + del enemy_ref + gc.collect() + +# Test 14: Stress test - create many entities, drop refs, verify all survive +for i in range(20): + ent = ZoneExit(target_zone=f"zone_{i}", grid_pos=(i % 20, i // 20)) + grid.entities.append(ent) + # Don't hold any Python reference +del ent +gc.collect() +gc.collect() + +zone_exits_found = 0 +for e in grid.entities: + if isinstance(e, ZoneExit) and hasattr(e, "target_zone"): + zone_exits_found += 1 + +test("stress: all 21 ZoneExits preserved", zone_exits_found == 21) + +# Summary +print(f"\n{'='*50}") +total = PASS + FAIL +if FAIL == 0: + print(f"PASS: all {PASS} subclass identity tests passed") + sys.exit(0) +else: + print(f"FAIL: {FAIL}/{total} tests failed") + sys.exit(1)