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

Grid code quality improvements

Browse source 
* Grid [x, y] subscript - convenience for `.at()`
* Extract UIEntityCollection - cleanup of UIGrid.cpp
* Thread-safe type cache - PyTypeCache
* Exception-safe extend() - validate before modify
This commit is contained in:
John McCardle 2026-01-10 08:37:31 -05:00
commit d6ef29f3cd
7 changed files with 1492 additions and 1097 deletions
Download patch file Download diff file Expand all files Collapse all files

13
src/McRFPy_API.cpp
View file

@ -3,6 +3,7 @@
#include "McRFPy_Automation.h"
#include "McRFPy_Libtcod.h"
#include "McRFPy_Doc.h"
#include "PyTypeCache.h" // Thread-safe cached Python types
#include "platform.h"
#include "PyAnimation.h"
#include "PyDrawable.h"
@ -549,12 +550,20 @@ PyObject* PyInit_mcrfpy()
PyObject* libtcod_module = McRFPy_Libtcod::init_libtcod_module();
if (libtcod_module != NULL) {
PyModule_AddObject(m, "libtcod", libtcod_module);
// Also add to sys.modules for proper import behavior
PyObject* sys_modules = PyImport_GetModuleDict();
PyDict_SetItemString(sys_modules, "mcrfpy.libtcod", libtcod_module);
}
// Initialize PyTypeCache for thread-safe type lookups
// This must be done after all types are added to the module
if (!PyTypeCache::initialize(m)) {
// Failed to initialize type cache - this is a critical error
// Error message already set by PyTypeCache::initialize
return NULL;
}
//McRFPy_API::mcrf_module = m;
return m;
}

135
src/PyTypeCache.cpp Normal file
View file

