Anim: make remaining sequencer features work with slotted actions

This is a follow-up to #128363, and fixes up the remaining areas of
the sequencer's code that didn't yet account for slotted actions:

1. Moving strips failed to move their animation with them.
2. Duplicating strips failed to duplicate their animation with them.
3. Deleting strips didn't delete their animation channels with them.

This also takes the opportunity to add depsgraph tagging and
notifiers that were already missing in the pre-slotted-actions
code, for the strip delete and strip paste operators. The absence
of these was making the UI not update and was also causing stale
animation data to get evaluated.

Pull Request: https://projects.blender.org/blender/blender/pulls/128440

source/blender/animrig/ANIM_action.hh

@@ -1401,6 +1401,12 @@ 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 span for which `predicate` returns true.
+ */
+Vector<FCurve *> fcurves_in_span_filtered(Span<FCurve *> fcurves,
+                                          FunctionRef<bool(const FCurve &fcurve)> predicate);
+
 /**
  * Return the F-Curves in the given listbase for which `predicate` returns
  * true.
@@ -1412,6 +1418,8 @@ Vector<FCurve *> fcurves_in_listbase_filtered(ListBase /* FCurve * */ fcurves,
  * 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.
  *
+ * Compatible with both legacy and layered Actions.
+ *
  *  \returns true if the given FCurve was removed.
  *
  * \see action_fcurve_detach

source/blender/animrig/intern/action.cc

@@ -2356,6 +2356,20 @@ Vector<FCurve *> fcurves_in_action_slot_filtered(bAction *act,
   return found;
 }
 
