Fix T87173: wrong Auto-Snap in animation editors

This was partially broken with {rBde9ea94fc6f}.

The `Frame Step` and `Second Step` snapping options were working as if
they were `Nearest Frame` and `Nearest Second` respectively in the
`Dope Sheet` and `NLA` editors.

In the `Graph Editor` the problem was more serious:
"Second Step: ... The keyframe itself moves along as though in snapping
were active at all, while its handles 'stay behind' until it reaches
the next second boundary, at which point the teleport handles to
'catch up'".

The snapping code for these modes was spread across the transform
mode code and `recalcData` of each data type. Therefore, create a
unified snapping code for these options so that all issues are fixed in
one place.

Differetial Revision: https://developer.blender.org/D12241

9 files changed, 119 insertions, 171 deletions

diff --git a/source/blender/editors/transform/transform_convert.c b/source/blender/editors/transform/transform_convert.c
index 3f730956dd0..094ae080de0 100644
--- a/source/blender/editors/transform/transform_convert.c
+++ b/source/blender/editors/transform/transform_convert.c
@@ -1662,6 +1662,23 @@ void animrecord_check_state(TransInfo *t, struct Object *ob)
   }
 }
 
+void transform_convert_flush_handle2D(TransData *td, TransData2D *td2d, const float inv_unit_scale)
+{
+  /* If the handles are to be moved too
+   * (as side-effect of keyframes moving, to keep the general effect)
+   * offset them by the same amount so that the general angles are maintained
+   * (i.e. won't change while handles are free-to-roam and keyframes are snap-locked).
+   */
+  if ((td->flag & TD_MOVEHANDLE1) && td2d->h1) {
+    td2d->h1[0] = td2d->ih1[0] + td->loc[0] - td->iloc[0];
+    td2d->h1[1] = td2d->ih1[1] + (td->loc[1] - td->iloc[1]) * inv_unit_scale;
+  }
+  if ((td->flag & TD_MOVEHANDLE2) && td2d->h2) {
+    td2d->h2[0] = td2d->ih2[0] + td->loc[0] - td->iloc[0];
+    td2d->h2[1] = td2d->ih2[1] + (td->loc[1] - td->iloc[1]) * inv_unit_scale;
+  }
+}
+
 /* called for updating while transform acts, once per redraw */
 void recalcData(TransInfo *t)
 {
diff --git a/source/blender/editors/transform/transform_convert.h b/source/blender/editors/transform/transform_convert.h
index 55731bfa321..fa34e2555d6 100644
--- a/source/blender/editors/transform/transform_convert.h
+++ b/source/blender/editors/transform/transform_convert.h
@@ -43,6 +43,9 @@ void sort_trans_data_dist(TransInfo *t);
 void createTransData(struct bContext *C, TransInfo *t);
 bool clipUVTransform(TransInfo *t, float vec[2], const bool resize);
 void clipUVData(TransInfo *t);
+void transform_convert_flush_handle2D(TransData *td,
+                                      TransData2D *td2d,
+                                      const float inv_unit_scale);
 void recalcData(TransInfo *t);
 
 /* transform_convert_mesh.c */
diff --git a/source/blender/editors/transform/transform_convert_action.c b/source/blender/editors/transform/transform_convert_action.c
index 30cb9fa20e5..8a3b254be5c 100644
--- a/source/blender/editors/transform/transform_convert_action.c
+++ b/source/blender/editors/transform/transform_convert_action.c
@@ -45,6 +45,8 @@
 #include "WM_types.h"
 
 #include "transform.h"
+#include "transform_snap.h"
+
 #include "transform_convert.h"
 
 /* helper struct for gp-frame transforms */
@@ -604,6 +606,19 @@ void recalcData_actedit(TransInfo *t)
     flushTransIntFrameActionData(t);
   }
 
