diff --git a/src/Animation.cpp b/src/Animation.cpp index 9ad8aac..b59c96e 100644 --- a/src/Animation.cpp +++ b/src/Animation.cpp @@ -977,14 +977,21 @@ void AnimationManager::addAnimation(std::shared_ptr animation, switch (conflict_mode) { case AnimationConflictMode::REPLACE: - // Complete the existing animation and replace it if (existingAnim) { - existingAnim->complete(); - // Remove from active animations - activeAnimations.erase( - std::remove(activeAnimations.begin(), activeAnimations.end(), existingAnim), - activeAnimations.end() - ); + if (isUpdating) { + // During update, just stop the animation without completing + // to avoid recursive callback issues and iterator invalidation. + // The update loop will clean up stopped animations. + existingAnim->stop(); + } else { + // Outside update loop, complete the animation (jump to final value) + // and remove it safely + existingAnim->complete(); + activeAnimations.erase( + std::remove(activeAnimations.begin(), activeAnimations.end(), existingAnim), + activeAnimations.end() + ); + } } // Fall through to add the new animation break; @@ -1046,21 +1053,26 @@ void AnimationManager::update(float deltaTime) { // Add any animations that were created during update if (!pendingAnimations.empty()) { - // Re-add pending animations through addAnimation to handle conflicts properly for (auto& anim : pendingAnimations) { if (anim && anim->hasValidTarget()) { - // Check if this was a queued animation or a new one void* target = getAnimationTarget(anim); std::string property = anim->getTargetProperty(); PropertyKey key{target, property}; - // If not already locked, add it + // Check if this animation is already the property lock holder + // (this happens when addAnimation was called during update) auto lockIt = propertyLocks.find(key); - if (lockIt == propertyLocks.end() || lockIt->second.expired()) { + bool isLockHolder = (lockIt != propertyLocks.end()) && + (lockIt->second.lock() == anim); + bool propertyFree = (lockIt == propertyLocks.end()) || + lockIt->second.expired(); + + if (isLockHolder || propertyFree) { + // This animation owns the lock or property is free - add it propertyLocks[key] = anim; activeAnimations.push_back(anim); } else { - // Property still locked, re-queue + // Property still locked by another animation, re-queue animationQueue.emplace_back(key, anim); } } diff --git a/tests/regression/recursive_animation_callback_segfault.py b/tests/regression/recursive_animation_callback_segfault.py new file mode 100644 index 0000000..a09c43f --- /dev/null +++ b/tests/regression/recursive_animation_callback_segfault.py @@ -0,0 +1,46 @@ +# Regression test: Recursive animation callbacks +# Issue: Animation callbacks that started new animations caused segfault in AnimationManager::update +# Fixed by: Deferring animation removal during update loop iteration +# +# This test verifies that: +# 1. Animation callbacks can start new animations on the same property (REPLACE mode) +# 2. No segfault occurs from iterator invalidation +# 3. The animation chain completes properly + +import mcrfpy +import sys + +scene = mcrfpy.Scene("test") +mcrfpy.current_scene = scene + +frame = mcrfpy.Frame(pos=(100, 100), size=(50, 50), fill_color=mcrfpy.Color(255, 0, 0)) +scene.children.append(frame) + +callback_count = 0 +MAX_CALLBACKS = 10 + +def recursive_callback(target, prop, value): + """Animation callback that starts another animation on the same property""" + global callback_count + callback_count += 1 + + if callback_count >= MAX_CALLBACKS: + print(f"PASS - {callback_count} recursive animation callbacks completed without segfault") + sys.exit(0) + + # Chain another animation - this used to cause segfault due to iterator invalidation + target.animate("x", 100 + (callback_count * 20), 0.1, mcrfpy.Easing.LINEAR, callback=recursive_callback) + +# Start the chain +frame.animate("x", 200, 0.1, mcrfpy.Easing.LINEAR, callback=recursive_callback) + +def timeout_check(timer, runtime): + """Safety timeout""" + if callback_count >= MAX_CALLBACKS: + print(f"PASS - {callback_count} callbacks completed") + sys.exit(0) + else: + print(f"FAIL - only {callback_count}/{MAX_CALLBACKS} callbacks executed") + sys.exit(1) + +safety_timer = mcrfpy.Timer("safety", timeout_check, 5000, once=True)