+Vector<FCurve *> fcurves_in_span_filtered(Span<FCurve *> fcurves,
+                                          FunctionRef<bool(const FCurve &fcurve)> predicate)
+{
+  Vector<FCurve *> found;
+
+  for (FCurve *fcurve : fcurves) {
+    if (predicate(*fcurve)) {
+      found.append(fcurve);
+    }
+  }
+
+  return found;
+}
+
 Vector<FCurve *> fcurves_in_listbase_filtered(ListBase /* FCurve * */ fcurves,
                                               FunctionRef<bool(const FCurve &fcurve)> predicate)
 {
@@ -2486,22 +2500,11 @@ FCurve *action_fcurve_ensure(Main *bmain,
 
 bool action_fcurve_remove(Action &action, FCurve &fcu)
 {
-  BLI_assert(action.is_action_layered());
-
-  for (Layer *layer : action.layers()) {
-    for (Strip *strip : layer->strips()) {
-      if (!(strip->type() == Strip::Type::Keyframe)) {
-        continue;
-      }
-      StripKeyframeData &strip_data = strip->data<StripKeyframeData>(action);
-      for (ChannelBag *bag : strip_data.channelbags()) {
-        const bool removed = bag->fcurve_remove(fcu);
-        if (removed) {
-          return true;
-        }
-      }
-    }
+  if (action_fcurve_detach(action, fcu)) {
+    BKE_fcurve_free(&fcu);
+    return true;
   }
+
   return false;
 }
 

source/blender/editors/space_sequencer/sequencer_clipboard.cc

@@ -488,8 +488,12 @@ int sequencer_clipboard_paste_exec(bContext *C, wmOperator *op)
   SEQ_animation_restore_original(scene_dst, &animation_backup);
 
   DEG_id_tag_update(&scene_dst->id, ID_RECALC_SEQUENCER_STRIPS);
+  if (scene_dst->adt && scene_dst->adt->action) {
+    DEG_id_tag_update(&scene_dst->adt->action->id, ID_RECALC_ANIMATION_NO_FLUSH);
+  }
   DEG_relations_tag_update(bmain_dst);
   WM_event_add_notifier(C, NC_SCENE | ND_SEQUENCER, scene_dst);
+  WM_event_add_notifier(C, NC_SCENE | ND_ANIMCHAN, scene_dst);
   ED_outliner_select_sync_from_sequence_tag(C);
 
   BKE_reportf(op->reports, RPT_INFO, "%d strips pasted", num_strips_to_paste);

source/blender/editors/space_sequencer/sequencer_edit.cc

@@ -1764,8 +1764,12 @@ static int sequencer_delete_exec(bContext *C, wmOperator *op)
   SEQ_edit_remove_flagged_sequences(scene, seqbasep);
 
   DEG_id_tag_update(&scene->id, ID_RECALC_SEQUENCER_STRIPS);
+  if (scene->adt && scene->adt->action) {
+    DEG_id_tag_update(&scene->adt->action->id, ID_RECALC_ANIMATION_NO_FLUSH);
+  }
   DEG_relations_tag_update(bmain);
   WM_event_add_notifier(C, NC_SCENE | ND_SEQUENCER, scene);
+  WM_event_add_notifier(C, NC_SCENE | ND_ANIMCHAN, scene);
   return OPERATOR_FINISHED;
 }
 

source/blender/sequencer/SEQ_animation.hh

@@ -28,8 +28,13 @@ void SEQ_offset_animdata(Scene *scene, Sequence *seq, int ofs);
  */
 bool SEQ_fcurve_matches(const Sequence &seq, const FCurve &fcurve);
 struct SeqAnimationBackup {
+  /* `curves` and `channel_bag` here represent effectively the same data (the
+   * fcurves that animate the Scene that the sequence belongs to), just for
+   * legacy and layered actions, respectively. Therefore only one or the other
+   * should ever have data stored in them, never both. */
   ListBase curves;
   blender::animrig::ChannelBag channel_bag;
+
   ListBase drivers;
 };
 /**

source/blender/sequencer/intern/animation.cc

@@ -47,9 +47,10 @@ void SEQ_offset_animdata(Scene *scene, Sequence *seq, int ofs)
     return;
   }
 
-  Vector<FCurve *> fcurves = animrig::fcurves_in_listbase_filtered(
-      scene->adt->action->curves,
-      [&](const FCurve &fcurve) { return SEQ_fcurve_matches(*seq, fcurve); });
+  Vector<FCurve *> fcurves = animrig::fcurves_in_action_slot_filtered(
+      scene->adt->action, scene->adt->slot_handle, [&](const FCurve &fcurve) {
+        return SEQ_fcurve_matches(*seq, fcurve);
+      });
 
   for (FCurve *fcu : fcurves) {
     uint i;
@@ -78,13 +79,14 @@ void SEQ_free_animdata(Scene *scene, Sequence *seq)
     return;
   }
 
-  Vector<FCurve *> fcurves = animrig::fcurves_in_listbase_filtered(
-      scene->adt->action->curves,
-      [&](const FCurve &fcurve) { return SEQ_fcurve_matches(*seq, fcurve); });
+  Vector<FCurve *> fcurves = animrig::fcurves_in_action_slot_filtered(
+      scene->adt->action, scene->adt->slot_handle, [&](const FCurve &fcurve) {
+        return SEQ_fcurve_matches(*seq, fcurve);
+      });
 
+  animrig::Action &action = scene->adt->action->wrap();
   for (FCurve *fcu : fcurves) {
-    BLI_remlink(&scene->adt->action->curves, fcu);
-    BKE_fcurve_free(fcu);
+    action_fcurve_remove(action, *fcu);
   }
 }
 
@@ -139,20 +141,66 @@ void SEQ_animation_restore_original(Scene *scene, SeqAnimationBackup *backup)
   }
 }
 
-static void seq_animation_duplicate(Scene *scene, Sequence *seq, ListBase *dst, ListBase *src)
+/**
+ * Duplicate the animation in `src` that matches items in `seq` into `dst`.
+ */
+static void seq_animation_duplicate(Sequence *seq,
+                                    animrig::Action &dst,
+                                    const animrig::slot_handle_t dst_slot_handle,
+                                    SeqAnimationBackup *src)
 {
   if (seq->type == SEQ_TYPE_META) {
     LISTBASE_FOREACH (Sequence *, meta_child, &seq->seqbase) {
-      seq_animation_duplicate(scene, meta_child, dst, src);
+      seq_animation_duplicate(meta_child, dst, dst_slot_handle, src);
+    }
+  }
+
+  Vector<FCurve *> fcurves = {};
+  BLI_assert_msg(BLI_listbase_is_empty(&src->curves) || src->channel_bag.fcurves().is_empty(),
+                 "SeqAnimationBackup has fcurves for both legacy and layered actions, which "
+                 "should never happen.");
+  if (BLI_listbase_is_empty(&src->curves)) {
+    fcurves = animrig::fcurves_in_span_filtered(
+        src->channel_bag.fcurves(),
+        [&](const FCurve &fcurve) { return SEQ_fcurve_matches(*seq, fcurve); });
+  }
+  else {
+    fcurves = animrig::fcurves_in_listbase_filtered(
+        src->curves, [&](const FCurve &fcurve) { return SEQ_fcurve_matches(*seq, fcurve); });
+  }
+
+  for (const FCurve *fcu : fcurves) {
+    FCurve *fcu_copy = BKE_fcurve_copy(fcu);
+
+    /* Handling groups properly requires more work, so we ignore them for now.
+     *
+     * Note that when legacy actions are deprecated, then we can handle channel
+     * groups way more easily because we know they're stored in the
+     * already-duplicated channelbag in `src`, and we therefore don't have to
+     * worry that they might have already been freed.*/
+    fcu_copy->grp = nullptr;
+
+    animrig::action_fcurve_attach(dst, dst_slot_handle, *fcu_copy, std::nullopt);
+  }
+}
+
+/**
+ * Duplicate the drivers in `src` that matches items in `seq` into `dst`.
+ */
+static void seq_drivers_duplicate(Sequence *seq, AnimData *dst, SeqAnimationBackup *src)
+{
+  if (seq->type == SEQ_TYPE_META) {
+    LISTBASE_FOREACH (Sequence *, meta_child, &seq->seqbase) {
+      seq_drivers_duplicate(meta_child, dst, src);
     }
   }
 
   Vector<FCurve *> fcurves = animrig::fcurves_in_listbase_filtered(
-      *src, [&](const FCurve &fcurve) { return SEQ_fcurve_matches(*seq, fcurve); });
+      src->drivers, [&](const FCurve &fcurve) { return SEQ_fcurve_matches(*seq, fcurve); });
 
   for (const FCurve *fcu : fcurves) {
     FCurve *fcu_cpy = BKE_fcurve_copy(fcu);
-    BLI_addtail(dst, fcu_cpy);
+    BLI_addtail(&dst->drivers, fcu_cpy);
   }
 }
 
@@ -160,10 +208,16 @@ void SEQ_animation_duplicate_backup_to_scene(Scene *scene,
                                              Sequence *seq,
                                              SeqAnimationBackup *backup)
 {
-  if (!BLI_listbase_is_empty(&backup->curves)) {
-    seq_animation_duplicate(scene, seq, &scene->adt->action->curves, &backup->curves);
+  BLI_assert(scene != nullptr);
+
+  if (!BLI_listbase_is_empty(&backup->curves) || !backup->channel_bag.fcurves().is_empty()) {
+    BLI_assert(scene->adt != nullptr);
+    BLI_assert(scene->adt->action != nullptr);
+    seq_animation_duplicate(seq, scene->adt->action->wrap(), scene->adt->slot_handle, backup);
   }
+
   if (!BLI_listbase_is_empty(&backup->drivers)) {
-    seq_animation_duplicate(scene, seq, &scene->adt->drivers, &backup->drivers);
+    BLI_assert(scene->adt != nullptr);
+    seq_drivers_duplicate(seq, scene->adt, backup);
   }
 }