+  /* Flush 2d vector. */
+  TransDataContainer *tc = TRANS_DATA_CONTAINER_FIRST_SINGLE(t);
+  const short autosnap = getAnimEdit_SnapMode(t);
+  TransData *td;
+  TransData2D *td2d;
+  int i = 0;
+  for (td = tc->data, td2d = tc->data_2d; i < tc->data_len; i++, td++, td2d++) {
+    if ((autosnap != SACTSNAP_OFF) && (t->state != TRANS_CANCEL) && !(td->flag & TD_NOTIMESNAP)) {
+      transform_snap_anim_flush_data(t, td, autosnap, td->loc);
+    }
+    transform_convert_flush_handle2D(td, td2d, 1.0f);
+  }
+
   if (ac.datatype != ANIMCONT_MASK) {
     /* Get animdata blocks visible in editor,
      * assuming that these will be the ones where things changed. */
diff --git a/source/blender/editors/transform/transform_convert_graph.c b/source/blender/editors/transform/transform_convert_graph.c
index c0acdd89b02..d22277f9d91 100644
--- a/source/blender/editors/transform/transform_convert_graph.c
+++ b/source/blender/editors/transform/transform_convert_graph.c
@@ -40,6 +40,8 @@
 #include "UI_view2d.h"
 
 #include "transform.h"
+#include "transform_snap.h"
+
 #include "transform_convert.h"
 #include "transform_snap.h"
 
@@ -662,10 +664,9 @@ static void flushTransGraphData(TransInfo *t)
   TransData *td;
   TransData2D *td2d;
   TransDataGraph *tdg;
-  Scene *scene = t->scene;
-  double secf = FPS;
   int a;
 
+  const short autosnap = getAnimEdit_SnapMode(t);
   TransDataContainer *tc = TRANS_DATA_CONTAINER_FIRST_SINGLE(t);
 
   /* flush to 2d vector from internally used 3d vector */
@@ -681,22 +682,8 @@ static void flushTransGraphData(TransInfo *t)
      * - Only apply to keyframes (but never to handles).
      * - Don't do this when canceling, or else these changes won't go away.
      */
-    if ((t->state != TRANS_CANCEL) && (td->flag & TD_NOTIMESNAP) == 0) {
-      const short autosnap = getAnimEdit_SnapMode(t);
-      switch (autosnap) {
-        case SACTSNAP_FRAME: /* snap to nearest frame */
-          td2d->loc[0] = floor((double)td2d->loc[0] + 0.5);
-          break;
-
-        case SACTSNAP_SECOND: /* snap to nearest second */
-          td2d->loc[0] = floor(((double)td2d->loc[0] / secf) + 0.5) * secf;
-          break;
-
-        case SACTSNAP_MARKER: /* snap to nearest marker */
-          td2d->loc[0] = (float)ED_markers_find_nearest_marker_time(&t->scene->markers,
-                                                                    td2d->loc[0]);
-          break;
-      }
+    if ((autosnap != SACTSNAP_OFF) && (t->state != TRANS_CANCEL) && !(td->flag & TD_NOTIMESNAP)) {
+      transform_snap_anim_flush_data(t, td, autosnap, td->loc);
     }
 
     /* we need to unapply the nla-mapping from the time in some situations */
@@ -707,32 +694,6 @@ static void flushTransGraphData(TransInfo *t)
       td2d->loc2d[0] = td2d->loc[0];
     }
 
-    /** Time-stepping auto-snapping modes don't get applied for Graph Editor transforms,
-     * as these use the generic transform modes which don't account for this sort of thing.
-     * These ones aren't affected by NLA mapping, so we do this after the conversion...
-     *
-     * \note We also have to apply to td->loc,
-     * as that's what the handle-adjustment step below looks to,
-     * otherwise we get "swimming handles".
-     *
-     * \note We don't do this when canceling transforms, or else these changes don't go away.
-     */
-    if ((t->state != TRANS_CANCEL) && (td->flag & TD_NOTIMESNAP) == 0) {
-      const short autosnap = getAnimEdit_SnapMode(t);
-      switch (autosnap) {
-        case SACTSNAP_STEP: /* frame step */
-          td2d->loc2d[0] = floor((double)td2d->loc[0] + 0.5);
-          td->loc[0] = floor((double)td->loc[0] + 0.5);
-          break;
-
-        case SACTSNAP_TSTEP: /* second step */
-          /* XXX: the handle behavior in this case is still not quite right... */
-          td2d->loc[0] = floor(((double)td2d->loc[0] / secf) + 0.5) * secf;
-          td->loc[0] = floor(((double)td->loc[0] / secf) + 0.5) * secf;
-          break;
-      }
-    }
-
     /* if int-values only, truncate to integers */
     if (td->flag & TD_INTVALUES) {
       td2d->loc2d[1] = floorf(td2d->loc[1] * inv_unit_scale - tdg->offset + 0.5f);
@@ -741,15 +702,7 @@ static void flushTransGraphData(TransInfo *t)
       td2d->loc2d[1] = td2d->loc[1] * inv_unit_scale - tdg->offset;
     }
 
-    if ((td->flag & TD_MOVEHANDLE1) && td2d->h1) {
-      td2d->h1[0] = td2d->ih1[0] + td->loc[0] - td->iloc[0];
-      td2d->h1[1] = td2d->ih1[1] + (td->loc[1] - td->iloc[1]) * inv_unit_scale;
-    }
-
-    if ((td->flag & TD_MOVEHANDLE2) && td2d->h2) {
-      td2d->h2[0] = td2d->ih2[0] + td->loc[0] - td->iloc[0];
-      td2d->h2[1] = td2d->ih2[1] + (td->loc[1] - td->iloc[1]) * inv_unit_scale;
-    }
+    transform_convert_flush_handle2D(td, td2d, inv_unit_scale);
   }
 }
 
