From e34feb347cfc03a2d9ffb0c9393afeb176179a67 Mon Sep 17 00:00:00 2001 From: Nathan Vegdahl Date: Thu, 3 Oct 2024 15:01:53 +0200 Subject: [PATCH] Anim: make copy-paste of sequencer strips work with slotted actions Before this, copy-pasting sequencer strips with animation on them would fail to copy-paste their animation along with them if they were animated by a slotted action. This fixes that. There are three remaining known issues in the sequencer when used with slotted actions that this PR does not fix, and will be addressed in a follow-up PR: 1. Moving strips fails to move the animation of their fcurves with them, which it should. 2. Duplicating strips (as distinct from copying followed by pasting) fails to duplicate their animation with them. 3. Deleting a strip does not delete its animation channels with it. Pull Request: https://projects.blender.org/blender/blender/pulls/128363 --- source/blender/animrig/ANIM_action.hh | 23 ++++ source/blender/animrig/intern/action.cc | 39 ++++++ .../space_sequencer/sequencer_clipboard.cc | 127 ++++++++++++++---- source/blender/sequencer/CMakeLists.txt | 1 + source/blender/sequencer/SEQ_animation.hh | 11 +- source/blender/sequencer/intern/animation.cc | 116 ++++++++-------- 6 files changed, 227 insertions(+), 90 deletions(-) diff --git a/source/blender/animrig/ANIM_action.hh b/source/blender/animrig/ANIM_action.hh index 481d7ec56d2..e759546438d 100644 --- a/source/blender/animrig/ANIM_action.hh +++ b/source/blender/animrig/ANIM_action.hh @@ -1398,6 +1398,13 @@ bool fcurve_matches_collection_path(const FCurve &fcurve, Vector fcurves_in_action_slot_filtered( bAction *act, slot_handle_t slot_handle, FunctionRef predicate); +/** + * Return the F-Curves in the given listbase for which `predicate` returns + * true. + */ +Vector fcurves_in_listbase_filtered(ListBase /* FCurve * */ fcurves, + FunctionRef predicate); + /** * Remove the given FCurve from the action by searching for it in all channelbags. * This assumes that an FCurve can only exist in an action once. @@ -1463,6 +1470,22 @@ void action_fcurve_move(Action &action_dst, Action &action_src, FCurve &fcurve); +/** + * Moves all F-Curves from one ChannelBag to the other. + * + * The ChannelBags do not need to be part of the same action, or even belong to + * an action at all. + * + * If the F-Curves belonged to channel groups, the group membership also carries + * over to the destination ChannelBag. If groups with the same names don't + * exist, they are created. \see blender::animrig::action_fcurve_detach + * + * The order of existing channel groups in the destination ChannelBag are not + * changed, and any new groups are placed after those in the order they appeared + * in the src group. + */ +void channelbag_fcurves_move(ChannelBag &channelbag_dst, ChannelBag &channelbag_src); + /** * Find an appropriate user of the given Action + Slot for keyframing purposes. * diff --git a/source/blender/animrig/intern/action.cc b/source/blender/animrig/intern/action.cc index e3ebc9ff31d..2a03fb04b21 100644 --- a/source/blender/animrig/intern/action.cc +++ b/source/blender/animrig/intern/action.cc @@ -2342,6 +2342,20 @@ Vector fcurves_in_action_slot_filtered(bAction *act, return found; } +Vector fcurves_in_listbase_filtered(ListBase /* FCurve * */ fcurves, + FunctionRef predicate) +{ + Vector found; + + LISTBASE_FOREACH (FCurve *, fcurve, &fcurves) { + if (predicate(*fcurve)) { + found.append(fcurve); + } + } + + return found; +} + FCurve *action_fcurve_ensure(Main *bmain, bAction *act, const char group[], @@ -2551,6 +2565,31 @@ void action_fcurve_move(Action &action_dst, action_fcurve_attach(action_dst, action_slot_dst, fcurve, group_name); } +void channelbag_fcurves_move(ChannelBag &channelbag_dst, ChannelBag &channelbag_src) +{ + while (!channelbag_src.fcurves().is_empty()) { + FCurve &fcurve = *channelbag_src.fcurve(0); + + /* Store the group name locally, as the group will be removed if this was its + * last F-Curve. */ + std::optional group_name; + if (fcurve.grp) { + group_name = fcurve.grp->name; + } + + const bool is_detached = channelbag_src.fcurve_detach(fcurve); + BLI_assert(is_detached); + UNUSED_VARS_NDEBUG(is_detached); + + channelbag_dst.fcurve_append(fcurve); + + if (group_name) { + bActionGroup &group = channelbag_dst.channel_group_ensure(*group_name); + channelbag_dst.fcurve_assign_to_channel_group(fcurve, group); + } + } +} + bool ChannelBag::fcurve_assign_to_channel_group(FCurve &fcurve, bActionGroup &to_group) { if (this->channel_groups().first_index_try(&to_group) == -1) { diff --git a/source/blender/editors/space_sequencer/sequencer_clipboard.cc b/source/blender/editors/space_sequencer/sequencer_clipboard.cc index 56aab269bc9..68f788c4bb0 100644 --- a/source/blender/editors/space_sequencer/sequencer_clipboard.cc +++ b/source/blender/editors/space_sequencer/sequencer_clipboard.cc @@ -50,6 +50,8 @@ #include "DEG_depsgraph.hh" #include "DEG_depsgraph_build.hh" +#include "ANIM_action.hh" +#include "ANIM_action_legacy.hh" #include "ANIM_animdata.hh" #include "WM_api.hh" @@ -62,6 +64,7 @@ /* Own include. */ #include "sequencer_intern.hh" +using namespace blender; using namespace blender::bke::blendfile; /* -------------------------------------------------------------------- */ @@ -80,27 +83,55 @@ static void sequencer_copy_animation_listbase(Scene *scene_src, } } - GSet *fcurves_src = SEQ_fcurves_by_strip_get(seq_dst, fcurve_base_src); - if (fcurves_src == nullptr) { - return; - } + Vector fcurves_src = animrig::fcurves_in_listbase_filtered( + *fcurve_base_src, + [&](const FCurve &fcurve) { return SEQ_fcurve_matches(*seq_dst, fcurve); }); - GSET_FOREACH_BEGIN (FCurve *, fcu_src, fcurves_src) { + for (FCurve *fcu_src : fcurves_src) { BLI_addtail(clipboard_dst, BKE_fcurve_copy(fcu_src)); } - GSET_FOREACH_END(); +} - BLI_gset_free(fcurves_src, nullptr); +/* This is effectively just a copy of `sequencer_copy_animation_listbase()` + * above, except that it copies from an action's animation to a vector rather + * than between two listbases. */ +static void sequencer_copy_animation_to_vector(Scene *scene_src, + Sequence *seq_dst, + Vector &clipboard_dst, + bAction &fcurves_src_action, + animrig::slot_handle_t fcurves_src_slot_handle) +{ + /* Add curves for strips inside meta strip. */ + if (seq_dst->type == SEQ_TYPE_META) { + LISTBASE_FOREACH (Sequence *, meta_child, &seq_dst->seqbase) { + sequencer_copy_animation_to_vector( + scene_src, meta_child, clipboard_dst, fcurves_src_action, fcurves_src_slot_handle); + } + } + + Vector fcurves_src = animrig::fcurves_in_action_slot_filtered( + &fcurves_src_action, fcurves_src_slot_handle, [&](const FCurve &fcurve) { + return SEQ_fcurve_matches(*seq_dst, fcurve); + }); + + for (FCurve *fcu_src : fcurves_src) { + FCurve *fcu_copy = BKE_fcurve_copy(fcu_src); + + /* Handling groups properly requires more work, so for now just ignore them. */ + fcu_copy->grp = nullptr; + + clipboard_dst.append(fcu_copy); + } } static void sequencer_copy_animation(Scene *scene_src, - ListBase *fcurves_dst, + Vector &fcurves_dst, ListBase *drivers_dst, Sequence *seq_dst) { - if (SEQ_animation_curves_exist(scene_src)) { - sequencer_copy_animation_listbase( - scene_src, seq_dst, fcurves_dst, &scene_src->adt->action->curves); + if (SEQ_animation_keyframes_exist(scene_src)) { + sequencer_copy_animation_to_vector( + scene_src, seq_dst, fcurves_dst, *scene_src->adt->action, scene_src->adt->slot_handle); } if (SEQ_animation_drivers_exist(scene_src)) { sequencer_copy_animation_listbase(scene_src, seq_dst, drivers_dst, &scene_src->adt->drivers); @@ -153,19 +184,52 @@ static bool sequencer_write_copy_paste_file(Main *bmain_src, } } - ListBase fcurves_dst = {nullptr, nullptr}; + Vector fcurves_dst = {}; ListBase drivers_dst = {nullptr, nullptr}; LISTBASE_FOREACH (Sequence *, seq_dst, &scene_dst->ed->seqbase) { - /* Copy animation curves from seq_dst (if any). */ - sequencer_copy_animation(scene_src, &fcurves_dst, &drivers_dst, seq_dst); + /* Copy any fcurves/drivers from `scene_src` that are relevant to `seq_dst`. */ + sequencer_copy_animation(scene_src, fcurves_dst, &drivers_dst, seq_dst); } - if (!BLI_listbase_is_empty(&fcurves_dst) || !BLI_listbase_is_empty(&drivers_dst)) { - BLI_assert(scene_dst->adt == nullptr); + BLI_assert(scene_dst->adt == nullptr); + + /* Copy over the fcurves. */ + if (!fcurves_dst.is_empty()) { + scene_dst->adt = BKE_animdata_ensure_id(&scene_dst->id); + animrig::Action &action_dst = + reinterpret_cast( + copy_buffer.id_create( + ID_AC, scene_name, nullptr, {PartialWriteContext::IDAddOperations::SET_FAKE_USER})) + ->wrap(); + + /* Assign the `dst_action` as either legacy or layered, depending on what + * the source action we're copying from is. */ + if (animrig::legacy::action_treat_as_legacy(*scene_src->adt->action)) { + const bool success = animrig::assign_action(&action_dst, scene_dst->id); + if (!success) { + return false; + } + } + else { + /* If we're copying from a layered action, also ensure a connected slot. */ + animrig::Slot *slot = animrig::assign_action_ensure_slot_for_keying(action_dst, + scene_dst->id); + if (slot == nullptr) { + return false; + } + } + + for (FCurve *fcurve : fcurves_dst) { + animrig::action_fcurve_attach(action_dst, + scene_dst->adt->slot_handle, + *fcurve, + fcurve->grp ? std::optional(fcurve->grp->name) : std::nullopt); + } + } + + /* Copy over the drivers. */ + if (!BLI_listbase_is_empty(&drivers_dst)) { scene_dst->adt = BKE_animdata_ensure_id(&scene_dst->id); - scene_dst->adt->action = reinterpret_cast(copy_buffer.id_create( - ID_AC, scene_name, nullptr, {PartialWriteContext::IDAddOperations::SET_FAKE_USER})); - BLI_movelisttolist(&scene_dst->adt->action->curves, &fcurves_dst); BLI_movelisttolist(&scene_dst->adt->drivers, &drivers_dst); } @@ -272,22 +336,27 @@ int sequencer_clipboard_copy_exec(bContext *C, wmOperator *op) static bool sequencer_paste_animation(Main *bmain_dst, Scene *scene_dst, Scene *scene_src) { - if (!SEQ_animation_curves_exist(scene_src) && !SEQ_animation_drivers_exist(scene_src)) { + if (!SEQ_animation_keyframes_exist(scene_src) && !SEQ_animation_drivers_exist(scene_src)) { return false; } - bAction *act_dst; + bAction *act_dst = animrig::id_action_ensure(bmain_dst, &scene_dst->id); - if (scene_dst->adt != nullptr && scene_dst->adt->action != nullptr) { - act_dst = scene_dst->adt->action; - } - else { - /* get action to add F-Curve+keyframe to */ - act_dst = blender::animrig::id_action_ensure(bmain_dst, &scene_dst->id); + /* For layered actions ensure we have an attached slot. */ + if (!animrig::legacy::action_treat_as_legacy(*act_dst)) { + const animrig::Slot *slot = animrig::assign_action_ensure_slot_for_keying(act_dst->wrap(), + scene_dst->id); + BLI_assert(slot != nullptr); + if (slot == nullptr) { + return false; + } } - LISTBASE_FOREACH (FCurve *, fcu, &scene_src->adt->action->curves) { - BLI_addtail(&act_dst->curves, BKE_fcurve_copy(fcu)); + for (FCurve *fcu : animrig::legacy::fcurves_for_assigned_action(scene_src->adt)) { + animrig::action_fcurve_attach(act_dst->wrap(), + scene_dst->adt->slot_handle, + *BKE_fcurve_copy(fcu), + fcu->grp ? std::optional(fcu->grp->name) : std::nullopt); } LISTBASE_FOREACH (FCurve *, fcu, &scene_src->adt->drivers) { BLI_addtail(&scene_dst->adt->drivers, BKE_fcurve_copy(fcu)); diff --git a/source/blender/sequencer/CMakeLists.txt b/source/blender/sequencer/CMakeLists.txt index 48bc56277e8..a643118421f 100644 --- a/source/blender/sequencer/CMakeLists.txt +++ b/source/blender/sequencer/CMakeLists.txt @@ -5,6 +5,7 @@ set(INC . intern + ../animrig ../blenkernel ../blenloader ../blentranslation diff --git a/source/blender/sequencer/SEQ_animation.hh b/source/blender/sequencer/SEQ_animation.hh index ad17fbdae04..4c6781ee770 100644 --- a/source/blender/sequencer/SEQ_animation.hh +++ b/source/blender/sequencer/SEQ_animation.hh @@ -10,19 +10,26 @@ #include "DNA_listBase.h" +#include "ANIM_action.hh" +#include "ANIM_action_legacy.hh" + struct GSet; struct ListBase; struct Scene; struct Sequence; struct SeqAnimationBackup; -bool SEQ_animation_curves_exist(Scene *scene); +bool SEQ_animation_keyframes_exist(Scene *scene); bool SEQ_animation_drivers_exist(Scene *scene); void SEQ_free_animdata(Scene *scene, Sequence *seq); void SEQ_offset_animdata(Scene *scene, Sequence *seq, int ofs); -GSet *SEQ_fcurves_by_strip_get(const Sequence *seq, ListBase *fcurve_base); +/** + * Return whether the fcurve targets the given sequence. + */ +bool SEQ_fcurve_matches(const Sequence &seq, const FCurve &fcurve); struct SeqAnimationBackup { ListBase curves; + blender::animrig::ChannelBag channel_bag; ListBase drivers; }; /** diff --git a/source/blender/sequencer/intern/animation.cc b/source/blender/sequencer/intern/animation.cc index 9fa4d4baae8..5caff09e052 100644 --- a/source/blender/sequencer/intern/animation.cc +++ b/source/blender/sequencer/intern/animation.cc @@ -22,10 +22,12 @@ #include "SEQ_animation.hh" -bool SEQ_animation_curves_exist(Scene *scene) +using namespace blender; + +bool SEQ_animation_keyframes_exist(Scene *scene) { return scene->adt != nullptr && scene->adt->action != nullptr && - !BLI_listbase_is_empty(&scene->adt->action->curves); + scene->adt->action->wrap().has_keyframes(scene->adt->slot_handle); } bool SEQ_animation_drivers_exist(Scene *scene) @@ -33,50 +35,23 @@ bool SEQ_animation_drivers_exist(Scene *scene) return scene->adt != nullptr && !BLI_listbase_is_empty(&scene->adt->drivers); } -/* r_prefix + [" + escaped_name + "] + \0 */ -#define SEQ_RNAPATH_MAXSTR ((30 + 2 + (SEQ_NAME_MAXSTR * 2) + 2) + 1) - -static size_t sequencer_rna_path_prefix(char str[SEQ_RNAPATH_MAXSTR], const char *name) +bool SEQ_fcurve_matches(const Sequence &seq, const FCurve &fcurve) { - char name_esc[SEQ_NAME_MAXSTR * 2]; - - BLI_str_escape(name_esc, name, sizeof(name_esc)); - return BLI_snprintf_rlen( - str, SEQ_RNAPATH_MAXSTR, "sequence_editor.sequences_all[\"%s\"]", name_esc); + return animrig::fcurve_matches_collection_path( + fcurve, "sequence_editor.sequences_all[", seq.name + 2); } -GSet *SEQ_fcurves_by_strip_get(const Sequence *seq, ListBase *fcurve_base) -{ - char rna_path[SEQ_RNAPATH_MAXSTR]; - size_t rna_path_len = sequencer_rna_path_prefix(rna_path, seq->name + 2); - - /* Only allocate `fcurves` if it's needed as it's possible there is no animation for `seq`. */ - GSet *fcurves = nullptr; - LISTBASE_FOREACH (FCurve *, fcurve, fcurve_base) { - if (STREQLEN(fcurve->rna_path, rna_path, rna_path_len)) { - if (fcurves == nullptr) { - fcurves = BLI_gset_ptr_new(__func__); - } - BLI_gset_add(fcurves, fcurve); - } - } - - return fcurves; -} - -#undef SEQ_RNAPATH_MAXSTR - void SEQ_offset_animdata(Scene *scene, Sequence *seq, int ofs) { - if (!SEQ_animation_curves_exist(scene) || ofs == 0) { - return; - } - GSet *fcurves = SEQ_fcurves_by_strip_get(seq, &scene->adt->action->curves); - if (fcurves == nullptr) { + if (!SEQ_animation_keyframes_exist(scene) || ofs == 0) { return; } - GSET_FOREACH_BEGIN (FCurve *, fcu, fcurves) { + Vector fcurves = animrig::fcurves_in_listbase_filtered( + scene->adt->action->curves, + [&](const FCurve &fcurve) { return SEQ_fcurve_matches(*seq, fcurve); }); + + for (FCurve *fcu : fcurves) { uint i; if (fcu->bezt) { for (i = 0; i < fcu->totvert; i++) { @@ -93,35 +68,43 @@ void SEQ_offset_animdata(Scene *scene, Sequence *seq, int ofs) } } } - GSET_FOREACH_END(); - BLI_gset_free(fcurves, nullptr); DEG_id_tag_update(&scene->adt->action->id, ID_RECALC_ANIMATION); } void SEQ_free_animdata(Scene *scene, Sequence *seq) { - if (!SEQ_animation_curves_exist(scene)) { - return; - } - GSet *fcurves = SEQ_fcurves_by_strip_get(seq, &scene->adt->action->curves); - if (fcurves == nullptr) { + if (!SEQ_animation_keyframes_exist(scene)) { return; } - GSET_FOREACH_BEGIN (FCurve *, fcu, fcurves) { + Vector fcurves = animrig::fcurves_in_listbase_filtered( + scene->adt->action->curves, + [&](const FCurve &fcurve) { return SEQ_fcurve_matches(*seq, fcurve); }); + + for (FCurve *fcu : fcurves) { BLI_remlink(&scene->adt->action->curves, fcu); BKE_fcurve_free(fcu); } - GSET_FOREACH_END(); - BLI_gset_free(fcurves, nullptr); } void SEQ_animation_backup_original(Scene *scene, SeqAnimationBackup *backup) { - if (SEQ_animation_curves_exist(scene)) { - BLI_movelisttolist(&backup->curves, &scene->adt->action->curves); + if (SEQ_animation_keyframes_exist(scene)) { + animrig::Action &action = scene->adt->action->wrap(); + + assert_baklava_phase_1_invariants(action); + + if (action.is_action_legacy()) { + BLI_movelisttolist(&backup->curves, &scene->adt->action->curves); + } + else if (animrig::ChannelBag *channel_bag = animrig::channelbag_for_action_slot( + action, scene->adt->slot_handle)) + { + animrig::channelbag_fcurves_move(backup->channel_bag, *channel_bag); + } } + if (SEQ_animation_drivers_exist(scene)) { BLI_movelisttolist(&backup->drivers, &scene->adt->drivers); } @@ -129,10 +112,29 @@ void SEQ_animation_backup_original(Scene *scene, SeqAnimationBackup *backup) void SEQ_animation_restore_original(Scene *scene, SeqAnimationBackup *backup) { - if (!BLI_listbase_is_empty(&backup->curves)) { - BLI_movelisttolist(&scene->adt->action->curves, &backup->curves); + if (!BLI_listbase_is_empty(&backup->curves) || !backup->channel_bag.fcurves().is_empty()) { + BLI_assert(scene->adt != nullptr && scene->adt->action != nullptr); + + animrig::Action &action = scene->adt->action->wrap(); + + assert_baklava_phase_1_invariants(action); + + if (action.is_action_legacy()) { + BLI_movelisttolist(&scene->adt->action->curves, &backup->curves); + } + else { + animrig::ChannelBag *channel_bag = animrig::channelbag_for_action_slot( + action, scene->adt->slot_handle); + /* The channel bag should exist if we got here, because otherwise the + * backup channel bag would have been empty. */ + BLI_assert(channel_bag != nullptr); + + animrig::channelbag_fcurves_move(*channel_bag, backup->channel_bag); + } } + if (!BLI_listbase_is_empty(&backup->drivers)) { + BLI_assert(scene->adt != nullptr); BLI_movelisttolist(&scene->adt->drivers, &backup->drivers); } } @@ -145,17 +147,13 @@ static void seq_animation_duplicate(Scene *scene, Sequence *seq, ListBase *dst, } } - GSet *fcurves = SEQ_fcurves_by_strip_get(seq, src); - if (fcurves == nullptr) { - return; - } + Vector fcurves = animrig::fcurves_in_listbase_filtered( + *src, [&](const FCurve &fcurve) { return SEQ_fcurve_matches(*seq, fcurve); }); - GSET_FOREACH_BEGIN (const FCurve *, fcu, fcurves) { + for (const FCurve *fcu : fcurves) { FCurve *fcu_cpy = BKE_fcurve_copy(fcu); BLI_addtail(dst, fcu_cpy); } - GSET_FOREACH_END(); - BLI_gset_free(fcurves, nullptr); } void SEQ_animation_duplicate_backup_to_scene(Scene *scene,