john/McRogueFace
1
0
Fork 0
Code Issues 26 Pull requests Projects Releases Wiki Activity

Convert raw pointers to coordinate-based access (closes #264, closes #265)

Browse source 
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 <noreply@anthropic.com>
This commit is contained in:
John McCardle 2026-03-07 23:30:32 -05:00
commit 115e16f4f2
5 changed files with 182 additions and 61 deletions
Download patch file Download diff file Expand all files Collapse all files

33
src/UIEntity.cpp
View file

@ -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;
}

8
src/UIGrid.cpp
View file

@ -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<UIEntity>(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;
}

100
src/UIGridPoint.cpp
View file

@ -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<int>(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<intptr_t>(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<intptr_t>(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<intptr_t>(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 << "<GridPoint (invalid internal object)>";
auto* gp = getGridPointData(self);
if (!gp) ss << "<GridPoint (invalid internal object)>";
else {
auto gp = self->data;
ss << "<GridPoint (walkable=" << (gp->walkable ? "True" : "False")
<< ", transparent=" << (gp->transparent ? "True" : "False")
<< ") at (" << gp->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<int>(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<intptr_t>(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<intptr_t>(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<int>(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 << "<GridPointState (invalid internal object)>";
auto* gps = getGridPointStateData(self);
if (!gps) ss << "<GridPointState (invalid internal object)>";
else {
auto gps = self->data;
ss << "<GridPointState (visible=" << (gps->visible ? "True" : "False") << ", discovered=" << (gps->discovered ? "True" : "False") <<
")>";
}
@ -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) {

5
src/UIGridPoint.h
View file

@ -22,16 +22,15 @@ class UIGridPointState;
typedef struct {
PyObject_HEAD
UIGridPoint* data;
std::shared_ptr<UIGrid> grid;
int x, y; // Grid coordinates - compute data pointer on access
} PyUIGridPointObject;
typedef struct {
PyObject_HEAD
UIGridPointState* data;
std::shared_ptr<UIGrid> grid;
std::shared_ptr<UIEntity> 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

97
tests/regression/issue_264_gridpoint_dangle_test.py Normal file
View file

@ -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)