diff --git a/source/blender/editors/transform/transform_convert_nla.c b/source/blender/editors/transform/transform_convert_nla.c
index 4d98b7a489f..7e5b80c2453 100644
--- a/source/blender/editors/transform/transform_convert_nla.c
+++ b/source/blender/editors/transform/transform_convert_nla.c
@@ -41,6 +41,8 @@
 #include "RNA_access.h"
 
 #include "transform.h"
+#include "transform_snap.h"
+
 #include "transform_convert.h"
 #include "transform_snap.h"
 
@@ -292,21 +294,30 @@ void createTransNlaData(bContext *C, TransInfo *t)
 void recalcData_nla(TransInfo *t)
 {
   SpaceNla *snla = (SpaceNla *)t->area->spacedata.first;
-  Scene *scene = t->scene;
-  double secf = FPS;
-  int i;
 
   TransDataContainer *tc = TRANS_DATA_CONTAINER_FIRST_SINGLE(t);
-  TransDataNla *tdn = tc->custom.type.data;
+
+  /* handle auto-snapping
+   * NOTE: only do this when transform is still running, or we can't restore
+   */
+  if (t->state != TRANS_CANCEL) {
+    const short autosnap = getAnimEdit_SnapMode(t);
+    if (autosnap != SACTSNAP_OFF) {
+      TransData *td = tc->data;
+      for (int i = 0; i < tc->data_len; i++, td++) {
+        transform_snap_anim_flush_data(t, td, autosnap, td->loc);
+      }
+    }
+  }
 
   /* For each strip we've got, perform some additional validation of the values
    * that got set before using RNA to set the value (which does some special
    * operations when setting these values to make sure that everything works ok).
    */
-  for (i = 0; i < tc->data_len; i++, tdn++) {
+  TransDataNla *tdn = tc->custom.type.data;
+  for (int i = 0; i < tc->data_len; i++, tdn++) {
     NlaStrip *strip = tdn->strip;
     PointerRNA strip_ptr;
-    short iter;
     int delta_y1, delta_y2;
 
     /* if this tdn has no handles, that means it is just a dummy that should be skipped */
@@ -370,8 +381,7 @@ void recalcData_nla(TransInfo *t)
       next = next->next;
     }
 
-    for (iter = 0; iter < 5; iter++) {
-
+    for (short iter = 0; iter < 5; iter++) {
       const bool pExceeded = (prev != NULL) && (tdn->h1[0] < prev->end);
       const bool nExceeded = (next != NULL) && (tdn->h2[0] > next->start);
 
@@ -410,51 +420,6 @@ void recalcData_nla(TransInfo *t)
       }
     }
 
-    /* handle auto-snapping
-     * NOTE: only do this when transform is still running, or we can't restore
-     */
-    if (t->state != TRANS_CANCEL) {
-      const short autosnap = getAnimEdit_SnapMode(t);
-      switch (autosnap) {
-        case SACTSNAP_FRAME: /* snap to nearest frame */
-        case SACTSNAP_STEP:  /* frame step - this is basically the same,
-                              * since we don't have any remapping going on */
-        {
-          tdn->h1[0] = floorf(tdn->h1[0] + 0.5f);
-          tdn->h2[0] = floorf(tdn->h2[0] + 0.5f);
-          break;
-        }
-
-        case SACTSNAP_SECOND: /* snap to nearest second */
-        case SACTSNAP_TSTEP:  /* second step - this is basically the same,
-                               * since we don't have any remapping going on */
-        {
-          /* This case behaves differently from the rest, since lengths of strips
-           * may not be multiples of a second. If we just naively resize adjust
-           * the handles, things may not work correctly. Instead, we only snap
-           * the first handle, and move the other to fit.
-           *
-           * FIXME: we do run into problems here when user attempts to negatively
-           *        scale the strip, as it then just compresses down and refuses
-           *        to expand out the other end.
-           */
-          float h1_new = (float)(floor(((double)tdn->h1[0] / secf) + 0.5) * secf);
-          float delta = h1_new - tdn->h1[0];
-
-          tdn->h1[0] = h1_new;
-          tdn->h2[0] += delta;
-          break;
-        }
-
-        case SACTSNAP_MARKER: /* snap to nearest marker */
-        {
-          tdn->h1[0] = (float)ED_markers_find_nearest_marker_time(&t->scene->markers, tdn->h1[0]);
-          tdn->h2[0] = (float)ED_markers_find_nearest_marker_time(&t->scene->markers, tdn->h2[0]);
-          break;
-        }
-      }
-    }
-
     /* Use RNA to write the values to ensure that constraints on these are obeyed
      * (e.g. for transition strips, the values are taken from the neighbors)
      *
diff --git a/source/blender/editors/transform/transform_mode_timescale.c b/source/blender/editors/transform/transform_mode_timescale.c
index 4123663e849..50fd714727b 100644
--- a/source/blender/editors/transform/transform_mode_timescale.c
+++ b/source/blender/editors/transform/transform_mode_timescale.c
@@ -65,7 +65,6 @@ static void headerTimeScale(TransInfo *t, char str[UI_MAX_DRAW_STR])
 static void applyTimeScaleValue(TransInfo *t, float value)
 {
   Scene *scene = t->scene;
-  const short autosnap = getAnimEdit_SnapMode(t);
 
   FOREACH_TRANS_DATA_CONTAINER (t, tc) {
     TransData *td = tc->data;
@@ -89,9 +88,6 @@ static void applyTimeScaleValue(TransInfo *t, float value)
 
       /* now, calculate the new value */
       *(td->val) = ((td->ival - startx) * fac) + startx;
-
-      /* apply nearest snapping */
-      doAnimEdit_SnapFrame(t, td, td2d, adt, autosnap);
     }
   }
 }
