Anim: Make it possible to enter/exit NLA tweak mode on slotted Actions

Properly track Action and Slot assignment when entering/exiting NLA
tweak mode.

This doesn't properly sync the length of the NLA strip when exiting
tweak mode. This and more NLA work is tracked at #127489.

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

source/blender/animrig/intern/keyframing_test.cc

@@ -109,7 +109,7 @@ class KeyframingTest : public testing::Test {
     strip->actend = 1000.0;
     strip->scale = 1.0;
     strip->blendmode = NLASTRIP_MODE_COMBINE;
-    BKE_nla_tweakmode_enter(adt);
+    BKE_nla_tweakmode_enter({object_with_nla->id, *adt});
   }
 
   void TearDown() override

source/blender/blenkernel/BKE_nla.hh

@@ -478,11 +478,11 @@ void BKE_nla_action_pushdown(OwnedAnimData owned_adt, bool is_liboverride);
  * Find the active strip + track combination, and set them up as the tweaking track,
  * and return if successful or not.
  */
-bool BKE_nla_tweakmode_enter(AnimData *adt);
+bool BKE_nla_tweakmode_enter(OwnedAnimData owned_adt);
 /**
  * Exit tweak-mode for this AnimData block.
  */
-void BKE_nla_tweakmode_exit(AnimData *adt);
+void BKE_nla_tweakmode_exit(OwnedAnimData owned_adt);
 
 /**
  * Clear all NLA Tweak Mode related flags on the ADT, tracks, and strips.
@@ -492,12 +492,12 @@ void BKE_nla_tweakmode_clear_flags(AnimData *adt);
 /**
  * Partially exit NLA tweak-mode for this AnimData block, without following any
  * pointers to other data-blocks. This means no strip length syncing (as that
- * needs to know info about the strip's Action), and no reference counting on
- * the Action.
+ * needs to know info about the strip's Action), no reference counting on the
+ * Action, and no user update on the Action Slot.
  *
  * This function just writes to the AnimData-owned data. It is intended to be
- * used in blend-file reading code, which performs a reference count later
- * anyway.
+ * used in blend-file reading code, which performs a reference count + rebuilds
+ * the slot user map later anyway.
  */
 void BKE_nla_tweakmode_exit_nofollowptr(AnimData *adt);
 

source/blender/blenkernel/intern/anim_data.cc

@@ -520,6 +520,10 @@ void BKE_animdata_merge_copy(
     dst->tmpact = src->tmpact;
     id_us_plus((ID *)dst->tmpact);
   }
