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

@@ -1398,6 +1398,13 @@ bool fcurve_matches_collection_path(const FCurve &fcurve,
 Vector<FCurve *> fcurves_in_action_slot_filtered(
     bAction *act, slot_handle_t slot_handle, FunctionRef<bool(const FCurve &fcurve)> predicate);
 
+/**
+ * Return the F-Curves in the given listbase for which `predicate` returns
+ * true.
+ */
+Vector<FCurve *> fcurves_in_listbase_filtered(ListBase /* FCurve * */ fcurves,
+                                              FunctionRef<bool(const FCurve &fcurve)> 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.
  *

source/blender/animrig/intern/action.cc

@@ -2342,6 +2342,20 @@ Vector<FCurve *> fcurves_in_action_slot_filtered(bAction *act,
   return found;
 }
 
+Vector<FCurve *> fcurves_in_listbase_filtered(ListBase /* FCurve * */ fcurves,
+                                              FunctionRef<bool(const FCurve &fcurve)> predicate)
+{
+  Vector<FCurve *> 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<std::string> 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) {

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<FCurve *> 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<FCurve *> &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<FCurve *> 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<FCurve *> &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<FCurve *> 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<bAction *>(
+            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<bAction *>(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));

source/blender/sequencer/CMakeLists.txt

@@ -5,6 +5,7 @@
 set(INC
   .
   intern
+  ../animrig
   ../blenkernel
   ../blenloader
   ../blentranslation

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;
 };
 /**

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<FCurve *> 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<FCurve *> 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<FCurve *> 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,