@ -0,0 +1,135 @@
// PyTypeCache.cpp - Thread-safe Python type caching implementation
#include "PyTypeCache.h"
// Static member definitions
std::atomic<PyTypeObject*> PyTypeCache::entity_type{nullptr};
std::atomic<PyTypeObject*> PyTypeCache::grid_type{nullptr};
std::atomic<PyTypeObject*> PyTypeCache::frame_type{nullptr};
std::atomic<PyTypeObject*> PyTypeCache::caption_type{nullptr};
std::atomic<PyTypeObject*> PyTypeCache::sprite_type{nullptr};
std::atomic<PyTypeObject*> PyTypeCache::texture_type{nullptr};
std::atomic<PyTypeObject*> PyTypeCache::color_type{nullptr};
std::atomic<PyTypeObject*> PyTypeCache::vector_type{nullptr};
std::atomic<PyTypeObject*> PyTypeCache::font_type{nullptr};
std::atomic<bool> PyTypeCache::initialized{false};
std::mutex PyTypeCache::init_mutex;
PyTypeObject* PyTypeCache::cacheType(PyObject* module, const char* name,
std::atomic<PyTypeObject*>& cache) {
PyObject* type_obj = PyObject_GetAttrString(module, name);
if (!type_obj) {
PyErr_Format(PyExc_RuntimeError,
"PyTypeCache: Failed to get type '%s' from module", name);
return nullptr;
}
if (!PyType_Check(type_obj)) {
Py_DECREF(type_obj);
PyErr_Format(PyExc_TypeError,
"PyTypeCache: '%s' is not a type object", name);
return nullptr;
}
// Store in cache - we keep the reference permanently
// Using memory_order_release ensures the pointer is visible to other threads
// after they see initialized=true
cache.store((PyTypeObject*)type_obj, std::memory_order_release);
return (PyTypeObject*)type_obj;
}
bool PyTypeCache::initialize(PyObject* module) {
std::lock_guard<std::mutex> lock(init_mutex);
// Double-check pattern - might have been initialized while waiting for lock
if (initialized.load(std::memory_order_acquire)) {
return true;
}
// Cache all types
if (!cacheType(module, "Entity", entity_type)) return false;
if (!cacheType(module, "Grid", grid_type)) return false;
if (!cacheType(module, "Frame", frame_type)) return false;
if (!cacheType(module, "Caption", caption_type)) return false;
if (!cacheType(module, "Sprite", sprite_type)) return false;
if (!cacheType(module, "Texture", texture_type)) return false;
if (!cacheType(module, "Color", color_type)) return false;
if (!cacheType(module, "Vector", vector_type)) return false;
if (!cacheType(module, "Font", font_type)) return false;
// Mark as initialized - release ensures all stores above are visible
initialized.store(true, std::memory_order_release);
return true;
}
void PyTypeCache::finalize() {
std::lock_guard<std::mutex> lock(init_mutex);
if (!initialized.load(std::memory_order_acquire)) {
return;
}
// Release all cached references
auto release = [](std::atomic<PyTypeObject*>& cache) {
PyTypeObject* type = cache.exchange(nullptr, std::memory_order_acq_rel);
if (type) {
Py_DECREF(type);
}
};
release(entity_type);
release(grid_type);
release(frame_type);
release(caption_type);
release(sprite_type);
release(texture_type);
release(color_type);
release(vector_type);
release(font_type);
initialized.store(false, std::memory_order_release);
}
bool PyTypeCache::isInitialized() {
return initialized.load(std::memory_order_acquire);
}
// Type accessors - lock-free reads after initialization
// Using memory_order_acquire ensures we see the pointer stored during init
PyTypeObject* PyTypeCache::Entity() {
return entity_type.load(std::memory_order_acquire);
}
PyTypeObject* PyTypeCache::Grid() {
return grid_type.load(std::memory_order_acquire);
}
PyTypeObject* PyTypeCache::Frame() {
return frame_type.load(std::memory_order_acquire);
}
PyTypeObject* PyTypeCache::Caption() {
return caption_type.load(std::memory_order_acquire);
}
PyTypeObject* PyTypeCache::Sprite() {
return sprite_type.load(std::memory_order_acquire);
}
PyTypeObject* PyTypeCache::Texture() {
return texture_type.load(std::memory_order_acquire);
}
PyTypeObject* PyTypeCache::Color() {
return color_type.load(std::memory_order_acquire);
}
PyTypeObject* PyTypeCache::Vector() {
return vector_type.load(std::memory_order_acquire);
}
PyTypeObject* PyTypeCache::Font() {
return font_type.load(std::memory_order_acquire);
}

64
src/PyTypeCache.h Normal file
View file

@ -0,0 +1,64 @@
#pragma once
// PyTypeCache.h - Thread-safe caching of Python type objects
//
// This module provides a centralized, thread-safe way to cache Python type
// references. It eliminates the refcount leaks from repeated
// PyObject_GetAttrString calls and is compatible with free-threading (PEP 703).
//
// Usage:
// PyTypeObject* entity_type = PyTypeCache::Entity();
// if (!entity_type) return NULL; // Error already set
//
// The cache is populated during module initialization and the types are
// held for the lifetime of the interpreter.
#include "Python.h"
#include <mutex>
#include <atomic>
class PyTypeCache {
public:
// Initialize the cache - call once during module init after types are ready
// Returns false and sets Python error on failure
static bool initialize(PyObject* module);
// Finalize - release references (call during module cleanup if needed)
static void finalize();
// Type accessors - return borrowed references (no DECREF needed)
// These are thread-safe and lock-free after initialization
static PyTypeObject* Entity();
static PyTypeObject* Grid();
static PyTypeObject* Frame();
static PyTypeObject* Caption();
static PyTypeObject* Sprite();
static PyTypeObject* Texture();
static PyTypeObject* Color();
static PyTypeObject* Vector();
static PyTypeObject* Font();
// Check if initialized
static bool isInitialized();
private:
// Cached type pointers - atomic for thread-safe reads
static std::atomic<PyTypeObject*> entity_type;
static std::atomic<PyTypeObject*> grid_type;
static std::atomic<PyTypeObject*> frame_type;
static std::atomic<PyTypeObject*> caption_type;
static std::atomic<PyTypeObject*> sprite_type;
static std::atomic<PyTypeObject*> texture_type;
static std::atomic<PyTypeObject*> color_type;
static std::atomic<PyTypeObject*> vector_type;
static std::atomic<PyTypeObject*> font_type;
// Initialization flag
static std::atomic<bool> initialized;
// Mutex for initialization (only used during init, not for reads)
static std::mutex init_mutex;
// Helper to fetch and cache a type
static PyTypeObject* cacheType(PyObject* module, const char* name,
std::atomic<PyTypeObject*>& cache);
};

