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

Refactor: Anim: add function to ensure an F-Curve exists in legacy Action

Browse Source 
Split the legacy Action handling code of `action_fcurve_ensure()` into a
new function `action_fcurve_ensure_legacy()`. This makes it possible for
unit tests to explicitly create a legacy Action for testing, regardless
of the 'Slotted Actions' experimental feature flag.

When we drop that flag, the unit tests that explicitly test legacy
behaviour will keep running.

Pull Request: https://projects.blender.org/blender/blender/pulls/128892
This commit is contained in:
Sybren A. Stüvel
2024-10-11 13:07:20 +02:00
parent f4fc683fc4
commit b7049a7c04
5 changed files with 140 additions and 61 deletions
Download Patch File Download Diff File Expand all files Collapse all files

21
source/blender/animrig/ANIM_action.hh
View File

@@ -1342,10 +1342,8 @@ Span<const FCurve *> fcurves_for_action_slot(const Action &action, slot_handle_t
* previously-unanimated depsgraph component may become animated now.
*
* \param ptr: RNA pointer for the struct the fcurve is being looked up/created
* for. For legacy actions this is optional and may be null, but if provided is
* used to do things like set the fcurve color properly. For layered actions
* this parameter is required, and is used to create and assign an appropriate
* slot if needed when creating the fcurve.
* for. It is used to create and assign an appropriate slot if needed when
* creating the fcurve, and set the fcurve color properly
*
* \param fcurve_descriptor: description of the fcurve to lookup/create. Note
* that this is *not* relative to `ptr` (e.g. if `ptr` is not an ID). It should
@@ -1357,6 +1355,21 @@ FCurve *action_fcurve_ensure(Main *bmain,
PointerRNA *ptr,
FCurveDescriptor fcurve_descriptor);
/**
* Same as above, but creates a legacy Action.
*
* \note this function should ONLY be used in unit tests, in order to create
* legacy Actions for testing. Or in the very rare cases where handling of
* legacy Actions is still necessary AND you have no PointerRNA. In all other
* cases, just call #action_fcurve_ensure, it'll do the right thing
* transparently on whatever Action you give it.
*/
FCurve *action_fcurve_ensure_legacy(Main *bmain,
bAction *act,
const char group[],
PointerRNA *ptr,
FCurveDescriptor fcurve_descriptor);
/**
* Find the F-Curve in the given Action.
*

101
source/blender/animrig/intern/action.cc
View File

@@ -2394,50 +2394,65 @@ FCurve *action_fcurve_ensure(Main *bmain,
return nullptr;
}
if (!animrig::legacy::action_treat_as_legacy(*act)) {
/* NOTE: for layered actions we require the following:
*
* - `ptr` is non-null.
* - `ptr` has an `owner_id` that already uses `act`.
*
* This isn't for any principled reason, but rather is because adding
* support for layered actions to this function was a fix to make Follow
* Path animation work properly with layered actions (see PR #124353), and
* those are the requirements the Follow Path code conveniently met.
* Moreover those requirements were also already met by the other call sites
* that potentially call this function with layered actions.
*
* Trying to puzzle out what "should" happen when these requirements don't
* hold, or if this is even the best place to handle the layered action
* cases at all, was leading to discussion of larger changes than made sense
* to tackle at that point. */
Action &action = act->wrap();
BLI_assert(ptr != nullptr);
if (ptr == nullptr || ptr->owner_id == nullptr) {
return nullptr;
}
ID &animated_id = *ptr->owner_id;
BLI_assert(get_action(animated_id) == &action);
if (get_action(animated_id) != &action) {
return nullptr;
}
/* Ensure the id has an assigned slot. */
Slot *slot = assign_action_ensure_slot_for_keying(action, animated_id);
if (!slot) {
/* This means the ID type is not animatable. */
return nullptr;
}
action.layer_keystrip_ensure();
assert_baklava_phase_1_invariants(action);
StripKeyframeData &strip_data = action.layer(0)->strip(0)->data<StripKeyframeData>(action);
return &strip_data.channelbag_for_slot_ensure(*slot).fcurve_ensure(bmain, fcurve_descriptor);
if (animrig::legacy::action_treat_as_legacy(*act)) {
return action_fcurve_ensure_legacy(bmain, act, group, ptr, fcurve_descriptor);
}
/* NOTE: for layered actions we require the following:
*
* - `ptr` is non-null.
* - `ptr` has an `owner_id` that already uses `act`.
*
* This isn't for any principled reason, but rather is because adding
* support for layered actions to this function was a fix to make Follow
* Path animation work properly with layered actions (see PR #124353), and
* those are the requirements the Follow Path code conveniently met.
* Moreover those requirements were also already met by the other call sites
* that potentially call this function with layered actions.
*
* Trying to puzzle out what "should" happen when these requirements don't
* hold, or if this is even the best place to handle the layered action
* cases at all, was leading to discussion of larger changes than made sense
* to tackle at that point. */
Action &action = act->wrap();
BLI_assert(ptr != nullptr);
if (ptr == nullptr || ptr->owner_id == nullptr) {
return nullptr;
}
ID &animated_id = *ptr->owner_id;
BLI_assert(get_action(animated_id) == &action);
if (get_action(animated_id) != &action) {
return nullptr;
}
/* Ensure the id has an assigned slot. */
Slot *slot = assign_action_ensure_slot_for_keying(action, animated_id);
if (!slot) {
/* This means the ID type is not animatable. */
return nullptr;
}
action.layer_keystrip_ensure();
assert_baklava_phase_1_invariants(action);
StripKeyframeData &strip_data = action.layer(0)->strip(0)->data<StripKeyframeData>(action);
return &strip_data.channelbag_for_slot_ensure(*slot).fcurve_ensure(bmain, fcurve_descriptor);
}
FCurve *action_fcurve_ensure_legacy(Main *bmain,
bAction *act,
const char group[],
PointerRNA *ptr,
FCurveDescriptor fcurve_descriptor)
{
if (!act) {
return nullptr;
}
BLI_assert(act->wrap().is_empty() || act->wrap().is_action_legacy());
/* Try to find f-curve matching for this setting.
* - add if not found and allowed to add one
* TODO: add auto-grouping support? how this works will need to be resolved
@@ -2463,7 +2478,7 @@ FCurve *action_fcurve_ensure(Main *bmain,
BLI_assert_msg(!fcurve_descriptor.prop_subtype.has_value(),
"Did not expect a prop_subtype to be passed in. This is fine, but does need some "
"changes to action_fcurve_ensure() to deal with it");
"changes to action_fcurve_ensure_legacy() to deal with it");
fcu = create_fcurve_for_channel(
{fcurve_descriptor.rna_path, fcurve_descriptor.array_index, prop_subtype});

24
source/blender/animrig/intern/action_test.cc
View File

@@ -823,8 +823,8 @@ TEST_F(ActionLayersTest, action_slot_get_id_for_keying__empty_action)
/* Double-check that the action is considered empty for the test. */
EXPECT_TRUE(action->is_empty());
/* A `primary_id` that uses the action should get returned. Every other case
* should return nullptr. */
/* None should return an ID, since there are no slots yet which could have this ID assigned.
* Assignment of the Action itself (cube) shouldn't matter. */
EXPECT_EQ(&cube->id, action_slot_get_id_for_keying(*bmain, *action, 0, &cube->id));
EXPECT_EQ(nullptr, action_slot_get_id_for_keying(*bmain, *action, 0, nullptr));
EXPECT_EQ(nullptr, action_slot_get_id_for_keying(*bmain, *action, 0, &suzanne->id));
@@ -832,7 +832,7 @@ TEST_F(ActionLayersTest, action_slot_get_id_for_keying__empty_action)
TEST_F(ActionLayersTest, action_slot_get_id_for_keying__legacy_action)
{
FCurve *fcurve = action_fcurve_ensure(bmain, action, nullptr, nullptr, {"location", 0});
FCurve *fcurve = action_fcurve_ensure_legacy(bmain, action, nullptr, nullptr, {"location", 0});
EXPECT_FALSE(fcurve == nullptr);
EXPECT_TRUE(assign_action(action, cube->id));
@@ -877,8 +877,10 @@ TEST_F(ActionLayersTest, action_slot_get_id_for_keying__layered_action)
TEST_F(ActionLayersTest, conversion_to_layered)
{
EXPECT_TRUE(action->is_empty());
FCurve *legacy_fcu_0 = action_fcurve_ensure(bmain, action, "Test", nullptr, {"location", 0});
FCurve *legacy_fcu_1 = action_fcurve_ensure(bmain, action, "Test", nullptr, {"location", 1});
FCurve *legacy_fcu_0 = action_fcurve_ensure_legacy(
bmain, action, "Test", nullptr, {"location", 0});
FCurve *legacy_fcu_1 = action_fcurve_ensure_legacy(
bmain, action, "Test", nullptr, {"location", 1});
KeyframeSettings settings;
settings.handle = HD_AUTO;
@@ -910,7 +912,7 @@ TEST_F(ActionLayersTest, conversion_to_layered)
Action *long_name_action = static_cast<Action *>(BKE_id_new(
bmain, ID_AC, "name_for_an_action_that_is_exactly_64_chars_which_is_MAX_ID_NAME"));
action_fcurve_ensure(bmain, long_name_action, "Long", nullptr, {"location", 0});
action_fcurve_ensure_legacy(bmain, long_name_action, "Long", nullptr, {"location", 0});
converted = convert_to_layered_action(*bmain, *long_name_action);
/* AC gets added automatically by Blender, the long name is shortened to make space for
* "_layered". */
@@ -921,11 +923,11 @@ TEST_F(ActionLayersTest, conversion_to_layered)
TEST_F(ActionLayersTest, conversion_to_layered_action_groups)
{
EXPECT_TRUE(action->is_empty());
action_fcurve_ensure(bmain, action, "Test", nullptr, {"location", 0});
action_fcurve_ensure(bmain, action, "Test", nullptr, {"rotation_euler", 1});
action_fcurve_ensure(bmain, action, "Test_Two", nullptr, {"scale", 1});
action_fcurve_ensure(bmain, action, "Test_Three", nullptr, {"show_name", 1});
action_fcurve_ensure(bmain, action, "Test_Rename", nullptr, {"show_axis", 1});
action_fcurve_ensure_legacy(bmain, action, "Test", nullptr, {"location", 0});
action_fcurve_ensure_legacy(bmain, action, "Test", nullptr, {"rotation_euler", 1});
action_fcurve_ensure_legacy(bmain, action, "Test_Two", nullptr, {"scale", 1});
action_fcurve_ensure_legacy(bmain, action, "Test_Three", nullptr, {"show_name", 1});
action_fcurve_ensure_legacy(bmain, action, "Test_Rename", nullptr, {"show_axis", 1});
bActionGroup *rename_group = static_cast<bActionGroup *>(BLI_findlink(&action->groups, 3));
ASSERT_NE(rename_group, nullptr);

53
source/blender/animrig/intern/keyframing_test.cc
View File

@@ -106,6 +106,7 @@ class KeyframingTest : public testing::Test {
object_with_nla = BKE_object_add_only_object(bmain, OB_EMPTY, "EmptyWithNLA");
object_with_nla_rna_pointer = RNA_id_pointer_create(&object_with_nla->id);
nla_action = static_cast<bAction *>(BKE_id_new(bmain, ID_AC, "NLAAction"));
this->ensure_action_is_legacy(*nla_action);
cube = BKE_object_add_only_object(bmain, OB_MESH, "cube");
cube_rna_pointer = RNA_id_pointer_create(&cube->id);
@@ -142,6 +143,40 @@ class KeyframingTest : public testing::Test {
{
BKE_main_free(bmain);
}
/**
* Create a legacy Action and assign it to the ID.
*
* Use of this function indicates that the unit test should be converted to
* use layered Actions.
*/
void ensure_legacy_action_assigned(ID &id)
{
AnimData *adt = BKE_animdata_from_id(&id);
BLI_assert(!adt || !adt->action);
UNUSED_VARS_NDEBUG(adt);
bAction &action = animrig::action_add(*bmain, "LegacyAction");
this->ensure_action_is_legacy(action);
const bool ok = assign_action(&action, id);
BLI_assert(ok);
UNUSED_VARS_NDEBUG(ok);
}
/**
* Add an F-Curve Group. This marks the Action as legacy. Since most tests are on F-Curves,
* adding a Group here is the less invasive way to make this a Legacy Action.
*
* Use of this function indicates the test should be converted to layered Actions.
*/
void ensure_action_is_legacy(bAction &action)
{
bActionGroup *new_group = static_cast<bActionGroup *>(
MEM_callocN(sizeof(bActionGroup), __func__));
STRNCPY(new_group->name, "Legacy Forcer");
BLI_addtail(&action.groups, new_group);
}
};
/* ------------------------------------------------------------
@@ -978,6 +1013,7 @@ TEST_F(KeyframingTest, insert_keyframes__legacy_action__non_array_property)
/* First time should create the AnimData, Action, and FCurve with a single
* key. */
object->empty_drawsize = 42.0;
ensure_legacy_action_assigned(object->id);
const CombinedKeyingResult result_1 = insert_keyframes(bmain,
&object_rna_pointer,
std::nullopt,
@@ -1039,6 +1075,7 @@ TEST_F(KeyframingTest, insert_keyframes__legacy_action__optional_frame)
AnimationEvalContext anim_eval_context = {nullptr, 5.0};
object->rotmode = ROT_MODE_XYZ;
ensure_legacy_action_assigned(object->id);
const CombinedKeyingResult result_1 = insert_keyframes(bmain,
&object_rna_pointer,
std::nullopt,
@@ -1075,6 +1112,7 @@ TEST_F(KeyframingTest, insert_keyframes__legacy_action__optional_channel_group)
AnimationEvalContext anim_eval_context = {nullptr, 1.0};
/* If the channel group is not explicitly passed, the default should be used. */
ensure_legacy_action_assigned(object->id);
const CombinedKeyingResult result_1 = insert_keyframes(
bmain,
&object_rna_pointer,
@@ -1122,6 +1160,7 @@ TEST_F(KeyframingTest, insert_keyframes__single_element)
{
AnimationEvalContext anim_eval_context = {nullptr, 1.0};
ensure_legacy_action_assigned(object->id);
const CombinedKeyingResult result = insert_keyframes(bmain,
&object_rna_pointer,
std::nullopt,
@@ -1143,6 +1182,7 @@ TEST_F(KeyframingTest, insert_keyframes__legacy_action__all_elements)
{
AnimationEvalContext anim_eval_context = {nullptr, 1.0};
ensure_legacy_action_assigned(object->id);
const CombinedKeyingResult result = insert_keyframes(bmain,
&object_rna_pointer,
std::nullopt,
@@ -1169,6 +1209,7 @@ TEST_F(KeyframingTest, insert_keyframes__legacy_action__pose_bone_rna_pointer)
PointerRNA pose_bone_rna_pointer = RNA_pointer_create(
&armature_object->id, &RNA_PoseBone, pchan);
ensure_legacy_action_assigned(armature_object->id);
const CombinedKeyingResult result = insert_keyframes(bmain,
&pose_bone_rna_pointer,
std::nullopt,
@@ -1192,6 +1233,7 @@ TEST_F(KeyframingTest, insert_keyframes__legacy_action__pose_bone_owner_id_point
{
AnimationEvalContext anim_eval_context = {nullptr, 1.0};
ensure_legacy_action_assigned(armature_object->id);
const CombinedKeyingResult result = insert_keyframes(
bmain,
&armature_object_rna_pointer,
@@ -1216,6 +1258,7 @@ TEST_F(KeyframingTest, insert_keyframes__legacy_action__multiple_properties)
{
AnimationEvalContext anim_eval_context = {nullptr, 1.0};
ensure_legacy_action_assigned(object->id);
const CombinedKeyingResult result =
insert_keyframes(bmain,
@@ -1269,6 +1312,10 @@ TEST_F(KeyframingTest, insert_keyframes__legacy_action__only_available)
* object without an action. */
ASSERT_EQ(nullptr, object->adt->action);
/* This will create & assign an Action, which is necessary for the
* insert_keyframes() function to take the 'legacy Action' code path. */
ensure_legacy_action_assigned(object->id);
/* Insert a key on two of the elements without using the flag so that there
* will be two fcurves. */
insert_keyframes(bmain,
@@ -1309,6 +1356,7 @@ TEST_F(KeyframingTest, insert_keyframes__legacy_action__only_replace)
object->rot[0] = 42.0;
object->rot[1] = 42.0;
object->rot[2] = 42.0;
ensure_legacy_action_assigned(object->id);
const CombinedKeyingResult result_1 = insert_keyframes(bmain,
&object_rna_pointer,
std::nullopt,
@@ -1397,6 +1445,7 @@ TEST_F(KeyframingTest, insert_keyframes__legacy_action__only_needed)
AnimationEvalContext anim_eval_context = {nullptr, 1.0};
/* First attempt should succeed, because there are no fcurves yet. */
ensure_legacy_action_assigned(object->id);
const CombinedKeyingResult result_1 = insert_keyframes(bmain,
&object_rna_pointer,
std::nullopt,
@@ -1528,7 +1577,7 @@ TEST_F(KeyframingTest, insert_keyframes__legacy_action__quaternion_on_nla__only_
/* Create an fcurve and key for a single quaternion channel. */
PointerRNA id_rna_ptr = RNA_id_pointer_create(&object_with_nla->id);
FCurve *fcu = action_fcurve_ensure(
FCurve *fcu = action_fcurve_ensure_legacy(
bmain, nla_action, nullptr, &id_rna_ptr, {"rotation_quaternion", 0});
const KeyframeSettings keyframe_settings = {BEZT_KEYTYPE_KEYFRAME, HD_AUTO_ANIM, BEZT_IPO_BEZ};
insert_vert_fcurve(fcu, {1.0, 1.0}, keyframe_settings, INSERTKEY_NOFLAGS);
@@ -1568,7 +1617,7 @@ TEST_F(KeyframingTest, insert_keyframes__legacy_action__quaternion_on_nla__only_
/* Directly create an fcurve and key for a single quaternion channel. */
PointerRNA id_rna_ptr = RNA_id_pointer_create(&object_with_nla->id);
FCurve *fcu = action_fcurve_ensure(
FCurve *fcu = action_fcurve_ensure_legacy(
bmain, nla_action, nullptr, &id_rna_ptr, {"rotation_quaternion", 0});
const KeyframeSettings keyframe_settings = {BEZT_KEYTYPE_KEYFRAME, HD_AUTO_ANIM, BEZT_IPO_BEZ};
insert_vert_fcurve(fcu, {11.0, 1.0}, keyframe_settings, INSERTKEY_NOFLAGS);

2
source/blender/makesrna/intern/rna_action.cc
View File

@@ -1127,7 +1127,7 @@ static FCurve *rna_Action_fcurve_new(bAction *act,
act->id.name + 2);
return nullptr;
}
return blender::animrig::action_fcurve_ensure(
return blender::animrig::action_fcurve_ensure_legacy(
bmain,
act,
fcurve_descriptor.channel_group ? fcurve_descriptor.channel_group->c_str() : nullptr,