diff --git a/source/blender/editors/transform/transform_mode_timetranslate.c b/source/blender/editors/transform/transform_mode_timetranslate.c
index 01b9a08cf27..294040946bd 100644
--- a/source/blender/editors/transform/transform_mode_timetranslate.c
+++ b/source/blender/editors/transform/transform_mode_timetranslate.c
@@ -59,10 +59,18 @@ static void headerTimeTranslate(TransInfo *t, char str[UI_MAX_DRAW_STR])
   }
   else {
     const short autosnap = getAnimEdit_SnapMode(t);
-    float val = t->values_final[0];
+    float ival = TRANS_DATA_CONTAINER_FIRST_OK(t)->data->ival;
+    float val = ival + t->values_final[0];
 
-    float snap_val;
-    snapFrameTransform(t, autosnap, false, val, &snap_val);
+    float snap_val = val;
+    snapFrameTransform(t, autosnap, ival, val, &snap_val);
+
+    if (ELEM(autosnap, SACTSNAP_SECOND, SACTSNAP_TSTEP)) {
+      /* Convert to seconds. */
+      const Scene *scene = t->scene;
+      const double secf = FPS;
+      snap_val /= secf;
+    }
 
     if (autosnap == SACTSNAP_FRAME) {
       BLI_snprintf(&tvec[0], NUM_STR_REP_LEN, "%.2f (%.4f)", snap_val, val);
@@ -88,24 +96,11 @@ static void headerTimeTranslate(TransInfo *t, char str[UI_MAX_DRAW_STR])
 
 static void applyTimeTranslateValue(TransInfo *t, const float deltax)
 {
-  const short autosnap = getAnimEdit_SnapMode(t);
-
   FOREACH_TRANS_DATA_CONTAINER (t, tc) {
+    /* It doesn't matter whether we apply to t->data. */
     TransData *td = tc->data;
-    TransData2D *td2d = tc->data_2d;
-    /* It doesn't matter whether we apply to t->data or
-     * t->data2d, but t->data2d is more convenient. */
-    for (int i = 0; i < tc->data_len; i++, td++, td2d++) {
-      /* It is assumed that td->extra is a pointer to the AnimData,
-       * whose active action is where this keyframe comes from.
-       * (this is only valid when not in NLA)
-       * (also: masks and gpencil don't have animadata)
-       */
-      AnimData *adt = (t->spacetype != SPACE_NLA) ? td->extra : NULL;
-
-      /* apply nearest snapping */
-      *(td->val) = td->ival + deltax * td->factor;
-      doAnimEdit_SnapFrame(t, td, td2d, adt, autosnap);
+    for (int i = 0; i < tc->data_len; i++, td++) {
+      *(td->val) = td->loc[0] = td->ival + deltax * td->factor;
     }
   }
 }
diff --git a/source/blender/editors/transform/transform_snap.h b/source/blender/editors/transform/transform_snap.h
index 20cc590d7cc..ed7f93304bc 100644
--- a/source/blender/editors/transform/transform_snap.h
+++ b/source/blender/editors/transform/transform_snap.h
@@ -83,11 +83,12 @@ void transform_snap_sequencer_apply_translate(TransInfo *t, float *vec);
 
 /* transform_snap_animation.c */
 short getAnimEdit_SnapMode(TransInfo *t);
-void snapFrameTransform(struct TransInfo *t,
+void snapFrameTransform(TransInfo *t,
                         const eAnimEdit_AutoSnap autosnap,
-                        const bool is_frame_value,
-                        const float delta,
-                        /* return args */
-                        float *r_val);
-void doAnimEdit_SnapFrame(
-    TransInfo *t, TransData *td, TransData2D *td2d, struct AnimData *adt, short autosnap);
+                        const float val_initial,
+                        const float val_final,
+                        float *r_val_final);
+void transform_snap_anim_flush_data(TransInfo *t,
+                                    TransData *td,
+                                    const eAnimEdit_AutoSnap autosnap,
+                                    float *r_val_final);
diff --git a/source/blender/editors/transform/transform_snap_animation.c b/source/blender/editors/transform/transform_snap_animation.c
index ac7c47e408f..be7fc65a6c2 100644
--- a/source/blender/editors/transform/transform_snap_animation.c
+++ b/source/blender/editors/transform/transform_snap_animation.c
@@ -90,67 +90,70 @@ short getAnimEdit_SnapMode(TransInfo *t)
 
 void snapFrameTransform(TransInfo *t,
                         const eAnimEdit_AutoSnap autosnap,
-                        const bool is_frame_value,
-                        const float delta,
-                        /* return args */
-                        float *r_val)
+                        const float val_initial,
+                        const float val_final,
+                        float *r_val_final)
 {
-  double val = delta;
+  float deltax = val_final - val_initial;
   switch (autosnap) {
-    case SACTSNAP_STEP:
     case SACTSNAP_FRAME:
-      val = floor(val + 0.5);
+      *r_val_final = floorf(val_final + 0.5f);
       break;
     case SACTSNAP_MARKER:
-      /* snap to nearest marker */
+      /* Snap to nearest marker. */
       /* TODO: need some more careful checks for where data comes from. */
-      val = ED_markers_find_nearest_marker_time(&t->scene->markers, (float)val);
+      *r_val_final = (float)ED_markers_find_nearest_marker_time(&t->scene->markers, val_final);
       break;
     case SACTSNAP_SECOND:
     case SACTSNAP_TSTEP: {
-      /* second step */
       const Scene *scene = t->scene;
       const double secf = FPS;
-      val = floor((val / secf) + 0.5);
-      if (is_frame_value) {
-        val *= secf;
+      if (autosnap == SACTSNAP_SECOND) {
+        *r_val_final = floorf((val_final / secf) + 0.5) * secf;
+      }
+      else {
+        deltax = (float)(floor((deltax / secf) + 0.5) * secf);
+        *r_val_final = val_initial + deltax;
       }
       break;
     }
-    case SACTSNAP_OFF: {
+    case SACTSNAP_STEP:
+      deltax = floorf(deltax + 0.5f);
+      *r_val_final = val_initial + deltax;
+      break;
+    case SACTSNAP_OFF:
       break;
-    }
   }
-  *r_val = (float)val;
 }
 
 /* This function is used by Animation Editor specific transform functions to do
  * the Snap Keyframe to Nearest Frame/Marker
  */
-void doAnimEdit_SnapFrame(
-    TransInfo *t, TransData *td, TransData2D *td2d, AnimData *adt, short autosnap)
+void transform_snap_anim_flush_data(TransInfo *t,
+                                    TransData *td,
+                                    const eAnimEdit_AutoSnap autosnap,
+                                    float *r_val_final)
 {
-  if (autosnap != SACTSNAP_OFF) {
-    float val;
+  BLI_assert(autosnap != SACTSNAP_OFF);
 
-    /* convert frame to nla-action time (if needed) */
-    if (adt && (t->spacetype != SPACE_SEQ)) {
-      val = BKE_nla_tweakedit_remap(adt, *(td->val), NLATIME_CONVERT_MAP);
-    }
-    else {
-      val = *(td->val);
-    }
+  float val = td->loc[0];
+  float ival = td->iloc[0];
+  AnimData *adt = (!ELEM(t->spacetype, SPACE_NLA, SPACE_SEQ)) ? td->extra : NULL;
 
-    snapFrameTransform(t, autosnap, true, val, &val);
+  /* Convert frame to nla-action time (if needed) */
+  if (adt) {
+    val = BKE_nla_tweakedit_remap(adt, val, NLATIME_CONVERT_MAP);
+    ival = BKE_nla_tweakedit_remap(adt, ival, NLATIME_CONVERT_MAP);
+  }
 
-    /* convert frame out of nla-action time */
-    if (adt && (t->spacetype != SPACE_SEQ)) {
-      *(td->val) = BKE_nla_tweakedit_remap(adt, val, NLATIME_CONVERT_UNMAP);
-    }
-    else {
-      *(td->val) = val;
-    }
+  snapFrameTransform(t, autosnap, ival, val, &val);
+
+  /* Convert frame out of nla-action time. */
+  if (adt) {
+    val = BKE_nla_tweakedit_remap(adt, val, NLATIME_CONVERT_UNMAP);
   }
+
+  *r_val_final = val;
 }
 
 /** \} */