Fix T84850: "Lock to Selection" causes unwanted jumps

Adding new tracks, mask points, mask primitives, changing selection was
causing an unwanted jumps in the view.

This change makes it so those operations are preserving view offset.

Differential Revision: https://developer.blender.org/D10146

6 files changed, 223 insertions, 123 deletions

diff --git a/source/blender/editors/space_clip/clip_editor.c b/source/blender/editors/space_clip/clip_editor.c
index bd11a746e11..af1d082d317 100644
--- a/source/blender/editors/space_clip/clip_editor.c
+++ b/source/blender/editors/space_clip/clip_editor.c
@@ -327,118 +327,18 @@ void ED_clip_update_frame(const Main *mainp, int cfra)
   }
 }
 
-static bool selected_tracking_boundbox(SpaceClip *sc, float min[2], float max[2])
+bool ED_clip_view_selection(const bContext *C, ARegion *UNUSED(region), bool fit)
 {
-  MovieClip *clip = ED_space_clip_get_clip(sc);
-  MovieTrackingTrack *track;
-  int width, height;
-  bool ok = false;
-  ListBase *tracksbase = BKE_tracking_get_active_tracks(&clip->tracking);
-  int framenr = ED_space_clip_get_clip_frame_number(sc);
-
-  INIT_MINMAX2(min, max);
-
-  ED_space_clip_get_size(sc, &width, &height);
-
-  track = tracksbase->first;
-  while (track) {
-    if (TRACK_VIEW_SELECTED(sc, track)) {
-      MovieTrackingMarker *marker = BKE_tracking_marker_get(track, framenr);
-
-      if (marker) {
-        float pos[3];
-
-        pos[0] = marker->pos[0] + track->offset[0];
-        pos[1] = marker->pos[1] + track->offset[1];
-        pos[2] = 0.0f;
-
-        /* undistortion happens for normalized coords */
-        if (sc->user.render_flag & MCLIP_PROXY_RENDER_UNDISTORT) {
-          /* undistortion happens for normalized coords */
-          ED_clip_point_undistorted_pos(sc, pos, pos);
-        }
-
-        pos[0] *= width;
-        pos[1] *= height;
-
-        mul_v3_m4v3(pos, sc->stabmat, pos);
-
-        minmax_v2v2_v2(min, max, pos);
-
-        ok = true;
-      }
-    }
-
-    track = track->next;
-  }
-
-  return ok;
-}
-
-static bool selected_boundbox(const bContext *C, float min[2], float max[2])
-{
-  SpaceClip *sc = CTX_wm_space_clip(C);
-  if (sc->mode == SC_MODE_TRACKING) {
-    return selected_tracking_boundbox(sc, min, max);
-  }
-
-  if (ED_mask_selected_minmax(C, min, max)) {
-    MovieClip *clip = ED_space_clip_get_clip(sc);
-    int width, height;
-    ED_space_clip_get_size(sc, &width, &height);
-    BKE_mask_coord_to_movieclip(clip, &sc->user, min, min);
-    BKE_mask_coord_to_movieclip(clip, &sc->user, max, max);
-    min[0] *= width;
-    min[1] *= height;
-    max[0] *= width;
-    max[1] *= height;
-    return true;
-  }
-  return false;
-}
-
-bool ED_clip_view_selection(const bContext *C, ARegion *region, bool fit)
-{
-  SpaceClip *sc = CTX_wm_space_clip(C);
-  int w, h, frame_width, frame_height;
-  float min[2], max[2];
-
-  ED_space_clip_get_size(sc, &frame_width, &frame_height);
-
-  if ((frame_width == 0) || (frame_height == 0) || (sc->clip == NULL)) {
+  float offset_x, offset_y;
+  float zoom;
+  if (!clip_view_calculate_view_selection(C, fit, &offset_x, &offset_y, &zoom)) {
     return false;
   }
 
-  if (!selected_boundbox(C, min, max)) {
-    return false;
-  }
-
-  /* center view */
-  clip_view_center_to_point(
-      sc, (max[0] + min[0]) / (2 * frame_width), (max[1] + min[1]) / (2 * frame_height));
-
-  w = max[0] - min[0];
-  h = max[1] - min[1];
-
-  /* set zoom to see all selection */
-  if (w > 0 && h > 0) {
-    int width, height;
-    float zoomx, zoomy, newzoom, aspx, aspy;
-
-    ED_space_clip_get_aspect(sc, &aspx, &aspy);
-
-    width = BLI_rcti_size_x(&region->winrct) + 1;
-    height = BLI_rcti_size_y(&region->winrct) + 1;
-
-    zoomx = (float)width / w / aspx;
-    zoomy = (float)height / h / aspy;
-
-    newzoom = 1.0f / power_of_2(1.0f / min_ff(zoomx, zoomy));
-
-    if (fit || sc->zoom > newzoom) {
-      sc->zoom = newzoom;
-    }
-  }
+  SpaceClip *sc = CTX_wm_space_clip(C);
+  sc->xof = offset_x;
+  sc->yof = offset_y;
+  sc->zoom = zoom;
 
   return true;
 }
@@ -1177,3 +1077,47 @@ void clip_start_prefetch_job(const bContext *C)
   /* and finally start the job */
   WM_jobs_start(CTX_wm_manager(C), wm_job);
 }
