diff --git a/source/blender/blenkernel/BKE_nla.h b/source/blender/blenkernel/BKE_nla.h index 8d796f29edb..d01a49f5878 100644 --- a/source/blender/blenkernel/BKE_nla.h +++ b/source/blender/blenkernel/BKE_nla.h @@ -151,6 +151,38 @@ void BKE_nlatrack_remove(ListBase *tracks, struct NlaTrack *nlt); */ void BKE_nlatrack_remove_and_free(ListBase *tracks, struct NlaTrack *nlt, bool do_id_user); +/** + * Compute the length of the passed strip's clip, unless the clip length + * is zero in which case a non-zero value is returned. + * + * WARNING: this function is *very narrow* and special-cased in its + * application. It was introduced as part of the fix for issue #107030, + * as a way to collect a bunch of whac-a-mole inline applications of this + * logic in one place. The logic itself isn't principled in any way, + * and should almost certainly not be used anywhere that it isn't already, + * short of one of those whac-a-mole inline places being overlooked. + * + * The underlying purpose of this function is to ensure that the computed + * clip length for an NLA strip is (in certain places) never zero, in order to + * avoid the strip's scale having to be infinity. In other words, it's a + * hack. But at least now it's a hack collected in one place. + * + */ +float BKE_nla_clip_length_get_nonzero(const NlaStrip *strip); + +/** + * Ensure the passed range has non-zero length, using the same logic as + * `BKE_nla_clip_length_get_nonzero` to determine the new non-zero length. + * + * See the documentation for `BKE_nla_clip_length_get_nonzero` for the + * reason this function exists and the issues around its use. + * + * Usage: both `actstart` and `r_actend` should already be set to the + * start/end values of a strip's clip. `r_actend` will be modified + * if necessary to ensure the range is non-zero in length. + */ +void BKE_nla_clip_length_ensure_nonzero(const float *actstart, float *r_actend); + /** * Create a NLA Strip referencing the given Action. */ diff --git a/source/blender/blenkernel/intern/action.cc b/source/blender/blenkernel/intern/action.cc index 964edb396b5..d38b45a81cb 100644 --- a/source/blender/blenkernel/intern/action.cc +++ b/source/blender/blenkernel/intern/action.cc @@ -1445,17 +1445,12 @@ void BKE_action_frame_range_calc(const bAction *act, } if (foundvert || foundmod) { - /* ensure that action is at least 1 frame long (for NLA strips to have a valid length) */ - if (min == max) { - max += 1.0f; - } - *r_start = max_ff(min, MINAFRAMEF); *r_end = min_ff(max, MAXFRAMEF); } else { *r_start = 0.0f; - *r_end = 1.0f; + *r_end = 0.0f; } } @@ -1469,10 +1464,7 @@ void BKE_action_frame_range_get(const bAction *act, float *r_start, float *r_end BKE_action_frame_range_calc(act, false, r_start, r_end); } - /* Ensure that action is at least 1 frame long (for NLA strips to have a valid length). */ - if (*r_start >= *r_end) { - *r_end = *r_start + 1.0f; - } + BLI_assert(*r_start <= *r_end); } bool BKE_action_is_cyclic(const bAction *act) diff --git a/source/blender/blenkernel/intern/action_test.cc b/source/blender/blenkernel/intern/action_test.cc index 26f30b6ed00..c3d39e056a0 100644 --- a/source/blender/blenkernel/intern/action_test.cc +++ b/source/blender/blenkernel/intern/action_test.cc @@ -223,4 +223,66 @@ TEST(action_assets, BKE_action_has_single_frame) } } +TEST(action, BKE_action_frame_range_calc) +{ + float start, end; + + /* No FCurves. */ + { + const bAction empty = {{nullptr}}; + BKE_action_frame_range_calc(&empty, false, &start, &end); + EXPECT_FLOAT_EQ(start, 0.0f); + EXPECT_FLOAT_EQ(end, 0.0f); + } + + /* One curve with one key. */ + { + FCurve fcu = {nullptr}; + std::unique_ptr bezt = allocate_keyframes(&fcu, 1); + add_keyframe(&fcu, 1.0f, 2.0f); + + bAction action = {{nullptr}}; + BLI_addtail(&action.curves, &fcu); + + BKE_action_frame_range_calc(&action, false, &start, &end); + EXPECT_FLOAT_EQ(start, 1.0f); + EXPECT_FLOAT_EQ(end, 1.0f); + } + + /* Two curves with one key each on different frames. */ + { + FCurve fcu1 = {nullptr}; + FCurve fcu2 = {nullptr}; + std::unique_ptr bezt1 = allocate_keyframes(&fcu1, 1); + std::unique_ptr bezt2 = allocate_keyframes(&fcu2, 1); + add_keyframe(&fcu1, 1.0f, 2.0f); + add_keyframe(&fcu2, 1.5f, 2.0f); + + bAction action = {{nullptr}}; + BLI_addtail(&action.curves, &fcu1); + BLI_addtail(&action.curves, &fcu2); + + BKE_action_frame_range_calc(&action, false, &start, &end); + EXPECT_FLOAT_EQ(start, 1.0f); + EXPECT_FLOAT_EQ(end, 1.5f); + } + + /* One curve with two keys. */ + { + FCurve fcu = {nullptr}; + std::unique_ptr bezt = allocate_keyframes(&fcu, 2); + add_keyframe(&fcu, 1.0f, 2.0f); + add_keyframe(&fcu, 1.5f, 2.0f); + + bAction action = {{nullptr}}; + BLI_addtail(&action.curves, &fcu); + + BKE_action_frame_range_calc(&action, false, &start, &end); + EXPECT_FLOAT_EQ(start, 1.0f); + EXPECT_FLOAT_EQ(end, 1.5f); + } + + /* TODO: action with fcurve modifiers. */ +} + } // namespace blender::bke::tests diff --git a/source/blender/blenkernel/intern/anim_sys.cc b/source/blender/blenkernel/intern/anim_sys.cc index ba595a47e5d..f38d5c73784 100644 --- a/source/blender/blenkernel/intern/anim_sys.cc +++ b/source/blender/blenkernel/intern/anim_sys.cc @@ -3223,10 +3223,9 @@ static void animsys_create_action_track_strip(const AnimData *adt, * (which making new strips doesn't do due to the troublesome nature of that). */ BKE_action_frame_range_calc( r_action_strip->act, true, &r_action_strip->actstart, &r_action_strip->actend); + BKE_nla_clip_length_ensure_nonzero(&r_action_strip->actstart, &r_action_strip->actend); r_action_strip->start = r_action_strip->actstart; - r_action_strip->end = IS_EQF(r_action_strip->actstart, r_action_strip->actend) ? - (r_action_strip->actstart + 1.0f) : - (r_action_strip->actend); + r_action_strip->end = r_action_strip->actend; r_action_strip->blendmode = adt->act_blendmode; r_action_strip->extendmode = adt->act_extendmode; diff --git a/source/blender/blenkernel/intern/nla.cc b/source/blender/blenkernel/intern/nla.cc index a9656f07431..c261ec2d14d 100644 --- a/source/blender/blenkernel/intern/nla.cc +++ b/source/blender/blenkernel/intern/nla.cc @@ -446,6 +446,21 @@ NlaTrack *BKE_nlatrack_new_tail(ListBase *nla_tracks, const bool is_liboverride) return BKE_nlatrack_new_after(nla_tracks, (NlaTrack *)nla_tracks->last, is_liboverride); } +float BKE_nla_clip_length_get_nonzero(const NlaStrip *strip) +{ + if (strip->actend <= strip->actstart) { + return 1.0f; + } + return strip->actend - strip->actstart; +} + +void BKE_nla_clip_length_ensure_nonzero(const float *actstart, float *r_actend) +{ + if (*r_actend <= *actstart) { + *r_actend = *actstart + 1.0f; + } +} + NlaStrip *BKE_nlastrip_new(bAction *act) { NlaStrip *strip; @@ -478,13 +493,11 @@ NlaStrip *BKE_nlastrip_new(bAction *act) strip->act = act; id_us_plus(&act->id); - /* determine initial range - * - strip length cannot be 0... ever... - */ + /* determine initial range */ BKE_action_frame_range_get(strip->act, &strip->actstart, &strip->actend); - + BKE_nla_clip_length_ensure_nonzero(&strip->actstart, &strip->actend); strip->start = strip->actstart; - strip->end = IS_EQF(strip->actstart, strip->actend) ? (strip->actstart + 1.0f) : strip->actend; + strip->end = strip->actend; /* strip should be referenced as-is */ strip->scale = 1.0f; @@ -606,7 +619,7 @@ void BKE_nlatrack_remove_and_free(ListBase *tracks, NlaTrack *nlt, bool do_id_us */ static float nlastrip_get_frame_actionclip(NlaStrip *strip, float cframe, short mode) { - float actlength, scale; + float scale; // float repeat; // UNUSED /* get number of repeats */ @@ -624,10 +637,7 @@ static float nlastrip_get_frame_actionclip(NlaStrip *strip, float cframe, short scale = fabsf(strip->scale); /* length of referenced action */ - actlength = strip->actend - strip->actstart; - if (IS_EQF(actlength, 0.0f)) { - actlength = 1.0f; - } + const float actlength = BKE_nla_clip_length_get_nonzero(strip); /* reversed = play strip backwards */ if (strip->flag & NLASTRIP_FLAG_REVERSE) { @@ -1587,6 +1597,7 @@ void BKE_nlastrip_recalculate_bounds_sync_action(NlaStrip *strip) prev_actstart = strip->actstart; BKE_action_frame_range_get(strip->act, &strip->actstart, &strip->actend); + BKE_nla_clip_length_ensure_nonzero(&strip->actstart, &strip->actend); /* Set start such that key's do not visually move, to preserve the overall animation result. */ strip->start += (strip->actstart - prev_actstart) * strip->scale; @@ -1595,7 +1606,7 @@ void BKE_nlastrip_recalculate_bounds_sync_action(NlaStrip *strip) } void BKE_nlastrip_recalculate_bounds(NlaStrip *strip) { - float actlen, mapping; + float mapping; /* sanity checks * - must have a strip @@ -1606,10 +1617,7 @@ void BKE_nlastrip_recalculate_bounds(NlaStrip *strip) } /* calculate new length factors */ - actlen = strip->actend - strip->actstart; - if (IS_EQF(actlen, 0.0f)) { - actlen = 1.0f; - } + const float actlen = BKE_nla_clip_length_get_nonzero(strip); mapping = strip->scale * strip->repeat; diff --git a/source/blender/blenkernel/intern/nla_test.cc b/source/blender/blenkernel/intern/nla_test.cc index b8ac562c1d9..de9de4feba0 100644 --- a/source/blender/blenkernel/intern/nla_test.cc +++ b/source/blender/blenkernel/intern/nla_test.cc @@ -66,6 +66,52 @@ TEST(nla_strip, BKE_nlastrips_add_strip) EXPECT_TRUE(BKE_nlastrips_add_strip(&strips, &strip2)); } +TEST(nla_strip, BKE_nla_clip_length_get_nonzero) +{ + NlaStrip strip{}; + + strip.actstart = 2.0f; + strip.actend = 4.0f; + EXPECT_FLOAT_EQ(BKE_nla_clip_length_get_nonzero(&strip), 2.0f); + + strip.actstart = 2.0f; + strip.actend = 3.0f; + EXPECT_FLOAT_EQ(BKE_nla_clip_length_get_nonzero(&strip), 1.0f); + + strip.actstart = 2.0f; + strip.actend = 2.0625f; + EXPECT_FLOAT_EQ(BKE_nla_clip_length_get_nonzero(&strip), 0.0625f); + + strip.actstart = 2.0f; + strip.actend = 2.0f; + EXPECT_FLOAT_EQ(BKE_nla_clip_length_get_nonzero(&strip), 1.0f); +} + +TEST(nla_strip, BKE_nla_clip_length_ensure_nonzero) +{ + float start, end; + + start = 2.0f; + end = 4.0f; + BKE_nla_clip_length_ensure_nonzero(&start, &end); + EXPECT_FLOAT_EQ(end, 4.0f); + + start = 2.0f; + end = 3.0f; + BKE_nla_clip_length_ensure_nonzero(&start, &end); + EXPECT_FLOAT_EQ(end, 3.0f); + + start = 2.0f; + end = 2.0625f; + BKE_nla_clip_length_ensure_nonzero(&start, &end); + EXPECT_FLOAT_EQ(end, 2.0625f); + + start = 2.0f; + end = 2.0f; + BKE_nla_clip_length_ensure_nonzero(&start, &end); + EXPECT_FLOAT_EQ(end, 3.0f); +} + TEST(nla_track, BKE_nlatrack_remove_strip) { NlaTrack track{}; diff --git a/source/blender/editors/space_nla/nla_draw.cc b/source/blender/editors/space_nla/nla_draw.cc index 96f02659a2f..e7b98545c7e 100644 --- a/source/blender/editors/space_nla/nla_draw.cc +++ b/source/blender/editors/space_nla/nla_draw.cc @@ -889,6 +889,7 @@ void draw_nla_main_data(bAnimContext *ac, SpaceNla *snla, ARegion *region) float r_start; float r_end; BKE_action_frame_range_get(static_cast(ale->data), &r_start, &r_end); + BKE_nla_clip_length_ensure_nonzero(&r_start, &r_end); immRectf(pos, r_end, ymin + NLACHANNEL_SKIP, v2d->cur.xmax, ymax - NLACHANNEL_SKIP); break; diff --git a/source/blender/makesrna/intern/rna_nla.cc b/source/blender/makesrna/intern/rna_nla.cc index d4b51443e36..9323e977a18 100644 --- a/source/blender/makesrna/intern/rna_nla.cc +++ b/source/blender/makesrna/intern/rna_nla.cc @@ -310,10 +310,7 @@ static void rna_NlaStrip_frame_end_ui_set(PointerRNA *ptr, float value) /* calculate the lengths the strip and its action : * * (Meta and transitions shouldn't be updated, but clip and sound should) */ if (data->type == NLASTRIP_TYPE_CLIP || data->type == NLASTRIP_TYPE_SOUND) { - float actlen = data->actend - data->actstart; - if (IS_EQF(actlen, 0.0f)) { - actlen = 1.0f; /* Only sanity check needed : we use this as divisor later on. */ - } + const float actlen = BKE_nla_clip_length_get_nonzero(data); /* Modify the strip's action end frame, or repeat based on : * - if data->repeat == 1.0f, modify the action end frame :