+  dst->slot_handle = src->slot_handle;
+  dst->tmp_slot_handle = src->tmp_slot_handle;
+  STRNCPY(dst->slot_name, src->slot_name);
+  STRNCPY(dst->tmp_slot_name, src->tmp_slot_name);
 
   /* duplicate NLA data */
   if (src->nla_tracks.first) {

source/blender/blenkernel/intern/nla.cc

@@ -2260,23 +2260,24 @@ static void nla_tweakmode_find_active(const ListBase /*NlaTrack*/ *nla_tracks,
   *r_active_strip = activeStrip;
 }
 
-bool BKE_nla_tweakmode_enter(AnimData *adt)
+bool BKE_nla_tweakmode_enter(const OwnedAnimData owned_adt)
 {
   NlaTrack *nlt, *activeTrack = nullptr;
   NlaStrip *activeStrip = nullptr;
+  AnimData &adt = owned_adt.adt;
 
   /* verify that data is valid */
-  if (ELEM(nullptr, adt, adt->nla_tracks.first)) {
+  if (ELEM(nullptr, adt.nla_tracks.first)) {
     return false;
   }
 
   /* If block is already in tweak-mode, just leave, but we should report
    * that this block is in tweak-mode (as our return-code). */
-  if (adt->flag & ADT_NLA_EDIT_ON) {
+  if (adt.flag & ADT_NLA_EDIT_ON) {
     return true;
   }
 
-  nla_tweakmode_find_active(&adt->nla_tracks, &activeTrack, &activeStrip);
+  nla_tweakmode_find_active(&adt.nla_tracks, &activeTrack, &activeStrip);
 
   if (ELEM(nullptr, activeTrack, activeStrip, activeStrip->act)) {
     if (G.debug & G_DEBUG) {
@@ -2289,7 +2290,7 @@ bool BKE_nla_tweakmode_enter(AnimData *adt)
   /* Go over all the tracks, tagging each strip that uses the same
    * action as the active strip, but leaving everything else alone.
    */
-  LISTBASE_FOREACH (NlaTrack *, nlt, &adt->nla_tracks) {
+  LISTBASE_FOREACH (NlaTrack *, nlt, &adt.nla_tracks) {
     LISTBASE_FOREACH (NlaStrip *, strip, &nlt->strips) {
       if (strip->act == activeStrip->act) {
         strip->flag |= NLASTRIP_FLAG_TWEAKUSER;
@@ -2309,7 +2310,7 @@ bool BKE_nla_tweakmode_enter(AnimData *adt)
    * on.
    */
   activeTrack->flag |= NLATRACK_DISABLED;
-  if ((adt->flag & ADT_NLA_EVAL_UPPER_TRACKS) == 0) {
+  if ((adt.flag & ADT_NLA_EVAL_UPPER_TRACKS) == 0) {
     for (nlt = activeTrack->next; nlt; nlt = nlt->next) {
       nlt->flag |= NLATRACK_DISABLED;
     }
@@ -2323,12 +2324,30 @@ bool BKE_nla_tweakmode_enter(AnimData *adt)
    * - Take note of the active strip for mapping-correction of keyframes
    *   in the action being edited.
    */
-  adt->tmpact = adt->action;
-  adt->action = activeStrip->act;
-  adt->act_track = activeTrack;
-  adt->actstrip = activeStrip;
-  id_us_plus(&activeStrip->act->id);
-  adt->flag |= ADT_NLA_EDIT_ON;
+  adt.tmpact = adt.action;
+  adt.tmp_slot_handle = adt.slot_handle;
+  STRNCPY(adt.tmp_slot_name, adt.slot_name);
+
+  /* Don't go through the regular animrig::unassign_action() call, as the old Action is still being
+   * used by this ID. But do reset the action pointer, as Action::assign_id() doesn't like it when
+   * another Action is already assigned. */
+  adt.action = nullptr;
+
+  if (activeStrip->act) {
+    animrig::Action &strip_action = activeStrip->act->wrap();
+    if (strip_action.is_action_layered()) {
+      animrig::Slot *strip_slot = strip_action.slot_for_handle(activeStrip->action_slot_handle);
+      strip_action.assign_id(strip_slot, owned_adt.owner_id);
+    }
+    else {
+      adt.action = activeStrip->act;
+      id_us_plus(&adt.action->id);
+    }
+  }
+
+  adt.act_track = activeTrack;
+  adt.actstrip = activeStrip;
+  adt.flag |= ADT_NLA_EDIT_ON;
 
   /* done! */
   return true;
@@ -2373,7 +2392,12 @@ static void nla_tweakmode_exit_nofollowptr(AnimData *adt)
   BKE_nla_tweakmode_clear_flags(adt);
 
   adt->action = adt->tmpact;
+  adt->slot_handle = adt->tmp_slot_handle;
+
   adt->tmpact = nullptr;
+  adt->tmp_slot_handle = animrig::Slot::unassigned;
+  STRNCPY(adt->slot_name, adt->tmp_slot_name);
+
   adt->act_track = nullptr;
   adt->actstrip = nullptr;
 }
@@ -2387,19 +2411,32 @@ void BKE_nla_tweakmode_exit_nofollowptr(AnimData *adt)
   nla_tweakmode_exit_nofollowptr(adt);
 }
 
-void BKE_nla_tweakmode_exit(AnimData *adt)
+void BKE_nla_tweakmode_exit(const OwnedAnimData owned_adt)
 {
-  if (!is_nla_in_tweakmode(adt)) {
+  if (!is_nla_in_tweakmode(&owned_adt.adt)) {
     return;
   }
 
-  if (adt->action) {
-    /* The Action will be replaced with adt->tmpact, and thus needs a decrement in user count. */
-    id_us_min(&adt->action->id);
+  if (owned_adt.adt.action) {
+    /* The Action will be replaced with adt->tmpact, and thus needs to be unassigned first. */
+    animrig::unassign_action(owned_adt.owner_id);
   }
 
-  nla_tweakmode_exit_sync_strip_lengths(adt);
-  nla_tweakmode_exit_nofollowptr(adt);
+  nla_tweakmode_exit_sync_strip_lengths(&owned_adt.adt);
+  nla_tweakmode_exit_nofollowptr(&owned_adt.adt);
+
+  /* nla_tweakmode_exit_nofollowptr() does not follow any pointers, and thus cannot update the slot
+   * user map. So it has to be done here now. This is safe to do here, as slot->users_add()
+   * gracefully handles duplicates. */
+  if (owned_adt.adt.action && owned_adt.adt.slot_handle != animrig::Slot::unassigned) {
+    animrig::Action &action = owned_adt.adt.action->wrap();
+    BLI_assert_msg(action.is_action_layered(),
+                   "when a slot is assigned, the action should layered");
+    animrig::Slot *slot = action.slot_for_handle(owned_adt.adt.slot_handle);
+    if (slot) {
+      slot->users_add(owned_adt.owner_id);
+    }
+  }
 }
 
 void BKE_nla_tweakmode_clear_flags(AnimData *adt)
@@ -2572,7 +2609,7 @@ void BKE_nla_blend_read_data(BlendDataReader *reader, ID *id_owner, ListBase *tr
 
 void BKE_nla_liboverride_post_process(ID *id, AnimData *adt)
 {
-  /* TODO(Sybren): Convert nla.h to C++ so that these can become references instead. */
+  /* TODO(Sybren): replace these two parameters with an OwnedAnimData struct. */
   BLI_assert(id);
   BLI_assert(adt);
 
@@ -2586,7 +2623,7 @@ void BKE_nla_liboverride_post_process(ID *id, AnimData *adt)
   if (!has_tracks) {
     if (is_tweak_mode) {
       /* No tracks, so it's impossible to actually be in tweak mode. */
-      BKE_nla_tweakmode_exit(adt);
+      BKE_nla_tweakmode_exit({*id, *adt});
     }
     return;
   }
@@ -2604,7 +2641,7 @@ void BKE_nla_liboverride_post_process(ID *id, AnimData *adt)
   nla_tweakmode_find_active(&adt->nla_tracks, &adt->act_track, &adt->actstrip);
   if (!adt->act_track || !adt->actstrip) {
     /* Could not find the active track/strip, so better to exit tweak mode. */
-    BKE_nla_tweakmode_exit(adt);
+    BKE_nla_tweakmode_exit({*id, *adt});
   }
 }
 

source/blender/blenloader/intern/versioning_400.cc

@@ -808,7 +808,7 @@ static void version_nla_tweakmode_incomplete(Main *bmain)
 
     /* Not enough info in the blend file to reliably stay in tweak mode. This is the most important
      * part of this versioning code, as it prevents future nullptr access. */
-    BKE_nla_tweakmode_exit(adt);
+    BKE_nla_tweakmode_exit({*id, *adt});
   }
   FOREACH_MAIN_ID_END;
 

source/blender/editors/space_action/action_data.cc

@@ -652,7 +652,7 @@ void ED_animedit_unlink_action(
 
   /* If in Tweak Mode, don't unlink. Instead, this becomes a shortcut to exit Tweak Mode. */
   if ((adt) && (adt->flag & ADT_NLA_EDIT_ON)) {
-    BKE_nla_tweakmode_exit(adt);
+    BKE_nla_tweakmode_exit({*id, *adt});
 
     Scene *scene = CTX_data_scene(C);
     if (scene != nullptr) {
@@ -793,13 +793,18 @@ static NlaStrip *action_layer_get_nlastrip(ListBase *strips, float ctime)
 }
 
 /* Switch NLA Strips/Actions. */
-static void action_layer_switch_strip(
-    AnimData *adt, NlaTrack *old_track, NlaStrip *old_strip, NlaTrack *nlt, NlaStrip *strip)
+static void action_layer_switch_strip(const OwnedAnimData owned_adt,
+                                      NlaTrack *old_track,
+                                      NlaStrip *old_strip,
+                                      NlaTrack *nlt,
+                                      NlaStrip *strip)
 {
+  AnimData *adt = &owned_adt.adt;
+
   /* Exit tweak-mode on old strip
    * NOTE: We need to manually clear this stuff ourselves, as tweak-mode exit doesn't do it
    */
-  BKE_nla_tweakmode_exit(adt);
+  BKE_nla_tweakmode_exit(owned_adt);
 
   if (old_strip) {
     old_strip->flag &= ~(NLASTRIP_FLAG_ACTIVE | NLASTRIP_FLAG_SELECT);
@@ -834,7 +839,7 @@ static void action_layer_switch_strip(
   }
 
   /* Enter tweak-mode again - hopefully we're now "it" */
-  BKE_nla_tweakmode_enter(adt);
+  BKE_nla_tweakmode_enter(owned_adt);
   BLI_assert(adt->actstrip == strip);
 }
 
@@ -882,14 +887,15 @@ static bool action_layer_next_poll(bContext *C)
 
 static int action_layer_next_exec(bContext *C, wmOperator *op)
 {
-  AnimData *adt = ED_actedit_animdata_from_context(C, nullptr);
-  NlaTrack *act_track;
+  ID *animated_id = nullptr;
+  AnimData *adt = ED_actedit_animdata_from_context(C, &animated_id);
+  const OwnedAnimData owned_adt{*animated_id, *adt};
 
   Scene *scene = CTX_data_scene(C);
   float ctime = BKE_scene_ctime_get(scene);
 
   /* Get active track */
-  act_track = BKE_nlatrack_find_tweaked(adt);
+  NlaTrack *act_track = BKE_nlatrack_find_tweaked(adt);
 
   if (act_track == nullptr) {
     BKE_report(op->reports, RPT_ERROR, "Could not find current NLA Track");
@@ -905,7 +911,7 @@ static int action_layer_next_exec(bContext *C, wmOperator *op)
       NlaStrip *strip = action_layer_get_nlastrip(&nlt->strips, ctime);
 
       if (strip) {
-        action_layer_switch_strip(adt, act_track, adt->actstrip, nlt, strip);
+        action_layer_switch_strip(owned_adt, act_track, adt->actstrip, nlt, strip);
         break;
       }
     }
@@ -914,7 +920,7 @@ static int action_layer_next_exec(bContext *C, wmOperator *op)
     /* No more actions (strips) - Go back to editing the original active action
      * NOTE: This will mean exiting tweak-mode...
      */
-    BKE_nla_tweakmode_exit(adt);
+    BKE_nla_tweakmode_exit(owned_adt);
 
     /* Deal with solo flags...
      * Assume: Solo Track == NLA Muting
@@ -997,7 +1003,8 @@ static bool action_layer_prev_poll(bContext *C)
 
 static int action_layer_prev_exec(bContext *C, wmOperator *op)
 {
-  AnimData *adt = ED_actedit_animdata_from_context(C, nullptr);
+  ID *animated_id = nullptr;
+  AnimData *adt = ED_actedit_animdata_from_context(C, &animated_id);
   NlaTrack *act_track;
   NlaTrack *nlt;
 
@@ -1029,7 +1036,7 @@ static int action_layer_prev_exec(bContext *C, wmOperator *op)
     NlaStrip *strip = action_layer_get_nlastrip(&nlt->strips, ctime);
 
     if (strip) {
-      action_layer_switch_strip(adt, act_track, adt->actstrip, nlt, strip);
+      action_layer_switch_strip({*animated_id, *adt}, act_track, adt->actstrip, nlt, strip);
       break;
     }
   }

source/blender/editors/space_nla/nla_edit.cc

@@ -126,7 +126,7 @@ static int nlaedit_enable_tweakmode_exec(bContext *C, wmOperator *op)
     }
 
     /* Try entering tweak-mode if valid. */
-    ok |= BKE_nla_tweakmode_enter(adt);
+    ok |= BKE_nla_tweakmode_enter({*ale->id, *adt});
 
     /* mark the active track as being "solo"? */
     if (do_solo && adt->actstrip) {
@@ -227,7 +227,7 @@ bool nlaedit_disable_tweakmode(bAnimContext *ac, bool do_solo)
     }
 
     /* To be sure that we're doing everything right, just exit tweak-mode. */
-    BKE_nla_tweakmode_exit(adt);
+    BKE_nla_tweakmode_exit({*ale->id, *adt});
 
     ale->update |= ANIM_UPDATE_DEPS;
   }
@@ -1282,7 +1282,7 @@ static int nlaedit_delete_exec(bContext *C, wmOperator * /*op*/)
         /* Fix for #109430. Defensively exit tweak mode before deleting
          * the active strip. */
         if (ale->adt && ale->adt->actstrip == strip) {
-          BKE_nla_tweakmode_exit(ale->adt);
+          BKE_nla_tweakmode_exit({*ale->id, *ale->adt});
         }
 
         /* if a strip either side of this was a transition, delete those too */

source/blender/makesdna/DNA_anim_types.h

@@ -1164,10 +1164,13 @@ typedef struct AnimData {
   uint8_t _pad0[2];
 
   /**
-   * Temp-storage for the 'real' active action (i.e. the one used before the tweaking-action
-   * took over to be edited in the Animation Editors)
+   * Temp-storage for the 'real' active action + slot (i.e. the ones used before
+   * NLA Tweak mode took over the Action to be edited in the Animation Editors).
    */
   bAction *tmpact;
+  int32_t tmp_slot_handle;
+  char tmp_slot_name[66]; /* MAX_ID_NAME */
+  uint8_t _pad1[2];
 
   /* nla-tracks */
   ListBase nla_tracks;
@@ -1206,7 +1209,7 @@ typedef struct AnimData {
   /** Influence for active action. */
   float act_influence;
 
-  uint8_t _pad1[4];
+  uint8_t _pad2[4];
 } AnimData;
 
 #ifdef __cplusplus

source/blender/makesrna/intern/rna_animation.cc

@@ -192,6 +192,7 @@ static void rna_AnimData_tmpact_set(PointerRNA *ptr, PointerRNA value, ReportLis
 
 static void rna_AnimData_tweakmode_set(PointerRNA *ptr, const bool value)
 {
+  ID *animated_id = ptr->owner_id;
   AnimData *adt = (AnimData *)ptr->data;
 
   /* NOTE: technically we should also set/unset SCE_NLA_EDIT_ON flag on the
@@ -200,10 +201,10 @@ static void rna_AnimData_tweakmode_set(PointerRNA *ptr, const bool value)
    * dealt with at some point. */
 
   if (value) {
-    BKE_nla_tweakmode_enter(adt);
+    BKE_nla_tweakmode_enter({*animated_id, *adt});
   }
   else {
-    BKE_nla_tweakmode_exit(adt);
+    BKE_nla_tweakmode_exit({*animated_id, *adt});
   }
 }
 

source/blender/makesrna/intern/rna_space.cc

@@ -2308,7 +2308,7 @@ static void rna_SpaceDopeSheetEditor_action_update(bContext *C, PointerRNA *ptr)
   }
 
   /* Exit editmode first - we cannot change actions while in tweak-mode. */
-  BKE_nla_tweakmode_exit(adt);
+  BKE_nla_tweakmode_exit({*id, *adt});
 
   /* To prevent data loss (i.e. if users flip between actions using the Browse menu),
    * stash this action if nothing else uses it.