From 367cceaab867eacd0cffcdc25fee5fdffbbb9ccc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Thu, 12 Sep 2024 16:48:02 +0200 Subject: [PATCH] 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 --- .../blender/animrig/intern/keyframing_test.cc | 2 +- source/blender/blenkernel/BKE_nla.hh | 12 +-- source/blender/blenkernel/intern/anim_data.cc | 4 + source/blender/blenkernel/intern/nla.cc | 81 ++++++++++++++----- .../blenloader/intern/versioning_400.cc | 2 +- .../editors/space_action/action_data.cc | 31 ++++--- source/blender/editors/space_nla/nla_edit.cc | 6 +- source/blender/makesdna/DNA_anim_types.h | 9 ++- .../blender/makesrna/intern/rna_animation.cc | 5 +- source/blender/makesrna/intern/rna_space.cc | 2 +- 10 files changed, 103 insertions(+), 51 deletions(-) diff --git a/source/blender/animrig/intern/keyframing_test.cc b/source/blender/animrig/intern/keyframing_test.cc index cccf9a1820d..a10161e2e6a 100644 --- a/source/blender/animrig/intern/keyframing_test.cc +++ b/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 diff --git a/source/blender/blenkernel/BKE_nla.hh b/source/blender/blenkernel/BKE_nla.hh index 9dde5771fc8..fe13f1c071d 100644 --- a/source/blender/blenkernel/BKE_nla.hh +++ b/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); diff --git a/source/blender/blenkernel/intern/anim_data.cc b/source/blender/blenkernel/intern/anim_data.cc index af01fad9a01..0bc643a1b44 100644 --- a/source/blender/blenkernel/intern/anim_data.cc +++ b/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) { diff --git a/source/blender/blenkernel/intern/nla.cc b/source/blender/blenkernel/intern/nla.cc index 435557fe570..1b31631a633 100644 --- a/source/blender/blenkernel/intern/nla.cc +++ b/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}); } } diff --git a/source/blender/blenloader/intern/versioning_400.cc b/source/blender/blenloader/intern/versioning_400.cc index 85713992735..4810ee947e3 100644 --- a/source/blender/blenloader/intern/versioning_400.cc +++ b/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; diff --git a/source/blender/editors/space_action/action_data.cc b/source/blender/editors/space_action/action_data.cc index cf02b7d7cb1..4bffc15a648 100644 --- a/source/blender/editors/space_action/action_data.cc +++ b/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; } } diff --git a/source/blender/editors/space_nla/nla_edit.cc b/source/blender/editors/space_nla/nla_edit.cc index 322c8d3344b..43cf691ffaa 100644 --- a/source/blender/editors/space_nla/nla_edit.cc +++ b/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 */ diff --git a/source/blender/makesdna/DNA_anim_types.h b/source/blender/makesdna/DNA_anim_types.h index c24a3f1deff..06c7fe5d333 100644 --- a/source/blender/makesdna/DNA_anim_types.h +++ b/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 diff --git a/source/blender/makesrna/intern/rna_animation.cc b/source/blender/makesrna/intern/rna_animation.cc index e181bf95743..5abfaa5d799 100644 --- a/source/blender/makesrna/intern/rna_animation.cc +++ b/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}); } } diff --git a/source/blender/makesrna/intern/rna_space.cc b/source/blender/makesrna/intern/rna_space.cc index 3dcedd46e4a..9012effb8cc 100644 --- a/source/blender/makesrna/intern/rna_space.cc +++ b/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.