1078
src/UIEntityCollection.cpp Normal file
View file

File diff suppressed because it is too large Load diff

145
src/UIEntityCollection.h Normal file
View file

@ -0,0 +1,145 @@
#pragma once
// UIEntityCollection.h - Collection type for managing entities on a grid
//
// Extracted from UIGrid.cpp as part of code organization cleanup.
// This is a Python sequence/mapping type that wraps std::list<std::shared_ptr<UIEntity>>
// with grid-aware semantics (entities can only belong to one grid at a time).
#include "Common.h"
#include "Python.h"
#include "structmember.h"
#include <list>
#include <memory>
// Forward declarations
class UIEntity;
class UIGrid;
// Python object for EntityCollection
typedef struct {
PyObject_HEAD
std::shared_ptr<std::list<std::shared_ptr<UIEntity>>> data;
std::shared_ptr<UIGrid> grid;
} PyUIEntityCollectionObject;
// Python object for EntityCollection iterator
typedef struct {
PyObject_HEAD
std::shared_ptr<std::list<std::shared_ptr<UIEntity>>> data;
std::list<std::shared_ptr<UIEntity>>::iterator current; // Actual list iterator - O(1) increment
std::list<std::shared_ptr<UIEntity>>::iterator end; // End iterator for bounds check
int start_size; // For detecting modification during iteration
} PyUIEntityCollectionIterObject;
// UIEntityCollection - Python collection wrapper
class UIEntityCollection {
public:
// Python sequence protocol
static PySequenceMethods sqmethods;
static PyMappingMethods mpmethods;
// Collection methods
static PyObject* append(PyUIEntityCollectionObject* self, PyObject* o);
static PyObject* extend(PyUIEntityCollectionObject* self, PyObject* o);
static PyObject* remove(PyUIEntityCollectionObject* self, PyObject* o);
static PyObject* pop(PyUIEntityCollectionObject* self, PyObject* args);
static PyObject* insert(PyUIEntityCollectionObject* self, PyObject* args);
static PyObject* index_method(PyUIEntityCollectionObject* self, PyObject* value);
static PyObject* count(PyUIEntityCollectionObject* self, PyObject* value);
static PyObject* find(PyUIEntityCollectionObject* self, PyObject* args, PyObject* kwds);
static PyMethodDef methods[];
// Python type slots
static PyObject* repr(PyUIEntityCollectionObject* self);
static int init(PyUIEntityCollectionObject* self, PyObject* args, PyObject* kwds);
static PyObject* iter(PyUIEntityCollectionObject* self);
// Sequence methods
static Py_ssize_t len(PyUIEntityCollectionObject* self);
static PyObject* getitem(PyUIEntityCollectionObject* self, Py_ssize_t index);
static int setitem(PyUIEntityCollectionObject* self, Py_ssize_t index, PyObject* value);
static int contains(PyUIEntityCollectionObject* self, PyObject* value);
static PyObject* concat(PyUIEntityCollectionObject* self, PyObject* other);
static PyObject* inplace_concat(PyUIEntityCollectionObject* self, PyObject* other);
// Mapping methods (for slice support)
static PyObject* subscript(PyUIEntityCollectionObject* self, PyObject* key);
static int ass_subscript(PyUIEntityCollectionObject* self, PyObject* key, PyObject* value);
};
// UIEntityCollectionIter - Iterator for EntityCollection
class UIEntityCollectionIter {
public:
static int init(PyUIEntityCollectionIterObject* self, PyObject* args, PyObject* kwds);
static PyObject* next(PyUIEntityCollectionIterObject* self);
static PyObject* repr(PyUIEntityCollectionIterObject* self);
};
// Python type objects - defined in mcrfpydef namespace
namespace mcrfpydef {
// Iterator type
inline PyTypeObject PyUIEntityCollectionIterType = {
.ob_base = {.ob_base = {.ob_refcnt = 1, .ob_type = NULL}, .ob_size = 0},
.tp_name = "mcrfpy.UIEntityCollectionIter",
.tp_basicsize = sizeof(PyUIEntityCollectionIterObject),
.tp_itemsize = 0,
.tp_dealloc = (destructor)[](PyObject* self)
{
PyUIEntityCollectionIterObject* obj = (PyUIEntityCollectionIterObject*)self;
obj->data.reset();
Py_TYPE(self)->tp_free(self);
},
.tp_repr = (reprfunc)UIEntityCollectionIter::repr,
.tp_flags = Py_TPFLAGS_DEFAULT,
.tp_doc = PyDoc_STR("Iterator for a collection of Entity objects"),
.tp_iter = PyObject_SelfIter,
.tp_iternext = (iternextfunc)UIEntityCollectionIter::next,
.tp_init = (initproc)UIEntityCollectionIter::init,
.tp_alloc = PyType_GenericAlloc,
.tp_new = [](PyTypeObject* type, PyObject* args, PyObject* kwds) -> PyObject*
{
PyErr_SetString(PyExc_TypeError, "UIEntityCollectionIter cannot be instantiated directly");
return NULL;
}
};
// Collection type
inline PyTypeObject PyUIEntityCollectionType = {
.ob_base = {.ob_base = {.ob_refcnt = 1, .ob_type = NULL}, .ob_size = 0},
.tp_name = "mcrfpy.EntityCollection",
.tp_basicsize = sizeof(PyUIEntityCollectionObject),
.tp_itemsize = 0,
.tp_dealloc = (destructor)[](PyObject* self)
{
PyUIEntityCollectionObject* obj = (PyUIEntityCollectionObject*)self;
obj->data.reset();
obj->grid.reset();
Py_TYPE(self)->tp_free(self);
},
.tp_repr = (reprfunc)UIEntityCollection::repr,
.tp_as_sequence = &UIEntityCollection::sqmethods,
.tp_as_mapping = &UIEntityCollection::mpmethods,
.tp_flags = Py_TPFLAGS_DEFAULT,
.tp_doc = PyDoc_STR("Iterable, indexable collection of Entity objects.\n\n"
"EntityCollection manages entities that belong to a Grid. "
"Entities can only belong to one grid at a time - adding an entity "
"to a new grid automatically removes it from its previous grid.\n\n"
"Supports list-like operations: indexing, slicing, append, extend, "
"remove, pop, insert, index, count, and find.\n\n"
"Example:\n"
" grid.entities.append(entity)\n"
" player = grid.entities.find(name='player')\n"
" for entity in grid.entities:\n"
" print(entity.pos)"),
.tp_iter = (getiterfunc)UIEntityCollection::iter,
.tp_methods = UIEntityCollection::methods,
.tp_init = (initproc)UIEntityCollection::init,
.tp_new = [](PyTypeObject* type, PyObject* args, PyObject* kwds) -> PyObject*
{
PyErr_SetString(PyExc_TypeError, "EntityCollection cannot be instantiated: a C++ data source is required.");
return NULL;
}
};
} // namespace mcrfpydef

