From 3dbede128ebcd3d9e9cc1ff7576308ebac8de9c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Thu, 20 Feb 2025 11:58:01 +0100 Subject: [PATCH] Fix #134581: Regression: Animation breaking going from 4.3 to 4.4 Fix an issue where the versioning of Action & slot assignments did not use RNA properties to do the slot assignment. This caused certain on-update callbacks to be missed, which in turn meant that an Action constraint could remain disabled even though its action slot assignment had been corrected. This is now resolved by actually using RNA to set the assigned slot in the versioning code. Unfortunately that does mean that any reporting done will be by the generic RNA code as well, and won't be specific to versioning. This shouldn't be much of an issue in practice, as any warning was only shown in the rare case of mis-matched `action.idroot` properties. Pull Request: https://projects.blender.org/blender/blender/pulls/134759 --- .../blender/animrig/ANIM_action_iterators.hh | 22 +++++ .../animrig/intern/action_iterators.cc | 91 +++++++++++++++++++ .../animrig/intern/action_iterators_test.cc | 34 +++++++ source/blender/animrig/intern/versioning.cc | 62 +++---------- 4 files changed, 162 insertions(+), 47 deletions(-) diff --git a/source/blender/animrig/ANIM_action_iterators.hh b/source/blender/animrig/ANIM_action_iterators.hh index e1647509235..2050c5c7a65 100644 --- a/source/blender/animrig/ANIM_action_iterators.hh +++ b/source/blender/animrig/ANIM_action_iterators.hh @@ -82,4 +82,26 @@ bool foreach_action_slot_use_with_references( slot_handle_t &slot_handle_ref, char *last_slot_identifier)> callback); +/** + * Essentially the same as foreach_action_slot_use(), except that it provides + * the ID as well as the RNA properties via which the callback can modify which + * Action/slot is assigned. + * + * The ID passed to the callback is always the same `animated_id` as is passed + * to this function. The actions & slots passed to the callback are *not* + * necessarily the direct action & slot of that ID: they can also be the action + * & slot of an Action Constraint or NLA Strip owned by the ID. + * + * \note this function CANNOT be used to change which Action is assigned, as that makes the + * PointerRNA/PropertyRNA values invalid. + * + * \see foreach_action_slot_use_with_references + */ +bool foreach_action_slot_use_with_rna(ID &animated_id, + FunctionRef callback); + } // namespace blender::animrig diff --git a/source/blender/animrig/intern/action_iterators.cc b/source/blender/animrig/intern/action_iterators.cc index a01e7a8fc0b..424ab7db3f6 100644 --- a/source/blender/animrig/intern/action_iterators.cc +++ b/source/blender/animrig/intern/action_iterators.cc @@ -16,6 +16,9 @@ #include "DNA_constraint_types.h" +#include "RNA_access.hh" +#include "RNA_prototypes.hh" + namespace blender::animrig { void foreach_fcurve_in_action(Action &action, FunctionRef callback) @@ -170,4 +173,92 @@ bool foreach_action_slot_use_with_references( return true; } +/* This function has to copy the logic of foreach_action_slot_use_with_references(), as it needs to + * know where exactly those pointers came from. */ +bool foreach_action_slot_use_with_rna(ID &animated_id, + FunctionRef callback) +{ + AnimData *adt = BKE_animdata_from_id(&animated_id); + + if (adt) { + if (adt->action) { + /* Direct assignment. */ + PointerRNA ptr = RNA_pointer_create_discrete(&animated_id, &RNA_AnimData, adt); + PropertyRNA *prop = RNA_struct_find_property(&ptr, "action_slot"); + if (!callback(animated_id, adt->action, ptr, *prop, adt->last_slot_identifier)) { + return false; + } + } + + /* NLA strips. */ + const bool looped_until_last_strip = bke::nla::foreach_strip_adt(*adt, [&](NlaStrip *strip) { + if (strip->act) { + PointerRNA ptr = RNA_pointer_create_discrete(&animated_id, &RNA_NlaStrip, strip); + PropertyRNA *prop = RNA_struct_find_property(&ptr, "action_slot"); + + if (!callback(animated_id, strip->act, ptr, *prop, strip->last_slot_identifier)) { + return false; + } + } + return true; + }); + if (!looped_until_last_strip) { + return false; + } + } + + /* The rest of the code deals with constraints, so only relevant when this is an Object. */ + if (GS(animated_id.name) != ID_OB) { + return true; + } + + const Object &object = reinterpret_cast(animated_id); + + /** + * Visit a constraint, and call the callback if it's an Action constraint. + * + * \returns whether to continue looping over possible uses of Actions, i.e. + * the return value of the callback. + */ + auto visit_constraint = [&](bConstraint &constraint) -> bool { + if (constraint.type != CONSTRAINT_TYPE_ACTION) { + return true; + } + bActionConstraint *constraint_data = static_cast(constraint.data); + if (!constraint_data->act) { + return true; + } + + PointerRNA ptr = RNA_pointer_create_discrete(&animated_id, &RNA_ActionConstraint, &constraint); + PropertyRNA *prop = RNA_struct_find_property(&ptr, "action_slot"); + + return callback( + animated_id, constraint_data->act, ptr, *prop, constraint_data->last_slot_identifier); + }; + + /* Visit Object constraints. */ + LISTBASE_FOREACH (bConstraint *, con, &object.constraints) { + if (!visit_constraint(*con)) { + return false; + } + } + + /* Visit Pose Bone constraints. */ + if (object.type == OB_ARMATURE) { + LISTBASE_FOREACH (bPoseChannel *, pchan, &object.pose->chanbase) { + LISTBASE_FOREACH (bConstraint *, con, &pchan->constraints) { + if (!visit_constraint(*con)) { + return false; + } + } + } + } + + return true; +} + } // namespace blender::animrig diff --git a/source/blender/animrig/intern/action_iterators_test.cc b/source/blender/animrig/intern/action_iterators_test.cc index 97632c81486..1f51ef3ef32 100644 --- a/source/blender/animrig/intern/action_iterators_test.cc +++ b/source/blender/animrig/intern/action_iterators_test.cc @@ -12,6 +12,9 @@ #include "DNA_anim_types.h" #include "DNA_object_types.h" +#include "RNA_access.hh" +#include "RNA_prototypes.hh" + #include "CLG_log.h" #include "testing/testing.h" @@ -156,4 +159,35 @@ TEST_F(ActionIteratorsTest, foreach_action_slot_use_with_references) << action_and_slot->second->identifier; } +TEST_F(ActionIteratorsTest, foreach_action_slot_use_with_rna) +{ + /* Create a cube and assign the Action + a slot. */ + Object *cube = static_cast(BKE_id_new(bmain, ID_OB, "OBCube")); + Slot *slot_cube = assign_action_ensure_slot_for_keying(*action, cube->id); + ASSERT_NE(slot_cube, nullptr); + Slot &another_slot = action->slot_add(); + + const auto assign_other_slot = [&](ID & /* animated_id */, + bAction *action, + PointerRNA &action_slot_owner_ptr, + PropertyRNA &action_slot_prop, + char * /*last_slot_identifier*/) -> bool { + PointerRNA rna_slot = RNA_pointer_create_discrete(&action->id, &RNA_ActionSlot, &another_slot); + RNA_property_pointer_set(&action_slot_owner_ptr, &action_slot_prop, rna_slot, nullptr); + return true; + }; + + foreach_action_slot_use_with_rna(cube->id, assign_other_slot); + + /* Check the result, the slot assignment should have been changed. */ + std::optional> action_and_slot = get_action_slot_pair(cube->id); + + ASSERT_TRUE(action_and_slot.has_value()); + EXPECT_EQ(action, action_and_slot->first) + << "Expected Action " << action->id.name << " but found " << action_and_slot->first->id.name; + EXPECT_EQ(&another_slot, action_and_slot->second) + << "Expected Slot " << another_slot.identifier << " but found " + << action_and_slot->second->identifier; +} + } // namespace blender::animrig::tests diff --git a/source/blender/animrig/intern/versioning.cc b/source/blender/animrig/intern/versioning.cc index a7f8d60b118..90ad814720f 100644 --- a/source/blender/animrig/intern/versioning.cc +++ b/source/blender/animrig/intern/versioning.cc @@ -29,6 +29,9 @@ #include "BLO_readfile.hh" +#include "RNA_access.hh" +#include "RNA_prototypes.hh" + namespace blender::animrig::versioning { bool action_is_layered(const bAction &dna_action) @@ -189,11 +192,12 @@ void tag_action_users_for_slotted_actions_conversion(Main &bmain) void convert_legacy_action_assignments(Main &bmain, ReportList *reports) { auto version_slot_assignment = [&](ID &animated_id, - bAction *&action_ptr_ref, - slot_handle_t &slot_handle_ref, + bAction *dna_action, + PointerRNA &action_slot_owner_ptr, + PropertyRNA &action_slot_prop, char *last_used_slot_identifier) { - BLI_assert(action_ptr_ref); /* Ensured by the foreach loop. */ - Action &action = action_ptr_ref->wrap(); + BLI_assert(dna_action); /* Ensured by the foreach loop. */ + Action &action = dna_action->wrap(); if (action.slot_array_num == 0) { /* animated_id is from an older file (because it is in the being-versioned-right-now bmain), @@ -242,47 +246,11 @@ void convert_legacy_action_assignments(Main &bmain, ReportList *reports) return true; } - const ActionSlotAssignmentResult result = generic_assign_action_slot( - slot_to_assign, animated_id, action_ptr_ref, slot_handle_ref, last_used_slot_identifier); - switch (result) { - case ActionSlotAssignmentResult::OK: - break; - case ActionSlotAssignmentResult::SlotNotSuitable: - /* If the slot wasn't suitable for the ID, we force assignment anyway, - * but with a warning. - * - * This happens when the legacy action assigned to the ID had a - * mismatched idroot, and therefore the created slot does as well. - * This mismatch can happen in a variety of ways, and we opt to - * preserve this unusual (but technically valid) state of affairs. - */ - slot_handle_ref = slot_to_assign->handle; - BLI_strncpy_utf8( - last_used_slot_identifier, slot_to_assign->identifier, Slot::identifier_length_max); - /* Not necessary to add this ID to the slot user list, as that list is - * going to get refreshed after versioning anyway. */ - - BKE_reportf( - reports, - RPT_WARNING, - "Legacy action \"%s\" is assigned to \"%s\", which does not match the " - "action's id_root \"%s\". The action has been upgraded to a slotted action with " - "slot \"%s\" with an id_type \"%s\", which has also been assigned to \"%s\" despite " - "this type mismatch. This likely indicates something odd about the blend file.\n", - action.id.name + 2, - animated_id.name, - slot_to_assign->idtype_string().c_str(), - slot_to_assign->identifier_without_prefix().c_str(), - slot_to_assign->idtype_string().c_str(), - animated_id.name); - break; - case ActionSlotAssignmentResult::SlotNotFromAction: - BLI_assert_msg(false, "SlotNotFromAction should not be returned here"); - break; - case ActionSlotAssignmentResult::MissingAction: - BLI_assert_msg(false, "MissingAction should not be returned here"); - break; - } + PointerRNA slot_to_assign_ptr = RNA_pointer_create_discrete( + &action.id, &RNA_ActionSlot, slot_to_assign); + RNA_property_pointer_set( + &action_slot_owner_ptr, &action_slot_prop, slot_to_assign_ptr, reports); + RNA_property_update_main(&bmain, nullptr, &action_slot_owner_ptr, &action_slot_prop); return true; }; @@ -291,7 +259,7 @@ void convert_legacy_action_assignments(Main &bmain, ReportList *reports) FOREACH_MAIN_ID_BEGIN (&bmain, id) { /* Process the ID itself. */ if (BLO_readfile_id_runtime_tags(*id).action_assignment_needs_slot) { - foreach_action_slot_use_with_references(*id, version_slot_assignment); + foreach_action_slot_use_with_rna(*id, version_slot_assignment); id->runtime.readfile_data->tags.action_assignment_needs_slot = false; } @@ -301,7 +269,7 @@ void convert_legacy_action_assignments(Main &bmain, ReportList *reports) * node tree. */ bNodeTree *node_tree = blender::bke::node_tree_from_id(id); if (node_tree && BLO_readfile_id_runtime_tags(node_tree->id).action_assignment_needs_slot) { - foreach_action_slot_use_with_references(node_tree->id, version_slot_assignment); + foreach_action_slot_use_with_rna(node_tree->id, version_slot_assignment); node_tree->id.runtime.readfile_data->tags.action_assignment_needs_slot = false; } }