+
+void ED_clip_view_lock_state_store(const bContext *C, ClipViewLockState *state)
+{
+  SpaceClip *space_clip = CTX_wm_space_clip(C);
+  BLI_assert(space_clip != NULL);
+
+  state->offset_x = space_clip->xof;
+  state->offset_y = space_clip->yof;
+  state->zoom = space_clip->zoom;
+
+  state->lock_offset_x = 0.0f;
+  state->lock_offset_y = 0.0f;
+
+  if ((space_clip->flag & SC_LOCK_SELECTION) == 0) {
+    return;
+  }
+
+  if (!clip_view_calculate_view_selection(
+          C, false, &state->offset_x, &state->offset_y, &state->zoom)) {
+    return;
+  }
+
+  state->lock_offset_x = space_clip->xlockof;
+  state->lock_offset_y = space_clip->ylockof;
+}
+
+void ED_clip_view_lock_state_restore_no_jump(const bContext *C, const ClipViewLockState *state)
+{
+  SpaceClip *space_clip = CTX_wm_space_clip(C);
+  BLI_assert(space_clip != NULL);
+
+  if ((space_clip->flag & SC_LOCK_SELECTION) == 0) {
+    return;
+  }
+
+  float offset_x, offset_y;
+  float zoom;
+  if (!clip_view_calculate_view_selection(C, false, &offset_x, &offset_y, &zoom)) {
+    return;
+  }
+
+  space_clip->xlockof = state->offset_x + state->lock_offset_x - offset_x;
+  space_clip->ylockof = state->offset_y + state->lock_offset_y - offset_y;
+}
diff --git a/source/blender/editors/space_clip/clip_intern.h b/source/blender/editors/space_clip/clip_intern.h
index 81df8cc8ecd..b9a69204281 100644
--- a/source/blender/editors/space_clip/clip_intern.h
+++ b/source/blender/editors/space_clip/clip_intern.h
@@ -171,8 +171,13 @@ void clip_delete_plane_track(struct bContext *C,
                              struct MovieClip *clip,
                              struct MovieTrackingPlaneTrack *plane_track);
 
+void clip_view_offset_for_center_to_point(
+    SpaceClip *sc, const float x, const float y, float *r_offset_x, float *r_offset_y);
 void clip_view_center_to_point(SpaceClip *sc, float x, float y);
 
+bool clip_view_calculate_view_selection(
+    const struct bContext *C, bool fit, float *r_offset_x, float *r_offset_y, float *r_zoom);
+
 void clip_draw_sfra_efra(struct View2D *v2d, struct Scene *scene);
 
 /* tracking_ops.c */
diff --git a/source/blender/editors/space_clip/clip_ops.c b/source/blender/editors/space_clip/clip_ops.c
index cd4a1ffb526..cb84ea6571c 100644
--- a/source/blender/editors/space_clip/clip_ops.c
+++ b/source/blender/editors/space_clip/clip_ops.c
@@ -1840,8 +1840,16 @@ void CLIP_OT_cursor_set(wmOperatorType *ot)
 static int lock_selection_togglee_exec(bContext *C, wmOperator *UNUSED(op))
 {
   SpaceClip *space_clip = CTX_wm_space_clip(C);
+
+  ClipViewLockState lock_state;
+  ED_clip_view_lock_state_store(C, &lock_state);
+
   space_clip->flag ^= SC_LOCK_SELECTION;
+
+  ED_clip_view_lock_state_restore_no_jump(C, &lock_state);
+
   WM_event_add_notifier(C, NC_SPACE | ND_SPACE_CLIP, NULL);
+
   return OPERATOR_FINISHED;
 }
 
diff --git a/source/blender/editors/space_clip/clip_utils.c b/source/blender/editors/space_clip/clip_utils.c
index c7f2a027ba8..faa3f18e8c1 100644
--- a/source/blender/editors/space_clip/clip_utils.c
+++ b/source/blender/editors/space_clip/clip_utils.c
@@ -27,10 +27,12 @@
 
 #include "BLI_listbase.h"
 #include "BLI_math.h"
+#include "BLI_rect.h"
 #include "BLI_utildefines.h"
 
 #include "BKE_animsys.h"
 #include "BKE_context.h"
