Preserve Python subclass identity for entities in grids (reopens #266)

The Phase 3 fix for #266 removed UIEntity::self which prevented
tp_dealloc from ever running. However, this also allowed Python
subclass wrappers (GameEntity, ZoneExit, etc.) to be GC'd while
the C++ entity lived on in a grid. Later access via grid.entities
returned a base Entity wrapper, losing all subclass methods.

Fix: Add UIEntity::pyobject field that holds a strong reference to
the Python wrapper. Set in init(), cleared when the entity leaves
a grid (die(), set_grid(None), collection removal). This keeps
subclass identity alive while in a grid, but allows proper GC when
the entity is removed. Added releasePyIdentity() helper called at
all grid exit points.

Regression test exercises Liber Noster patterns: subclass hierarchy,
isinstance() checks, combat mixins, tooltip/send methods, GC
survival, die(), pop(), remove(), and stress test with 20 entities.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

src/UIEntity.cpp

@@ -26,6 +26,7 @@ UIEntity::UIEntity()
 }
 
 UIEntity::~UIEntity() {
+    releasePyIdentity();
     if (serial_number != 0) {
         PythonObjectCache::getInstance().remove(serial_number);
     }
@@ -240,6 +241,13 @@ int UIEntity::init(PyUIEntityObject* self, PyObject* args, PyObject* kwds) {
         }
     }
 
+    // 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) {
         self->data->sprite = UISprite(texture_ptr, sprite_index, sf::Vector2f(0,0), 1.0);
@@ -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;

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<UIGrid> grid;
     std::vector<UIGridPointState> gridstate;
     UISprite sprite;
@@ -71,6 +72,16 @@ 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);

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;
 

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)