From 1b18115a33bd87b6bcf5c8bd3ee1ecba92bf582f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20T=C3=B6nne?= Date: Tue, 4 Mar 2025 11:19:51 +0100 Subject: [PATCH 1/4] Fix #135101: Grease Pencil: Eraser crashes when handling multiple layers Changing the `affected_drawings_` set in a threaded loop is not safe. Since running the eraser in parallel over multiple drawings is unlikely to improve performance on top of internal threading, handling layers in a simple sequential loop is preferable here. Pull Request: https://projects.blender.org/blender/blender/pulls/135428 --- source/blender/editors/sculpt_paint/grease_pencil_erase.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/source/blender/editors/sculpt_paint/grease_pencil_erase.cc b/source/blender/editors/sculpt_paint/grease_pencil_erase.cc index 1ffdccc440f..5d80804dd7b 100644 --- a/source/blender/editors/sculpt_paint/grease_pencil_erase.cc +++ b/source/blender/editors/sculpt_paint/grease_pencil_erase.cc @@ -1023,10 +1023,9 @@ struct EraseOperationExecutor { /* Erase on all editable drawings. */ const Vector drawings = ed::greasepencil::retrieve_editable_drawings(*scene, grease_pencil); - threading::parallel_for_each( - drawings, [&](const ed::greasepencil::MutableDrawingInfo &info) { - execute_eraser_on_drawing(info.layer_index, info.frame_number, info.drawing); - }); + for (const ed::greasepencil::MutableDrawingInfo &info : drawings) { + execute_eraser_on_drawing(info.layer_index, info.frame_number, info.drawing); + } } if (changed) { From ca980dc4f92c12d4565023b1ab33cb2d92017e84 Mon Sep 17 00:00:00 2001 From: Nathan Vegdahl Date: Tue, 4 Mar 2025 13:52:03 +0100 Subject: [PATCH 2/4] Fix #134746: Action Slot keyframe summary incorrect in Action Editor and Dopesheet In the Action Editor and Dopesheet, Action/Slot summaries erroneously showed keys that were hidden in their sub-channels. The root cause was that the functions for building Action/Slot summary key lists, `action_to_keylist()` and `action_slot_to_keylist()`, weren't doing animation filtering *at all*, instead just looping over all f-curves. This PR fixes the bug by replacing `action_slot_to_keylist()` with a new `action_slot_summary_to_keylist()` function, which calls into the animation filtering code to determine which f-curves to include in their key list, and also using it in place of `action_to_keylist()` at the relevant call sites. Additionally, properly filtering the channels for the summaries requires both a `bAnimContext` and the ID that's being animated, so this PR threads those through to `action_slot_summary_to_keylist()`. Note that `action_to_keylist()` is still used in some places that actually *want* an unfiltered list of keys. Those call sites have been left as-is, and `action_to_keylist()` remains for that purpose. Pull Request: https://projects.blender.org/blender/blender/pulls/134922 --- .../blender/editors/animation/anim_filter.cc | 64 +++--- .../editors/animation/keyframes_draw.cc | 43 +++- .../editors/animation/keyframes_keylist.cc | 61 ++++-- .../animation/keyframes_keylist_test.cc | 198 ++++++++++++++++++ source/blender/editors/include/ED_anim_api.hh | 32 +++ .../editors/include/ED_keyframes_draw.hh | 2 + .../editors/include/ED_keyframes_keylist.hh | 58 ++++- .../editors/space_action/action_draw.cc | 2 + .../editors/space_action/action_select.cc | 18 +- 9 files changed, 419 insertions(+), 59 deletions(-) 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; From 7913237d33e4436b37fa6af57c5a2782ddcef8ea Mon Sep 17 00:00:00 2001 From: Sebastian Parborg Date: Fri, 28 Feb 2025 12:00:57 +0100 Subject: [PATCH 3/4] Change make_source_archive to include all submodule files It will not include submodules that are not checked out by default. It now also has an explicit variable for folders to skip. Pull Request: https://projects.blender.org/blender/blender/pulls/135293 --- build_files/utils/make_source_archive.py | 68 ++++++++++++++++-------- 1 file changed, 47 insertions(+), 21 deletions(-) diff --git a/build_files/utils/make_source_archive.py b/build_files/utils/make_source_archive.py index 647d3a396e1..0d065f3ac90 100755 --- a/build_files/utils/make_source_archive.py +++ b/build_files/utils/make_source_archive.py @@ -28,15 +28,20 @@ from typing import ( # NOTE: while the Python part of this script is portable, # it relies on external commands typically found on GNU/Linux. # Support for other platforms could be added by moving GNU `tar` & `md5sum` use to Python. +# This also relies on having a Unix shell (sh) to run some git commands. -SKIP_NAMES = { +SKIP_NAMES = ( ".gitignore", ".gitmodules", ".gitattributes", ".git-blame-ignore-revs", ".arcconfig", ".svn", -} +) + +SKIP_FOLDERS = ( + "release/datafiles/assets/working", +) def main() -> None: @@ -63,6 +68,10 @@ def main() -> None: os.chdir(curdir) blender_srcdir = blender_srcdir.relative_to(curdir) + # Update our SKIP_FOLDERS blacklist with the source directory name + global SKIP_FOLDERS + SKIP_FOLDERS = tuple([f"{blender_srcdir}/{entry}" for entry in SKIP_FOLDERS]) + print(f"Output dir: {curdir}") version = make_utils.parse_blender_version() @@ -125,7 +134,6 @@ def create_manifest( print(f'Building manifest of files: "{outpath}"...', end="", flush=True) with outpath.open("w", encoding="utf-8") as outfile: main_files_to_manifest(blender_srcdir, outfile) - assets_to_manifest(blender_srcdir, outfile) if packages_dir: packages_to_manifest(outfile, packages_dir) @@ -134,20 +142,9 @@ def create_manifest( def main_files_to_manifest(blender_srcdir: Path, outfile: TextIO) -> None: assert not blender_srcdir.is_absolute() - for path in git_ls_files(blender_srcdir): - print(path, file=outfile) - - -def assets_to_manifest(blender_srcdir: Path, outfile: TextIO) -> None: - assert not blender_srcdir.is_absolute() - - assets_dir = blender_srcdir / "release" / "datafiles" / "assets" - for path in assets_dir.glob("*"): - if path.name == "working": - continue - if path.name in SKIP_NAMES: - continue - print(path, file=outfile) + for git_repo in git_gather_all_folders_to_package(blender_srcdir): + for path in git_ls_files(git_repo): + print(path, file=outfile) def packages_to_manifest(outfile: TextIO, packages_dir: Path) -> None: @@ -227,28 +224,57 @@ def cleanup(manifest: Path) -> None: # Low-level commands +def git_gather_all_folders_to_package(directory: Path = Path(".")) -> Iterable[Path]: + """Generator, yields lines which represents each directory to gather git files from. + + Each directory represents either the top level git repository or a submodule. + All submodules that have the 'update = none' setting will be excluded from this list. + + The directory path given to this function will be included in the yielded paths + """ + + # For each submodule (recurse into submodules within submodules if they exist) + git_main_command = "submodule --quiet foreach --recursive" + # Return the path to the submodule and what the value is of their "update" setting + # If the "update" setting doesn't exist, only the path to the submodule is returned + git_command_args = "'echo $displaypath $(git config --file \"$toplevel/.gitmodules\" --get submodule.$name.update)'" + + # Yield the root directory as this is our top level git repo + yield directory + + for line in git_command(f"-C '{directory}' {git_main_command} {git_command_args}"): + # Check if we shouldn't include the directory on this line + split_line = line.rsplit(maxsplit=1) + if len(split_line) > 1 and split_line[-1] == "none": + continue + path = directory / split_line[0] + yield path + + def git_ls_files(directory: Path = Path(".")) -> Iterable[Path]: """Generator, yields lines of output from 'git ls-files'. Only lines that are actually files (so no directories, sockets, etc.) are returned, and never one from SKIP_NAMES. """ - for line in git_command("-C", str(directory), "ls-files"): + for line in git_command(f"-C '{directory}' ls-files"): path = directory / line if not path.is_file() or path.name in SKIP_NAMES: continue + if path.as_posix().startswith(SKIP_FOLDERS): + continue yield path -def git_command(*cli_args: Union[bytes, str, Path]) -> Iterable[str]: +def git_command(cli_args: str) -> Iterable[str]: """Generator, yields lines of output from a Git command.""" - command = ("git", *cli_args) + command = "git " + cli_args # import shlex # print(">", " ".join(shlex.quote(arg) for arg in command)) git = subprocess.run( - command, stdout=subprocess.PIPE, check=True, text=True, timeout=30 + command, stdout=subprocess.PIPE, shell=True, check=True, text=True, timeout=30 ) for line in git.stdout.split("\n"): if line: From 277add8fc9d1dcd8877b2819c5fa586d57a2a819 Mon Sep 17 00:00:00 2001 From: Sebastian Parborg Date: Fri, 28 Feb 2025 17:53:36 +0100 Subject: [PATCH 4/4] Add ability to create a test data archive with make_source_archive This makes it so that we ship the test data for every major release in its own separate archive. (In case someone wants to easily run the tests for a specific older release without using git) Pull Request: https://projects.blender.org/blender/blender/pulls/135293 --- GNUmakefile | 2 ++ build_files/utils/make_source_archive.py | 18 ++++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/GNUmakefile b/GNUmakefile index c251905cf8d..d39524454a1 100644 --- a/GNUmakefile +++ b/GNUmakefile @@ -572,6 +572,8 @@ source_archive_complete: .FORCE -DCMAKE_BUILD_TYPE_INIT:STRING=$(BUILD_TYPE) -DPACKAGE_USE_UPSTREAM_SOURCES=OFF # This assumes CMake is still using a default `PACKAGE_DIR` variable: @$(PYTHON) ./build_files/utils/make_source_archive.py --include-packages "$(BUILD_DIR)/source_archive/packages" +# We assume that the tests will not change for minor releases so only package them for major versions + @$(PYTHON) ./build_files/utils/make_source_archive.py --package-test-data icons_geom: .FORCE @BLENDER_BIN=$(BLENDER_BIN) \ diff --git a/build_files/utils/make_source_archive.py b/build_files/utils/make_source_archive.py index 0d065f3ac90..6eb2b060aaf 100755 --- a/build_files/utils/make_source_archive.py +++ b/build_files/utils/make_source_archive.py @@ -51,7 +51,8 @@ def main() -> None: description="Create a tarball of the Blender sources, optionally including sources of dependencies.", epilog="This script is intended to be run by `make source_archive_complete`.", ) - cli_parser.add_argument( + group = cli_parser.add_mutually_exclusive_group() + group.add_argument( "-p", "--include-packages", type=Path, @@ -59,6 +60,12 @@ def main() -> None: metavar="PACKAGE_PATH", help="Include all source files from the given package directory as well.", ) + group.add_argument( + "-t", + "--package-test-data", + action='store_true', + help="Package all test data into its own archive", + ) cli_args = cli_parser.parse_args() @@ -79,7 +86,12 @@ def main() -> None: manifest = manifest_path(tarball) packages_dir = packages_path(curdir, cli_args) - create_manifest(version, manifest, blender_srcdir, packages_dir) + if cli_args.package_test_data: + print("Creating an archive of all test data.") + create_manifest(version, manifest, blender_srcdir / "tests/data", packages_dir) + else: + create_manifest(version, manifest, blender_srcdir, packages_dir) + create_tarball(version, tarball, manifest, blender_srcdir, packages_dir) create_checksum_file(tarball) cleanup(manifest) @@ -90,6 +102,8 @@ def tarball_path(output_dir: Path, version: make_utils.BlenderVersion, cli_args: extra = "" if cli_args.include_packages: extra = "-with-libraries" + elif cli_args.package_test_data: + extra = "-test-data" return output_dir / f"blender{extra}-{version}.tar.xz"