Fix animation segfaults with RAII weak_ptr implementation
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 <noreply@anthropic.com>
This commit is contained in:
parent
98fc49a978
commit
062e4dadc4
|
|
@ -23,60 +23,60 @@ Animation::Animation(const std::string& targetProperty,
|
|||
{
|
||||
}
|
||||
|
||||
void Animation::start(UIDrawable* target) {
|
||||
currentTarget = target;
|
||||
void Animation::start(std::shared_ptr<UIDrawable> 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<decltype(targetVal)>;
|
||||
|
||||
if constexpr (std::is_same_v<T, float>) {
|
||||
float value;
|
||||
if (currentTarget->getProperty(targetProperty, value)) {
|
||||
if (target->getProperty(targetProperty, value)) {
|
||||
startValue = value;
|
||||
}
|
||||
}
|
||||
else if constexpr (std::is_same_v<T, int>) {
|
||||
int value;
|
||||
if (currentTarget->getProperty(targetProperty, value)) {
|
||||
if (target->getProperty(targetProperty, value)) {
|
||||
startValue = value;
|
||||
}
|
||||
}
|
||||
else if constexpr (std::is_same_v<T, std::vector<int>>) {
|
||||
// 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<T, sf::Color>) {
|
||||
sf::Color value;
|
||||
if (currentTarget->getProperty(targetProperty, value)) {
|
||||
if (target->getProperty(targetProperty, value)) {
|
||||
startValue = value;
|
||||
}
|
||||
}
|
||||
else if constexpr (std::is_same_v<T, sf::Vector2f>) {
|
||||
sf::Vector2f value;
|
||||
if (currentTarget->getProperty(targetProperty, value)) {
|
||||
if (target->getProperty(targetProperty, value)) {
|
||||
startValue = value;
|
||||
}
|
||||
}
|
||||
else if constexpr (std::is_same_v<T, std::string>) {
|
||||
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<UIEntity> 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<UIDrawable> target = targetWeak.lock();
|
||||
std::shared_ptr<UIEntity> 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<decltype(value)>;
|
||||
|
||||
if (currentTarget) {
|
||||
// Handle UIDrawable targets
|
||||
if constexpr (std::is_same_v<T, float>) {
|
||||
currentTarget->setProperty(targetProperty, value);
|
||||
// Apply to whichever target is valid
|
||||
if (target) {
|
||||
applyValue(target.get(), currentValue);
|
||||
} else if (entity) {
|
||||
applyValue(entity.get(), currentValue);
|
||||
}
|
||||
else if constexpr (std::is_same_v<T, int>) {
|
||||
currentTarget->setProperty(targetProperty, value);
|
||||
}
|
||||
else if constexpr (std::is_same_v<T, sf::Color>) {
|
||||
currentTarget->setProperty(targetProperty, value);
|
||||
}
|
||||
else if constexpr (std::is_same_v<T, sf::Vector2f>) {
|
||||
currentTarget->setProperty(targetProperty, value);
|
||||
}
|
||||
else if constexpr (std::is_same_v<T, std::string>) {
|
||||
currentTarget->setProperty(targetProperty, value);
|
||||
}
|
||||
}
|
||||
else if (currentEntityTarget) {
|
||||
// Handle UIEntity targets
|
||||
if constexpr (std::is_same_v<T, float>) {
|
||||
currentEntityTarget->setProperty(targetProperty, value);
|
||||
}
|
||||
else if constexpr (std::is_same_v<T, int>) {
|
||||
currentEntityTarget->setProperty(targetProperty, value);
|
||||
}
|
||||
// Entities don't support other types yet
|
||||
}
|
||||
}, 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<decltype(val)>;
|
||||
|
||||
if constexpr (std::is_same_v<T, float>) {
|
||||
target->setProperty(targetProperty, val);
|
||||
}
|
||||
else if constexpr (std::is_same_v<T, int>) {
|
||||
target->setProperty(targetProperty, val);
|
||||
}
|
||||
else if constexpr (std::is_same_v<T, sf::Color>) {
|
||||
target->setProperty(targetProperty, val);
|
||||
}
|
||||
else if constexpr (std::is_same_v<T, sf::Vector2f>) {
|
||||
target->setProperty(targetProperty, val);
|
||||
}
|
||||
else if constexpr (std::is_same_v<T, std::string>) {
|
||||
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<decltype(val)>;
|
||||
|
||||
if constexpr (std::is_same_v<T, float>) {
|
||||
entity->setProperty(targetProperty, val);
|
||||
}
|
||||
else if constexpr (std::is_same_v<T, int>) {
|
||||
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> 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<Animation>& anim) {
|
||||
return anim->isComplete();
|
||||
[deltaTime](std::shared_ptr<Animation>& 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();
|
||||
}
|
||||
|
|
@ -39,10 +39,13 @@ public:
|
|||
bool delta = false);
|
||||
|
||||
// Apply this animation to a drawable
|
||||
void start(UIDrawable* target);
|
||||
void start(std::shared_ptr<UIDrawable> 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<UIEntity> 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<UIDrawable> targetWeak;
|
||||
std::weak_ptr<UIEntity> 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;
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
if (entity->data) {
|
||||
self->data->startEntity(entity->data);
|
||||
AnimationManager::getInstance().addAnimation(self->data);
|
||||
|
||||
Py_RETURN_NONE;
|
||||
}
|
||||
}
|
||||
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}
|
||||
};
|
||||
|
|
@ -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[];
|
||||
|
|
|
|||
Loading…
Reference in New Issue