From 63cf0d089082fc3c40d3bd1471bbefd786488aea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Mon, 23 May 2022 16:05:49 +0200 Subject: [PATCH] Fix T98196: Crash when moving tweaked NLA strip with empty track below `BKE_nlastrip_find_active()` and `update_active_strip()` now do a more thorough search for the active strip, by also recursing into meta-strips. There were some assumptions in the NLA code that the active strips is contained in the active track. This assumption turned out to be false: when there is a meta-strip, the active strip could be inside that, and then it's not contained in `active_track.strips` directly. Apart from the above, there are other situations in which the track pointed to by `AnimData::act_track` does *not* contain the active strip (even indirectly). Both these cases can happen when the transform system is moving a strip that's in tweak mode. Entering tweak mode doesn't just search for the active track/strip, but also falls back to the last-selected ones. This means that the track/strip pointers & flags can be out of sync with what's being tweaked/transformed. Because of this, the assumption that there is any relation between "active strip" and "active track" has been removed from the `update_active_strip()` function. All this searching could be prevented by passing the `AnimData` to the code that duplicates tracks & strips. That way, when the active track/strip is dup'ed, the `AnimData` pointers can be updated as well. That would require more refactoring and thus could introduce other bugs, so that's left for later. --- source/blender/blenkernel/intern/nla.c | 79 +++++++++++++++++--------- 1 file changed, 51 insertions(+), 28 deletions(-) diff --git a/source/blender/blenkernel/intern/nla.c b/source/blender/blenkernel/intern/nla.c index 5d804f53779..a5f6c453ed4 100644 --- a/source/blender/blenkernel/intern/nla.c +++ b/source/blender/blenkernel/intern/nla.c @@ -244,24 +244,38 @@ void BKE_nla_tracks_copy(Main *bmain, ListBase *dst, const ListBase *src, const } } -/* Set adt_dest->actstrip to the strip with the same index as adt_source->actstrip. */ -static void update_active_strip(AnimData *adt_dest, - NlaTrack *track_dest, - const AnimData *adt_source, - NlaTrack *track_source) +static void update_active_strip_from_listbase(AnimData *adt_dest, + NlaTrack *track_dest, + const NlaStrip *active_strip, + const ListBase /* NlaStrip */ *strips_source) { - BLI_assert(BLI_listbase_count(&track_source->strips) == BLI_listbase_count(&track_dest->strips)); - NlaStrip *strip_dest = track_dest->strips.first; - LISTBASE_FOREACH (NlaStrip *, strip_source, &track_source->strips) { - if (strip_source == adt_source->actstrip) { + LISTBASE_FOREACH (const NlaStrip *, strip_source, strips_source) { + if (strip_source == active_strip) { adt_dest->actstrip = strip_dest; + return; + } + + if (strip_source->type == NLASTRIP_TYPE_META) { + update_active_strip_from_listbase(adt_dest, track_dest, active_strip, &strip_source->strips); } strip_dest = strip_dest->next; } } +/* Set adt_dest->actstrip to the strip with the same index as adt_source->actstrip. */ +static void update_active_strip(AnimData *adt_dest, + NlaTrack *track_dest, + const AnimData *adt_source, + const NlaTrack *track_source) +{ + BLI_assert(BLI_listbase_count(&track_source->strips) == BLI_listbase_count(&track_dest->strips)); + + update_active_strip_from_listbase( + adt_dest, track_dest, adt_source->actstrip, &track_source->strips); +} + /* Set adt_dest->act_track to the track with the same index as adt_source->act_track. */ static void update_active_track(AnimData *adt_dest, const AnimData *adt_source) { @@ -272,20 +286,20 @@ static void update_active_track(AnimData *adt_dest, const AnimData *adt_source) LISTBASE_FOREACH (NlaTrack *, track_source, &adt_source->nla_tracks) { if (track_source == adt_source->act_track) { adt_dest->act_track = track_dest; - /* Assumption: the active strip is on the active track. */ - update_active_strip(adt_dest, track_dest, adt_source, track_source); } + update_active_strip(adt_dest, track_dest, adt_source, track_source); track_dest = track_dest->next; } - /* If the above assumption failed to hold, do a more thorough search for the active strip. */ - if (adt_source->actstrip != NULL && adt_dest->actstrip == NULL) { - nla_tweakmode_find_active(&adt_source->nla_tracks, &track_dest, &adt_dest->actstrip); +#ifndef NDEBUG + { + const bool source_has_actstrip = adt_source->actstrip != NULL; + const bool dest_has_actstrip = adt_dest->actstrip != NULL; + BLI_assert_msg(source_has_actstrip == dest_has_actstrip, + "Active strip did not copy correctly"); } - - BLI_assert_msg((adt_source->actstrip == NULL) == (adt_dest->actstrip == NULL), - "Active strip did not copy correctly"); +#endif } void BKE_nla_tracks_copy_from_adt(Main *bmain, @@ -1171,26 +1185,35 @@ bool BKE_nlatrack_is_nonlocal_in_liboverride(const ID *id, const NlaTrack *nlt) /* NLA Strips -------------------------------------- */ -NlaStrip *BKE_nlastrip_find_active(NlaTrack *nlt) +static NlaStrip *nlastrip_find_active(ListBase /* NlaStrip */ *strips) { - NlaStrip *strip; - - /* sanity check */ - if (ELEM(NULL, nlt, nlt->strips.first)) { - return NULL; - } - - /* try to find the first active strip */ - for (strip = nlt->strips.first; strip; strip = strip->next) { + LISTBASE_FOREACH (NlaStrip *, strip, strips) { if (strip->flag & NLASTRIP_FLAG_ACTIVE) { return strip; } + + if (strip->type != NLASTRIP_TYPE_META) { + continue; + } + + NlaStrip *inner_active = nlastrip_find_active(&strip->strips); + if (inner_active != NULL) { + return inner_active; + } } - /* none found */ return NULL; } +NlaStrip *BKE_nlastrip_find_active(NlaTrack *nlt) +{ + if (nlt == NULL) { + return NULL; + } + + return nlastrip_find_active(&nlt->strips); +} + void BKE_nlastrip_set_active(AnimData *adt, NlaStrip *strip) { NlaTrack *nlt;