From 257e52327b363956355c1823d133bfbe02f641b2 Mon Sep 17 00:00:00 2001 From: John McCardle Date: Mon, 19 Jan 2026 23:37:49 -0500 Subject: [PATCH] Fix #219: Add threading support with mcrfpy.lock() context manager Enables background Python threads to safely modify UI objects by synchronizing with the render loop at frame boundaries. Implementation: - FrameLock class provides mutex/condvar synchronization - GIL released during window.display() allowing background threads to run - Safe window opens between frames for synchronized UI updates - mcrfpy.lock() context manager blocks until safe window, then executes - Main thread detection: lock() is a no-op when called from callbacks or script initialization (already synchronized) Usage: import threading import mcrfpy def background_worker(): with mcrfpy.lock(): # Blocks until safe player.x = new_x # Safe to modify UI threading.Thread(target=background_worker).start() The lock works transparently from any context - background threads get actual synchronization, main thread calls (callbacks, init) get no-op. Co-Authored-By: Claude Opus 4.5 --- src/GameEngine.cpp | 60 +++++++++++++++++++++++++++ src/GameEngine.h | 65 +++++++++++++++++++++++++++++ src/McRFPy_API.cpp | 20 ++++++++- src/PyLock.cpp | 100 +++++++++++++++++++++++++++++++++++++++++++++ src/PyLock.h | 25 ++++++++++++ 5 files changed, 269 insertions(+), 1 deletion(-) create mode 100644 src/PyLock.cpp create mode 100644 src/PyLock.h diff --git a/src/GameEngine.cpp b/src/GameEngine.cpp index 0427ab7..ea7df09 100644 --- a/src/GameEngine.cpp +++ b/src/GameEngine.cpp @@ -10,6 +10,48 @@ #include "imgui.h" #include "imgui-SFML.h" #include +#include + +// #219 - FrameLock implementation for thread-safe UI updates + +void FrameLock::acquire() { + waiting++; + std::unique_lock lock(mtx); + + // Release GIL while waiting for safe window + Py_BEGIN_ALLOW_THREADS + cv.wait(lock, [this]{ return safe_window; }); + Py_END_ALLOW_THREADS + + waiting--; + active++; +} + +void FrameLock::release() { + std::unique_lock lock(mtx); + active--; + if (active == 0) { + cv.notify_all(); // Wake up closeWindow() if it's waiting + } +} + +void FrameLock::openWindow() { + std::lock_guard lock(mtx); + safe_window = true; + cv.notify_all(); // Wake up all waiting threads +} + +void FrameLock::closeWindow() { + std::unique_lock lock(mtx); + // First wait for all waiting threads to have entered the critical section + // (or confirm none were waiting). This prevents the race where we set + // safe_window=false before a waiting thread can check the condition. + cv.wait(lock, [this]{ return waiting == 0; }); + // Then wait for all active threads to finish their critical sections + cv.wait(lock, [this]{ return active == 0; }); + // Now safe to close the window + safe_window = false; +} GameEngine::GameEngine() : GameEngine(McRogueFaceConfig{}) { @@ -18,6 +60,9 @@ GameEngine::GameEngine() : GameEngine(McRogueFaceConfig{}) GameEngine::GameEngine(const McRogueFaceConfig& cfg) : config(cfg), headless(cfg.headless) { + // #219 - Store main thread ID for lock() thread detection + main_thread_id = std::this_thread::get_id(); + Resources::font.loadFromFile("./assets/JetbrainsMono.ttf"); Resources::game = this; window_title = "McRogueFace Engine"; @@ -307,15 +352,30 @@ void GameEngine::run() metrics.workTime = clock.getElapsedTime().asSeconds() * 1000.0f; // Display the frame + // #219 - Release GIL during display() to allow background threads to run if (headless) { + Py_BEGIN_ALLOW_THREADS headless_renderer->display(); + Py_END_ALLOW_THREADS // Take screenshot if requested if (config.take_screenshot) { headless_renderer->saveScreenshot(config.screenshot_path.empty() ? "screenshot.png" : config.screenshot_path); config.take_screenshot = false; // Only take one screenshot } } else { + Py_BEGIN_ALLOW_THREADS window->display(); + Py_END_ALLOW_THREADS + } + + // #219 - Safe window for background threads to modify UI + // This runs AFTER display() but BEFORE the next frame's processing + if (frameLock.hasWaiting()) { + frameLock.openWindow(); + // Release GIL so waiting threads can proceed with their mcrfpy.lock() blocks + Py_BEGIN_ALLOW_THREADS + frameLock.closeWindow(); // Wait for all lock holders to complete + Py_END_ALLOW_THREADS } currentFrame++; diff --git a/src/GameEngine.h b/src/GameEngine.h index c5d5aba..f6cb27e 100644 --- a/src/GameEngine.h +++ b/src/GameEngine.h @@ -13,6 +13,10 @@ #include "ImGuiConsole.h" #include #include +#include +#include +#include +#include /** * @brief Performance profiling metrics structure @@ -79,6 +83,59 @@ struct ProfilingMetrics { } }; +/** + * @brief Thread synchronization primitive for safe UI updates from background threads (#219) + * + * Allows background Python threads to safely update UI objects by waiting for + * a "safe window" between frames when the render loop is not iterating the scene graph. + * + * Usage from Python: + * with mcrfpy.lock(): + * frame.x = new_value # Safe to modify UI here + */ +class FrameLock { +private: + std::mutex mtx; + std::condition_variable cv; + bool safe_window = false; + std::atomic waiting{0}; + std::atomic active{0}; + +public: + /** + * @brief Acquire the lock, blocking until safe window opens + * + * Called by mcrfpy.lock().__enter__. Releases GIL while waiting. + */ + void acquire(); + + /** + * @brief Release the lock + * + * Called by mcrfpy.lock().__exit__ + */ + void release(); + + /** + * @brief Open the safe window, allowing waiting threads to proceed + * + * Called by render loop between frames + */ + void openWindow(); + + /** + * @brief Close the safe window, waiting for all active threads to finish + * + * Called by render loop before resuming rendering + */ + void closeWindow(); + + /** + * @brief Check if any threads are waiting for the lock + */ + bool hasWaiting() const { return waiting.load() > 0; } +}; + class GameEngine { public: @@ -136,6 +193,10 @@ private: ImGuiConsole console; bool imguiInitialized = false; + // #219 - Thread synchronization for background Python threads + FrameLock frameLock; + std::thread::id main_thread_id; // For detecting if lock() is called from main thread + void updateViewport(); void testTimers(); @@ -198,6 +259,10 @@ public: int getSimulationTime() const { return simulation_time; } void renderScene(); // Force render current scene (for synchronous screenshot) + // #219 - Thread synchronization for background threads + FrameLock& getFrameLock() { return frameLock; } + bool isMainThread() const { return std::this_thread::get_id() == main_thread_id; } + // global textures for scripts to access std::vector textures; diff --git a/src/McRFPy_API.cpp b/src/McRFPy_API.cpp index dee9c1e..afda758 100644 --- a/src/McRFPy_API.cpp +++ b/src/McRFPy_API.cpp @@ -25,6 +25,7 @@ #include "PyHeightMap.h" // Procedural generation heightmap (#193) #include "PyBSP.h" // Procedural generation BSP (#202-206) #include "PyNoiseSource.h" // Procedural generation noise (#207-208) +#include "PyLock.h" // Thread synchronization (#219) #include "McRogueFaceVersion.h" #include "GameEngine.h" #include "ImGuiConsole.h" @@ -292,6 +293,17 @@ static PyMethodDef mcrfpyMethods[] = { {"__getattr__", mcrfpy_module_getattr, METH_VARARGS, "Module-level __getattr__ for dynamic properties (current_scene, scenes)"}, + // #219: Thread synchronization + {"lock", PyLock::lock, METH_NOARGS, + MCRF_FUNCTION(lock, + MCRF_SIG("()", "_LockContext"), + MCRF_DESC("Get a context manager for thread-safe UI updates from background threads."), + MCRF_RETURNS("_LockContext: A context manager that blocks until safe to modify UI") + MCRF_NOTE("Use with `with mcrfpy.lock():` to safely modify UI objects from a background thread. " + "The context manager blocks until the render loop reaches a safe point between frames. " + "Without this, modifying UI from threads may cause visual glitches or crashes.") + )}, + {NULL, NULL, 0, NULL} }; @@ -479,7 +491,13 @@ PyObject* PyInit_mcrfpy() PyUILineType.tp_weaklistoffset = offsetof(PyUILineObject, weakreflist); PyUICircleType.tp_weaklistoffset = offsetof(PyUICircleObject, weakreflist); PyUIArcType.tp_weaklistoffset = offsetof(PyUIArcObject, weakreflist); - + + // #219 - Initialize PyLock context manager type + if (PyLock::init() < 0) { + std::cout << "ERROR: PyLock::init() failed" << std::endl; + return NULL; + } + // Process exported types - PyType_Ready AND add to module int i = 0; auto t = exported_types[i]; diff --git a/src/PyLock.cpp b/src/PyLock.cpp new file mode 100644 index 0000000..d6aa7d7 --- /dev/null +++ b/src/PyLock.cpp @@ -0,0 +1,100 @@ +// #219 - Thread synchronization context manager for mcrfpy.lock() +#include "PyLock.h" +#include "GameEngine.h" +#include "Resources.h" + +// Forward declarations +static PyObject* PyLockContext_enter(PyLockContextObject* self, PyObject* args); +static PyObject* PyLockContext_exit(PyLockContextObject* self, PyObject* args); +static void PyLockContext_dealloc(PyLockContextObject* self); +static PyObject* PyLockContext_new(PyTypeObject* type, PyObject* args, PyObject* kwds); + +// Context manager methods +static PyMethodDef PyLockContext_methods[] = { + {"__enter__", (PyCFunction)PyLockContext_enter, METH_NOARGS, + "Acquire the frame lock, blocking until safe window opens"}, + {"__exit__", (PyCFunction)PyLockContext_exit, METH_VARARGS, + "Release the frame lock"}, + {NULL} +}; + +// Type definition +PyTypeObject PyLockContextType = { + .ob_base = {.ob_base = {.ob_refcnt = 1, .ob_type = NULL}, .ob_size = 0}, + .tp_name = "mcrfpy._LockContext", + .tp_basicsize = sizeof(PyLockContextObject), + .tp_itemsize = 0, + .tp_dealloc = (destructor)PyLockContext_dealloc, + .tp_flags = Py_TPFLAGS_DEFAULT, + .tp_doc = "Thread synchronization context manager for safe UI updates from background threads", + .tp_methods = PyLockContext_methods, + .tp_new = PyLockContext_new, +}; + +static PyObject* PyLockContext_new(PyTypeObject* type, PyObject* args, PyObject* kwds) { + PyLockContextObject* self = (PyLockContextObject*)type->tp_alloc(type, 0); + if (self) { + self->acquired = false; + } + return (PyObject*)self; +} + +static void PyLockContext_dealloc(PyLockContextObject* self) { + // Safety: if we're destroyed while holding the lock, release it + if (self->acquired && Resources::game) { + Resources::game->getFrameLock().release(); + } + Py_TYPE(self)->tp_free((PyObject*)self); +} + +static PyObject* PyLockContext_enter(PyLockContextObject* self, PyObject* args) { + if (!Resources::game) { + PyErr_SetString(PyExc_RuntimeError, "Game engine not initialized"); + return NULL; + } + + // #219 - If we're on the main thread, no-op (already synchronized) + // This allows the same code to work from callbacks, script init, AND background threads + if (Resources::game->isMainThread()) { + self->acquired = false; // Don't try to release what we didn't acquire + Py_INCREF(self); + return (PyObject*)self; + } + + // Acquire the frame lock - this will block (with GIL released) until safe window + Resources::game->getFrameLock().acquire(); + self->acquired = true; + + // Return self for the `with` statement + Py_INCREF(self); + return (PyObject*)self; +} + +static PyObject* PyLockContext_exit(PyLockContextObject* self, PyObject* args) { + // args contains (exc_type, exc_val, exc_tb) - we don't suppress exceptions + if (self->acquired && Resources::game) { + Resources::game->getFrameLock().release(); + self->acquired = false; + } + + // Return False to not suppress any exceptions + Py_RETURN_FALSE; +} + +// The lock() function that users call - returns a new context manager +PyObject* PyLock::lock(PyObject* self, PyObject* args) { + if (!Resources::game) { + PyErr_SetString(PyExc_RuntimeError, "Game engine not initialized"); + return NULL; + } + + // Create and return a new context manager object + return PyObject_CallObject((PyObject*)&PyLockContextType, NULL); +} + +int PyLock::init() { + if (PyType_Ready(&PyLockContextType) < 0) { + return -1; + } + return 0; +} diff --git a/src/PyLock.h b/src/PyLock.h new file mode 100644 index 0000000..0f6a40a --- /dev/null +++ b/src/PyLock.h @@ -0,0 +1,25 @@ +#pragma once +// #219 - Thread synchronization context manager for mcrfpy.lock() + +#include + +// Forward declaration +class GameEngine; + +// PyLockContext - the context manager object returned by mcrfpy.lock() +typedef struct { + PyObject_HEAD + bool acquired; // Track if we've acquired the lock +} PyLockContextObject; + +// The type object for the context manager +extern PyTypeObject PyLockContextType; + +// Module initialization function - adds lock() function to module +namespace PyLock { + // Create the lock() function that returns a context manager + PyObject* lock(PyObject* self, PyObject* args); + + // Initialize the type (call PyType_Ready) + int init(); +}