+#include "BKE_mask.h"
 #include "BKE_movieclip.h"
 #include "BKE_tracking.h"
 
@@ -44,6 +46,7 @@
 #include "WM_types.h"
 
 #include "ED_clip.h"
+#include "ED_mask.h"
 #include "ED_screen.h"
 
 #include "UI_interface.h"
@@ -395,16 +398,152 @@ void clip_delete_plane_track(bContext *C, MovieClip *clip, MovieTrackingPlaneTra
   DEG_id_tag_update(&clip->id, 0);
 }
 
-void clip_view_center_to_point(SpaceClip *sc, float x, float y)
+/* Calculate space clip offset to be centered at the given point.  */
+void clip_view_offset_for_center_to_point(
+    SpaceClip *sc, const float x, const float y, float *r_offset_x, float *r_offset_y)
 {
   int width, height;
+  ED_space_clip_get_size(sc, &width, &height);
+
   float aspx, aspy;
+  ED_space_clip_get_aspect(sc, &aspx, &aspy);
+
+  *r_offset_x = (x - 0.5f) * width * aspx;
+  *r_offset_y = (y - 0.5f) * height * aspy;
+}
+
+void clip_view_center_to_point(SpaceClip *sc, float x, float y)
+{
+  clip_view_offset_for_center_to_point(sc, x, y, &sc->xof, &sc->yof);
+}
+
+static bool selected_tracking_boundbox(SpaceClip *sc, float min[2], float max[2])
+{
+  MovieClip *clip = ED_space_clip_get_clip(sc);
+  MovieTrackingTrack *track;
+  int width, height;
+  bool ok = false;
+  ListBase *tracksbase = BKE_tracking_get_active_tracks(&clip->tracking);
+  int framenr = ED_space_clip_get_clip_frame_number(sc);
+
+  INIT_MINMAX2(min, max);
 
   ED_space_clip_get_size(sc, &width, &height);
-  ED_space_clip_get_aspect(sc, &aspx, &aspy);
 
-  sc->xof = (x - 0.5f) * width * aspx;
-  sc->yof = (y - 0.5f) * height * aspy;
+  track = tracksbase->first;
+  while (track) {
+    if (TRACK_VIEW_SELECTED(sc, track)) {
+      MovieTrackingMarker *marker = BKE_tracking_marker_get(track, framenr);
+
+      if (marker) {
+        float pos[3];
+
+        pos[0] = marker->pos[0] + track->offset[0];
+        pos[1] = marker->pos[1] + track->offset[1];
+        pos[2] = 0.0f;
+
+        /* undistortion happens for normalized coords */
+        if (sc->user.render_flag & MCLIP_PROXY_RENDER_UNDISTORT) {
+          /* undistortion happens for normalized coords */
+          ED_clip_point_undistorted_pos(sc, pos, pos);
+        }
+
+        pos[0] *= width;
+        pos[1] *= height;
+
+        mul_v3_m4v3(pos, sc->stabmat, pos);
+
+        minmax_v2v2_v2(min, max, pos);
+
+        ok = true;
+      }
+    }
+
+    track = track->next;
+  }
+
+  return ok;
+}
+
+static bool selected_boundbox(const bContext *C, float min[2], float max[2], bool include_handles)
+{
+  SpaceClip *sc = CTX_wm_space_clip(C);
+  if (sc->mode == SC_MODE_TRACKING) {
+    return selected_tracking_boundbox(sc, min, max);
+  }
+
+  if (ED_mask_selected_minmax(C, min, max, include_handles)) {
+    MovieClip *clip = ED_space_clip_get_clip(sc);
+    int width, height;
+    ED_space_clip_get_size(sc, &width, &height);
+    BKE_mask_coord_to_movieclip(clip, &sc->user, min, min);
+    BKE_mask_coord_to_movieclip(clip, &sc->user, max, max);
+    min[0] *= width;
+    min[1] *= height;
+    max[0] *= width;
+    max[1] *= height;
+    return true;
+  }
+  return false;
+}
+
+bool clip_view_calculate_view_selection(
+    const bContext *C, bool fit, float *r_offset_x, float *r_offset_y, float *r_zoom)
+{
+  SpaceClip *sc = CTX_wm_space_clip(C);
+
+  int frame_width, frame_height;
+  ED_space_clip_get_size(sc, &frame_width, &frame_height);
+
+  if ((frame_width == 0) || (frame_height == 0) || (sc->clip == NULL)) {
+    return false;
+  }
+
+  /* NOTE: The `fit` argment is set to truth when doing "View to Selected" operator, and it set to
+   * false when this function is used for Lock-to-Selection functionality. When locking to
+   * selection the handles are to be ignored. So we can deriver the `include_handles` from `fit`.
+   *
+   * TODO(sergey): Make such decision more explicit. Maybe pass usecase for the calculation to tell
+   * operator from lock-to-selection apart. */
+  float min[2], max[2];
+  if (!selected_boundbox(C, min, max, fit)) {
+    return false;
+  }
+
+  /* center view */
+  clip_view_offset_for_center_to_point(sc,
+                                       (max[0] + min[0]) / (2 * frame_width),
+                                       (max[1] + min[1]) / (2 * frame_height),
+                                       r_offset_x,
+                                       r_offset_y);
+
+  const int w = max[0] - min[0];
+  const int h = max[1] - min[1];
+
+  /* set zoom to see all selection */
+  *r_zoom = sc->zoom;
+  if (w > 0 && h > 0) {
+    ARegion *region = CTX_wm_region(C);
+
+    int width, height;
+    float zoomx, zoomy, newzoom, aspx, aspy;
+
+    ED_space_clip_get_aspect(sc, &aspx, &aspy);
+
+    width = BLI_rcti_size_x(&region->winrct) + 1;
+    height = BLI_rcti_size_y(&region->winrct) + 1;
+
+    zoomx = (float)width / w / aspx;
+    zoomy = (float)height / h / aspy;
+
+    newzoom = 1.0f / power_of_2(1.0f / min_ff(zoomx, zoomy));
+
+    if (fit) {
+      *r_zoom = newzoom;
+    }
+  }
+
+  return true;
 }
 
 void clip_draw_sfra_efra(View2D *v2d, Scene *scene)
