diff options
author | Sybren A. Stüvel <sybren@blender.org> | 2022-05-23 17:05:49 +0300 |
---|---|---|
committer | Sybren A. Stüvel <sybren@blender.org> | 2022-05-23 17:12:09 +0300 |
commit | 63cf0d089082fc3c40d3bd1471bbefd786488aea (patch) | |
tree | 316ad6be0835e6296a9a0b6812a5f735d28f648e /source | |
parent | 9bb4bf57485475faa491e31b094bded67d5dd2dd (diff) |
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.
Diffstat (limited to 'source')
-rw-r--r-- | source/blender/blenkernel/intern/nla.c | 79 |
1 files 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; |