From 2d2c333cd7b24c50ff17c85fabdb91955578e997 Mon Sep 17 00:00:00 2001 From: John McCardle Date: Thu, 11 Jun 2026 00:51:22 -0400 Subject: [PATCH] Refactor UIEntity::grid to shared_ptr; add entity.texture; closes #313 UIEntity now depends on the grid DATA layer only: - GridData gains cell_width_px/cell_height_px (mirrored from the grid texture in UIGrid's 5-arg ctor; texture is write-once) so entity tile<->pixel math no longer reaches into rendering (getTexture()). - GridData gains markDirty()/markCompositeDirty(): set the flag on the UIGrid subobject AND notify owning_view, covering both render paths. UIGrid disambiguates via 'using UIDrawable::markDirty' so all pre-existing UIGrid-receiver calls resolve exactly as before. - The three Python wrappers that still need the full UIGrid (GridPoint from entity.at(), the _GridData fallback in get_grid, find_path's temp wrapper) reconstruct it via a single aliasing-downcast helper (grid_as_uigrid) that documents the never-independently-allocated GridData invariant; init/set_grid simplify (share grid_data directly). Removing the casts is deferred to #252. entity.texture (new, frozen surface +1): thin get/set over the entity's own UISprite. Entities render with their OWN texture (default_texture fallback at construction); the grid's texture only determines cell size. Setter preserves sprite_index; rejects non-Texture (TypeError), null-data Texture wrappers (ValueError), and deletion. Adversarial review fixes folded in: - set_texture/get_texture guard uninitialized Entity wrappers (RuntimeError), isinstance errors, and null-data Textures. - PyUIGridViewType tp_dealloc no longer unconditionally severs GridData::owning_view: gated on last-owner (#251 use_count pattern) plus owning-view identity. Previously ANY Grid wrapper GC while the view lived (e.g. scene.children.append(mcrfpy.Grid(...))) silently broke entity.grid -> Grid identity and data-layer dirty notification. Tests: tests/regression/issue_313_entity_grid_data_test.py (texture semantics, grid-cell-size invariance, entity.grid identity, #251 gate survival, GridPoint outliving teardown, review-fix guards, owning_view survival) + tests/unit/entity_texture_test.py. API snapshot golden re-baselined: exactly +1 surface line (Entity.texture) + writability probe flip. Docs/stubs regenerated. Native + Emscripten builds verified. Known edges recorded in docs/api-audit-2026-04.md: texture read-back is a fresh wrapper each get (no Texture __eq__); sprite_index not re-validated against a new atlas. Multi-view markDirty broadcast and pure-GridData wrappers remain deferred to #252. Addresses #314. Co-Authored-By: Claude Fable 5 --- docs/API_REFERENCE_DYNAMIC.md | 16 +- docs/api-audit-2026-04.md | 6 +- docs/api_reference_dynamic.html | 16 +- docs/mcrfpy.3 | 35 ++-- src/GridData.cpp | 19 ++ src/GridData.h | 19 ++ src/UIEntity.cpp | 112 +++++++++--- src/UIEntity.h | 12 +- src/UIGrid.cpp | 6 + src/UIGrid.h | 7 + src/UIGridView.h | 12 +- stubs/mcrfpy.pyi | 13 +- .../issue_313_entity_grid_data_test.py | 164 ++++++++++++++++++ tests/snapshots/api_surface.golden.txt | 3 +- tests/unit/entity_texture_test.py | 48 +++++ 15 files changed, 428 insertions(+), 60 deletions(-) create mode 100644 tests/regression/issue_313_entity_grid_data_test.py create mode 100644 tests/unit/entity_texture_test.py diff --git a/docs/API_REFERENCE_DYNAMIC.md b/docs/API_REFERENCE_DYNAMIC.md index 26b7299..ebd335b 100644 --- a/docs/API_REFERENCE_DYNAMIC.md +++ b/docs/API_REFERENCE_DYNAMIC.md @@ -1,6 +1,6 @@ # McRogueFace API Reference -*Generated on 2026-04-18 13:35:02* +*Generated on 2026-06-10 20:23:39* *This documentation was dynamically generated from the compiled module.* @@ -1775,6 +1775,7 @@ Attributes: grid_x, grid_y (int): Integer tile coordinate components draw_pos (Vector): Fractional tile position for smooth animation perspective_map (DiscreteMap | None): 3-state per-entity FOV memory + texture (Texture): Texture atlas used by the entity's sprite sprite_index (int): Current sprite index visible (bool): Visibility state opacity (float): Opacity value @@ -1785,15 +1786,15 @@ Attributes: **Properties:** - `behavior_type` *(read-only)*: Current behavior type (int, read-only). Use set_behavior() to change. -- `cell_pos`: Integer logical cell position (Vector). Decoupled from draw_pos. Determines which cell this entity logically occupies for collision, pathfinding, etc. -- `cell_x`: Integer X cell coordinate. -- `cell_y`: Integer Y cell coordinate. +- `cell_pos`: Integer logical cell position (Vector). Alias for grid_pos (the canonical name). +- `cell_x`: Integer X cell coordinate. Alias for grid_x. +- `cell_y`: Integer Y cell coordinate. Alias for grid_y. - `default_behavior`: Default behavior type (int, maps to Behavior enum). Entity reverts to this after DONE trigger. Default: 0 (IDLE). - `draw_pos`: Fractional tile position for rendering (Vector). Use for smooth animation between grid cells. - `grid`: Grid this entity belongs to. Get: Returns the Grid or None. Set: Assign a Grid to move entity, or None to remove from grid. -- `grid_pos`: Grid position as integer cell coordinates (Vector). Alias for cell_pos. -- `grid_x`: Grid X position as integer cell coordinate. Alias for cell_x. -- `grid_y`: Grid Y position as integer cell coordinate. Alias for cell_y. +- `grid_pos`: Integer logical cell position (Vector). Canonical cell-position property; matches the 'grid_pos' constructor argument. Decoupled from draw_pos. Determines which cell this entity logically occupies for collision, pathfinding, etc. +- `grid_x`: Integer X cell coordinate. Canonical; matches grid_pos. +- `grid_y`: Integer Y cell coordinate. Canonical; matches grid_pos. - `labels`: Set of string labels for collision/targeting (frozenset). Assign any iterable of strings to replace all labels. - `move_speed`: Animation duration for behavior movement in seconds (float). 0 = instant. Default: 0.15. - `name`: Name for finding elements @@ -1809,6 +1810,7 @@ Attributes: - `sprite_offset_y`: Y component of sprite pixel offset. - `step`: Step callback for grid.step() turn management. Called with (trigger, data) when behavior triggers fire. Set to None to clear. - `target_label`: Label to search for with TARGET trigger (str or None). Default: None. +- `texture`: Sprite texture atlas (Texture). Defaults to mcrfpy.default_texture when the entity is constructed without one. Setting preserves sprite_index (the index is not re-validated against the new atlas). The grid's texture only determines cell size; entities draw with their own. - `tile_height`: Entity height in tiles (int). Must be >= 1. Default 1. - `tile_size`: Entity size in tiles as (width, height) Vector. Default (1, 1). - `tile_width`: Entity width in tiles (int). Must be >= 1. Default 1. diff --git a/docs/api-audit-2026-04.md b/docs/api-audit-2026-04.md index 3f7ac29..7271aca 100644 --- a/docs/api-audit-2026-04.md +++ b/docs/api-audit-2026-04.md @@ -971,7 +971,11 @@ F8 (`Font` methods) = Future, explicitly NOT 1.0 blockers. - **`entity.texture` (new in #313):** additive read/write property; getter returns the entity's real texture, `None` only in the degenerate (default_texture-null) case — never re-derefs a null default_texture. Added to the frozen contract + stubs + docs when #313 lands (golden gains exactly - one line). + one line). Known edges, frozen as-is (2026-06-11 adversarial review): the getter mints a NEW + Texture wrapper per access and Texture has no `__eq__`, so `e.texture == e.texture` is False — + compare `.source`/sprite dims instead (same behavior as the pre-existing `Sprite.texture`); + setting does not re-validate `sprite_index` against the new atlas; setter rejects non-Texture + (TypeError), null-data Texture wrappers (ValueError, mirrors `Sprite.texture`), and deletion. **1.0 freeze scope — class classification** (the snapshot segregates FROZEN vs EXPERIMENTAL): diff --git a/docs/api_reference_dynamic.html b/docs/api_reference_dynamic.html index f33bb90..8705ac1 100644 --- a/docs/api_reference_dynamic.html +++ b/docs/api_reference_dynamic.html @@ -108,7 +108,7 @@

