From 5100a80f7f7d6c5ca41cc6e860fc7c8541d7a5b3 Mon Sep 17 00:00:00 2001 From: Christoph Lendenfeld Date: Tue, 25 Feb 2025 12:11:15 +0100 Subject: [PATCH] Fix #110650: Drivers not removed if removing their container This applies to modifiers, constraints and shape keys. Any driver on such data was not removed with it, leaving invalid drivers on objects. With this patch, the drivers are removed, but animation is left untouched. This is because animation might be reused somewhere else and we don't want to introduce potential side effects there. This also adds unit tests for the fixed cases Pull Request: https://projects.blender.org/blender/blender/pulls/134511 --- source/blender/blenkernel/BKE_animsys.h | 19 ++++++ source/blender/blenkernel/BKE_constraint.h | 3 +- source/blender/blenkernel/intern/anim_data.cc | 23 +++++++ .../blender/blenkernel/intern/constraint.cc | 8 ++- source/blender/blenkernel/intern/object.cc | 4 ++ .../editors/object/object_constraint.cc | 2 +- .../blender/editors/object/object_modifier.cc | 2 + .../editors/object/object_relations.cc | 2 +- source/blender/makesrna/intern/rna_object.cc | 2 +- source/blender/makesrna/intern/rna_pose.cc | 2 +- tests/python/bl_animation_drivers.py | 62 +++++++++++++++++++ 11 files changed, 121 insertions(+), 8 deletions(-) diff --git a/source/blender/blenkernel/BKE_animsys.h b/source/blender/blenkernel/BKE_animsys.h index 0825dd5c839..f28326b12e7 100644 --- a/source/blender/blenkernel/BKE_animsys.h +++ b/source/blender/blenkernel/BKE_animsys.h @@ -187,6 +187,25 @@ void BKE_animdata_fix_paths_rename_all(struct ID *ref_id, */ bool BKE_animdata_fix_paths_remove(struct ID *id, const char *prefix); +/** + * Remove drivers that have an RNA path starting with `prefix`. + * + * \return true if any driver was removed. + */ +bool BKE_animdata_driver_path_remove(struct ID *id, const char *prefix); + +/** + * Remove all drivers from the given struct. + * + * \param type needs to be a struct owned by the given ID. + * \param data the actual struct data, needs to be the data for the StructRNA. + * + * \return true if any driver was removed. + */ +bool BKE_animdata_drivers_remove_for_rna_struct(struct ID &owner_id, + struct StructRNA &type, + void *data); + /* -------------------------------------- */ typedef struct AnimationBasePathChange { diff --git a/source/blender/blenkernel/BKE_constraint.h b/source/blender/blenkernel/BKE_constraint.h index d8f8e82e51e..835e68c9678 100644 --- a/source/blender/blenkernel/BKE_constraint.h +++ b/source/blender/blenkernel/BKE_constraint.h @@ -243,11 +243,10 @@ struct bConstraint *BKE_constraint_add_for_pose(struct Object *ob, const char *name, short type); -bool BKE_constraint_remove_ex(ListBase *list, struct Object *ob, struct bConstraint *con); /** * Remove the specified constraint from the given constraint stack. */ -bool BKE_constraint_remove(ListBase *list, struct bConstraint *con); +bool BKE_constraint_remove_ex(ListBase *list, struct Object *ob, struct bConstraint *con); /** * Apply the specified constraint in the given constraint stack. diff --git a/source/blender/blenkernel/intern/anim_data.cc b/source/blender/blenkernel/intern/anim_data.cc index d57f55b2645..b8628fd7ce3 100644 --- a/source/blender/blenkernel/intern/anim_data.cc +++ b/source/blender/blenkernel/intern/anim_data.cc @@ -1216,6 +1216,29 @@ bool BKE_animdata_fix_paths_remove(ID *id, const char *prefix) return any_removed; } +bool BKE_animdata_driver_path_remove(ID *id, const char *prefix) +{ + AnimData *adt = BKE_animdata_from_id(id); + if (!adt) { + return false; + } + + const bool any_removed = fcurves_path_remove_from_listbase(prefix, &adt->drivers); + return any_removed; +} + +bool BKE_animdata_drivers_remove_for_rna_struct(ID &owner_id, StructRNA &type, void *data) +{ + PointerRNA constraint_ptr = RNA_pointer_create_discrete(&owner_id, &type, data); + const std::optional base_path = RNA_path_from_ID_to_struct(&constraint_ptr); + if (!base_path.has_value()) { + /* The data should exist, so the path should always resolve. */ + BLI_assert_unreachable(); + } + + return BKE_animdata_driver_path_remove(&owner_id, base_path.value().c_str()); +} + /* Apply Op to All FCurves in Database --------------------------- */ /* Helper for adt_apply_all_fcurves_cb() - Apply wrapped operator to list of F-Curves */ diff --git a/source/blender/blenkernel/intern/constraint.cc b/source/blender/blenkernel/intern/constraint.cc index 29294c3082f..5131a257409 100644 --- a/source/blender/blenkernel/intern/constraint.cc +++ b/source/blender/blenkernel/intern/constraint.cc @@ -72,6 +72,8 @@ #include "BIK_api.h" +#include "RNA_prototypes.hh" + #include "DEG_depsgraph.hh" #include "DEG_depsgraph_query.hh" @@ -5728,7 +5730,7 @@ void BKE_constraints_free(ListBase *list) BKE_constraints_free_ex(list, true); } -bool BKE_constraint_remove(ListBase *list, bConstraint *con) +static bool constraint_remove(ListBase *list, bConstraint *con) { if (con) { BKE_constraint_free_data(con); @@ -5741,8 +5743,10 @@ bool BKE_constraint_remove(ListBase *list, bConstraint *con) bool BKE_constraint_remove_ex(ListBase *list, Object *ob, bConstraint *con) { + BKE_animdata_drivers_remove_for_rna_struct(ob->id, RNA_Constraint, con); + const short type = con->type; - if (BKE_constraint_remove(list, con)) { + if (constraint_remove(list, con)) { /* ITASC needs to be rebuilt once a constraint is removed #26920. */ if (ELEM(type, CONSTRAINT_TYPE_KINEMATIC, CONSTRAINT_TYPE_SPLINEIK)) { BIK_clear_data(ob->pose); diff --git a/source/blender/blenkernel/intern/object.cc b/source/blender/blenkernel/intern/object.cc index 15691a16bf1..affbe95bb48 100644 --- a/source/blender/blenkernel/intern/object.cc +++ b/source/blender/blenkernel/intern/object.cc @@ -137,6 +137,8 @@ #include "ANIM_action_legacy.hh" +#include "RNA_prototypes.hh" + #ifdef WITH_PYTHON # include "BPY_extern.hh" #endif @@ -4459,6 +4461,8 @@ bool BKE_object_shapekey_remove(Main *bmain, Object *ob, KeyBlock *kb) return false; } + BKE_animdata_drivers_remove_for_rna_struct(key->id, RNA_ShapeKey, kb); + kb_index = BLI_findindex(&key->block, kb); BLI_assert(kb_index != -1); diff --git a/source/blender/editors/object/object_constraint.cc b/source/blender/editors/object/object_constraint.cc index 6855854d0b3..bfc6c4e5091 100644 --- a/source/blender/editors/object/object_constraint.cc +++ b/source/blender/editors/object/object_constraint.cc @@ -2759,7 +2759,7 @@ static int pose_ik_clear_exec(bContext *C, wmOperator * /*op*/) for (con = static_cast(pchan->constraints.first); con; con = next) { next = con->next; if (con->type == CONSTRAINT_TYPE_KINEMATIC) { - BKE_constraint_remove(&pchan->constraints, con); + BKE_constraint_remove_ex(&pchan->constraints, ob, con); } } pchan->constflag &= ~(PCHAN_HAS_IK | PCHAN_HAS_NO_TARGET); diff --git a/source/blender/editors/object/object_modifier.cc b/source/blender/editors/object/object_modifier.cc index 4feb6f03ce2..c31f383b526 100644 --- a/source/blender/editors/object/object_modifier.cc +++ b/source/blender/editors/object/object_modifier.cc @@ -384,6 +384,8 @@ static bool object_modifier_remove( ob->mode &= ~OB_MODE_PARTICLE_EDIT; } + BKE_animdata_drivers_remove_for_rna_struct(ob->id, RNA_Modifier, md); + BKE_modifier_remove_from_list(ob, md); BKE_modifier_free(md); BKE_object_free_derived_caches(ob); diff --git a/source/blender/editors/object/object_relations.cc b/source/blender/editors/object/object_relations.cc index 436e8b6a672..7bab504f690 100644 --- a/source/blender/editors/object/object_relations.cc +++ b/source/blender/editors/object/object_relations.cc @@ -1199,7 +1199,7 @@ static int object_track_clear_exec(bContext *C, wmOperator *op) CONSTRAINT_TYPE_LOCKTRACK, CONSTRAINT_TYPE_DAMPTRACK)) { - BKE_constraint_remove(&ob->constraints, con); + BKE_constraint_remove_ex(&ob->constraints, ob, con); } } diff --git a/source/blender/makesrna/intern/rna_object.cc b/source/blender/makesrna/intern/rna_object.cc index c6d1fc2f263..f440bf8ed6b 100644 --- a/source/blender/makesrna/intern/rna_object.cc +++ b/source/blender/makesrna/intern/rna_object.cc @@ -1630,7 +1630,7 @@ static void rna_Object_constraints_remove(Object *object, return; } - BKE_constraint_remove(&object->constraints, con); + BKE_constraint_remove_ex(&object->constraints, object, con); RNA_POINTER_INVALIDATE(con_ptr); blender::ed::object::constraint_update(bmain, object); diff --git a/source/blender/makesrna/intern/rna_pose.cc b/source/blender/makesrna/intern/rna_pose.cc index d04876511f8..8a11b4a1796 100644 --- a/source/blender/makesrna/intern/rna_pose.cc +++ b/source/blender/makesrna/intern/rna_pose.cc @@ -388,7 +388,7 @@ static void rna_PoseChannel_constraints_remove( return; } - BKE_constraint_remove(&pchan->constraints, con); + BKE_constraint_remove_ex(&pchan->constraints, ob, con); RNA_POINTER_INVALIDATE(con_ptr); blender::ed::object::constraint_update(bmain, ob); diff --git a/tests/python/bl_animation_drivers.py b/tests/python/bl_animation_drivers.py index c6414aeccc9..dc290efaa50 100644 --- a/tests/python/bl_animation_drivers.py +++ b/tests/python/bl_animation_drivers.py @@ -194,6 +194,68 @@ class ContextViewLayerDriverTest(AbstractEmptyDriverTest, unittest.TestCase): self.assertPropValue('test_fallback', 321) +class SubDataDriverRemovalTest(AbstractEmptyDriverTest, unittest.TestCase): + + def test_remove_object_modifier(self): + # Removing a modifier with a driver should also delete the driver + modifier = self.obj.modifiers.new("test", 'ARRAY') + # No animation data means no drivers. + self.assertEqual(self.obj.animation_data, None) + self.obj.driver_add('modifiers["test"].count') + self.assertEqual(len(self.obj.animation_data.drivers), 1) + self.obj.modifiers.remove(modifier) + self.assertEqual(len(self.obj.modifiers), 0) + self.assertEqual(len(self.obj.animation_data.drivers), 0, + "Removing the modifier should remove the driver on it") + + def test_remove_object_constraint(self): + # Using limit distance constraint because that has a property that can have a driver. + constraint = self.obj.constraints.new('LIMIT_DISTANCE') + constraint.name = "test" + # No animation data means no drivers. + self.assertEqual(self.obj.animation_data, None) + self.obj.driver_add('constraints["test"].distance') + self.assertEqual(len(self.obj.animation_data.drivers), 1) + self.obj.constraints.remove(constraint) + self.assertEqual(len(self.obj.constraints), 0) + self.assertEqual(len(self.obj.animation_data.drivers), 0, + "Removing the constraint should remove the driver on it") + + def test_remove_bone_constraint(self): + arm = bpy.data.armatures.new('Armature') + arm_ob = bpy.data.objects.new('ArmObject', arm) + bpy.context.scene.collection.objects.link(arm_ob) + bpy.context.view_layer.objects.active = arm_ob + bpy.ops.object.mode_set(mode='EDIT') + ebone = arm.edit_bones.new(name="test") + ebone.tail = (1, 0, 0) + bpy.ops.object.mode_set(mode='POSE') + pose_bone = arm_ob.pose.bones["test"] + constraint = pose_bone.constraints.new('LIMIT_DISTANCE') + constraint.name = "test" + self.assertEqual(len(pose_bone.constraints), 1) + arm_ob.driver_add('pose.bones["test"].constraints["test"].distance') + self.assertEqual(len(arm_ob.animation_data.drivers), 1) + pose_bone.constraints.remove(constraint) + self.assertEqual(len(pose_bone.constraints), 0) + self.assertEqual(len(arm_ob.animation_data.drivers), 0, + "Removing the constraint should remove the driver on it") + + def test_remove_shapekey(self): + self.obj.shape_key_add(name="base") + test_key = self.obj.shape_key_add(name="test") + # Due to the weirdness of shapekeys, this is an ID. + shape_key_id = self.obj.data.shape_keys + self.assertEqual(len(shape_key_id.key_blocks), 2) + shape_key_id.driver_add('key_blocks["test"].value') + self.assertEqual(len(shape_key_id.animation_data.drivers), 1) + + self.obj.shape_key_remove(test_key) + self.assertEqual(len(shape_key_id.key_blocks), 1) + self.assertEqual(len(shape_key_id.animation_data.drivers), 0, + "Removing the shape key should remove any driver on it") + + def main(): global args import argparse