1040
src/UIGrid.cpp
View file

File diff suppressed because it is too large Load diff

114
src/UIGrid.h
View file

@ -23,6 +23,7 @@
#include "GridLayers.h"
#include "GridChunk.h"
#include "SpatialHash.h"
#include "UIEntityCollection.h" // EntityCollection types (extracted from UIGrid)
class UIGrid: public UIDrawable
{
@ -180,6 +181,8 @@ public:
static PyMethodDef methods[];
static PyGetSetDef getsetters[];
static PyMappingMethods mpmethods; // For grid[x, y] subscript access
static PyObject* subscript(PyUIGridObject* self, PyObject* key); // __getitem__
static PyObject* get_entities(PyUIGridObject* self, void* closure);
static PyObject* get_children(PyUIGridObject* self, void* closure);
static PyObject* repr(PyUIGridObject* self);
@ -200,54 +203,7 @@ public:
static PyObject* py_layer(PyUIGridObject* self, PyObject* args);
};
typedef struct {
PyObject_HEAD
std::shared_ptr<std::list<std::shared_ptr<UIEntity>>> data;
std::shared_ptr<UIGrid> grid;
} PyUIEntityCollectionObject;
class UIEntityCollection {
public:
static PySequenceMethods sqmethods;
static PyMappingMethods mpmethods;
static PyObject* append(PyUIEntityCollectionObject* self, PyObject* o);
static PyObject* extend(PyUIEntityCollectionObject* self, PyObject* o);
static PyObject* remove(PyUIEntityCollectionObject* self, PyObject* o);
static PyObject* pop(PyUIEntityCollectionObject* self, PyObject* args);
static PyObject* insert(PyUIEntityCollectionObject* self, PyObject* args);
static PyObject* index_method(PyUIEntityCollectionObject* self, PyObject* value);
static PyObject* count(PyUIEntityCollectionObject* self, PyObject* value);
static PyObject* find(PyUIEntityCollectionObject* self, PyObject* args, PyObject* kwds);
static PyMethodDef methods[];
static PyObject* repr(PyUIEntityCollectionObject* self);
static int init(PyUIEntityCollectionObject* self, PyObject* args, PyObject* kwds);
static PyObject* iter(PyUIEntityCollectionObject* self);
static Py_ssize_t len(PyUIEntityCollectionObject* self);
static PyObject* getitem(PyUIEntityCollectionObject* self, Py_ssize_t index);
static int setitem(PyUIEntityCollectionObject* self, Py_ssize_t index, PyObject* value);
static int contains(PyUIEntityCollectionObject* self, PyObject* value);
static PyObject* concat(PyUIEntityCollectionObject* self, PyObject* other);
static PyObject* inplace_concat(PyUIEntityCollectionObject* self, PyObject* other);
static PyObject* subscript(PyUIEntityCollectionObject* self, PyObject* key);
static int ass_subscript(PyUIEntityCollectionObject* self, PyObject* key, PyObject* value);
};
typedef struct {
PyObject_HEAD
std::shared_ptr<std::list<std::shared_ptr<UIEntity>>> data;
std::list<std::shared_ptr<UIEntity>>::iterator current; // Actual list iterator - O(1) increment
std::list<std::shared_ptr<UIEntity>>::iterator end; // End iterator for bounds check
int start_size; // For detecting modification during iteration
} PyUIEntityCollectionIterObject;
class UIEntityCollectionIter {
public:
static int init(PyUIEntityCollectionIterObject* self, PyObject* args, PyObject* kwds);
static PyObject* next(PyUIEntityCollectionIterObject* self);
static PyObject* repr(PyUIEntityCollectionIterObject* self);
static PyObject* getitem(PyUIEntityCollectionObject* self, Py_ssize_t index);
};
// UIEntityCollection types are now in UIEntityCollection.h
// Forward declaration of methods array
extern PyMethodDef UIGrid_all_methods[];
@ -268,11 +224,8 @@ namespace mcrfpydef {
obj->data.reset();
Py_TYPE(self)->tp_free(self);
},
//TODO - PyUIGrid REPR def:
.tp_repr = (reprfunc)UIGrid::repr,
//.tp_hash = NULL,
//.tp_iter
//.tp_iternext
.tp_as_mapping = &UIGrid::mpmethods, // Enable grid[x, y] subscript access
.tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,
.tp_doc = PyDoc_STR("Grid(pos=None, size=None, grid_size=None, texture=None, **kwargs)\n\n"
"A grid-based UI element for tile-based rendering and entity management.\n\n"
@ -330,61 +283,6 @@ namespace mcrfpydef {
}
};
// #189 - Use inline instead of static to ensure single instance across translation units
inline PyTypeObject PyUIEntityCollectionIterType = {
.ob_base = {.ob_base = {.ob_refcnt = 1, .ob_type = NULL}, .ob_size = 0},
.tp_name = "mcrfpy.UIEntityCollectionIter",
.tp_basicsize = sizeof(PyUIEntityCollectionIterObject),
.tp_itemsize = 0,
.tp_dealloc = (destructor)[](PyObject* self)
{
PyUIEntityCollectionIterObject* obj = (PyUIEntityCollectionIterObject*)self;
obj->data.reset();
Py_TYPE(self)->tp_free(self);
},
.tp_repr = (reprfunc)UIEntityCollectionIter::repr,
.tp_flags = Py_TPFLAGS_DEFAULT,
.tp_doc = PyDoc_STR("Iterator for a collection of UI objects"),
.tp_iter = PyObject_SelfIter,
.tp_iternext = (iternextfunc)UIEntityCollectionIter::next,
//.tp_getset = UIEntityCollection::getset,
.tp_init = (initproc)UIEntityCollectionIter::init, // just raise an exception
.tp_alloc = PyType_GenericAlloc,
.tp_new = [](PyTypeObject* type, PyObject* args, PyObject* kwds) -> PyObject*
{
PyErr_SetString(PyExc_TypeError, "UICollection cannot be instantiated: a C++ data source is required.");
return NULL;
}
};
// #189 - Use inline instead of static to ensure single instance across translation units
inline PyTypeObject PyUIEntityCollectionType = {
.ob_base = {.ob_base = {.ob_refcnt = 1, .ob_type = NULL}, .ob_size = 0},
.tp_name = "mcrfpy.EntityCollection",
.tp_basicsize = sizeof(PyUIEntityCollectionObject),
.tp_itemsize = 0,
.tp_dealloc = (destructor)[](PyObject* self)
{
PyUIEntityCollectionObject* obj = (PyUIEntityCollectionObject*)self;
obj->data.reset();
Py_TYPE(self)->tp_free(self);
},
.tp_repr = (reprfunc)UIEntityCollection::repr,
.tp_as_sequence = &UIEntityCollection::sqmethods,
.tp_as_mapping = &UIEntityCollection::mpmethods,
.tp_flags = Py_TPFLAGS_DEFAULT,
.tp_doc = PyDoc_STR("Iterable, indexable collection of Entities"),
.tp_iter = (getiterfunc)UIEntityCollection::iter,
.tp_methods = UIEntityCollection::methods, // append, remove
//.tp_getset = UIEntityCollection::getset,
.tp_init = (initproc)UIEntityCollection::init, // just raise an exception
.tp_new = [](PyTypeObject* type, PyObject* args, PyObject* kwds) -> PyObject*
{
// Does PyUIEntityCollectionType need __new__ if it's not supposed to be instantiable by the user?
// Should I just raise an exception? Or is the uninitialized shared_ptr enough of a blocker?
PyErr_SetString(PyExc_TypeError, "EntityCollection cannot be instantiated: a C++ data source is required.");
return NULL;
}
};
// EntityCollection types moved to UIEntityCollection.h
}