diff --git a/source/blender/editors/animation/anim_filter.cc b/source/blender/editors/animation/anim_filter.cc index a1ddd2b2e20..9d07e53e4f3 100644 --- a/source/blender/editors/animation/anim_filter.cc +++ b/source/blender/editors/animation/anim_filter.cc @@ -1550,16 +1550,37 @@ static size_t animfilter_act_group(bAnimContext *ac, return items; } -/** - * Add a channel for each Slot, with their FCurves when the Slot is expanded. - */ -static size_t animfilter_action_slot(bAnimContext *ac, - ListBase *anim_data, - animrig::Action &action, - animrig::Slot &slot, - const eAnimFilter_Flags filter_mode, - ID *animated_id) +size_t ANIM_animfilter_action_slot(bAnimContext *ac, + ListBase * /* bAnimListElem */ anim_data, + animrig::Action &action, + animrig::Slot &slot, + const eAnimFilter_Flags filter_mode, + ID *animated_id) { + BLI_assert(ac); + + /* In some cases (see `ob_to_keylist()` and friends) fake bDopeSheet and fake bAnimContext are + * created. These are mostly null-initialized, and so do not have a bmain. This means that + * lookup of the animated ID is not possible, which can result in failure to look up the proper + * F-Curve display name. For the `..._to_keylist` functions that doesn't matter, as those are + * only interested in the key data anyway. So rather than trying to get a reliable `bmain` + * through the maze, this code just treats it as optional (even though ideally it should always + * be known). */ + ID *slot_user_id = nullptr; + if (ac->bmain) { + slot_user_id = animrig::action_slot_get_id_best_guess(*ac->bmain, slot, animated_id); + } + if (!slot_user_id) { + BLI_assert(animated_id); + /* At the time of writing this (PR #134922), downstream code (see e.g. + * `animfilter_fcurves_span()`) assumes this is non-null, so we need to set + * it to *something*. If it's not an actual user of the slot then channels + * might not resolve to an actual property and thus be displayed oddly in + * the channel list, but that's not technically a problem, it's just a + * little strange for the end user. */ + slot_user_id = animated_id; + } + /* Don't include anything from this animation if it is linked in from another * file, and we're getting stuff for editing... */ if ((filter_mode & ANIMFILTER_FOREDIT) && @@ -1584,7 +1605,7 @@ static size_t animfilter_action_slot(bAnimContext *ac, const bool show_slot_channel = (is_action_mode && selection_ok_for_slot && include_summary_channels); if (show_slot_channel) { - ANIMCHANNEL_NEW_CHANNEL(ac->bmain, &slot, ANIMTYPE_ACTION_SLOT, animated_id, &action.id); + ANIMCHANNEL_NEW_CHANNEL(ac->bmain, &slot, ANIMTYPE_ACTION_SLOT, slot_user_id, &action.id); items++; } @@ -1605,7 +1626,7 @@ static size_t animfilter_action_slot(bAnimContext *ac, /* Add channel groups and their member channels. */ for (bActionGroup *group : channelbag->channel_groups()) { items += animfilter_act_group( - ac, anim_data, &action, slot.handle, group, filter_mode, animated_id); + ac, anim_data, &action, slot.handle, group, filter_mode, slot_user_id); } /* Add ungrouped channels. */ @@ -1619,7 +1640,7 @@ static size_t animfilter_action_slot(bAnimContext *ac, Span fcurves = channelbag->fcurves().drop_front(first_ungrouped_fcurve_index); items += animfilter_fcurves_span( - ac, anim_data, fcurves, slot.handle, filter_mode, animated_id, &action.id); + ac, anim_data, fcurves, slot.handle, filter_mode, slot_user_id, &action.id); } return items; @@ -1643,22 +1664,7 @@ static size_t animfilter_action_slots(bAnimContext *ac, for (animrig::Slot *slot : action.slots()) { BLI_assert(slot); - /* In some cases (see `ob_to_keylist()` and friends) fake bDopeSheet and fake bAnimContext are - * created. These are mostly null-initialized, and so do not have a bmain. This means that - * lookup of the animated ID is not possible, which can result in failure to look up the proper - * F-Curve display name. For the `..._to_keylist` functions that doesn't matter, as those are - * only interested in the key data anyway. So rather than trying to get a reliable `bmain` - * through the maze, this code just treats it as optional (even though ideally it should always - * be known). */ - ID *animated_id = nullptr; - if (ac->bmain) { - animated_id = animrig::action_slot_get_id_best_guess(*ac->bmain, *slot, owner_id); - } - if (!animated_id) { - /* This is not necessarily correct, but at least it prevents nullptr dereference. */ - animated_id = owner_id; - } - num_items += animfilter_action_slot(ac, anim_data, action, *slot, filter_mode, animated_id); + num_items += ANIM_animfilter_action_slot(ac, anim_data, action, *slot, filter_mode, owner_id); } return num_items; @@ -1726,7 +1732,7 @@ static size_t animfilter_action(bAnimContext *ac, /* Can happen when an Action is assigned, but not a Slot. */ return 0; } - return animfilter_action_slot(ac, anim_data, action, *slot, filter_mode, owner_id); + return ANIM_animfilter_action_slot(ac, anim_data, action, *slot, filter_mode, owner_id); } /* Include NLA-Data for NLA-Editor: diff --git a/source/blender/editors/animation/keyframes_draw.cc b/source/blender/editors/animation/keyframes_draw.cc index dafd913d004..d78e5d14c15 100644 --- a/source/blender/editors/animation/keyframes_draw.cc +++ b/source/blender/editors/animation/keyframes_draw.cc @@ -420,6 +420,7 @@ struct ChannelListElement { bDopeSheet *ads; Scene *sce; Object *ob; + ID *animated_id; /* The ID that adt (below) belongs to. */ AnimData *adt; FCurve *fcu; bAction *act; @@ -453,18 +454,36 @@ static void build_channel_keylist(ChannelListElement *elem, blender::float2 rang break; } case ChannelType::ACTION_LAYERED: { - action_to_keylist(elem->adt, elem->act, elem->keylist, elem->saction_flag, range); + /* This is only called for action summaries in the Dopesheet, *not* the + * Action Editor. Therefore despite the name `ACTION_LAYERED`, this is + * only used to show a *single slot* of the action: the slot used by the + * ID the action is listed under. + * + * Thus we use the same function as the `ChannelType::ACTION_SLOT` case + * below because in practice the only distinction between these cases is + * where they get the slot from. In this case, we get it from `elem`'s + * ADT. */ + BLI_assert(elem->act); + BLI_assert(elem->adt); + action_slot_summary_to_keylist(elem->ac, + elem->animated_id, + elem->act->wrap(), + elem->adt->slot_handle, + elem->keylist, + elem->saction_flag, + range); break; } case ChannelType::ACTION_SLOT: { BLI_assert(elem->act); BLI_assert(elem->action_slot); - action_slot_to_keylist(elem->adt, - elem->act->wrap(), - elem->action_slot->handle, - elem->keylist, - elem->saction_flag, - range); + action_slot_summary_to_keylist(elem->ac, + elem->animated_id, + elem->act->wrap(), + elem->action_slot->handle, + elem->keylist, + elem->saction_flag, + range); break; } case ChannelType::ACTION_LEGACY: { @@ -716,6 +735,7 @@ void ED_add_fcurve_channel(ChannelDrawList *channel_list, ChannelListElement *draw_elem = channel_list_add_element( channel_list, ChannelType::FCURVE, ypos, yscale_fac, eSAction_Flag(saction_flag)); + draw_elem->animated_id = ale->id; draw_elem->adt = ale->adt; draw_elem->fcu = fcu; draw_elem->channel_locked = locked; @@ -735,12 +755,14 @@ void ED_add_action_group_channel(ChannelDrawList *channel_list, ChannelListElement *draw_elem = channel_list_add_element( channel_list, ChannelType::ACTION_GROUP, ypos, yscale_fac, eSAction_Flag(saction_flag)); + draw_elem->animated_id = ale->id; draw_elem->adt = ale->adt; draw_elem->agrp = agrp; draw_elem->channel_locked = locked; } void ED_add_action_layered_channel(ChannelDrawList *channel_list, + bAnimContext *ac, bAnimListElem *ale, bAction *action, const float ypos, @@ -755,12 +777,15 @@ void ED_add_action_layered_channel(ChannelDrawList *channel_list, ChannelListElement *draw_elem = channel_list_add_element( channel_list, ChannelType::ACTION_LAYERED, ypos, yscale_fac, eSAction_Flag(saction_flag)); + draw_elem->ac = ac; + draw_elem->animated_id = ale->id; draw_elem->adt = ale->adt; draw_elem->act = action; draw_elem->channel_locked = locked; } void ED_add_action_slot_channel(ChannelDrawList *channel_list, + bAnimContext *ac, bAnimListElem *ale, animrig::Action &action, animrig::Slot &slot, @@ -773,6 +798,8 @@ void ED_add_action_slot_channel(ChannelDrawList *channel_list, ChannelListElement *draw_elem = channel_list_add_element( channel_list, ChannelType::ACTION_SLOT, ypos, yscale_fac, eSAction_Flag(saction_flag)); + draw_elem->ac = ac; + draw_elem->animated_id = ale->id; draw_elem->adt = ale->adt; draw_elem->act = &action; draw_elem->action_slot = &slot; @@ -793,6 +820,7 @@ void ED_add_action_channel(ChannelDrawList *channel_list, ChannelListElement *draw_elem = channel_list_add_element( channel_list, ChannelType::ACTION_LEGACY, ypos, yscale_fac, eSAction_Flag(saction_flag)); + draw_elem->animated_id = ale->id; draw_elem->adt = ale->adt; draw_elem->act = act; draw_elem->channel_locked = locked; @@ -813,6 +841,7 @@ void ED_add_grease_pencil_datablock_channel(ChannelDrawList *channel_list, eSAction_Flag(saction_flag)); /* GreasePencil properties can be animated via an Action, so the GP-related * animation data is not limited to GP drawings. */ + draw_elem->animated_id = ale->id; draw_elem->adt = ale->adt; draw_elem->act = ale->adt ? ale->adt->action : nullptr; draw_elem->grease_pencil = grease_pencil; diff --git a/source/blender/editors/animation/keyframes_keylist.cc b/source/blender/editors/animation/keyframes_keylist.cc index 03359714dea..5961c585546 100644 --- a/source/blender/editors/animation/keyframes_keylist.cc +++ b/source/blender/editors/animation/keyframes_keylist.cc @@ -978,6 +978,50 @@ void summary_to_keylist(bAnimContext *ac, ANIM_animdata_freelist(&anim_data); } +void action_slot_summary_to_keylist(bAnimContext *ac, + ID *animated_id, + animrig::Action &action, + const animrig::slot_handle_t slot_handle, + AnimKeylist *keylist, + const int /* eSAction_Flag */ saction_flag, + blender::float2 range) +{ + /* TODO: downstream code depends on this being non-null (see e.g. + * `ANIM_animfilter_action_slot()` and `animfilter_fcurves_span()`). Either + * change this parameter to be a reference, or modify the downstream code to + * not assume that it's non-null and do something reasonable when it is null. */ + BLI_assert(animated_id); + + if (!ac) { + return; + } + + animrig::Slot *slot = action.slot_for_handle(slot_handle); + BLI_assert(slot); + + ListBase anim_data = {nullptr, nullptr}; + + /* Get F-Curves to take keyframes from. */ + const eAnimFilter_Flags filter = ANIMFILTER_DATA_VISIBLE; + ANIM_animfilter_action_slot(ac, &anim_data, action, *slot, filter, animated_id); + + LISTBASE_FOREACH (const bAnimListElem *, ale, &anim_data) { + /* As of the writing of this code, Actions ultimately only contain FCurves. + * If/when that changes in the future, this may need to be updated. */ + if (ale->datatype != ALE_FCURVE) { + continue; + } + fcurve_to_keylist(ale->adt, + static_cast(ale->data), + keylist, + saction_flag, + range, + ANIM_nla_mapping_allowed(ale)); + } + + ANIM_animdata_freelist(&anim_data); +} + void scene_to_keylist(bDopeSheet *ads, Scene *sce, AnimKeylist *keylist, @@ -1238,19 +1282,6 @@ void action_group_to_keylist(AnimData *adt, } } -void action_slot_to_keylist(AnimData *adt, - animrig::Action &action, - const animrig::slot_handle_t slot_handle, - AnimKeylist *keylist, - const int saction_flag, - blender::float2 range) -{ - BLI_assert(GS(action.id.name) == ID_AC); - for (FCurve *fcurve : fcurves_for_action_slot(action, slot_handle)) { - fcurve_to_keylist(adt, fcurve, keylist, saction_flag, range, true); - } -} - void action_to_keylist(AnimData *adt, bAction *dna_action, AnimKeylist *keylist, @@ -1276,7 +1307,9 @@ void action_to_keylist(AnimData *adt, * have things like reference strips, where the strip can reference another slot handle. */ BLI_assert(adt); - action_slot_to_keylist(adt, action, adt->slot_handle, keylist, saction_flag, range); + for (FCurve *fcurve : fcurves_for_action_slot(action, adt->slot_handle)) { + fcurve_to_keylist(adt, fcurve, keylist, saction_flag, range, true); + } } void gpencil_to_keylist(bDopeSheet *ads, bGPdata *gpd, AnimKeylist *keylist, const bool active) diff --git a/source/blender/editors/animation/keyframes_keylist_test.cc b/source/blender/editors/animation/keyframes_keylist_test.cc index 6002e92a83a..6769f47a4dd 100644 --- a/source/blender/editors/animation/keyframes_keylist_test.cc +++ b/source/blender/editors/animation/keyframes_keylist_test.cc @@ -4,6 +4,10 @@ #include "testing/testing.h" +#include "ANIM_action.hh" +#include "ANIM_fcurve.hh" + +#include "ED_anim_api.hh" #include "ED_keyframes_keylist.hh" #include "DNA_anim_types.h" @@ -11,7 +15,19 @@ #include "MEM_guardedalloc.h" +#include "BKE_action.hh" +#include "BKE_armature.hh" #include "BKE_fcurve.hh" +#include "BKE_global.hh" +#include "BKE_idtype.hh" +#include "BKE_lib_id.hh" +#include "BKE_main.hh" +#include "BKE_object.hh" + +#include "BLI_string.h" + +#include "CLG_log.h" +#include "testing/testing.h" #include #include @@ -138,4 +154,186 @@ TEST(keylist, find_exact) ED_keylist_free(keylist); } +class KeylistSummaryTest : public testing::Test { + public: + Main *bmain; + blender::animrig::Action *action; + Object *cube; + Object *armature; + bArmature *armature_data; + Bone *bone1; + Bone *bone2; + + SpaceAction saction = {nullptr}; + bAnimContext ac = {nullptr}; + + static void SetUpTestSuite() + { + /* BKE_id_free() hits a code path that uses CLOG, which crashes if not initialized properly. */ + CLG_init(); + + /* To make id_can_have_animdata() and friends work, the `id_types` array needs to be set up. */ + BKE_idtype_init(); + } + + static void TearDownTestSuite() + { + CLG_exit(); + } + + void SetUp() override + { + bmain = BKE_main_new(); + G_MAIN = bmain; /* For BKE_animdata_free(). */ + + action = &static_cast(BKE_id_new(bmain, ID_AC, "ACÄnimåtië"))->wrap(); + cube = BKE_object_add_only_object(bmain, OB_EMPTY, "Küüübus"); + + armature_data = BKE_armature_add(bmain, "ARArmature"); + bone1 = reinterpret_cast(MEM_callocN(sizeof(Bone), "KeylistSummaryTest")); + bone2 = reinterpret_cast(MEM_callocN(sizeof(Bone), "KeylistSummaryTest")); + STRNCPY(bone1->name, "Bone.001"); + STRNCPY(bone2->name, "Bone.002"); + BLI_addtail(&armature_data->bonebase, bone1); + BLI_addtail(&armature_data->bonebase, bone2); + BKE_armature_bone_hash_make(armature_data); + + armature = BKE_object_add_only_object(bmain, OB_ARMATURE, "OBArmature"); + armature->data = armature_data; + BKE_pose_ensure(bmain, armature, armature_data, false); + + /* + * Fill in the common bits for the mock bAnimContext, for an Action editor. + * + * Tests should fill in: + * - saction.action_slot_handle + * - ac.obact + */ + saction.action = action; + saction.ads.filterflag = eDopeSheet_FilterFlag(0); + ac.bmain = bmain; + ac.datatype = ANIMCONT_ACTION; + ac.data = action; + ac.spacetype = SPACE_ACTION; + ac.sl = reinterpret_cast(&saction); + ac.ads = &saction.ads; + } + + void TearDown() override + { + saction.action_slot_handle = blender::animrig::Slot::unassigned; + ac.obact = nullptr; + + BKE_main_free(bmain); + G_MAIN = nullptr; + } +}; + +TEST_F(KeylistSummaryTest, slot_summary_simple) +{ + /* Test that a key summary is generated correctly for a slot that's animating + * an object's transforms. */ + + using namespace blender::animrig; + + Slot &slot_cube = action->slot_add_for_id(cube->id); + ASSERT_EQ(ActionSlotAssignmentResult::OK, assign_action_and_slot(action, &slot_cube, cube->id)); + Channelbag &channelbag = action_channelbag_ensure(*action, cube->id); + + FCurve &loc_x = channelbag.fcurve_ensure(bmain, {"location", 0}); + FCurve &loc_y = channelbag.fcurve_ensure(bmain, {"location", 1}); + FCurve &loc_z = channelbag.fcurve_ensure(bmain, {"location", 2}); + + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&loc_x, {1.0, 0.0}, {}, {})); + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&loc_x, {2.0, 1.0}, {}, {})); + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&loc_y, {2.0, 2.0}, {}, {})); + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&loc_y, {3.0, 3.0}, {}, {})); + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&loc_z, {2.0, 4.0}, {}, {})); + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&loc_z, {5.0, 5.0}, {}, {})); + + /* Generate slot summary keylist. */ + AnimKeylist *keylist = ED_keylist_create(); + saction.action_slot_handle = slot_cube.handle; + ac.obact = cube; + action_slot_summary_to_keylist( + &ac, &cube->id, *action, slot_cube.handle, keylist, 0, {0.0, 6.0}); + ED_keylist_prepare_for_direct_access(keylist); + + const ActKeyColumn *col_0 = ED_keylist_find_exact(keylist, 0.0); + const ActKeyColumn *col_1 = ED_keylist_find_exact(keylist, 1.0); + const ActKeyColumn *col_2 = ED_keylist_find_exact(keylist, 2.0); + const ActKeyColumn *col_3 = ED_keylist_find_exact(keylist, 3.0); + const ActKeyColumn *col_4 = ED_keylist_find_exact(keylist, 4.0); + const ActKeyColumn *col_5 = ED_keylist_find_exact(keylist, 5.0); + const ActKeyColumn *col_6 = ED_keylist_find_exact(keylist, 6.0); + + /* Check that we only have columns at the frames with keys. */ + EXPECT_EQ(nullptr, col_0); + EXPECT_NE(nullptr, col_1); + EXPECT_NE(nullptr, col_2); + EXPECT_NE(nullptr, col_3); + EXPECT_EQ(nullptr, col_4); + EXPECT_NE(nullptr, col_5); + EXPECT_EQ(nullptr, col_6); + + /* Check that the right number of keys are indicated in each column. */ + EXPECT_EQ(1, col_1->totkey); + EXPECT_EQ(3, col_2->totkey); + EXPECT_EQ(1, col_3->totkey); + EXPECT_EQ(1, col_5->totkey); + + ED_keylist_free(keylist); +} + +TEST_F(KeylistSummaryTest, slot_summary_bone_selection) +{ + /* Test that a key summary is generated correctly, excluding keys for + * unselected bones when filter-by-selection is on. */ + + using namespace blender::animrig; + + Slot &slot_armature = action->slot_add_for_id(armature->id); + ASSERT_EQ(ActionSlotAssignmentResult::OK, + assign_action_and_slot(action, &slot_armature, armature->id)); + Channelbag &channelbag = action_channelbag_ensure(*action, armature->id); + + FCurve &bone1_loc_x = channelbag.fcurve_ensure( + bmain, {"pose.bones[\"Bone.001\"].location", 0, std::nullopt, "Bone.001"}); + FCurve &bone2_loc_x = channelbag.fcurve_ensure( + bmain, {"pose.bones[\"Bone.002\"].location", 0, std::nullopt, "Bone.002"}); + + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&bone1_loc_x, {1.0, 0.0}, {}, {})); + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&bone1_loc_x, {2.0, 1.0}, {}, {})); + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&bone2_loc_x, {2.0, 2.0}, {}, {})); + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&bone2_loc_x, {3.0, 3.0}, {}, {})); + + /* Select only Bone.001. */ + bone1->flag |= BONE_SELECTED; + bone2->flag &= ~BONE_SELECTED; + + /* Generate slot summary keylist. */ + AnimKeylist *keylist = ED_keylist_create(); + saction.ads.filterflag = ADS_FILTER_ONLYSEL; /* Filter by selection. */ + saction.action_slot_handle = slot_armature.handle; + ac.obact = armature; + action_slot_summary_to_keylist( + &ac, &armature->id, *action, slot_armature.handle, keylist, 0, {0.0, 6.0}); + ED_keylist_prepare_for_direct_access(keylist); + + const ActKeyColumn *col_1 = ED_keylist_find_exact(keylist, 1.0); + const ActKeyColumn *col_2 = ED_keylist_find_exact(keylist, 2.0); + const ActKeyColumn *col_3 = ED_keylist_find_exact(keylist, 3.0); + + /* Check that we only have columns at the frames with keys for Bone.001. */ + EXPECT_NE(nullptr, col_1); + EXPECT_NE(nullptr, col_2); + EXPECT_EQ(nullptr, col_3); + + /* Check that the right number of keys are indicated in each column. */ + EXPECT_EQ(1, col_1->totkey); + EXPECT_EQ(1, col_2->totkey); + + ED_keylist_free(keylist); +} + } // namespace blender::editor::animation::tests diff --git a/source/blender/editors/include/ED_anim_api.hh b/source/blender/editors/include/ED_anim_api.hh index 0b72a1e36fc..54d1d23464f 100644 --- a/source/blender/editors/include/ED_anim_api.hh +++ b/source/blender/editors/include/ED_anim_api.hh @@ -55,6 +55,11 @@ struct PropertyRNA; struct MPathTarget; +namespace blender::animrig { +class Action; +class Slot; +} // namespace blender::animrig + /* ************************************************ */ /* ANIMATION CHANNEL FILTERING */ /* `anim_filter.cc` */ @@ -517,6 +522,33 @@ ENUM_OPERATORS(eAnimFilter_Flags, ANIMFILTER_TMP_IGNORE_ONLYSEL); /** \name Public API * \{ */ +/** + * Add the channel and sub-channels for an Action Slot to `anim_data`, filtered + * according to `filter_mode`. + * + * \param action: the action containing the slot to generate the channels for. + * + * \param slot: the slot to generate the channels for. + * + * \param filter_mode: the filters to use for deciding what channels get + * included. + * + * \param animated_id: the particular animated ID that the slot channels are + * being generated for. This is needed for filtering channels based on bone + * selection, and also for resolving the names of animated properties. This + * should never be null, but it's okay(ish) if it's an ID not actually animated + * by the slot, in which case it will act as a fallback in case an ID actually + * animated by the slot can't be found. + * + * \return The number of items added to `anim_data`. + */ +size_t ANIM_animfilter_action_slot(bAnimContext *ac, + ListBase * /* bAnimListElem */ anim_data, + blender::animrig::Action &action, + blender::animrig::Slot &slot, + eAnimFilter_Flags filter_mode, + ID *animated_id); + /** * This function filters the active data source to leave only animation channels suitable for * usage by the caller. It will return the length of the list diff --git a/source/blender/editors/include/ED_keyframes_draw.hh b/source/blender/editors/include/ED_keyframes_draw.hh index d8ba6c2fc0f..1d6ff267c51 100644 --- a/source/blender/editors/include/ED_keyframes_draw.hh +++ b/source/blender/editors/include/ED_keyframes_draw.hh @@ -75,6 +75,7 @@ void ED_add_action_group_channel(ChannelDrawList *channel_list, int saction_flag); /* Layered Action Summary. */ void ED_add_action_layered_channel(ChannelDrawList *channel_list, + bAnimContext *ac, bAnimListElem *ale, bAction *action, const float ypos, @@ -82,6 +83,7 @@ void ED_add_action_layered_channel(ChannelDrawList *channel_list, int saction_flag); /* Action Slot summary. */ void ED_add_action_slot_channel(ChannelDrawList *channel_list, + bAnimContext *ac, bAnimListElem *ale, blender::animrig::Action &action, blender::animrig::Slot &slot, diff --git a/source/blender/editors/include/ED_keyframes_keylist.hh b/source/blender/editors/include/ED_keyframes_keylist.hh index 66bb5982040..669bdee30f7 100644 --- a/source/blender/editors/include/ED_keyframes_keylist.hh +++ b/source/blender/editors/include/ED_keyframes_keylist.hh @@ -180,17 +180,27 @@ void action_group_to_keylist(AnimData *adt, int saction_flag, blender::float2 range); /* Action */ + +/** + * Generate a full list of the keys in `dna_action` that are within the frame + * range `range`. + * + * For layered actions, this is limited to the keys that are for the slot + * assigned to `adt`. + * + * Note: this should only be used in places that need or want the *full* list of + * keys, without any filtering by e.g. channel selection/visibility, etc. For + * use cases that need such filtering, use `action_slot_summary_to_keylist()` + * instead. + * + * \see action_slot_summary_to_keylist() + */ void action_to_keylist(AnimData *adt, bAction *dna_action, AnimKeylist *keylist, int saction_flag, blender::float2 range); -void action_slot_to_keylist(AnimData *adt, - blender::animrig::Action &action, - blender::animrig::slot_handle_t slot_handle, - AnimKeylist *keylist, - int saction_flag, - blender::float2 range); + /* Object */ void ob_to_keylist( bDopeSheet *ads, Object *ob, AnimKeylist *keylist, int saction_flag, blender::float2 range); @@ -208,6 +218,42 @@ void summary_to_keylist(bAnimContext *ac, int saction_flag, blender::float2 range); +/** + * Generate a summary channel keylist for the specified slot, merging it into + * `keylist`. + * + * This filters the keys to be consistent with the visible channels in the + * editor indicated by `ac` + * + * \param animated_id: the particular animated ID that the slot summary is being + * generated for. This is needed for filtering channels based on bone selection, + * etc. NOTE: despite being passed as a pointer, this should never be null. It's + * currently passed as a pointer to be defensive because I (Nathan) am not 100% + * confident at the time of writing (PR #134922) that the callers of this + * actually guarantee a non-null pointer (they should, but bugs). This way we + * can assert internally to catch if that ever happens. + * + * \param action: the action containing the slot to generate the summary for. + * + * \param slot_handle: the handle of the slot to generate the summary for. + * + * \param keylist: the keylist that the generated summary will be merged into. + * + * \param saction_flag: needed for the `SACTION_SHOW_EXTREMES` flag, to + * determine whether to compute and store the data needed to determine which + * keys are "extremes" (local maxima/minima). + * + * \param range: only keys within this time range will be included in the + * summary. + */ +void action_slot_summary_to_keylist(bAnimContext *ac, + ID *animated_id, + blender::animrig::Action &action, + blender::animrig::slot_handle_t slot_handle, + AnimKeylist *keylist, + int /* eSAction_Flag */ saction_flag, + blender::float2 range); + /* Grease Pencil datablock summary (Legacy) */ void gpencil_to_keylist(bDopeSheet *ads, bGPdata *gpd, AnimKeylist *keylist, bool active); diff --git a/source/blender/editors/space_action/action_draw.cc b/source/blender/editors/space_action/action_draw.cc index 81c2c348839..4ebe2b28635 100644 --- a/source/blender/editors/space_action/action_draw.cc +++ b/source/blender/editors/space_action/action_draw.cc @@ -370,6 +370,7 @@ static void draw_keyframes(bAnimContext *ac, break; case ALE_ACTION_LAYERED: ED_add_action_layered_channel(draw_list, + ac, ale, static_cast(ale->key_data), ycenter, @@ -378,6 +379,7 @@ static void draw_keyframes(bAnimContext *ac, break; case ALE_ACTION_SLOT: ED_add_action_slot_channel(draw_list, + ac, ale, static_cast(ale->key_data)->wrap(), *static_cast(ale->data), diff --git a/source/blender/editors/space_action/action_select.cc b/source/blender/editors/space_action/action_select.cc index 8781bf3088a..799fd7d349a 100644 --- a/source/blender/editors/space_action/action_select.cc +++ b/source/blender/editors/space_action/action_select.cc @@ -110,8 +110,19 @@ static void actkeys_list_element_to_keylist(bAnimContext *ac, break; } case ALE_ACTION_LAYERED: { - bAction *action = (bAction *)ale->key_data; - action_to_keylist(ale->adt, action, keylist, 0, range); + /* This is only called for action summaries in the Dopesheet, *not* the + * Action Editor. Therefore despite the name `ALE_ACTION_LAYERED`, this + * is only used to show a *single slot* of the action: the slot used by + * the ID the action is listed under. + * + * Thus we use the same function as the `ALE_ACTION_SLOT` case below + * because in practice the only distinction between these cases is where + * they get the slot from. In this case, we get it from `elem`'s ADT. */ + animrig::Action *action = static_cast(ale->key_data); + BLI_assert(action); + BLI_assert(ale->adt); + action_slot_summary_to_keylist( + ac, ale->id, *action, ale->adt->slot_handle, keylist, 0, range); break; } case ALE_ACTION_SLOT: { @@ -119,10 +130,11 @@ static void actkeys_list_element_to_keylist(bAnimContext *ac, animrig::Slot *slot = static_cast(ale->data); BLI_assert(action); BLI_assert(slot); - action_slot_to_keylist(ale->adt, *action, slot->handle, keylist, 0, range); + action_slot_summary_to_keylist(ac, ale->id, *action, slot->handle, keylist, 0, range); break; } case ALE_ACT: { + /* Legacy action. */ bAction *act = (bAction *)ale->key_data; action_to_keylist(ale->adt, act, keylist, 0, range); break;