From 062e4dadc42833bf5a3559e5d7c4ceb4abb7e9c0 Mon Sep 17 00:00:00 2001 From: John McCardle Date: Sat, 12 Jul 2025 10:21:48 -0400 Subject: [PATCH] Fix animation segfaults with RAII weak_ptr implementation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolved two critical segmentation faults in AnimationManager: 1. Race condition when creating multiple animations in timer callbacks 2. Exit crash when animations outlive their target objects Changes: - Replace raw pointers with std::weak_ptr for automatic target invalidation - Add Animation::complete() to jump animations to final value - Add Animation::hasValidTarget() to check if target still exists - Update AnimationManager to auto-remove invalid animations - Add AnimationManager::clear() call to GameEngine::cleanup() - Update Python bindings to pass shared_ptr instead of raw pointers This ensures animations can never reference destroyed objects, following proper RAII principles. Tested with sizzle_reel_final.py and stress tests creating/destroying hundreds of animated objects. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- src/Animation.cpp | 168 ++++++++++++++++++++++++++++---------------- src/Animation.h | 26 ++++--- src/GameEngine.cpp | 9 ++- src/PyAnimation.cpp | 69 ++++++++++++------ src/PyAnimation.h | 2 + 5 files changed, 180 insertions(+), 94 deletions(-) diff --git a/src/Animation.cpp b/src/Animation.cpp index 7fa27ce..2e061e7 100644 --- a/src/Animation.cpp +++ b/src/Animation.cpp @@ -23,60 +23,60 @@ Animation::Animation(const std::string& targetProperty, { } -void Animation::start(UIDrawable* target) { - currentTarget = target; +void Animation::start(std::shared_ptr target) { + if (!target) return; + + targetWeak = target; elapsed = 0.0f; - // Capture startValue from target based on targetProperty - if (!currentTarget) return; - - // Try to get the current value based on the expected type - std::visit([this](const auto& targetVal) { + // Capture start value from target + std::visit([this, &target](const auto& targetVal) { using T = std::decay_t; if constexpr (std::is_same_v) { float value; - if (currentTarget->getProperty(targetProperty, value)) { + if (target->getProperty(targetProperty, value)) { startValue = value; } } else if constexpr (std::is_same_v) { int value; - if (currentTarget->getProperty(targetProperty, value)) { + if (target->getProperty(targetProperty, value)) { startValue = value; } } else if constexpr (std::is_same_v>) { // For sprite animation, get current sprite index int value; - if (currentTarget->getProperty(targetProperty, value)) { + if (target->getProperty(targetProperty, value)) { startValue = value; } } else if constexpr (std::is_same_v) { sf::Color value; - if (currentTarget->getProperty(targetProperty, value)) { + if (target->getProperty(targetProperty, value)) { startValue = value; } } else if constexpr (std::is_same_v) { sf::Vector2f value; - if (currentTarget->getProperty(targetProperty, value)) { + if (target->getProperty(targetProperty, value)) { startValue = value; } } else if constexpr (std::is_same_v) { std::string value; - if (currentTarget->getProperty(targetProperty, value)) { + if (target->getProperty(targetProperty, value)) { startValue = value; } } }, targetValue); } -void Animation::startEntity(UIEntity* target) { - currentEntityTarget = target; - currentTarget = nullptr; // Clear drawable target +void Animation::startEntity(std::shared_ptr target) { + if (!target) return; + + entityTargetWeak = target; elapsed = 0.0f; // Capture the starting value from the entity @@ -99,8 +99,36 @@ void Animation::startEntity(UIEntity* target) { }, targetValue); } +bool Animation::hasValidTarget() const { + return !targetWeak.expired() || !entityTargetWeak.expired(); +} + +void Animation::complete() { + // Jump to end of animation + elapsed = duration; + + // Apply final value + if (auto target = targetWeak.lock()) { + AnimationValue finalValue = interpolate(1.0f); + applyValue(target.get(), finalValue); + } + else if (auto entity = entityTargetWeak.lock()) { + AnimationValue finalValue = interpolate(1.0f); + applyValue(entity.get(), finalValue); + } +} + bool Animation::update(float deltaTime) { - if ((!currentTarget && !currentEntityTarget) || isComplete()) { + // Try to lock weak_ptr to get shared_ptr + std::shared_ptr target = targetWeak.lock(); + std::shared_ptr entity = entityTargetWeak.lock(); + + // If both are null, target was destroyed + if (!target && !entity) { + return false; // Remove this animation + } + + if (isComplete()) { return false; } @@ -114,39 +142,12 @@ bool Animation::update(float deltaTime) { // Get interpolated value AnimationValue currentValue = interpolate(easedT); - // Apply currentValue to target (either drawable or entity) - std::visit([this](const auto& value) { - using T = std::decay_t; - - if (currentTarget) { - // Handle UIDrawable targets - if constexpr (std::is_same_v) { - currentTarget->setProperty(targetProperty, value); - } - else if constexpr (std::is_same_v) { - currentTarget->setProperty(targetProperty, value); - } - else if constexpr (std::is_same_v) { - currentTarget->setProperty(targetProperty, value); - } - else if constexpr (std::is_same_v) { - currentTarget->setProperty(targetProperty, value); - } - else if constexpr (std::is_same_v) { - currentTarget->setProperty(targetProperty, value); - } - } - else if (currentEntityTarget) { - // Handle UIEntity targets - if constexpr (std::is_same_v) { - currentEntityTarget->setProperty(targetProperty, value); - } - else if constexpr (std::is_same_v) { - currentEntityTarget->setProperty(targetProperty, value); - } - // Entities don't support other types yet - } - }, currentValue); + // Apply to whichever target is valid + if (target) { + applyValue(target.get(), currentValue); + } else if (entity) { + applyValue(entity.get(), currentValue); + } return !isComplete(); } @@ -254,6 +255,46 @@ AnimationValue Animation::interpolate(float t) const { }, targetValue); } +void Animation::applyValue(UIDrawable* target, const AnimationValue& value) { + if (!target) return; + + std::visit([this, target](const auto& val) { + using T = std::decay_t; + + if constexpr (std::is_same_v) { + target->setProperty(targetProperty, val); + } + else if constexpr (std::is_same_v) { + target->setProperty(targetProperty, val); + } + else if constexpr (std::is_same_v) { + target->setProperty(targetProperty, val); + } + else if constexpr (std::is_same_v) { + target->setProperty(targetProperty, val); + } + else if constexpr (std::is_same_v) { + target->setProperty(targetProperty, val); + } + }, value); +} + +void Animation::applyValue(UIEntity* entity, const AnimationValue& value) { + if (!entity) return; + + std::visit([this, entity](const auto& val) { + using T = std::decay_t; + + if constexpr (std::is_same_v) { + entity->setProperty(targetProperty, val); + } + else if constexpr (std::is_same_v) { + entity->setProperty(targetProperty, val); + } + // Entities don't support other types yet + }, value); +} + // Easing functions implementation namespace EasingFunctions { @@ -502,26 +543,31 @@ AnimationManager& AnimationManager::getInstance() { } void AnimationManager::addAnimation(std::shared_ptr animation) { - activeAnimations.push_back(animation); + if (animation && animation->hasValidTarget()) { + activeAnimations.push_back(animation); + } } void AnimationManager::update(float deltaTime) { - for (auto& anim : activeAnimations) { - anim->update(deltaTime); - } - cleanup(); -} - -void AnimationManager::cleanup() { + // Remove completed or invalid animations activeAnimations.erase( std::remove_if(activeAnimations.begin(), activeAnimations.end(), - [](const std::shared_ptr& anim) { - return anim->isComplete(); + [deltaTime](std::shared_ptr& anim) { + return !anim || !anim->update(deltaTime); }), activeAnimations.end() ); } -void AnimationManager::clear() { + +void AnimationManager::clear(bool completeAnimations) { + if (completeAnimations) { + // Complete all animations before clearing + for (auto& anim : activeAnimations) { + if (anim) { + anim->complete(); + } + } + } activeAnimations.clear(); } \ No newline at end of file diff --git a/src/Animation.h b/src/Animation.h index 6308f32..38fb660 100644 --- a/src/Animation.h +++ b/src/Animation.h @@ -39,10 +39,13 @@ public: bool delta = false); // Apply this animation to a drawable - void start(UIDrawable* target); + void start(std::shared_ptr target); // Apply this animation to an entity (special case since Entity doesn't inherit from UIDrawable) - void startEntity(UIEntity* target); + void startEntity(std::shared_ptr target); + + // Complete the animation immediately (jump to final value) + void complete(); // Update animation (called each frame) // Returns true if animation is still running, false if complete @@ -51,6 +54,9 @@ public: // Get current interpolated value AnimationValue getCurrentValue() const; + // Check if animation has valid target + bool hasValidTarget() const; + // Animation properties std::string getTargetProperty() const { return targetProperty; } float getDuration() const { return duration; } @@ -67,11 +73,16 @@ private: EasingFunction easingFunc; // Easing function to use bool delta; // If true, targetValue is relative to start - UIDrawable* currentTarget = nullptr; // Current target being animated - UIEntity* currentEntityTarget = nullptr; // Current entity target (alternative to drawable) + // RAII: Use weak_ptr for safe target tracking + std::weak_ptr targetWeak; + std::weak_ptr entityTargetWeak; // Helper to interpolate between values AnimationValue interpolate(float t) const; + + // Helper to apply value to target + void applyValue(UIDrawable* target, const AnimationValue& value); + void applyValue(UIEntity* entity, const AnimationValue& value); }; // Easing functions library @@ -134,11 +145,8 @@ public: // Update all animations void update(float deltaTime); - // Remove completed animations - void cleanup(); - - // Clear all animations - void clear(); + // Clear all animations (optionally completing them first) + void clear(bool completeAnimations = false); private: AnimationManager() = default; diff --git a/src/GameEngine.cpp b/src/GameEngine.cpp index 5b35d79..dcba0e4 100644 --- a/src/GameEngine.cpp +++ b/src/GameEngine.cpp @@ -16,7 +16,7 @@ GameEngine::GameEngine(const McRogueFaceConfig& cfg) { Resources::font.loadFromFile("./assets/JetbrainsMono.ttf"); Resources::game = this; - window_title = "Crypt of Sokoban - 7DRL 2025, McRogueface Engine"; + window_title = "McRogueFace Engine"; // Initialize rendering based on headless mode if (headless) { @@ -91,6 +91,9 @@ void GameEngine::cleanup() if (cleaned_up) return; cleaned_up = true; + // Clear all animations first (RAII handles invalidation) + AnimationManager::getInstance().clear(); + // Clear Python references before destroying C++ objects // Clear all timers (they hold Python callables) timers.clear(); @@ -182,7 +185,7 @@ void GameEngine::setWindowScale(float multiplier) void GameEngine::run() { - std::cout << "GameEngine::run() starting main loop..." << std::endl; + //std::cout << "GameEngine::run() starting main loop..." << std::endl; float fps = 0.0; frameTime = 0.016f; // Initialize to ~60 FPS clock.restart(); @@ -259,7 +262,7 @@ void GameEngine::run() int tenth_fps = (metrics.fps * 10) % 10; if (!headless && window) { - window->setTitle(window_title + " " + std::to_string(whole_fps) + "." + std::to_string(tenth_fps) + " FPS"); + window->setTitle(window_title); } // In windowed mode, check if window was closed diff --git a/src/PyAnimation.cpp b/src/PyAnimation.cpp index 720b8d9..1adddb1 100644 --- a/src/PyAnimation.cpp +++ b/src/PyAnimation.cpp @@ -126,50 +126,50 @@ PyObject* PyAnimation::start(PyAnimationObject* self, PyObject* args) { return NULL; } - // Get the UIDrawable from the Python object - UIDrawable* drawable = nullptr; - // Check type by comparing type names const char* type_name = Py_TYPE(target_obj)->tp_name; if (strcmp(type_name, "mcrfpy.Frame") == 0) { PyUIFrameObject* frame = (PyUIFrameObject*)target_obj; - drawable = frame->data.get(); + if (frame->data) { + self->data->start(frame->data); + AnimationManager::getInstance().addAnimation(self->data); + } } else if (strcmp(type_name, "mcrfpy.Caption") == 0) { PyUICaptionObject* caption = (PyUICaptionObject*)target_obj; - drawable = caption->data.get(); + if (caption->data) { + self->data->start(caption->data); + AnimationManager::getInstance().addAnimation(self->data); + } } else if (strcmp(type_name, "mcrfpy.Sprite") == 0) { PyUISpriteObject* sprite = (PyUISpriteObject*)target_obj; - drawable = sprite->data.get(); + if (sprite->data) { + self->data->start(sprite->data); + AnimationManager::getInstance().addAnimation(self->data); + } } else if (strcmp(type_name, "mcrfpy.Grid") == 0) { PyUIGridObject* grid = (PyUIGridObject*)target_obj; - drawable = grid->data.get(); + if (grid->data) { + self->data->start(grid->data); + AnimationManager::getInstance().addAnimation(self->data); + } } else if (strcmp(type_name, "mcrfpy.Entity") == 0) { // Special handling for Entity since it doesn't inherit from UIDrawable PyUIEntityObject* entity = (PyUIEntityObject*)target_obj; - // Start the animation directly on the entity - self->data->startEntity(entity->data.get()); - - // Add to AnimationManager - AnimationManager::getInstance().addAnimation(self->data); - - Py_RETURN_NONE; + if (entity->data) { + self->data->startEntity(entity->data); + AnimationManager::getInstance().addAnimation(self->data); + } } else { PyErr_SetString(PyExc_TypeError, "Target must be a Frame, Caption, Sprite, Grid, or Entity"); return NULL; } - // Start the animation - self->data->start(drawable); - - // Add to AnimationManager - AnimationManager::getInstance().addAnimation(self->data); - Py_RETURN_NONE; } @@ -214,6 +214,20 @@ PyObject* PyAnimation::get_current_value(PyAnimationObject* self, PyObject* args }, value); } +PyObject* PyAnimation::complete(PyAnimationObject* self, PyObject* args) { + if (self->data) { + self->data->complete(); + } + Py_RETURN_NONE; +} + +PyObject* PyAnimation::has_valid_target(PyAnimationObject* self, PyObject* args) { + if (self->data && self->data->hasValidTarget()) { + Py_RETURN_TRUE; + } + Py_RETURN_FALSE; +} + PyGetSetDef PyAnimation::getsetters[] = { {"property", (getter)get_property, NULL, "Target property name", NULL}, {"duration", (getter)get_duration, NULL, "Animation duration in seconds", NULL}, @@ -225,10 +239,23 @@ PyGetSetDef PyAnimation::getsetters[] = { PyMethodDef PyAnimation::methods[] = { {"start", (PyCFunction)start, METH_VARARGS, - "Start the animation on a target UIDrawable"}, + "start(target) -> None\n\n" + "Start the animation on a target UI element.\n\n" + "Args:\n" + " target: The UI element to animate (Frame, Caption, Sprite, Grid, or Entity)\n\n" + "Note:\n" + " The animation will automatically stop if the target is destroyed."}, {"update", (PyCFunction)update, METH_VARARGS, "Update the animation by deltaTime (returns True if still running)"}, {"get_current_value", (PyCFunction)get_current_value, METH_NOARGS, "Get the current interpolated value"}, + {"complete", (PyCFunction)complete, METH_NOARGS, + "complete() -> None\n\n" + "Complete the animation immediately by jumping to the final value."}, + {"hasValidTarget", (PyCFunction)has_valid_target, METH_NOARGS, + "hasValidTarget() -> bool\n\n" + "Check if the animation still has a valid target.\n\n" + "Returns:\n" + " True if the target still exists, False if it was destroyed."}, {NULL} }; \ No newline at end of file diff --git a/src/PyAnimation.h b/src/PyAnimation.h index 9976cb2..ccb4f36 100644 --- a/src/PyAnimation.h +++ b/src/PyAnimation.h @@ -28,6 +28,8 @@ public: static PyObject* start(PyAnimationObject* self, PyObject* args); static PyObject* update(PyAnimationObject* self, PyObject* args); static PyObject* get_current_value(PyAnimationObject* self, PyObject* args); + static PyObject* complete(PyAnimationObject* self, PyObject* args); + static PyObject* has_valid_target(PyAnimationObject* self, PyObject* args); static PyGetSetDef getsetters[]; static PyMethodDef methods[];