griefith/test
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Fix #134581: Regression: Animation breaking going from 4.3 to 4.4

Browse Source 
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
This commit is contained in:
Sybren A. Stüvel
2025-02-20 11:58:01 +01:00
parent 8ff405bc06
commit 3dbede128e
4 changed files with 162 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
source/blender/animrig/ANIM_action_iterators.hh
View File

@@ -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<bool(ID &animated_id,
bAction *action,
PointerRNA &action_slot_owner_ptr,
PropertyRNA &action_slot_prop,
char *last_slot_identifier)> callback);
} // namespace blender::animrig

91
source/blender/animrig/intern/action_iterators.cc
View File

@@ -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<void(FCurve &fcurve)> 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<bool(ID &animated_id,
bAction *action,
PointerRNA &action_slot_ptr,
PropertyRNA &action_slot_prop,
char *last_slot_identifier)> 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<const Object &>(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<bActionConstraint *>(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

34
source/blender/animrig/intern/action_iterators_test.cc
View File

@@ -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<Object *>(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<std::pair<Action *, Slot *>> 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

62
source/blender/animrig/intern/versioning.cc
View File

@@ -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;
}
}