From 115e16f4f28411664ca16c43052cc2a04669fcfb Mon Sep 17 00:00:00 2001 From: John McCardle Date: Sat, 7 Mar 2026 23:30:32 -0500 Subject: [PATCH] Convert raw pointers to coordinate-based access (closes #264, closes #265) GridPoint and GridPointState Python objects now store (grid, x, y) coordinates instead of raw C++ pointers. Data addresses are computed on each property access, preventing dangling pointers after vector resizes. Co-Authored-By: Claude Opus 4.6 --- src/UIEntity.cpp | 33 +++--- src/UIGrid.cpp | 8 +- src/UIGridPoint.cpp | 100 ++++++++++++------ src/UIGridPoint.h | 5 +- .../issue_264_gridpoint_dangle_test.py | 97 +++++++++++++++++ 5 files changed, 182 insertions(+), 61 deletions(-) create mode 100644 tests/regression/issue_264_gridpoint_dangle_test.py diff --git a/src/UIEntity.cpp b/src/UIEntity.cpp index 95734e2..013238a 100644 --- a/src/UIEntity.cpp +++ b/src/UIEntity.cpp @@ -124,7 +124,6 @@ PyObject* UIEntity::at(PyUIEntityObject* self, PyObject* args, PyObject* kwds) { // Use type directly since GridPointState is internal-only (not exported to module) auto type = &mcrfpydef::PyUIGridPointStateType; auto obj = (PyUIGridPointStateObject*)type->tp_alloc(type, 0); - obj->data = &(self->data->gridstate[y * self->data->grid->grid_w + x]); obj->grid = self->data->grid; obj->entity = self->data; obj->x = x; // #16 - Store position for .point property @@ -324,24 +323,20 @@ sf::Vector2i PyObject_to_sfVector2i(PyObject* obj) { } PyObject* UIGridPointState_to_PyObject(const UIGridPointState& state) { - // Create a new GridPointState Python object (detached - no grid/entity context) - // Use type directly since GridPointState is internal-only (not exported to module) - auto type = &mcrfpydef::PyUIGridPointStateType; - auto obj = (PyUIGridPointStateObject*)type->tp_alloc(type, 0); - if (!obj) { - return NULL; - } - - // Allocate new data and copy values - obj->data = new UIGridPointState(); - obj->data->visible = state.visible; - obj->data->discovered = state.discovered; - - // Initialize context fields (detached state has no grid/entity context) - obj->grid = nullptr; - obj->entity = nullptr; - obj->x = -1; - obj->y = -1; + // Return a simple namespace with visible/discovered attributes + // (detached snapshot — not backed by a live entity's gridstate) + PyObject* types_mod = PyImport_ImportModule("types"); + if (!types_mod) return NULL; + PyObject* ns_type = PyObject_GetAttrString(types_mod, "SimpleNamespace"); + Py_DECREF(types_mod); + if (!ns_type) return NULL; + PyObject* kwargs = Py_BuildValue("{s:O,s:O}", + "visible", state.visible ? Py_True : Py_False, + "discovered", state.discovered ? Py_True : Py_False); + if (!kwargs) { Py_DECREF(ns_type); return NULL; } + PyObject* obj = PyObject_Call(ns_type, PyTuple_New(0), kwargs); + Py_DECREF(ns_type); + Py_DECREF(kwargs); return (PyObject*)obj; } diff --git a/src/UIGrid.cpp b/src/UIGrid.cpp index b6dfd0c..bd58697 100644 --- a/src/UIGrid.cpp +++ b/src/UIGrid.cpp @@ -1353,10 +1353,9 @@ PyObject* UIGrid::py_at(PyUIGridObject* self, PyObject* args, PyObject* kwds) // Use the type directly since GridPoint is internal-only (not exported to module) auto type = &mcrfpydef::PyUIGridPointType; auto obj = (PyUIGridPointObject*)type->tp_alloc(type, 0); - //auto target = std::static_pointer_cast(target); - // #123 - Use at() method to route through chunks for large grids - obj->data = &(self->data->at(x, y)); obj->grid = self->data; + obj->x = x; + obj->y = y; return (PyObject*)obj; } @@ -1396,8 +1395,9 @@ PyObject* UIGrid::subscript(PyUIGridObject* self, PyObject* key) auto obj = (PyUIGridPointObject*)type->tp_alloc(type, 0); if (!obj) return NULL; - obj->data = &(self->data->at(x, y)); obj->grid = self->data; + obj->x = x; + obj->y = y; return (PyObject*)obj; } diff --git a/src/UIGridPoint.cpp b/src/UIGridPoint.cpp index c50a80b..13eabf5 100644 --- a/src/UIGridPoint.cpp +++ b/src/UIGridPoint.cpp @@ -40,26 +40,44 @@ sf::Color PyObject_to_sfColor(PyObject* obj) { // #150 - Removed get_color/set_color - now handled by layers +// Helper to safely get the GridPoint data from coordinates +static UIGridPoint* getGridPointData(PyUIGridPointObject* self) { + if (!self->grid) return nullptr; + int idx = self->y * self->grid->grid_w + self->x; + if (idx < 0 || idx >= static_cast(self->grid->points.size())) return nullptr; + return &self->grid->points[idx]; +} + PyObject* UIGridPoint::get_bool_member(PyUIGridPointObject* self, void* closure) { + auto* data = getGridPointData(self); + if (!data) { + PyErr_SetString(PyExc_RuntimeError, "GridPoint data is no longer valid"); + return NULL; + } if (reinterpret_cast(closure) == 0) { // walkable - return PyBool_FromLong(self->data->walkable); + return PyBool_FromLong(data->walkable); } else { // transparent - return PyBool_FromLong(self->data->transparent); + return PyBool_FromLong(data->transparent); } } int UIGridPoint::set_bool_member(PyUIGridPointObject* self, PyObject* value, void* closure) { + auto* data = getGridPointData(self); + if (!data) { + PyErr_SetString(PyExc_RuntimeError, "GridPoint data is no longer valid"); + return -1; + } if (value == Py_True) { if (reinterpret_cast(closure) == 0) { // walkable - self->data->walkable = true; + data->walkable = true; } else { // transparent - self->data->transparent = true; + data->transparent = true; } } else if (value == Py_False) { if (reinterpret_cast(closure) == 0) { // walkable - self->data->walkable = false; + data->walkable = false; } else { // transparent - self->data->transparent = false; + data->transparent = false; } } else { PyErr_SetString(PyExc_ValueError, "Expected a boolean value"); @@ -67,8 +85,8 @@ int UIGridPoint::set_bool_member(PyUIGridPointObject* self, PyObject* value, voi } // Sync with TCOD map if parent grid exists - if (self->data->parent_grid && self->data->grid_x >= 0 && self->data->grid_y >= 0) { - self->data->parent_grid->syncTCODMapCell(self->data->grid_x, self->data->grid_y); + if (data->parent_grid && data->grid_x >= 0 && data->grid_y >= 0) { + data->parent_grid->syncTCODMapCell(data->grid_x, data->grid_y); } return 0; @@ -83,8 +101,8 @@ PyObject* UIGridPoint::get_entities(PyUIGridPointObject* self, void* closure) { return NULL; } - int target_x = self->data->grid_x; - int target_y = self->data->grid_y; + int target_x = self->x; + int target_y = self->y; PyObject* list = PyList_New(0); if (!list) return NULL; @@ -115,7 +133,7 @@ PyObject* UIGridPoint::get_entities(PyUIGridPointObject* self, void* closure) { // #177 - Get grid position as tuple PyObject* UIGridPoint::get_grid_pos(PyUIGridPointObject* self, void* closure) { - return Py_BuildValue("(ii)", self->data->grid_x, self->data->grid_y); + return Py_BuildValue("(ii)", self->x, self->y); } PyGetSetDef UIGridPoint::getsetters[] = { @@ -129,26 +147,44 @@ PyGetSetDef UIGridPoint::getsetters[] = { PyObject* UIGridPoint::repr(PyUIGridPointObject* self) { std::ostringstream ss; - if (!self->data) ss << ""; + auto* gp = getGridPointData(self); + if (!gp) ss << ""; else { - auto gp = self->data; ss << "grid_x << ", " << gp->grid_y << ")>"; + << ") at (" << self->x << ", " << self->y << ")>"; } std::string repr_str = ss.str(); return PyUnicode_DecodeUTF8(repr_str.c_str(), repr_str.size(), "replace"); } +// Helper to safely get the GridPointState data from coordinates +static UIGridPointState* getGridPointStateData(PyUIGridPointStateObject* self) { + if (!self->entity || !self->entity->grid) return nullptr; + int idx = self->y * self->entity->grid->grid_w + self->x; + if (idx < 0 || idx >= static_cast(self->entity->gridstate.size())) return nullptr; + return &self->entity->gridstate[idx]; +} + PyObject* UIGridPointState::get_bool_member(PyUIGridPointStateObject* self, void* closure) { + auto* data = getGridPointStateData(self); + if (!data) { + PyErr_SetString(PyExc_RuntimeError, "GridPointState data is no longer valid"); + return NULL; + } if (reinterpret_cast(closure) == 0) { // visible - return PyBool_FromLong(self->data->visible); + return PyBool_FromLong(data->visible); } else { // discovered - return PyBool_FromLong(self->data->discovered); + return PyBool_FromLong(data->discovered); } } int UIGridPointState::set_bool_member(PyUIGridPointStateObject* self, PyObject* value, void* closure) { + auto* data = getGridPointStateData(self); + if (!data) { + PyErr_SetString(PyExc_RuntimeError, "GridPointState data is no longer valid"); + return -1; + } if (!PyBool_Check(value)) { PyErr_SetString(PyExc_TypeError, "Value must be a boolean"); return -1; @@ -156,13 +192,13 @@ int UIGridPointState::set_bool_member(PyUIGridPointStateObject* self, PyObject* int truthValue = PyObject_IsTrue(value); if (truthValue < 0) { - return -1; // PyObject_IsTrue returns -1 on error + return -1; } if (reinterpret_cast(closure) == 0) { // visible - self->data->visible = truthValue; + data->visible = truthValue; } else { // discovered - self->data->discovered = truthValue; + data->discovered = truthValue; } return 0; @@ -171,7 +207,8 @@ int UIGridPointState::set_bool_member(PyUIGridPointStateObject* self, PyObject* // #16 - Get GridPoint at this position (None if not discovered) PyObject* UIGridPointState::get_point(PyUIGridPointStateObject* self, void* closure) { // Return None if entity hasn't discovered this cell - if (!self->data->discovered) { + auto* data = getGridPointStateData(self); + if (!data || !data->discovered) { Py_RETURN_NONE; } @@ -185,16 +222,9 @@ PyObject* UIGridPointState::get_point(PyUIGridPointStateObject* self, void* clos auto obj = (PyUIGridPointObject*)type->tp_alloc(type, 0); if (!obj) return NULL; - // Get the GridPoint from the grid - int idx = self->y * self->grid->grid_w + self->x; - if (idx < 0 || idx >= static_cast(self->grid->points.size())) { - Py_DECREF(obj); - PyErr_SetString(PyExc_IndexError, "GridPointState position out of bounds"); - return NULL; - } - - obj->data = &(self->grid->points[idx]); obj->grid = self->grid; + obj->x = self->x; + obj->y = self->y; return (PyObject*)obj; } @@ -207,9 +237,9 @@ PyGetSetDef UIGridPointState::getsetters[] = { PyObject* UIGridPointState::repr(PyUIGridPointStateObject* self) { std::ostringstream ss; - if (!self->data) ss << ""; + auto* gps = getGridPointStateData(self); + if (!gps) ss << ""; else { - auto gps = self->data; ss << ""; } @@ -243,8 +273,8 @@ PyObject* UIGridPoint::getattro(PyUIGridPointObject* self, PyObject* name) { return nullptr; } - int x = self->data->grid_x; - int y = self->data->grid_y; + int x = self->x; + int y = self->y; // Get value based on layer type if (layer->type == GridLayerType::Color) { @@ -285,8 +315,8 @@ int UIGridPoint::setattro(PyUIGridPointObject* self, PyObject* name, PyObject* v return -1; } - int x = self->data->grid_x; - int y = self->data->grid_y; + int x = self->x; + int y = self->y; // Set value based on layer type if (layer->type == GridLayerType::Color) { diff --git a/src/UIGridPoint.h b/src/UIGridPoint.h index c62b14f..902df2d 100644 --- a/src/UIGridPoint.h +++ b/src/UIGridPoint.h @@ -22,16 +22,15 @@ class UIGridPointState; typedef struct { PyObject_HEAD - UIGridPoint* data; std::shared_ptr grid; + int x, y; // Grid coordinates - compute data pointer on access } PyUIGridPointObject; typedef struct { PyObject_HEAD - UIGridPointState* data; std::shared_ptr grid; std::shared_ptr entity; - int x, y; // Position in grid (needed for .point property) + int x, y; // Position in grid - compute state on access } PyUIGridPointStateObject; // UIGridPoint - grid cell data for pathfinding and layer access diff --git a/tests/regression/issue_264_gridpoint_dangle_test.py b/tests/regression/issue_264_gridpoint_dangle_test.py new file mode 100644 index 0000000..9dc0590 --- /dev/null +++ b/tests/regression/issue_264_gridpoint_dangle_test.py @@ -0,0 +1,97 @@ +"""Regression test: GridPoint/GridPointState coordinate-based access (#264, #265). + +Bug: PyUIGridPointObject stored a raw UIGridPoint* pointer into the grid's +points vector. PyUIGridPointStateObject stored a raw UIGridPointState* +into the entity's gridstate vector. If either vector was resized, these +pointers would dangle. + +Fix: Remove raw pointers. Store (grid, x, y) coordinates and compute +the data address on each property access. +""" +import mcrfpy +import sys + +def test_gridpoint_access(): + """grid.at(x,y) returns working GridPoint via coordinate lookup""" + grid = mcrfpy.Grid(grid_size=(10, 10)) + gp = grid.at(3, 4) + assert gp.walkable == False # default + gp.walkable = True + assert gp.walkable == True + gp.transparent = True + assert gp.transparent == True + print(" PASS: gridpoint_access") + +def test_gridpoint_grid_pos(): + """GridPoint.grid_pos returns correct coordinates""" + grid = mcrfpy.Grid(grid_size=(10, 10)) + gp = grid.at(7, 3) + pos = gp.grid_pos + assert pos == (7, 3), f"Expected (7, 3), got {pos}" + print(" PASS: gridpoint_grid_pos") + +def test_gridpointstate_access(): + """entity.at(x,y) returns working GridPointState via coordinate lookup""" + grid = mcrfpy.Grid(grid_size=(10, 10)) + entity = mcrfpy.Entity(grid_pos=(5, 5), grid=grid) + entity.update_visibility() + + state = entity.at(5, 5) + # Should be accessible + assert state is not None + # visible/discovered should be boolean + assert isinstance(state.visible, bool) + assert isinstance(state.discovered, bool) + print(" PASS: gridpointstate_access") + +def test_gridpointstate_after_grid_transfer(): + """GridPointState access works after entity transfers to new grid""" + small = mcrfpy.Grid(grid_size=(10, 10)) + large = mcrfpy.Grid(grid_size=(20, 20)) + entity = mcrfpy.Entity(grid_pos=(5, 5), grid=small) + entity.update_visibility() + + # Get state on small grid + state1 = entity.at(3, 3) + assert state1 is not None + + # Transfer to large grid (gridstate resizes) + entity.grid = large + entity.update_visibility() + + # Access a cell that didn't exist on small grid + state2 = entity.at(15, 15) + assert state2 is not None + print(" PASS: gridpointstate_after_grid_transfer") + +def test_gridpoint_subscript(): + """grid[x, y] returns working GridPoint""" + grid = mcrfpy.Grid(grid_size=(10, 10)) + gp = grid[3, 4] + gp.walkable = True + assert grid.at(3, 4).walkable == True + print(" PASS: gridpoint_subscript") + +def test_gridstate_list(): + """entity.gridstate returns list with visible/discovered attrs""" + grid = mcrfpy.Grid(grid_size=(5, 5)) + entity = mcrfpy.Entity(grid_pos=(2, 2), grid=grid) + entity.update_visibility() + + gs = entity.gridstate + assert len(gs) == 25, f"Expected 25, got {len(gs)}" + # Each element should have visible and discovered + for state in gs: + assert hasattr(state, 'visible') + assert hasattr(state, 'discovered') + print(" PASS: gridstate_list") + +print("Testing GridPoint/GridPointState coordinate-based access...") +test_gridpoint_access() +test_gridpoint_grid_pos() +test_gridpointstate_access() +test_gridpointstate_after_grid_transfer() +test_gridpoint_subscript() +test_gridstate_list() +print("PASS: all GridPoint/GridPointState tests passed") +sys.exit(0)