McRogueFace API Reference

-

Generated on 2026-04-18 13:35:02

+

Generated on 2026-06-10 20:23:39

This documentation was dynamically generated from the compiled module.

@@ -1935,6 +1935,7 @@ Attributes: grid_x, grid_y (int): Integer tile coordinate components draw_pos (Vector): Fractional tile position for smooth animation perspective_map (DiscreteMap | None): 3-state per-entity FOV memory + texture (Texture): Texture atlas used by the entity's sprite sprite_index (int): Current sprite index visible (bool): Visibility state opacity (float): Opacity value @@ -1945,15 +1946,15 @@ Attributes:

Properties:

  • behavior_type (read-only): Current behavior type (int, read-only). Use set_behavior() to change.
    • -
  • cell_pos: Integer logical cell position (Vector). Decoupled from draw_pos. Determines which cell this entity logically occupies for collision, pathfinding, etc.
    • -
  • cell_x: Integer X cell coordinate.
    • -
  • cell_y: Integer Y cell coordinate.
    • +
  • cell_pos: Integer logical cell position (Vector). Alias for grid_pos (the canonical name).
    • +
  • cell_x: Integer X cell coordinate. Alias for grid_x.
    • +
  • cell_y: Integer Y cell coordinate. Alias for grid_y.
  • default_behavior: Default behavior type (int, maps to Behavior enum). Entity reverts to this after DONE trigger. Default: 0 (IDLE).
  • draw_pos: Fractional tile position for rendering (Vector). Use for smooth animation between grid cells.
  • grid: Grid this entity belongs to. Get: Returns the Grid or None. Set: Assign a Grid to move entity, or None to remove from grid.
    • -
  • grid_pos: Grid position as integer cell coordinates (Vector). Alias for cell_pos.
    • -
  • grid_x: Grid X position as integer cell coordinate. Alias for cell_x.
    • -
  • grid_y: Grid Y position as integer cell coordinate. Alias for cell_y.
    • +
  • grid_pos: Integer logical cell position (Vector). Canonical cell-position property; matches the 'grid_pos' constructor argument. Decoupled from draw_pos. Determines which cell this entity logically occupies for collision, pathfinding, etc.
    • +
  • grid_x: Integer X cell coordinate. Canonical; matches grid_pos.
    • +
  • grid_y: Integer Y cell coordinate. Canonical; matches grid_pos.
  • labels: Set of string labels for collision/targeting (frozenset). Assign any iterable of strings to replace all labels.
  • move_speed: Animation duration for behavior movement in seconds (float). 0 = instant. Default: 0.15.
  • name: Name for finding elements
    • @@ -1969,6 +1970,7 @@ Attributes:
  • sprite_offset_y: Y component of sprite pixel offset.
  • step: Step callback for grid.step() turn management. Called with (trigger, data) when behavior triggers fire. Set to None to clear.
  • target_label: Label to search for with TARGET trigger (str or None). Default: None.
    • +
  • texture: Sprite texture atlas (Texture). Defaults to mcrfpy.default_texture when the entity is constructed without one. Setting preserves sprite_index (the index is not re-validated against the new atlas). The grid's texture only determines cell size; entities draw with their own.
  • tile_height: Entity height in tiles (int). Must be >= 1. Default 1.
  • tile_size: Entity size in tiles as (width, height) Vector. Default (1, 1).
  • tile_width: Entity width in tiles (int). Must be >= 1. Default 1.
    • diff --git a/docs/mcrfpy.3 b/docs/mcrfpy.3 index 0008515..c965a3c 100644 --- a/docs/mcrfpy.3 +++ b/docs/mcrfpy.3 @@ -14,11 +14,11 @@ . ftr VB CB . ftr VBI CBI .\} -.TH "MCRFPY" "3" "2026-04-18" "McRogueFace 0.2.7-prerelease-7drl2026-93-g0f7254e" "" +.TH "MCRFPY" "3" "2026-06-10" "McRogueFace 0.2.7-prerelease-7drl2026-99-g16adc92" "" .hy .SH McRogueFace API Reference .PP -\f[I]Generated on 2026-04-18 13:35:02\f[R] +\f[I]Generated on 2026-06-10 20:23:39\f[R] .PP \f[I]This documentation was dynamically generated from the compiled module.\f[R] @@ -1894,6 +1894,7 @@ attachment) grid_pos (Vector): Integer tile coordinates (logical game position) grid_x, grid_y (int): Integer tile coordinate components draw_pos (Vector): Fractional tile position for smooth animation perspective_map (DiscreteMap | None): 3-state per-entity FOV memory +texture (Texture): Texture atlas used by the entity\[cq]s sprite sprite_index (int): Current sprite index visible (bool): Visibility state opacity (float): Opacity value name (str): Element name sprite_offset (Vector): Pixel offset for oversized sprites @@ -1904,11 +1905,11 @@ sprite_offset_x (float): X component of sprite offset sprite_offset_y Current behavior type (int, read-only). Use set_behavior() to change. - \f[V]cell_pos\f[R]: Integer logical cell position (Vector). -Decoupled from draw_pos. -Determines which cell this entity logically occupies for collision, -pathfinding, etc. +Alias for grid_pos (the canonical name). - \f[V]cell_x\f[R]: Integer X cell coordinate. +Alias for grid_x. - \f[V]cell_y\f[R]: Integer Y cell coordinate. +Alias for grid_y. - \f[V]default_behavior\f[R]: Default behavior type (int, maps to Behavior enum). Entity reverts to this after DONE trigger. @@ -1918,13 +1919,16 @@ Use for smooth animation between grid cells. - \f[V]grid\f[R]: Grid this entity belongs to. Get: Returns the Grid or None. Set: Assign a Grid to move entity, or None to remove from grid. -- \f[V]grid_pos\f[R]: Grid position as integer cell coordinates -(Vector). -Alias for cell_pos. -- \f[V]grid_x\f[R]: Grid X position as integer cell coordinate. -Alias for cell_x. -- \f[V]grid_y\f[R]: Grid Y position as integer cell coordinate. -Alias for cell_y. +- \f[V]grid_pos\f[R]: Integer logical cell position (Vector). +Canonical cell-position property; matches the `grid_pos' constructor +argument. +Decoupled from draw_pos. +Determines which cell this entity logically occupies for collision, +pathfinding, etc. +- \f[V]grid_x\f[R]: Integer X cell coordinate. +Canonical; matches grid_pos. +- \f[V]grid_y\f[R]: Integer Y cell coordinate. +Canonical; matches grid_pos. - \f[V]labels\f[R]: Set of string labels for collision/targeting (frozenset). Assign any iterable of strings to replace all labels. @@ -1970,6 +1974,13 @@ Set to None to clear. - \f[V]target_label\f[R]: Label to search for with TARGET trigger (str or None). Default: None. +- \f[V]texture\f[R]: Sprite texture atlas (Texture). +Defaults to mcrfpy.default_texture when the entity is constructed +without one. +Setting preserves sprite_index (the index is not re-validated against +the new atlas). +The grid\[cq]s texture only determines cell size; entities draw with +their own. - \f[V]tile_height\f[R]: Entity height in tiles (int). Must be >= 1. Default 1. diff --git a/src/GridData.cpp b/src/GridData.cpp index 8829723..c385cad 100644 --- a/src/GridData.cpp +++ b/src/GridData.cpp @@ -2,8 +2,27 @@ #include "GridData.h" #include "UIEntity.h" #include "PyTexture.h" +#include "UIGrid.h" // #313 - markDirty forwards to the UIGrid subobject +#include "UIGridView.h" // #313 - and notifies owning_view #include +// #313 - Render invalidation from the data layer (see GridData.h). +// GridData is never independently heap-allocated (always a UIGrid base +// subobject), so the downcast is valid; remove once #252 allows pure GridData. +void GridData::markDirty() { + static_cast(this)->UIDrawable::markDirty(); + if (auto view = owning_view.lock()) { + view->markDirty(); + } +} + +void GridData::markCompositeDirty() { + static_cast(this)->UIDrawable::markCompositeDirty(); + if (auto view = owning_view.lock()) { + view->markCompositeDirty(); + } +} + GridData::GridData() { entities = std::make_shared>>(); diff --git a/src/GridData.h b/src/GridData.h index 2f5d15a..878e643 100644 --- a/src/GridData.h +++ b/src/GridData.h @@ -39,6 +39,14 @@ public: // ========================================================================= int grid_w = 0, grid_h = 0; + // #313 - Cell pixel dimensions, mirrored from the owning UIGrid's texture + // at construction (UIGrid::ptex is write-once, ctor only). Lets the data + // layer do tile<->pixel math without reaching into rendering state. + int cell_width_px = 16; + int cell_height_px = 16; + int cell_width() const { return cell_width_px; } + int cell_height() const { return cell_height_px; } + // #123 - Chunk-based storage for large grid support std::unique_ptr chunk_manager; // Legacy flat storage (kept for small grids or compatibility) @@ -136,6 +144,17 @@ public: // ========================================================================= std::weak_ptr owning_view; + // #313 - Render invalidation from the data layer. Entities hold + // shared_ptr but still need to invalidate rendering when their + // visual state changes. These set the dirty flags on the UIGrid subobject + // (GridData is never independently heap-allocated -- always a UIGrid base) + // AND notify owning_view, covering both render paths (a bare _GridData + // rendered directly, and the normal GridView). Within UIGrid itself the + // UIDrawable versions win via using-declarations (see UIGrid.h). + // Multi-view broadcast (secondary views) is deferred to #252. + void markDirty(); + void markCompositeDirty(); + protected: // Initialize grid storage (flat or chunked) and TCOD map void initStorage(int gx, int gy, GridData* parent_ref); diff --git a/src/UIEntity.cpp b/src/UIEntity.cpp index 8f71d0b..d0480d1 100644 --- a/src/UIEntity.cpp +++ b/src/UIEntity.cpp @@ -20,8 +20,21 @@ #include "PyUniformCollection.h" // #106: Uniform collection support // UIDrawable methods now in UIBase.h #include "UIEntityPyMethods.h" +#include - +// #313: UIEntity::grid holds the GridData base, but some Python wrappers +// (PyUIGridObject, PyUIGridPointObject) and pathfinding helpers still take the +// full UIGrid. GridData is never independently heap-allocated -- it is always +// a UIGrid base subobject (see GridData.h) -- so this aliasing downcast is +// valid and shares the original control block (never mints a new one, which +// would double-free and break the #251 use_count dealloc gate). +// TODO(#252): remove once those wrappers accept pure GridData. +static std::shared_ptr grid_as_uigrid(const std::shared_ptr& grid) +{ + if (!grid) return nullptr; + assert(dynamic_cast(grid.get()) != nullptr); + return std::shared_ptr(grid, static_cast(grid.get())); +} UIEntity::UIEntity() : grid(nullptr), position(0.0f, 0.0f), sprite_offset(0.0f, 0.0f) @@ -133,7 +146,7 @@ PyObject* UIEntity::at(PyUIEntityObject* self, PyObject* args, PyObject* kwds) { auto type = &mcrfpydef::PyUIGridPointType; auto obj = (PyUIGridPointObject*)type->tp_alloc(type, 0); if (!obj) return NULL; - obj->grid = entity->grid; + obj->grid = grid_as_uigrid(entity->grid); // #313: wrapper still holds UIGrid obj->x = x; obj->y = y; return (PyObject*)obj; @@ -310,14 +323,11 @@ int UIEntity::init(PyUIEntityObject* self, PyObject* args, PyObject* kwds) { // Handle grid attachment if (grid_obj) { - std::shared_ptr grid_ptr; + std::shared_ptr grid_ptr; if (PyObject_IsInstance(grid_obj, (PyObject*)&mcrfpydef::PyUIGridViewType)) { - // #252: GridView (unified Grid) - extract internal UIGrid + // #252: GridView (unified Grid) - share its grid data directly (#313) PyUIGridViewObject* pyview = (PyUIGridViewObject*)grid_obj; - if (pyview->data->grid_data) { - grid_ptr = std::shared_ptr( - pyview->data->grid_data, static_cast(pyview->data->grid_data.get())); - } + grid_ptr = pyview->data->grid_data; } else { // Internal _GridData type PyUIGridObject* pygrid = (PyUIGridObject*)grid_obj; @@ -495,6 +505,50 @@ int UIEntity::set_spritenumber(PyUIEntityObject* self, PyObject* value, void* cl return 0; } +// #313 - texture property: thin wrapper over the entity's own UISprite. +// Entities render from their own texture (falling back to default_texture at +// construction); the grid's texture is only used for cell-size math. +PyObject* UIEntity::get_texture(PyUIEntityObject* self, void* closure) { + if (!self->data) { + // Entity.__new__ without __init__ leaves data null + PyErr_SetString(PyExc_RuntimeError, "Invalid Entity object"); + return NULL; + } + auto tex = self->data->sprite.getTexture(); + if (!tex) { + // Only reachable if default_texture was null at construction + Py_RETURN_NONE; + } + return tex->pyObject(); +} + +int UIEntity::set_texture(PyUIEntityObject* self, PyObject* value, void* closure) { + if (!self->data) { + PyErr_SetString(PyExc_RuntimeError, "Invalid Entity object"); + return -1; + } + if (!value) { + PyErr_SetString(PyExc_TypeError, "Cannot delete texture attribute"); + return -1; + } + int is_texture = PyObject_IsInstance(value, (PyObject*)&mcrfpydef::PyTextureType); + if (is_texture == -1) return -1; // isinstance itself raised + if (!is_texture) { + PyErr_SetString(PyExc_TypeError, "texture must be a mcrfpy.Texture instance"); + return -1; + } + auto pytexture = (PyTextureObject*)value; + if (!pytexture->data) { + // Texture.__new__ without __init__ leaves data null (same guard as UISprite) + PyErr_SetString(PyExc_ValueError, "Invalid texture object"); + return -1; + } + // Preserves sprite_index (not re-validated against the new atlas) + self->data->sprite.setTexture(pytexture->data); + if (self->data->grid) self->data->grid->markDirty(); + return 0; +} + PyObject* UIEntity::get_float_member(PyUIEntityObject* self, void* closure) { auto member_ptr = reinterpret_cast(closure); @@ -550,14 +604,15 @@ int UIEntity::set_float_member(PyUIEntityObject* self, PyObject* value, void* cl // #176 - Helper to get cell dimensions from grid static void get_cell_dimensions(UIEntity* entity, float& cell_width, float& cell_height) { - // Default cell dimensions when no texture + // Default cell dimensions when no grid is attached constexpr float DEFAULT_CELL_WIDTH = 16.0f; constexpr float DEFAULT_CELL_HEIGHT = 16.0f; if (entity->grid) { - auto ptex = entity->grid->getTexture(); - cell_width = ptex ? static_cast(ptex->sprite_width) : DEFAULT_CELL_WIDTH; - cell_height = ptex ? static_cast(ptex->sprite_height) : DEFAULT_CELL_HEIGHT; + // #313: cell size lives on the data layer (mirrored from the grid's + // texture at construction) -- entities no longer reach into rendering. + cell_width = static_cast(entity->grid->cell_width()); + cell_height = static_cast(entity->grid->cell_height()); } else { cell_width = DEFAULT_CELL_WIDTH; cell_height = DEFAULT_CELL_HEIGHT; @@ -737,8 +792,11 @@ PyObject* UIEntity::get_grid(PyUIEntityObject* self, void* closure) } // Fallback: return internal _GridData wrapper (no owning view) - if (grid->serial_number != 0) { - PyObject* cached = PythonObjectCache::getInstance().lookup(grid->serial_number); + // #313: serial_number lives on the UIDrawable side; recover the full + // UIGrid via the aliasing helper (same control block, no new ownership). + auto uigrid = grid_as_uigrid(grid); + if (uigrid->serial_number != 0) { + PyObject* cached = PythonObjectCache::getInstance().lookup(uigrid->serial_number); if (cached) { return cached; } @@ -748,15 +806,15 @@ PyObject* UIEntity::get_grid(PyUIEntityObject* self, void* closure) auto pyGrid = (PyUIGridObject*)grid_type->tp_alloc(grid_type, 0); if (pyGrid) { - pyGrid->data = grid; + pyGrid->data = uigrid; pyGrid->weakreflist = NULL; - if (grid->serial_number == 0) { - grid->serial_number = PythonObjectCache::getInstance().assignSerial(); + if (uigrid->serial_number == 0) { + uigrid->serial_number = PythonObjectCache::getInstance().assignSerial(); } PyObject* weakref = PyWeakref_NewRef((PyObject*)pyGrid, NULL); if (weakref) { - PythonObjectCache::getInstance().registerObject(grid->serial_number, weakref); + PythonObjectCache::getInstance().registerObject(uigrid->serial_number, weakref); Py_DECREF(weakref); } } @@ -793,12 +851,11 @@ int UIEntity::set_grid(PyUIEntityObject* self, PyObject* value, void* closure) } // #252: Accept both internal _GridData and GridView (unified Grid) - std::shared_ptr new_grid; + std::shared_ptr new_grid; if (PyObject_IsInstance(value, (PyObject*)&mcrfpydef::PyUIGridViewType)) { PyUIGridViewObject* pyview = (PyUIGridViewObject*)value; if (pyview->data->grid_data) { - new_grid = std::shared_ptr( - pyview->data->grid_data, static_cast(pyview->data->grid_data.get())); + new_grid = pyview->data->grid_data; // #313: share data directly } else { PyErr_SetString(PyExc_ValueError, "Grid has no data"); return -1; @@ -1140,9 +1197,11 @@ PyObject* UIEntity::find_path(PyUIEntityObject* self, PyObject* args, PyObject* auto grid = self->data->grid; // Extract target position + // #313: ExtractPosition still takes UIGrid*; the downcast is valid because + // GridData is always a UIGrid base subobject (see grid_as_uigrid). int target_x, target_y; if (!UIGridPathfinding::ExtractPosition(target_obj, &target_x, &target_y, - grid.get(), "target")) { + static_cast(grid.get()), "target")) { return NULL; } @@ -1158,10 +1217,11 @@ PyObject* UIEntity::find_path(PyUIEntityObject* self, PyObject* args, PyObject* // Build args to delegate to Grid.find_path // Create a temporary PyUIGridObject wrapper for the grid (internal _GridData type) + // #313: wrapper holds shared_ptr; alias-cast from the data ptr. auto* grid_type = &mcrfpydef::PyUIGridType; auto pyGrid = (PyUIGridObject*)grid_type->tp_alloc(grid_type, 0); if (!pyGrid) return NULL; - new (&pyGrid->data) std::shared_ptr(grid); + new (&pyGrid->data) std::shared_ptr(grid_as_uigrid(grid)); // Build keyword args for Grid.find_path PyObject* start_tuple = Py_BuildValue("(ii)", start_x, start_y); @@ -1817,6 +1877,12 @@ PyGetSetDef UIEntity::getsetters[] = { "Get: Returns the Grid or None. " "Set: Assign a Grid to move entity, or None to remove from grid.", NULL}, {"sprite_index", (getter)UIEntity::get_spritenumber, (setter)UIEntity::set_spritenumber, "Sprite index on the texture on the display", NULL}, + // #313 - entities render from their OWN texture, not the grid's + {"texture", (getter)UIEntity::get_texture, (setter)UIEntity::set_texture, + "Sprite texture atlas (Texture). Defaults to mcrfpy.default_texture when " + "the entity is constructed without one. Setting preserves sprite_index " + "(the index is not re-validated against the new atlas). The grid's " + "texture only determines cell size; entities draw with their own.", NULL}, {"visible", (getter)UIEntity_get_visible, (setter)UIEntity_set_visible, "Visibility flag", NULL}, {"opacity", (getter)UIEntity_get_opacity, (setter)UIEntity_set_opacity, "Opacity (0.0 = transparent, 1.0 = opaque)", NULL}, {"name", (getter)UIEntity_get_name, (setter)UIEntity_set_name, "Name for finding elements", NULL}, diff --git a/src/UIEntity.h b/src/UIEntity.h index 4daccc4..003cd89 100644 --- a/src/UIEntity.h +++ b/src/UIEntity.h @@ -23,6 +23,7 @@ #include class UIGrid; +class GridData; // UIEntity /* @@ -65,7 +66,12 @@ class UIEntity : public std::enable_shared_from_this 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; + // #313: Entities depend on the grid DATA layer only (cells, entities, + // FOV, pathfinding, cell size). This is always an aliasing shared_ptr + // sharing a UIGrid's control block (GridData is never independently + // heap-allocated); Python wrappers that still need the full UIGrid use + // grid_as_uigrid() in UIEntity.cpp. + std::shared_ptr grid; // Per-entity perspective memory (#294): 3-state DiscreteMap -- // 0 = unknown, 1 = discovered, 2 = visible. Lazily allocated on first // access to entity.perspective_map (when a grid is set) and whenever @@ -159,6 +165,9 @@ public: static int set_perspective_map(PyUIEntityObject* self, PyObject* value, void* closure); static PyObject* get_spritenumber(PyUIEntityObject* self, void* closure); static int set_spritenumber(PyUIEntityObject* self, PyObject* value, void* closure); + // #313 - texture property (thin wrapper over the entity's own UISprite) + static PyObject* get_texture(PyUIEntityObject* self, void* closure); + static int set_texture(PyUIEntityObject* self, PyObject* value, void* closure); static PyObject* get_float_member(PyUIEntityObject* self, void* closure); static int set_float_member(PyUIEntityObject* self, PyObject* value, void* closure); @@ -265,6 +274,7 @@ namespace mcrfpydef { " grid_x, grid_y (int): Integer tile coordinate components\n" " draw_pos (Vector): Fractional tile position for smooth animation\n" " perspective_map (DiscreteMap | None): 3-state per-entity FOV memory\n" + " texture (Texture): Texture atlas used by the entity's sprite\n" " sprite_index (int): Current sprite index\n" " visible (bool): Visibility state\n" " opacity (float): Opacity value\n" diff --git a/src/UIGrid.cpp b/src/UIGrid.cpp index 738e4a2..c7e0de2 100644 --- a/src/UIGrid.cpp +++ b/src/UIGrid.cpp @@ -58,6 +58,12 @@ UIGrid::UIGrid(int gx, int gy, std::shared_ptr _ptex, sf::Vector2f _x int cell_width = _ptex ? _ptex->sprite_width : DEFAULT_CELL_WIDTH; int cell_height = _ptex ? _ptex->sprite_height : DEFAULT_CELL_HEIGHT; + // #313: mirror into the GridData base so the data layer (and entities + // holding shared_ptr) can do tile<->pixel math. ptex is + // write-once (ctor only), so this never goes stale. + cell_width_px = cell_width; + cell_height_px = cell_height; + center_x = (gx/2) * cell_width; center_y = (gy/2) * cell_height; diff --git a/src/UIGrid.h b/src/UIGrid.h index 42a9fdc..b2d2d00 100644 --- a/src/UIGrid.h +++ b/src/UIGrid.h @@ -46,6 +46,13 @@ public: UIGrid(); UIGrid(int, int, std::shared_ptr, sf::Vector2f, sf::Vector2f); ~UIGrid(); + + // #313: GridData also declares markDirty/markCompositeDirty (data-layer + // invalidation for entities holding shared_ptr). Within UIGrid, + // keep resolving unqualified calls to the UIDrawable flag-setters -- + // identical to pre-#313 behavior. + using UIDrawable::markDirty; + using UIDrawable::markCompositeDirty; void update(); void render(sf::Vector2f, sf::RenderTarget&) override final; PyObjectsEnum derived_type() override final; diff --git a/src/UIGridView.h b/src/UIGridView.h index ee46af7..726b7c1 100644 --- a/src/UIGridView.h +++ b/src/UIGridView.h @@ -138,8 +138,16 @@ namespace mcrfpydef { if (obj->weakreflist != NULL) { PyObject_ClearWeakRefs(self); } - // Clear owning_view back-reference before releasing grid_data - if (obj->data && obj->data->grid_data) { + // Clear owning_view back-reference before releasing grid_data -- + // but only when this wrapper is the LAST owner of the view (#251 + // pattern, mirrors PyUIGridType) AND the dying view is actually + // the one owning_view points at. The previous ungated reset + // severed the back-reference whenever ANY Python wrapper was + // GC'd while the C++ view lived on (e.g. held by scene.children), + // breaking entity.grid -> Grid identity and the #313 data-layer + // dirty notifications. + if (obj->data && obj->data->grid_data && obj->data.use_count() <= 1 && + obj->data->grid_data->owning_view.lock() == obj->data) { obj->data->grid_data->owning_view.reset(); } obj->data.reset(); diff --git a/stubs/mcrfpy.pyi b/stubs/mcrfpy.pyi index b635983..180645b 100644 --- a/stubs/mcrfpy.pyi +++ b/stubs/mcrfpy.pyi @@ -653,15 +653,15 @@ class Entity: """A game entity that exists on a grid with sprite rendering.""" def __init__(self, grid_pos=None, texture=None, sprite_index=0, **kwargs) -> None: ... behavior_type: int # Current behavior type (int, read-only). Use set_behavior() to change. - cell_pos: Vector # Integer logical cell position (Vector). Decoupled from draw_pos. Determines which cell this entity logically occupies for collision, pathfinding, etc. - cell_x: Any # Integer X cell coordinate. - cell_y: Any # Integer Y cell coordinate. + cell_pos: Vector # Integer logical cell position (Vector). Alias for grid_pos (the canonical name). + cell_x: Any # Integer X cell coordinate. Alias for grid_x. + cell_y: Any # Integer Y cell coordinate. Alias for grid_y. default_behavior: int # Default behavior type (int, maps to Behavior enum). Entity reverts to this after DONE trigger. Default: 0 (IDLE). draw_pos: Vector # Fractional tile position for rendering (Vector). Use for smooth animation between grid cells. grid: Any # Grid this entity belongs to. Get: Returns the Grid or None. Set: Assign a Grid to move entity, or None to remove from grid. - grid_pos: Vector # Grid position as integer cell coordinates (Vector). Alias for cell_pos. - grid_x: Any # Grid X position as integer cell coordinate. Alias for cell_x. - grid_y: Any # Grid Y position as integer cell coordinate. Alias for cell_y. + grid_pos: Vector # Integer logical cell position (Vector). Canonical cell-position property; matches the 'grid_pos' constructor argument. Decoupled from draw_pos. Determines wh... + grid_x: Any # Integer X cell coordinate. Canonical; matches grid_pos. + grid_y: Any # Integer Y cell coordinate. Canonical; matches grid_pos. labels: frozenset # Set of string labels for collision/targeting (frozenset). Assign any iterable of strings to replace all labels. move_speed: float # Animation duration for behavior movement in seconds (float). 0 = instant. Default: 0.15. name: Any # Name for finding elements @@ -677,6 +677,7 @@ class Entity: sprite_offset_y: Any # Y component of sprite pixel offset. step: Any # Step callback for grid.step() turn management. Called with (trigger, data) when behavior triggers fire. Set to None to clear. target_label: Any # Label to search for with TARGET trigger (str or None). Default: None. + texture: Texture # Sprite texture atlas (Texture). Defaults to mcrfpy.default_texture when the entity is constructed without one. Setting preserves sprite_index (the index is n... tile_height: int # Entity height in tiles (int). Must be >= 1. Default 1. tile_size: Any # Entity size in tiles as (width, height) Vector. Default (1, 1). tile_width: int # Entity width in tiles (int). Must be >= 1. Default 1. diff --git a/tests/regression/issue_313_entity_grid_data_test.py b/tests/regression/issue_313_entity_grid_data_test.py new file mode 100644 index 0000000..b270dc9 --- /dev/null +++ b/tests/regression/issue_313_entity_grid_data_test.py @@ -0,0 +1,164 @@ +# Regression test for #313: UIEntity::grid -> shared_ptr + entity.texture +# +# Guards the invariants that must survive the #313 refactor: +# (a) entity.texture getter returns the entity's real Texture (never crashes) +# (b) setter accepts Texture only (TypeError otherwise), reflected on read-back, +# and preserves sprite_index +# (c) entity pixel position (x/y/pos) is derived from the GRID's cell size, +# NOT the entity's own texture -- setting a different-sized texture on the +# entity must not move it +# (d) entity.grid returns the same Grid (GridView) object across step/FOV cycles +# (PythonObjectCache identity) +# (e) find_path() and at() still work, and constructing their temporary internal +# grid wrappers does not destroy the grid's cell callbacks (#251 guard) +# (f) a GridPoint obtained from entity.at() outlives the entity and the grid +# reference without crashing (aliasing shared_ptr keeps the grid alive) +import mcrfpy +import gc +import sys + +failures = [] + +def check(cond, label): + status = "ok" if cond else "FAIL" + print(" %s: %s" % (status, label)) + if not cond: + failures.append(label) + +def main(): + tex16 = mcrfpy.Texture("assets/kenney_tinydungeon.png", 16, 16) + grid = mcrfpy.Grid(grid_size=(10, 10), texture=tex16, pos=(0, 0), size=(160, 160)) + for y in range(10): + for x in range(10): + p = grid.at(x, y) + p.walkable = True + p.transparent = True + + entity = mcrfpy.Entity(grid_pos=(2, 2), texture=tex16, sprite_index=5, grid=grid) + + # --- (a) getter returns the real texture --- + t = entity.texture + check(t is not None, "(a) entity.texture is not None") + check(isinstance(t, mcrfpy.Texture), "(a) entity.texture is a Texture") + check(t.sprite_width == 16 and t.sprite_height == 16, + "(a) entity.texture has the constructor texture's sprite dims") + check(t.source == tex16.source, "(a) entity.texture.source matches constructor texture") + + # --- (c) pixel position comes from the grid's cell size, not the entity texture --- + x_before, y_before = entity.x, entity.y + check(x_before == 2 * 16 and y_before == 2 * 16, + "(c) pixel pos = grid_pos * grid cell size before texture change") + + # an 8x8 texture with 8x8 sprites (single red sprite) + tex8 = mcrfpy.Texture.from_bytes(bytes([255, 0, 0, 255] * 64), 8, 8, 8, 8, + name="") + + # --- (b) setter semantics --- + entity.texture = tex8 + check(entity.texture.sprite_width == 8 and entity.texture.sprite_height == 8, + "(b) texture set is reflected on read-back") + check(entity.sprite_index == 5, "(b) sprite_index preserved across texture set") + try: + entity.texture = 42 + check(False, "(b) non-Texture assignment raises TypeError") + except TypeError: + check(True, "(b) non-Texture assignment raises TypeError") + try: + entity.texture = None + check(False, "(b) None assignment raises TypeError") + except TypeError: + check(True, "(b) None assignment raises TypeError") + try: + del entity.texture + check(False, "(b) attribute deletion raises TypeError") + except TypeError: + check(True, "(b) attribute deletion raises TypeError") + + # --- (c) continued: position unchanged after the 8x8 texture swap --- + check(entity.x == x_before and entity.y == y_before, + "(c) pixel pos unchanged after setting a different-sized entity texture") + + # --- (d) entity.grid identity across a step/FOV cycle --- + check(entity.grid is grid, "(d) entity.grid is the user-created Grid object") + grid.compute_fov((2, 2), radius=5) + grid.step(1) + check(entity.grid is grid, "(d) entity.grid identity survives step + compute_fov") + + # --- (e) find_path / at() work; cell callbacks survive temp grid wrappers --- + grid.on_cell_click = lambda pos, btn, action: None + path = entity.find_path((5, 5)) + check(path is not None, "(e) find_path returns a result on an open grid") + check(grid.on_cell_click is not None, + "(e) grid.on_cell_click survives find_path's temporary grid wrapper (#251)") + + entity.update_visibility() + gp = entity.at(2, 2) + check(gp is not None, "(e) entity.at(own cell) returns a GridPoint when visible") + check(gp.walkable is True, "(e) GridPoint from entity.at() reads cell data") + check(grid.on_cell_click is not None, + "(e) grid.on_cell_click survives entity.at()'s grid wrapper (#251)") + + # --- (g) hardening guards from the #313 adversarial review --- + # Null-data Texture (Texture.__new__ without __init__) must be rejected, + # not crash (same ValueError guard as UISprite.texture). + null_tex = mcrfpy.Texture.__new__(mcrfpy.Texture) + try: + entity.texture = null_tex + check(False, "(g) null-data Texture assignment raises ValueError") + except ValueError: + check(True, "(g) null-data Texture assignment raises ValueError") + # An object whose __class__ raises must not crash isinstance handling. + class EvilMeta: + @property + def __class__(self): + raise RuntimeError("no class for you") + try: + entity.texture = EvilMeta() + check(False, "(g) isinstance-error during assignment raises, no crash") + except (TypeError, RuntimeError): + check(True, "(g) isinstance-error during assignment raises, no crash") + # Uninitialized Entity wrapper (Entity.__new__ without __init__) must + # raise RuntimeError on texture access, not segfault. + uninit = mcrfpy.Entity.__new__(mcrfpy.Entity) + try: + _ = uninit.texture + check(False, "(g) uninitialized Entity texture get raises RuntimeError") + except RuntimeError: + check(True, "(g) uninitialized Entity texture get raises RuntimeError") + try: + uninit.texture = tex16 + check(False, "(g) uninitialized Entity texture set raises RuntimeError") + except RuntimeError: + check(True, "(g) uninitialized Entity texture set raises RuntimeError") + + # --- (h) owning_view survives Python wrapper GC while the C++ view lives --- + # The canonical idiom: append the Grid to a scene, drop the Python + # reference. The data-layer dirty notifications and entity.grid -> Grid + # identity both depend on owning_view staying intact. + scene = mcrfpy.Scene("issue313_ov") + g2 = mcrfpy.Grid(grid_size=(4, 4), texture=tex16, pos=(0, 0), size=(64, 64)) + e2 = mcrfpy.Entity(grid_pos=(1, 1), texture=tex16, grid=g2) + scene.children.append(g2) + del g2 + gc.collect() + check(type(e2.grid).__name__ == "Grid", + "(h) entity.grid still returns Grid after wrapper GC (owning_view intact)") + + # --- (f) GridPoint outlives entity + grid references --- + entity.die() + del entity + del grid + gc.collect() + check(gp.walkable is True, "(f) GridPoint usable after entity.die() and grid release") + check(tuple(gp.grid_pos) == (2, 2), "(f) GridPoint position intact after teardown") + del gp + gc.collect() + + if failures: + print("FAIL: %d check(s) failed" % len(failures)) + sys.exit(1) + print("PASS") + sys.exit(0) + +if __name__ == "__main__": + main() diff --git a/tests/snapshots/api_surface.golden.txt b/tests/snapshots/api_surface.golden.txt index 53c0130..33690af 100644 --- a/tests/snapshots/api_surface.golden.txt +++ b/tests/snapshots/api_surface.golden.txt @@ -465,6 +465,7 @@ submodule automation prop sprite_offset_y: Any (rw) prop step: Any (rw) prop target_label: Any (rw) + prop texture: Texture (rw) prop tile_height: int (rw) prop tile_size: Any (rw) prop tile_width: int (rw) @@ -1229,4 +1230,4 @@ func typewrite :: typewrite(message, interval=0.0) - Type text with optional int === WRITABILITY PROBES (#313-touched properties) === Entity.grid: writable Entity.sprite_index: writable - Entity.texture: ABSENT + Entity.texture: writable diff --git a/tests/unit/entity_texture_test.py b/tests/unit/entity_texture_test.py new file mode 100644 index 0000000..dc0527f --- /dev/null +++ b/tests/unit/entity_texture_test.py @@ -0,0 +1,48 @@ +# Unit test for entity.texture (#313): happy-path get/set semantics. +import mcrfpy +import sys + +failures = [] + +def check(cond, label): + status = "ok" if cond else "FAIL" + print(" %s: %s" % (status, label)) + if not cond: + failures.append(label) + +def main(): + # Entity constructed WITHOUT a texture falls back to mcrfpy.default_texture + e_default = mcrfpy.Entity(grid_pos=(0, 0)) + dt = mcrfpy.default_texture + check(e_default.texture is not None, "no-texture entity exposes a texture") + check(isinstance(e_default.texture, mcrfpy.Texture), + "no-texture entity texture is a Texture") + check(e_default.texture.source == dt.source and + e_default.texture.sprite_width == dt.sprite_width, + "no-texture entity uses mcrfpy.default_texture") + + # Entity constructed WITH a texture exposes that texture + tex = mcrfpy.Texture("assets/kenney_tinydungeon.png", 16, 16) + e = mcrfpy.Entity(grid_pos=(1, 1), texture=tex, sprite_index=3) + check(e.texture.source == tex.source, "constructor texture is exposed") + check(e.texture.sprite_count == tex.sprite_count, + "exposed texture matches constructor texture's sprite_count") + + # Setting a texture is reflected on read-back; sprite_index preserved + tex2 = mcrfpy.Texture.from_bytes(bytes([0, 255, 0, 255] * 256), 16, 16, 16, 16, + name="") + e.texture = tex2 + check(e.texture.source == tex2.source, "assigned texture is exposed") + check(e.sprite_index == 3, "sprite_index preserved across texture assignment") + + # Works without a grid attachment (no crash, no grid needed) + check(e_default.texture is not None, "texture access works without grid attachment") + + if failures: + print("FAIL: %d check(s) failed" % len(failures)) + sys.exit(1) + print("PASS") + sys.exit(0) + +if __name__ == "__main__": + main()