From 6f67fbb51efaf70e52fba8c939298dcdff50450a Mon Sep 17 00:00:00 2001 From: John McCardle Date: Mon, 14 Jul 2025 00:35:00 -0400 Subject: [PATCH] Fix animation callback crashes from iterator invalidation (#119) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolved segfaults caused by creating new animations from within animation callbacks. The issue was iterator invalidation in AnimationManager::update() when callbacks modified the active animations vector. Changes: - Add deferred animation queue to AnimationManager - New animations created during update are queued and added after - Set isUpdating flag to track when in update loop - Properly handle Animation destructor during callback execution - Add clearCallback() method for safe cleanup scenarios This fixes the "free(): invalid pointer" and "malloc(): unaligned fastbin chunk detected" errors that occurred with rapid animation creation in callbacks. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- src/Animation.cpp | 66 ++++++++++++++++++++++++++++++++++++++++------- src/Animation.h | 8 +++++- 2 files changed, 63 insertions(+), 11 deletions(-) diff --git a/src/Animation.cpp b/src/Animation.cpp index abf2d41..20b8fad 100644 --- a/src/Animation.cpp +++ b/src/Animation.cpp @@ -1,6 +1,8 @@ #include "Animation.h" #include "UIDrawable.h" #include "UIEntity.h" +#include "PyAnimation.h" +#include "McRFPy_API.h" #include #include #include @@ -9,6 +11,11 @@ #define M_PI 3.14159265358979323846 #endif +// Forward declaration of PyAnimation type +namespace mcrfpydef { + extern PyTypeObject PyAnimationType; +} + // Animation implementation Animation::Animation(const std::string& targetProperty, const AnimationValue& targetValue, @@ -30,10 +37,13 @@ Animation::Animation(const std::string& targetProperty, } Animation::~Animation() { - // Decrease reference count for Python callback - if (pythonCallback) { + // Decrease reference count for Python callback if we still own it + PyObject* callback = pythonCallback; + if (callback) { + pythonCallback = nullptr; + PyGILState_STATE gstate = PyGILState_Ensure(); - Py_DECREF(pythonCallback); + Py_DECREF(callback); PyGILState_Release(gstate); } } @@ -43,6 +53,7 @@ void Animation::start(std::shared_ptr target) { targetWeak = target; elapsed = 0.0f; + callbackTriggered = false; // Reset callback state // Capture start value from target std::visit([this, &target](const auto& targetVal) { @@ -93,6 +104,7 @@ void Animation::startEntity(std::shared_ptr target) { entityTargetWeak = target; elapsed = 0.0f; + callbackTriggered = false; // Reset callback state // Capture the starting value from the entity std::visit([this, target](const auto& val) { @@ -118,6 +130,19 @@ bool Animation::hasValidTarget() const { return !targetWeak.expired() || !entityTargetWeak.expired(); } +void Animation::clearCallback() { + // Safely clear the callback when PyAnimation is being destroyed + PyObject* callback = pythonCallback; + if (callback) { + pythonCallback = nullptr; + callbackTriggered = true; // Prevent future triggering + + PyGILState_STATE gstate = PyGILState_Ensure(); + Py_DECREF(callback); + PyGILState_Release(gstate); + } +} + void Animation::complete() { // Jump to end of animation elapsed = duration; @@ -165,9 +190,9 @@ bool Animation::update(float deltaTime) { } // Trigger callback when animation completes + // Check pythonCallback again in case it was cleared during update if (isComplete() && !callbackTriggered && pythonCallback) { triggerCallback(); - callbackTriggered = true; } return !isComplete(); @@ -319,13 +344,14 @@ void Animation::applyValue(UIEntity* entity, const AnimationValue& value) { void Animation::triggerCallback() { if (!pythonCallback) return; + // Ensure we only trigger once + if (callbackTriggered) return; + callbackTriggered = true; + PyGILState_STATE gstate = PyGILState_Ensure(); - // We need to create PyAnimation wrapper for this animation - // and PyObject wrapper for the target - // For now, we'll pass None for both as a simple implementation - // TODO: In future, wrap the animation and target objects properly - + // TODO: In future, create PyAnimation wrapper for this animation + // For now, pass None for both parameters PyObject* args = PyTuple_New(2); Py_INCREF(Py_None); Py_INCREF(Py_None); @@ -338,6 +364,7 @@ void Animation::triggerCallback() { if (!result) { // Print error but don't crash PyErr_Print(); + PyErr_Clear(); // Clear the error state } else { Py_DECREF(result); } @@ -594,11 +621,19 @@ AnimationManager& AnimationManager::getInstance() { void AnimationManager::addAnimation(std::shared_ptr animation) { if (animation && animation->hasValidTarget()) { - activeAnimations.push_back(animation); + if (isUpdating) { + // Defer adding during update to avoid iterator invalidation + pendingAnimations.push_back(animation); + } else { + activeAnimations.push_back(animation); + } } } void AnimationManager::update(float deltaTime) { + // Set flag to defer new animations + isUpdating = true; + // Remove completed or invalid animations activeAnimations.erase( std::remove_if(activeAnimations.begin(), activeAnimations.end(), @@ -607,6 +642,17 @@ void AnimationManager::update(float deltaTime) { }), activeAnimations.end() ); + + // Clear update flag + isUpdating = false; + + // Add any animations that were created during update + if (!pendingAnimations.empty()) { + activeAnimations.insert(activeAnimations.end(), + pendingAnimations.begin(), + pendingAnimations.end()); + pendingAnimations.clear(); + } } diff --git a/src/Animation.h b/src/Animation.h index 0879ab5..181bec4 100644 --- a/src/Animation.h +++ b/src/Animation.h @@ -62,6 +62,9 @@ public: // Check if animation has valid target bool hasValidTarget() const; + // Clear the callback (called when PyAnimation is deallocated) + void clearCallback(); + // Animation properties std::string getTargetProperty() const { return targetProperty; } float getDuration() const { return duration; } @@ -83,8 +86,9 @@ private: std::weak_ptr entityTargetWeak; // Callback support - PyObject* pythonCallback = nullptr; // Python callback function + PyObject* pythonCallback = nullptr; // Python callback function (we own a reference) bool callbackTriggered = false; // Ensure callback only fires once + PyObject* pyAnimationWrapper = nullptr; // Weak reference to PyAnimation if created from Python // Helper to interpolate between values AnimationValue interpolate(float t) const; @@ -163,4 +167,6 @@ public: private: AnimationManager() = default; std::vector> activeAnimations; + std::vector> pendingAnimations; // Animations to add after update + bool isUpdating = false; // Flag to track if we're in update loop }; \ No newline at end of file