Fix #107030: return accurate action frame ranges from the Python API

This changes the `action.frame_range` Python API to return an accurate
frame range for actions.  Specifically, it was previously special-cased
to return a range with length 1 whenever the length was actually 0.  This
led to a bizarre situation where a real frame range of `[0.0, 0.2]` would
return that range as-is, but a real frame range of `[0.0, 0.0]` would
instead return a range of `[0.0, 1.0]`.

The new behavior simply always returns the real frame range.

The reason for the previous behavior was obscure: the relevant code was
also used internally in Blender's NLA system, and returning a zero-length
range could result in NLA strips getting infinite scale.  The code is now
separated out appropriately so that the NLA system still gets the
non-zero-length range, while the Python API for actions returns the real
range.

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

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.
  */

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)

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<BezTriple[]> 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<BezTriple[]> bezt1 = allocate_keyframes(&fcu1, 1);
+    std::unique_ptr<BezTriple[]> 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<BezTriple[]> 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

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;

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;
 

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{};

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<bAction *>(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;

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 :