diff --git a/source/blender/editors/space_clip/tracking_ops.c b/source/blender/editors/space_clip/tracking_ops.c
index 10574063ebd..0f4fc2a2160 100644
--- a/source/blender/editors/space_clip/tracking_ops.c
+++ b/source/blender/editors/space_clip/tracking_ops.c
@@ -88,17 +88,16 @@ static int add_marker_exec(bContext *C, wmOperator *op)
   MovieClip *clip = ED_space_clip_get_clip(sc);
   float pos[2];
 
+  ClipViewLockState lock_state;
+  ED_clip_view_lock_state_store(C, &lock_state);
+
   RNA_float_get_array(op->ptr, "location", pos);
 
   if (!add_marker(C, pos[0], pos[1])) {
     return OPERATOR_CANCELLED;
   }
 
-  /* Reset offset from locked position, so frame jumping wouldn't be so
-   * confusing.
-   */
-  sc->xlockof = 0;
-  sc->ylockof = 0;
+  ED_clip_view_lock_state_restore_no_jump(C, &lock_state);
 
   WM_event_add_notifier(C, NC_MOVIECLIP | NA_EDITED, clip);
 
diff --git a/source/blender/editors/space_clip/tracking_select.c b/source/blender/editors/space_clip/tracking_select.c
index 063ea9592aa..ecd73f82e22 100644
--- a/source/blender/editors/space_clip/tracking_select.c
+++ b/source/blender/editors/space_clip/tracking_select.c
@@ -304,6 +304,9 @@ static int mouse_select(bContext *C, const float co[2], const bool extend, const
   track = find_nearest_track(sc, tracksbase, co, &distance_to_track);
   plane_track = find_nearest_plane_track(sc, plane_tracks_base, co, &distance_to_plane_track);
 
+  ClipViewLockState lock_state;
+  ED_clip_view_lock_state_store(C, &lock_state);
+
   /* Do not select beyond some reasonable distance, that is useless and
    * prevents the 'deselect on nothing' behavior. */
   if (distance_to_track > 0.05f) {
@@ -377,10 +380,7 @@ static int mouse_select(bContext *C, const float co[2], const bool extend, const
     ED_mask_deselect_all(C);
   }
 
-  if (!extend) {
-    sc->xlockof = 0.0f;
-    sc->ylockof = 0.0f;
-  }
+  ED_clip_view_lock_state_restore_no_jump(C, &lock_state);
 
   BKE_tracking_dopesheet_tag_update(tracking);
 
@@ -867,15 +867,20 @@ static int select_all_exec(bContext *C, wmOperator *op)
   MovieClip *clip = ED_space_clip_get_clip(sc);
   MovieTracking *tracking = &clip->tracking;
 
-  int action = RNA_enum_get(op->ptr, "action");
+  const int action = RNA_enum_get(op->ptr, "action");
 
-  bool has_selection = false;
+  ClipViewLockState lock_state;
+  ED_clip_view_lock_state_store(C, &lock_state);
 
+  bool has_selection = false;
   ED_clip_select_all(sc, action, &has_selection);
 
   if (!has_selection) {
     sc->flag &= ~SC_LOCK_SELECTION;
   }
+  else {
+    ED_clip_view_lock_state_restore_no_jump(C, &lock_state);
+  }
 
   BKE_tracking_dopesheet_tag_update(tracking);