diff --git a/src/PyBSP.cpp b/src/PyBSP.cpp index 67f8c0d..a387a1d 100644 --- a/src/PyBSP.cpp +++ b/src/PyBSP.cpp @@ -85,11 +85,10 @@ PyObject* PyTraversal::create_enum_class(PyObject* module) { return NULL; } - // Cache reference + // Cache reference (borrowed by module after AddObject) traversal_enum_class = traversal_class; - Py_INCREF(traversal_enum_class); - // Add to module + // Add to module (steals reference) if (PyModule_AddObject(module, "Traversal", traversal_class) < 0) { Py_DECREF(traversal_class); traversal_enum_class = nullptr; @@ -99,6 +98,12 @@ PyObject* PyTraversal::create_enum_class(PyObject* module) { return traversal_class; } +void PyTraversal::cleanup() { + // The enum class is owned by the module after PyModule_AddObject + // We just clear our pointer; the module handles cleanup + traversal_enum_class = nullptr; +} + int PyTraversal::from_arg(PyObject* arg, int* out_order) { // Accept None -> default to LEVEL_ORDER if (arg == NULL || arg == Py_None) { @@ -182,29 +187,41 @@ int PyTraversal::from_arg(PyObject* arg, int* out_order) { PyGetSetDef PyBSP::getsetters[] = { {"bounds", (getter)PyBSP::get_bounds, NULL, MCRF_PROPERTY(bounds, "Root node bounds as ((x, y), (w, h)). Read-only."), NULL}, + {"pos", (getter)PyBSP::get_pos, NULL, + MCRF_PROPERTY(pos, "Top-left position (x, y). Read-only."), NULL}, + {"size", (getter)PyBSP::get_size, NULL, + MCRF_PROPERTY(size, "Dimensions (width, height). Read-only."), NULL}, {"root", (getter)PyBSP::get_root, NULL, MCRF_PROPERTY(root, "Reference to the root BSPNode. Read-only."), NULL}, {NULL} }; +// Sequence methods for len() and iteration +PySequenceMethods PyBSP::sequence_methods = { + .sq_length = (lenfunc)PyBSP::len, +}; + // ==================== BSP Method Definitions ==================== PyMethodDef PyBSP::methods[] = { {"split_once", (PyCFunction)PyBSP::split_once, METH_VARARGS | METH_KEYWORDS, MCRF_METHOD(BSP, split_once, MCRF_SIG("(horizontal: bool, position: int)", "BSP"), - MCRF_DESC("Split the root node once at the specified position."), + MCRF_DESC("Split the root node once at the specified position. " + "horizontal=True creates a horizontal divider, producing top/bottom rooms. " + "horizontal=False creates a vertical divider, producing left/right rooms."), MCRF_ARGS_START - MCRF_ARG("horizontal", "True for horizontal split, False for vertical") + MCRF_ARG("horizontal", "True for horizontal divider (top/bottom), False for vertical (left/right)") MCRF_ARG("position", "Split coordinate (y for horizontal, x for vertical)") MCRF_RETURNS("BSP: self, for method chaining") )}, {"split_recursive", (PyCFunction)PyBSP::split_recursive, METH_VARARGS | METH_KEYWORDS, MCRF_METHOD(BSP, split_recursive, MCRF_SIG("(depth: int, min_size: tuple[int, int], max_ratio: float = 1.5, seed: int = None)", "BSP"), - MCRF_DESC("Recursively split to the specified depth."), + MCRF_DESC("Recursively split to the specified depth. " + "WARNING: Invalidates all existing BSPNode references from this tree."), MCRF_ARGS_START - MCRF_ARG("depth", "Maximum recursion depth. Creates up to 2^depth leaves.") + MCRF_ARG("depth", "Maximum recursion depth (1-16). Creates up to 2^depth leaves.") MCRF_ARG("min_size", "Minimum (width, height) for a node to be split.") MCRF_ARG("max_ratio", "Maximum aspect ratio before forcing split direction. Default: 1.5.") MCRF_ARG("seed", "Random seed. None for random.") @@ -213,13 +230,14 @@ PyMethodDef PyBSP::methods[] = { {"clear", (PyCFunction)PyBSP::clear, METH_NOARGS, MCRF_METHOD(BSP, clear, MCRF_SIG("()", "BSP"), - MCRF_DESC("Remove all children, keeping only the root node with original bounds."), + MCRF_DESC("Remove all children, keeping only the root node with original bounds. " + "WARNING: Invalidates all existing BSPNode references from this tree."), MCRF_RETURNS("BSP: self, for method chaining") )}, {"leaves", (PyCFunction)PyBSP::leaves, METH_NOARGS, MCRF_METHOD(BSP, leaves, MCRF_SIG("()", "Iterator[BSPNode]"), - MCRF_DESC("Iterate all leaf nodes (the actual rooms)."), + MCRF_DESC("Iterate all leaf nodes (the actual rooms). Same as iterating the BSP directly."), MCRF_RETURNS("Iterator yielding BSPNode objects") )}, {"traverse", (PyCFunction)PyBSP::traverse, METH_VARARGS | METH_KEYWORDS, @@ -264,41 +282,33 @@ PyObject* PyBSP::pynew(PyTypeObject* type, PyObject* args, PyObject* kwds) self->orig_y = 0; self->orig_w = 0; self->orig_h = 0; + self->generation = 0; } return (PyObject*)self; } int PyBSP::init(PyBSPObject* self, PyObject* args, PyObject* kwds) { - static const char* keywords[] = {"bounds", nullptr}; - PyObject* bounds_obj = nullptr; + static const char* keywords[] = {"pos", "size", nullptr}; + PyObject* pos_obj = nullptr; + PyObject* size_obj = nullptr; - if (!PyArg_ParseTupleAndKeywords(args, kwds, "O", const_cast(keywords), - &bounds_obj)) { + if (!PyArg_ParseTupleAndKeywords(args, kwds, "OO", const_cast(keywords), + &pos_obj, &size_obj)) { return -1; } - // Parse bounds: ((x, y), (w, h)) - if (!PyTuple_Check(bounds_obj) || PyTuple_Size(bounds_obj) != 2) { - PyErr_SetString(PyExc_TypeError, "bounds must be ((x, y), (w, h)) tuple"); + // Parse position using PyPositionHelper pattern + int x, y; + if (!PyPosition_FromObjectInt(pos_obj, &x, &y)) { + PyErr_SetString(PyExc_TypeError, "pos must be a tuple (x, y), list, or Vector"); return -1; } - PyObject* pos_obj = PyTuple_GetItem(bounds_obj, 0); - PyObject* size_obj = PyTuple_GetItem(bounds_obj, 1); - - if (!PyTuple_Check(pos_obj) || PyTuple_Size(pos_obj) != 2 || - !PyTuple_Check(size_obj) || PyTuple_Size(size_obj) != 2) { - PyErr_SetString(PyExc_TypeError, "bounds must be ((x, y), (w, h)) tuple"); - return -1; - } - - int x = (int)PyLong_AsLong(PyTuple_GetItem(pos_obj, 0)); - int y = (int)PyLong_AsLong(PyTuple_GetItem(pos_obj, 1)); - int w = (int)PyLong_AsLong(PyTuple_GetItem(size_obj, 0)); - int h = (int)PyLong_AsLong(PyTuple_GetItem(size_obj, 1)); - - if (PyErr_Occurred()) { + // Parse size using PyPositionHelper pattern + int w, h; + if (!PyPosition_FromObjectInt(size_obj, &w, &h)) { + PyErr_SetString(PyExc_TypeError, "size must be a tuple (w, h), list, or Vector"); return -1; } @@ -307,6 +317,14 @@ int PyBSP::init(PyBSPObject* self, PyObject* args, PyObject* kwds) return -1; } + // Validate against GRID_MAX like HeightMap does + if (w > GRID_MAX || h > GRID_MAX) { + PyErr_Format(PyExc_ValueError, + "BSP dimensions cannot exceed %d (got %dx%d)", + GRID_MAX, w, h); + return -1; + } + // Clean up any existing BSP if (self->root) { TCOD_bsp_delete(self->root); @@ -324,6 +342,7 @@ int PyBSP::init(PyBSPObject* self, PyObject* args, PyObject* kwds) self->orig_y = y; self->orig_w = w; self->orig_h = h; + self->generation = 0; return 0; } @@ -373,6 +392,26 @@ PyObject* PyBSP::get_bounds(PyBSPObject* self, void* closure) self->root->w, self->root->h); } +// Property: pos +PyObject* PyBSP::get_pos(PyBSPObject* self, void* closure) +{ + if (!self->root) { + PyErr_SetString(PyExc_RuntimeError, "BSP not initialized"); + return nullptr; + } + return Py_BuildValue("(ii)", self->root->x, self->root->y); +} + +// Property: size +PyObject* PyBSP::get_size(PyBSPObject* self, void* closure) +{ + if (!self->root) { + PyErr_SetString(PyExc_RuntimeError, "BSP not initialized"); + return nullptr; + } + return Py_BuildValue("(ii)", self->root->w, self->root->h); +} + // Property: root PyObject* PyBSP::get_root(PyBSPObject* self, void* closure) { @@ -400,6 +439,8 @@ PyObject* PyBSP::split_once(PyBSPObject* self, PyObject* args, PyObject* kwds) return nullptr; } + // Note: split_once only adds children, doesn't remove any nodes + // Root node pointer remains valid, so we don't increment generation TCOD_bsp_split_once(self->root, horizontal ? true : false, position); Py_INCREF(self); @@ -425,21 +466,22 @@ PyObject* PyBSP::split_recursive(PyBSPObject* self, PyObject* args, PyObject* kw return nullptr; } - // Parse min_size tuple - if (!PyTuple_Check(min_size_obj) || PyTuple_Size(min_size_obj) != 2) { - PyErr_SetString(PyExc_TypeError, "min_size must be (width, height) tuple"); + // Parse min_size using PyPositionHelper pattern + int min_w, min_h; + if (!PyPosition_FromObjectInt(min_size_obj, &min_w, &min_h)) { + PyErr_SetString(PyExc_TypeError, "min_size must be (width, height) tuple, list, or Vector"); return nullptr; } - int min_w = (int)PyLong_AsLong(PyTuple_GetItem(min_size_obj, 0)); - int min_h = (int)PyLong_AsLong(PyTuple_GetItem(min_size_obj, 1)); - - if (PyErr_Occurred()) { + if (depth < 1) { + PyErr_SetString(PyExc_ValueError, "depth must be at least 1"); return nullptr; } - if (depth < 0) { - PyErr_SetString(PyExc_ValueError, "depth must be non-negative"); + if (depth > BSP_MAX_DEPTH) { + PyErr_Format(PyExc_ValueError, + "depth cannot exceed %d (got %d) to prevent memory exhaustion", + BSP_MAX_DEPTH, depth); return nullptr; } @@ -462,6 +504,9 @@ PyObject* PyBSP::split_recursive(PyBSPObject* self, PyObject* args, PyObject* kw rnd = TCOD_random_new_from_seed(TCOD_RNG_MT, seed); } + // Increment generation BEFORE splitting - invalidates existing nodes + self->generation++; + TCOD_bsp_split_recursive(self->root, rnd, depth, min_w, min_h, max_ratio, max_ratio); if (rnd) { @@ -480,6 +525,9 @@ PyObject* PyBSP::clear(PyBSPObject* self, PyObject* Py_UNUSED(args)) return nullptr; } + // Increment generation BEFORE clearing - invalidates existing nodes + self->generation++; + TCOD_bsp_remove_sons(self->root); // Restore original bounds @@ -489,6 +537,31 @@ PyObject* PyBSP::clear(PyBSPObject* self, PyObject* Py_UNUSED(args)) return (PyObject*)self; } +// Sequence protocol: len(bsp) returns leaf count +Py_ssize_t PyBSP::len(PyBSPObject* self) +{ + if (!self->root) { + PyErr_SetString(PyExc_RuntimeError, "BSP not initialized"); + return -1; + } + + int leaf_count = 0; + TCOD_bsp_traverse_pre_order(self->root, [](TCOD_bsp_t* node, void* data) -> bool { + if (TCOD_bsp_is_leaf(node)) { + (*(int*)data)++; + } + return true; + }, &leaf_count); + + return (Py_ssize_t)leaf_count; +} + +// __iter__ is shorthand for leaves() +PyObject* PyBSP::iter(PyBSPObject* self) +{ + return PyBSP::leaves(self, nullptr); +} + // ==================== BSP Iteration ==================== // Traversal callback to collect nodes @@ -527,6 +600,7 @@ PyObject* PyBSP::leaves(PyBSPObject* self, PyObject* Py_UNUSED(args)) iter->index = 0; iter->bsp_owner = (PyObject*)self; + iter->generation = self->generation; // Capture generation for validity check Py_INCREF(self); return (PyObject*)iter; @@ -584,6 +658,7 @@ PyObject* PyBSP::traverse(PyBSPObject* self, PyObject* args, PyObject* kwds) iter->index = 0; iter->bsp_owner = (PyObject*)self; + iter->generation = self->generation; // Capture generation for validity check Py_INCREF(self); return (PyObject*)iter; @@ -633,13 +708,8 @@ PyObject* PyBSP::to_heightmap(PyBSPObject* self, PyObject* args, PyObject* kwds) int width = self->root->w; int height = self->root->h; if (size_obj != nullptr && size_obj != Py_None) { - if (!PyTuple_Check(size_obj) || PyTuple_Size(size_obj) != 2) { - PyErr_SetString(PyExc_TypeError, "size must be (width, height) tuple"); - return nullptr; - } - width = (int)PyLong_AsLong(PyTuple_GetItem(size_obj, 0)); - height = (int)PyLong_AsLong(PyTuple_GetItem(size_obj, 1)); - if (PyErr_Occurred()) { + if (!PyPosition_FromObjectInt(size_obj, &width, &height)) { + PyErr_SetString(PyExc_TypeError, "size must be (width, height) tuple, list, or Vector"); return nullptr; } } @@ -751,6 +821,10 @@ PyObject* PyBSP::to_heightmap(PyBSPObject* self, PyObject* args, PyObject* kwds) PyGetSetDef PyBSPNode::getsetters[] = { {"bounds", (getter)PyBSPNode::get_bounds, NULL, MCRF_PROPERTY(bounds, "Node bounds as ((x, y), (w, h)). Read-only."), NULL}, + {"pos", (getter)PyBSPNode::get_pos, NULL, + MCRF_PROPERTY(pos, "Top-left position (x, y). Read-only."), NULL}, + {"size", (getter)PyBSPNode::get_size, NULL, + MCRF_PROPERTY(size, "Dimensions (width, height). Read-only."), NULL}, {"level", (getter)PyBSPNode::get_level, NULL, MCRF_PROPERTY(level, "Depth in tree (0 for root). Read-only."), NULL}, {"is_leaf", (getter)PyBSPNode::get_is_leaf, NULL, @@ -792,6 +866,30 @@ PyMethodDef PyBSPNode::methods[] = { // ==================== BSPNode Implementation ==================== +// Validity check - returns false and sets error if node is stale +bool PyBSPNode::checkValid(PyBSPNodeObject* self) +{ + if (!self->node) { + PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid (null pointer)"); + return false; + } + + if (!self->bsp_owner) { + PyErr_SetString(PyExc_RuntimeError, "BSPNode has no parent BSP"); + return false; + } + + PyBSPObject* bsp = (PyBSPObject*)self->bsp_owner; + if (self->generation != bsp->generation) { + PyErr_SetString(PyExc_RuntimeError, + "BSPNode is stale: parent BSP was modified (clear() or split_recursive() called). " + "Re-fetch nodes from the BSP object."); + return false; + } + + return true; +} + PyObject* PyBSPNode::pynew(PyTypeObject* type, PyObject* args, PyObject* kwds) { // BSPNode cannot be directly instantiated @@ -817,20 +915,52 @@ void PyBSPNode::dealloc(PyBSPNodeObject* self) PyObject* PyBSPNode::repr(PyObject* obj) { PyBSPNodeObject* self = (PyBSPNodeObject*)obj; + + // Check validity without raising error for repr + PyBSPObject* bsp = self->bsp_owner ? (PyBSPObject*)self->bsp_owner : nullptr; + bool is_valid = self->node && bsp && self->generation == bsp->generation; + std::ostringstream ss; - if (self->node) { + if (is_valid) { const char* type = TCOD_bsp_is_leaf(self->node) ? "leaf" : "split"; ss << "node->x << ", " << self->node->y << ") size (" << self->node->w << " x " << self->node->h << ") level " << (int)self->node->level << ">"; } else { - ss << ""; + ss << ""; } return PyUnicode_FromString(ss.str().c_str()); } +// Rich comparison for BSPNode - compares underlying pointers +PyObject* PyBSPNode::richcompare(PyObject* self, PyObject* other, int op) +{ + // Only support == and != + if (op != Py_EQ && op != Py_NE) { + Py_RETURN_NOTIMPLEMENTED; + } + + // Check if other is also a BSPNode + if (!PyObject_TypeCheck(other, &mcrfpydef::PyBSPNodeType)) { + if (op == Py_EQ) Py_RETURN_FALSE; + else Py_RETURN_TRUE; + } + + PyBSPNodeObject* self_node = (PyBSPNodeObject*)self; + PyBSPNodeObject* other_node = (PyBSPNodeObject*)other; + + // Compare wrapped pointers + bool equal = (self_node->node == other_node->node); + + if (op == Py_EQ) { + return PyBool_FromLong(equal); + } else { + return PyBool_FromLong(!equal); + } +} + // Helper to create a BSPNode wrapper PyObject* PyBSPNode::create(TCOD_bsp_t* node, PyObject* bsp_owner) { @@ -846,6 +976,7 @@ PyObject* PyBSPNode::create(TCOD_bsp_t* node, PyObject* bsp_owner) py_node->node = node; py_node->bsp_owner = bsp_owner; + py_node->generation = ((PyBSPObject*)bsp_owner)->generation; Py_INCREF(bsp_owner); return (PyObject*)py_node; @@ -854,42 +985,44 @@ PyObject* PyBSPNode::create(TCOD_bsp_t* node, PyObject* bsp_owner) // Property: bounds PyObject* PyBSPNode::get_bounds(PyBSPNodeObject* self, void* closure) { - if (!self->node) { - PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid"); - return nullptr; - } + if (!checkValid(self)) return nullptr; return Py_BuildValue("((ii)(ii))", self->node->x, self->node->y, self->node->w, self->node->h); } +// Property: pos +PyObject* PyBSPNode::get_pos(PyBSPNodeObject* self, void* closure) +{ + if (!checkValid(self)) return nullptr; + return Py_BuildValue("(ii)", self->node->x, self->node->y); +} + +// Property: size +PyObject* PyBSPNode::get_size(PyBSPNodeObject* self, void* closure) +{ + if (!checkValid(self)) return nullptr; + return Py_BuildValue("(ii)", self->node->w, self->node->h); +} + // Property: level PyObject* PyBSPNode::get_level(PyBSPNodeObject* self, void* closure) { - if (!self->node) { - PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid"); - return nullptr; - } + if (!checkValid(self)) return nullptr; return PyLong_FromLong(self->node->level); } // Property: is_leaf PyObject* PyBSPNode::get_is_leaf(PyBSPNodeObject* self, void* closure) { - if (!self->node) { - PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid"); - return nullptr; - } + if (!checkValid(self)) return nullptr; return PyBool_FromLong(TCOD_bsp_is_leaf(self->node)); } // Property: split_horizontal PyObject* PyBSPNode::get_split_horizontal(PyBSPNodeObject* self, void* closure) { - if (!self->node) { - PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid"); - return nullptr; - } + if (!checkValid(self)) return nullptr; if (TCOD_bsp_is_leaf(self->node)) { Py_RETURN_NONE; } @@ -899,10 +1032,7 @@ PyObject* PyBSPNode::get_split_horizontal(PyBSPNodeObject* self, void* closure) // Property: split_position PyObject* PyBSPNode::get_split_position(PyBSPNodeObject* self, void* closure) { - if (!self->node) { - PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid"); - return nullptr; - } + if (!checkValid(self)) return nullptr; if (TCOD_bsp_is_leaf(self->node)) { Py_RETURN_NONE; } @@ -912,40 +1042,28 @@ PyObject* PyBSPNode::get_split_position(PyBSPNodeObject* self, void* closure) // Property: left PyObject* PyBSPNode::get_left(PyBSPNodeObject* self, void* closure) { - if (!self->node) { - PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid"); - return nullptr; - } + if (!checkValid(self)) return nullptr; return PyBSPNode::create(TCOD_bsp_left(self->node), self->bsp_owner); } // Property: right PyObject* PyBSPNode::get_right(PyBSPNodeObject* self, void* closure) { - if (!self->node) { - PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid"); - return nullptr; - } + if (!checkValid(self)) return nullptr; return PyBSPNode::create(TCOD_bsp_right(self->node), self->bsp_owner); } // Property: parent PyObject* PyBSPNode::get_parent(PyBSPNodeObject* self, void* closure) { - if (!self->node) { - PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid"); - return nullptr; - } + if (!checkValid(self)) return nullptr; return PyBSPNode::create(TCOD_bsp_father(self->node), self->bsp_owner); } // Property: sibling PyObject* PyBSPNode::get_sibling(PyBSPNodeObject* self, void* closure) { - if (!self->node) { - PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid"); - return nullptr; - } + if (!checkValid(self)) return nullptr; TCOD_bsp_t* parent = TCOD_bsp_father(self->node); if (!parent) { @@ -965,10 +1083,7 @@ PyObject* PyBSPNode::get_sibling(PyBSPNodeObject* self, void* closure) // Method: contains(pos) -> bool PyObject* PyBSPNode::contains(PyBSPNodeObject* self, PyObject* args, PyObject* kwds) { - if (!self->node) { - PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid"); - return nullptr; - } + if (!checkValid(self)) return nullptr; int x, y; if (!PyPosition_ParseInt(args, kwds, &x, &y)) { @@ -981,10 +1096,7 @@ PyObject* PyBSPNode::contains(PyBSPNodeObject* self, PyObject* args, PyObject* k // Method: center() -> (x, y) PyObject* PyBSPNode::center(PyBSPNodeObject* self, PyObject* Py_UNUSED(args)) { - if (!self->node) { - PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid"); - return nullptr; - } + if (!checkValid(self)) return nullptr; int cx = self->node->x + self->node->w / 2; int cy = self->node->y + self->node->h / 2; @@ -1017,6 +1129,16 @@ PyObject* PyBSPIter::next(PyBSPIterObject* self) return NULL; } + // Check for tree modification during iteration + if (self->bsp_owner) { + PyBSPObject* bsp = (PyBSPObject*)self->bsp_owner; + if (self->generation != bsp->generation) { + PyErr_SetString(PyExc_RuntimeError, + "BSP tree was modified during iteration (clear() or split_recursive() called)"); + return NULL; + } + } + if (self->index >= self->nodes->size()) { PyErr_SetNone(PyExc_StopIteration); return NULL; diff --git a/src/PyBSP.h b/src/PyBSP.h index 36a599c..430092b 100644 --- a/src/PyBSP.h +++ b/src/PyBSP.h @@ -3,17 +3,23 @@ #include "Python.h" #include #include +#include // Forward declarations class PyBSP; class PyBSPNode; +// Maximum recursion depth to prevent memory exhaustion +// 2^16 = 65536 potential leaf nodes, which is already excessive +constexpr int BSP_MAX_DEPTH = 16; + // Python object structure for BSP tree (root owner) typedef struct { PyObject_HEAD TCOD_bsp_t* root; // libtcod BSP root (owned, will be deleted) int orig_x, orig_y; // Original bounds for clear() int orig_w, orig_h; + uint64_t generation; // Incremented on structural changes (clear, split) } PyBSPObject; // Python object structure for BSPNode (lightweight reference) @@ -21,6 +27,7 @@ typedef struct { PyObject_HEAD TCOD_bsp_t* node; // libtcod BSP node (NOT owned) PyObject* bsp_owner; // Reference to PyBSPObject to prevent dangling + uint64_t generation; // Generation at time of creation (for validity check) } PyBSPNodeObject; // BSP iterator for traverse() @@ -29,6 +36,7 @@ typedef struct { std::vector* nodes; // Pre-collected nodes size_t index; PyObject* bsp_owner; // Reference to PyBSPObject + uint64_t generation; // Generation at iterator creation } PyBSPIterObject; class PyBSP @@ -42,6 +50,8 @@ public: // Properties static PyObject* get_bounds(PyBSPObject* self, void* closure); + static PyObject* get_pos(PyBSPObject* self, void* closure); + static PyObject* get_size(PyBSPObject* self, void* closure); static PyObject* get_root(PyBSPObject* self, void* closure); // Splitting methods (#202) @@ -59,9 +69,14 @@ public: // HeightMap conversion (#206) static PyObject* to_heightmap(PyBSPObject* self, PyObject* args, PyObject* kwds); + // Sequence protocol + static Py_ssize_t len(PyBSPObject* self); + static PyObject* iter(PyBSPObject* self); + // Method and property definitions static PyMethodDef methods[]; static PyGetSetDef getsetters[]; + static PySequenceMethods sequence_methods; }; class PyBSPNode @@ -73,8 +88,13 @@ public: static void dealloc(PyBSPNodeObject* self); static PyObject* repr(PyObject* obj); + // Comparison + static PyObject* richcompare(PyObject* self, PyObject* other, int op); + // Properties (#203) static PyObject* get_bounds(PyBSPNodeObject* self, void* closure); + static PyObject* get_pos(PyBSPNodeObject* self, void* closure); + static PyObject* get_size(PyBSPNodeObject* self, void* closure); static PyObject* get_level(PyBSPNodeObject* self, void* closure); static PyObject* get_is_leaf(PyBSPNodeObject* self, void* closure); static PyObject* get_split_horizontal(PyBSPNodeObject* self, void* closure); @@ -93,6 +113,9 @@ public: // Helper to create a BSPNode from a TCOD_bsp_t* static PyObject* create(TCOD_bsp_t* node, PyObject* bsp_owner); + // Validity check - returns false and sets error if node is stale + static bool checkValid(PyBSPNodeObject* self); + // Method and property definitions static PyMethodDef methods[]; static PyGetSetDef getsetters[]; @@ -116,6 +139,8 @@ public: static PyObject* traversal_enum_class; static PyObject* create_enum_class(PyObject* module); static int from_arg(PyObject* arg, int* out_order); + // Cleanup for module finalization (optional) + static void cleanup(); }; namespace mcrfpydef { @@ -127,23 +152,31 @@ namespace mcrfpydef { .tp_itemsize = 0, .tp_dealloc = (destructor)PyBSP::dealloc, .tp_repr = PyBSP::repr, + .tp_as_sequence = &PyBSP::sequence_methods, .tp_flags = Py_TPFLAGS_DEFAULT, .tp_doc = PyDoc_STR( - "BSP(bounds: tuple[tuple[int, int], tuple[int, int]])\n\n" + "BSP(pos: tuple[int, int], size: tuple[int, int])\n\n" "Binary Space Partitioning tree for procedural dungeon generation.\n\n" "BSP recursively divides a rectangular region into smaller sub-regions, " "creating a tree structure perfect for generating dungeon rooms and corridors.\n\n" "Args:\n" - " bounds: ((x, y), (w, h)) - Position and size of the root region.\n\n" + " pos: (x, y) - Top-left position of the root region.\n" + " size: (w, h) - Width and height of the root region.\n\n" "Properties:\n" - " bounds ((x, y), (w, h)): Read-only. Root node bounds.\n" + " pos (tuple[int, int]): Read-only. Top-left position (x, y).\n" + " size (tuple[int, int]): Read-only. Dimensions (width, height).\n" + " bounds ((pos), (size)): Read-only. Combined position and size.\n" " root (BSPNode): Read-only. Reference to the root node.\n\n" + "Iteration:\n" + " for leaf in bsp: # Iterates over leaf nodes (rooms)\n" + " len(bsp) # Returns number of leaf nodes\n\n" "Example:\n" - " bsp = mcrfpy.BSP(bounds=((0, 0), (80, 50)))\n" + " bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 50))\n" " bsp.split_recursive(depth=4, min_size=(8, 8))\n" - " for leaf in bsp.leaves():\n" - " print(f'Room at {leaf.bounds}')\n" + " for leaf in bsp:\n" + " print(f'Room at {leaf.pos}, size {leaf.size}')\n" ), + .tp_iter = (getiterfunc)PyBSP::iter, .tp_methods = nullptr, // Set in McRFPy_API.cpp .tp_getset = nullptr, // Set in McRFPy_API.cpp .tp_init = (initproc)PyBSP::init, @@ -163,8 +196,13 @@ namespace mcrfpydef { "BSPNode - Lightweight reference to a node in a BSP tree.\n\n" "BSPNode provides read-only access to node properties and navigation.\n" "Nodes are created by BSP methods, not directly instantiated.\n\n" + "WARNING: BSPNode references become invalid after BSP.clear() or\n" + "BSP.split_recursive(). Accessing properties of an invalid node\n" + "raises RuntimeError.\n\n" "Properties:\n" - " bounds ((x, y), (w, h)): Position and size of this node.\n" + " pos (tuple[int, int]): Top-left position (x, y).\n" + " size (tuple[int, int]): Dimensions (width, height).\n" + " bounds ((pos), (size)): Combined position and size.\n" " level (int): Depth in tree (0 for root).\n" " is_leaf (bool): True if this node has no children.\n" " split_horizontal (bool | None): Split orientation, None if leaf.\n" @@ -174,6 +212,7 @@ namespace mcrfpydef { " parent (BSPNode | None): Parent node, or None if root.\n" " sibling (BSPNode | None): Other child of parent, or None.\n" ), + .tp_richcompare = PyBSPNode::richcompare, .tp_methods = nullptr, // Set in McRFPy_API.cpp .tp_getset = nullptr, // Set in McRFPy_API.cpp .tp_init = (initproc)PyBSPNode::init, diff --git a/tests/unit/bsp_simple_test.py b/tests/unit/bsp_simple_test.py index e96497f..242927d 100644 --- a/tests/unit/bsp_simple_test.py +++ b/tests/unit/bsp_simple_test.py @@ -5,7 +5,7 @@ import mcrfpy print("Step 1: Import complete") print("Step 2: Creating BSP...") -bsp = mcrfpy.BSP(bounds=((0, 0), (100, 80))) +bsp = mcrfpy.BSP(pos=(0, 0), size=(100, 80)) print("Step 2: BSP created:", bsp) print("Step 3: Getting bounds...") diff --git a/tests/unit/bsp_test.py b/tests/unit/bsp_test.py index 96bd899..da51ee6 100644 --- a/tests/unit/bsp_test.py +++ b/tests/unit/bsp_test.py @@ -11,14 +11,18 @@ import sys import mcrfpy def test_bsp_construction(): - """Test BSP construction with bounds.""" + """Test BSP construction with pos/size.""" print("Testing BSP construction...") - # Basic construction - bsp = mcrfpy.BSP(bounds=((0, 0), (100, 80))) + # Basic construction with pos/size kwargs + bsp = mcrfpy.BSP(pos=(0, 0), size=(100, 80)) assert bsp is not None, "BSP should be created" - # Check bounds property + # Check pos and size properties + assert bsp.pos == (0, 0), f"pos should be (0, 0), got {bsp.pos}" + assert bsp.size == (100, 80), f"size should be (100, 80), got {bsp.size}" + + # Check bounds property (combines pos and size) bounds = bsp.bounds assert bounds == ((0, 0), (100, 80)), f"Bounds should be ((0, 0), (100, 80)), got {bounds}" @@ -28,7 +32,7 @@ def test_bsp_construction(): assert root.bounds == ((0, 0), (100, 80)), f"Root bounds mismatch" # Construction with offset - bsp2 = mcrfpy.BSP(bounds=((10, 20), (50, 40))) + bsp2 = mcrfpy.BSP(pos=(10, 20), size=(50, 40)) assert bsp2.bounds == ((10, 20), (50, 40)), "Offset bounds not preserved" print(" BSP construction: PASS") @@ -37,7 +41,7 @@ def test_bsp_split_once(): """Test single split operation (#202).""" print("Testing BSP split_once...") - bsp = mcrfpy.BSP(bounds=((0, 0), (100, 80))) + bsp = mcrfpy.BSP(pos=(0, 0), size=(100, 80)) # Before split, root should be a leaf assert bsp.root.is_leaf, "Root should be leaf before split" @@ -66,7 +70,7 @@ def test_bsp_split_recursive(): """Test recursive splitting (#202).""" print("Testing BSP split_recursive...") - bsp = mcrfpy.BSP(bounds=((0, 0), (80, 60))) + bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60)) # Recursive split with seed for reproducibility result = bsp.split_recursive(depth=3, min_size=(8, 8), max_ratio=1.5, seed=42) @@ -91,7 +95,7 @@ def test_bsp_clear(): """Test clear operation (#202).""" print("Testing BSP clear...") - bsp = mcrfpy.BSP(bounds=((0, 0), (100, 80))) + bsp = mcrfpy.BSP(pos=(0, 0), size=(100, 80)) bsp.split_recursive(depth=4, min_size=(8, 8), seed=42) # Should have multiple leaves @@ -116,7 +120,7 @@ def test_bspnode_properties(): """Test BSPNode properties (#203).""" print("Testing BSPNode properties...") - bsp = mcrfpy.BSP(bounds=((0, 0), (100, 80))) + bsp = mcrfpy.BSP(pos=(0, 0), size=(100, 80)) bsp.split_recursive(depth=3, min_size=(8, 8), seed=42) root = bsp.root @@ -155,7 +159,7 @@ def test_bspnode_navigation(): """Test BSPNode navigation (#203).""" print("Testing BSPNode navigation...") - bsp = mcrfpy.BSP(bounds=((0, 0), (100, 80))) + bsp = mcrfpy.BSP(pos=(0, 0), size=(100, 80)) bsp.split_once(horizontal=True, position=40) root = bsp.root @@ -182,7 +186,7 @@ def test_bspnode_contains(): """Test BSPNode contains method (#203).""" print("Testing BSPNode contains...") - bsp = mcrfpy.BSP(bounds=((10, 20), (50, 40))) # x: 10-60, y: 20-60 + bsp = mcrfpy.BSP(pos=(10, 20), size=(50, 40)) # x: 10-60, y: 20-60 root = bsp.root @@ -203,7 +207,7 @@ def test_bsp_leaves_iteration(): """Test leaves iteration (#204).""" print("Testing BSP leaves iteration...") - bsp = mcrfpy.BSP(bounds=((0, 0), (80, 60))) + bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60)) bsp.split_recursive(depth=3, min_size=(8, 8), seed=42) # Iterate leaves @@ -224,7 +228,7 @@ def test_bsp_traverse(): """Test traverse with different orders (#204).""" print("Testing BSP traverse...") - bsp = mcrfpy.BSP(bounds=((0, 0), (80, 60))) + bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60)) bsp.split_recursive(depth=2, min_size=(8, 8), seed=42) # Test all traversal orders @@ -261,7 +265,7 @@ def test_bsp_find(): """Test find method (#205).""" print("Testing BSP find...") - bsp = mcrfpy.BSP(bounds=((0, 0), (80, 60))) + bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60)) bsp.split_recursive(depth=3, min_size=(8, 8), seed=42) # Find a point inside bounds @@ -285,7 +289,7 @@ def test_bsp_to_heightmap(): """Test to_heightmap conversion (#206).""" print("Testing BSP to_heightmap...") - bsp = mcrfpy.BSP(bounds=((0, 0), (50, 40))) + bsp = mcrfpy.BSP(pos=(0, 0), size=(50, 40)) bsp.split_recursive(depth=2, min_size=(8, 8), seed=42) # Basic conversion @@ -346,7 +350,7 @@ def test_bsp_chaining(): """Test method chaining.""" print("Testing BSP method chaining...") - bsp = mcrfpy.BSP(bounds=((0, 0), (80, 60))) + bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60)) # Chain multiple operations result = bsp.split_recursive(depth=2, min_size=(8, 8), seed=42).clear().split_once(True, 30) @@ -358,7 +362,7 @@ def test_bsp_repr(): """Test repr output.""" print("Testing BSP repr...") - bsp = mcrfpy.BSP(bounds=((0, 0), (80, 60))) + bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60)) repr_str = repr(bsp) assert "BSP" in repr_str, f"repr should contain BSP: {repr_str}" assert "80" in repr_str and "60" in repr_str, f"repr should contain size: {repr_str}" @@ -373,6 +377,159 @@ def test_bsp_repr(): print(" BSP repr: PASS") +def test_bsp_stale_node_detection(): + """Test that stale nodes are detected after clear()/split_recursive() (#review).""" + print("Testing BSP stale node detection...") + + bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60)) + bsp.split_recursive(depth=2, min_size=(8, 8), seed=42) + + # Save reference to a node + old_root = bsp.root + old_leaf = list(bsp.leaves())[0] + + # Clear invalidates all nodes + bsp.clear() + + # Accessing stale node should raise RuntimeError + try: + _ = old_root.bounds + assert False, "Accessing stale root bounds should raise RuntimeError" + except RuntimeError as e: + assert "stale" in str(e).lower() or "invalid" in str(e).lower(), \ + f"Error should mention staleness: {e}" + + try: + _ = old_leaf.is_leaf + assert False, "Accessing stale leaf should raise RuntimeError" + except RuntimeError as e: + pass # Expected + + # split_recursive also invalidates (rebuilds tree) + bsp2 = mcrfpy.BSP(pos=(0, 0), size=(80, 60)) + bsp2.split_recursive(depth=2, min_size=(8, 8), seed=42) + saved_node = bsp2.root + bsp2.split_recursive(depth=3, min_size=(8, 8), seed=99) + + try: + _ = saved_node.bounds + assert False, "Accessing node after split_recursive should raise RuntimeError" + except RuntimeError: + pass # Expected + + print(" BSP stale node detection: PASS") + +def test_bsp_grid_max_validation(): + """Test GRID_MAX validation (#review).""" + print("Testing BSP GRID_MAX validation...") + + # Should succeed with valid size + bsp = mcrfpy.BSP(pos=(0, 0), size=(1000, 1000)) + assert bsp is not None + + # Should fail with size exceeding GRID_MAX (8192) + try: + bsp_too_big = mcrfpy.BSP(pos=(0, 0), size=(10000, 100)) + assert False, "Should raise ValueError for size > GRID_MAX" + except ValueError as e: + assert "8192" in str(e) or "exceed" in str(e).lower(), f"Error should mention limit: {e}" + + try: + bsp_too_big = mcrfpy.BSP(pos=(0, 0), size=(100, 10000)) + assert False, "Should raise ValueError for height > GRID_MAX" + except ValueError as e: + pass # Expected + + print(" BSP GRID_MAX validation: PASS") + +def test_bsp_depth_cap(): + """Test depth is capped at 16 (#review).""" + print("Testing BSP depth cap...") + + bsp = mcrfpy.BSP(pos=(0, 0), size=(1000, 1000)) + + # Should cap depth at 16 or raise ValueError + try: + bsp.split_recursive(depth=20, min_size=(1, 1), seed=42) + # If it succeeds, verify reasonable number of leaves (not 2^20) + leaves = list(bsp.leaves()) + assert len(leaves) <= 2**16, f"Too many leaves: {len(leaves)}" + except ValueError as e: + # It's also acceptable to reject excessive depth + assert "16" in str(e) or "depth" in str(e).lower(), f"Error should mention depth limit: {e}" + + print(" BSP depth cap: PASS") + +def test_bsp_len(): + """Test __len__ returns leaf count (#review).""" + print("Testing BSP __len__...") + + bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60)) + assert len(bsp) == 1, f"Unsplit BSP should have 1 leaf, got {len(bsp)}" + + bsp.split_once(horizontal=True, position=30) + assert len(bsp) == 2, f"After one split should have 2 leaves, got {len(bsp)}" + + bsp.clear() + bsp.split_recursive(depth=3, min_size=(8, 8), seed=42) + expected = len(list(bsp.leaves())) + assert len(bsp) == expected, f"len() mismatch: {len(bsp)} != {expected}" + + print(" BSP __len__: PASS") + +def test_bsp_iter(): + """Test __iter__ as shorthand for leaves() (#review).""" + print("Testing BSP __iter__...") + + bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60)) + bsp.split_recursive(depth=2, min_size=(8, 8), seed=42) + + # Direct iteration should yield same results as leaves() + iter_list = list(bsp) + leaves_list = list(bsp.leaves()) + + assert len(iter_list) == len(leaves_list), \ + f"Iterator count mismatch: {len(iter_list)} != {len(leaves_list)}" + + # All items should be leaves + for node in bsp: + assert node.is_leaf, f"Iterator should yield leaves: {node}" + + # Can iterate multiple times + count1 = sum(1 for _ in bsp) + count2 = sum(1 for _ in bsp) + assert count1 == count2, "Should be able to iterate multiple times" + + print(" BSP __iter__: PASS") + +def test_bspnode_equality(): + """Test BSPNode __eq__ comparison (#review).""" + print("Testing BSPNode equality...") + + bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60)) + bsp.split_recursive(depth=2, min_size=(8, 8), seed=42) + + # Same node should be equal + root1 = bsp.root + root2 = bsp.root + assert root1 == root2, "Same node should be equal" + + # Different nodes should not be equal + leaves = list(bsp.leaves()) + assert len(leaves) >= 2, "Need at least 2 leaves for comparison" + assert leaves[0] != leaves[1], "Different nodes should not be equal" + + # Parent vs child should not be equal + root = bsp.root + left = root.left + assert root != left, "Parent and child should not be equal" + + # Not equal to non-BSPNode + assert not (root == "not a node"), "BSPNode should not equal string" + assert not (root == 42), "BSPNode should not equal int" + + print(" BSPNode equality: PASS") + def run_all_tests(): """Run all BSP tests.""" print("\n=== BSP Unit Tests ===\n") @@ -392,6 +549,12 @@ def run_all_tests(): test_traversal_enum() test_bsp_chaining() test_bsp_repr() + test_bsp_stale_node_detection() + test_bsp_grid_max_validation() + test_bsp_depth_cap() + test_bsp_len() + test_bsp_iter() + test_bspnode_equality() print("\n=== ALL BSP TESTS PASSED ===\n") sys.exit(0)