From 053b7420fe6348c7199b3036fd228d3c8850a6a9 Mon Sep 17 00:00:00 2001 From: Guillem Hermida Date: Wed, 14 Oct 2026 23:09:11 +0200 Subject: [PATCH] LR-CROP-STRAIGHTEN: turning a crop shrinks it about its centre instead of swapping it for the largest crop (#1794) CropGeometry::with_angle replaced a crop that no longer fit at the new angle with the largest centred crop of its aspect. For a crop dragged in from a corner (which still touches two edges) that happened at the first fraction of a degree, so the crop jumped back to nearly the whole photo. A crop that no longer fits now shrinks about its own centre, keeping its aspect, to the largest size that fits there. The full photo, or a crop an earlier turn shrank to the largest of its aspect, still follows the largest crop and grows back towards 0 degrees (#1388). A crop whose centre is off the turned photo is centred at its own size at most. No crop the user made ever grows. --- crates/engine/src/cmd/develop.rs | 4 +- crates/engine/src/tests_crop_angle.rs | 27 ++++++++ crates/geom/src/crop.rs | 95 ++++++++++++++++++++++++--- docs/parity-checklist.md | 2 +- 4 files changed, 117 insertions(+), 11 deletions(-) diff --git a/crates/engine/src/cmd/develop.rs b/crates/engine/src/cmd/develop.rs index 6f064b260..1f3aa59ba 100644 --- a/crates/engine/src/cmd/develop.rs +++ b/crates/engine/src/cmd/develop.rs @@ -588,14 +588,14 @@ pub fn specs() -> Vec { "Straighten", [], None, - "{angle: degrees -45..45} — keeps the largest crop of the current aspect", + "{angle: degrees -45..45} — keeps the crop if it still fits, else shrinks it about its centre (the largest crop of its aspect stays the largest)", has_active, |s, p| { let id = active(s, "crop.straighten")?; let (w, h) = crop_dims(s, id); let angle = f64_req(p, "angle", "crop.straighten")?.clamp(-45.0, 45.0); edit(s, "crop.straighten", "Straighten", |d| { - // keep the current crop if it still fits, else the largest of its aspect that does + // keep the current crop if it still fits, else shrink it about its centre until it does d.crop.geometry = d.crop.geometry.with_angle(angle, w, h); Ok(()) }) diff --git a/crates/engine/src/tests_crop_angle.rs b/crates/engine/src/tests_crop_angle.rs index 90a12b2d0..bac941239 100644 --- a/crates/engine/src/tests_crop_angle.rs +++ b/crates/engine/src/tests_crop_angle.rs @@ -107,6 +107,33 @@ fn a_straighten_slider_drag_fits_live_and_is_one_undo_step() { assert!(close(geometry(&s), start)); } +#[test] +fn turning_a_crop_dragged_in_from_a_corner_keeps_that_crop() { + // issue #1794: lock the aspect, drag a corner in, then rotate: the crop went back to the photo's + // size, because the dragged crop still touches two edges and stops fitting at once + let mut s = Session::with_demo(); + let (w, h) = dims(&s); + s.execute("crop.aspect", &json!({"aspect": "original"})).unwrap(); + s.execute("crop.drag", &json!({"handle": "bottomRight", "from": [1.0, 1.0], "to": [0.6, 0.6]})).unwrap(); + let made = geometry(&s).rect_px(w, h); + assert!(made.width() < 0.7 * w, "the drag cropped: {made:?}"); + s.execute("develop.beginInteraction", &json!({"label": "Crop"})).unwrap(); + for angle in [0.25, 1.0, 2.5, 5.0, -3.0] { + s.execute("crop.straighten", &json!({"angle": angle})).unwrap(); + let g = geometry(&s); + let r = g.rect_px(w, h); + assert!(g.is_within_image(w, h), "{angle}°: inside the photo"); + assert!(r.width() <= made.width() + 1e-6, "{angle}°: no larger than the crop made, got {r:?} from {made:?}"); + assert!((r.aspect() - made.aspect()).abs() < 1e-6, "{angle}°: the locked aspect holds"); + } + s.execute("develop.endInteraction", &json!({})).unwrap(); + // the first small turn only trims it + s.execute("edit.undo", &json!({})).unwrap(); + s.execute("crop.straighten", &json!({"angle": 0.25})).unwrap(); + let r = geometry(&s).rect_px(w, h); + assert!(r.width() > 0.97 * made.width(), "0.25° keeps nearly all of it: {r:?} from {made:?}"); +} + #[test] fn a_crop_shrunk_by_straightening_grows_back_towards_0() { // issue #1388: slide the Straighten slider out and back, in one drag or in several diff --git a/crates/geom/src/crop.rs b/crates/geom/src/crop.rs index 857ea19b9..df145322a 100644 --- a/crates/geom/src/crop.rs +++ b/crates/geom/src/crop.rs @@ -7,7 +7,7 @@ use serde::{Deserialize, Serialize}; -use crate::{Affine, Point, Rect, Vec2}; +use crate::{Affine, MIN_CROP_FRACTION, Point, Rect, Vec2}; #[derive(Clone, Copy, Debug, PartialEq, Serialize, Deserialize)] pub struct CropGeometry { @@ -81,12 +81,14 @@ impl CropGeometry { } /// This crop turned to `angle_deg` on a `w × h` image: kept as it is when it still fits inside - /// the rotated image, else the largest crop of its aspect that fits, centred (Lightroom's - /// "Constrain to image"). Every way of setting the angle goes through here (the rotate handle, - /// the Straighten slider and Angle field, `crop.straighten`, `develop.set` on `crop.angle`), so - /// they all give the same crop (issue #742). A crop that is the largest of its aspect at its - /// own angle (the full photo, or one an earlier turn shrank to fit) stays the largest at the new - /// angle, so it grows back as the angle returns towards 0° (issue #1388). + /// the rotated image, else shrunk about its own centre, keeping its aspect, until it fits + /// (Lightroom's "Constrain to image"). Every way of setting the angle goes through here (the + /// rotate handle, the Straighten slider and Angle field, `crop.straighten`, `develop.set` on + /// `crop.angle`), so they all give the same crop (issue #742). A crop that is the largest of its + /// aspect at its own angle (the full photo, or one an earlier turn shrank to fit) stays the + /// largest at the new angle, so it grows back as the angle returns towards 0° (issue #1388). + /// Any other crop never grows: a crop dragged in from a corner still touches two edges of the + /// photo, and turning it used to swap it for the largest crop of its aspect (issue #1794). pub fn with_angle(&self, angle_deg: f64, w: f64, h: f64) -> CropGeometry { let r = self.rect_px(w, h); // a degenerate rect has no aspect to keep: fall back to the image's own @@ -99,7 +101,45 @@ impl CropGeometry { if !is_largest && cand.is_within_image(w, h) { return cand; } - crop_fit_angle(w, h, angle_deg, aspect) + let fitted = crop_fit_angle(w, h, angle_deg, aspect); + if is_largest || aspect.is_none() { + return fitted; + } + // shrunk in place, as long as that leaves a crop the drag model would allow + let min_side = MIN_CROP_FRACTION * w.min(h); + if let Some(s) = cand.fit_scale_about_centre(w, h).filter(|s| r.width().min(r.height()) * s >= min_side) { + return CropGeometry { rect: Rect::from_center(r.center(), r.width() * s, r.height() * s).scale(1.0 / w, 1.0 / h), angle: angle_deg }; + } + // its centre is off the turned photo (a sliver in a corner): centred, at its own size at most + let f = fitted.rect_px(w, h); + let Some(s) = Some(r.width() / f.width()).filter(|s| s.is_finite() && *s < 1.0) else { + return fitted; + }; + CropGeometry { rect: Rect::from_center(f.center(), f.width() * s, f.height() * s).scale(1.0 / w, 1.0 / h), angle: angle_deg } + } + + /// The largest scale, at most 1, at which this crop shrunk about its own centre lies inside the + /// rotated image; `None` when its centre is outside it. + fn fit_scale_about_centre(&self, w: f64, h: f64) -> Option { + let r = self.rect_px(w, h); + let back = Affine::rotate_about(-self.angle.to_radians(), Point::new(w / 2.0, h / 2.0)); + let c = back.apply(r.center()); + if !(0.0..=w).contains(&c.x) || !(0.0..=h).contains(&c.y) { + return None; + } + // each corner, in the image frame, is c + s·d: keep both coordinates in 0..w and 0..h + let mut s: f64 = 1.0; + for corner in r.corners() { + let d = back.apply(corner) - c; + for (q, dq, max) in [(c.x, d.x, w), (c.y, d.y, h)] { + if dq > 0.0 { + s = s.min((max - q) / dq); + } else if dq < 0.0 { + s = s.min(q / -dq); + } + } + } + Some(s).filter(|s| s.is_finite() && *s > 0.0) } } @@ -195,6 +235,45 @@ mod tests { } } + #[test] + fn with_angle_shrinks_a_crop_against_the_edge_about_its_own_centre() { + // issue #1794: a crop dragged in from one corner keeps the opposite corner on the photo's, + // so it stops fitting at the first fraction of a degree; it used to become the largest crop + // of its aspect, nearly the whole photo + let (w, h) = (600.0, 400.0); + let c = CropGeometry { rect: Rect::new(0.0, 0.0, 0.6, 0.6), angle: 0.0 }; + let a = c.rect_px(w, h); + for ang in [-10.0, -1.0, 0.5, 3.0, 20.0] { + let t = c.with_angle(ang, w, h); + let b = t.rect_px(w, h); + assert!(t.is_within_image(w, h), "{ang}°"); + assert_eq!(t.angle, ang); + assert!(b.width() <= a.width() + 1e-9, "{ang}°: never larger than the crop made, {} vs {}", b.width(), a.width()); + assert!((a.aspect() - b.aspect()).abs() < 1e-9, "{ang}°: aspect kept"); + assert!(b.center().dist(a.center()) < 1e-9, "{ang}°: shrunk about its own centre"); + // as large as fits there: 1 % bigger about the same centre does not + let big = CropGeometry { rect: Rect::from_center(t.rect.center(), t.rect.width() * 1.01, t.rect.height() * 1.01), angle: ang }; + assert!(!big.is_within_image(w, h), "{ang}°: the largest about its centre"); + } + // a small turn is a small change, not a jump + let t = c.with_angle(0.5, w, h).rect_px(w, h); + assert!(t.width() > 0.97 * a.width(), "0.5° keeps nearly all of the crop: {} of {}", t.width(), a.width()); + } + + #[test] + fn with_angle_never_grows_a_crop_whose_centre_leaves_the_photo() { + // a sliver in the corner: at 45° its centre is outside the turned photo, so it can't shrink + // in place; it moves to the centre at its own size, not at the largest of its aspect + let (w, h) = (600.0, 400.0); + let c = CropGeometry { rect: Rect::new(0.0, 0.0, 0.05, 0.05), angle: 0.0 }; + let a = c.rect_px(w, h); + let t = c.with_angle(45.0, w, h); + let b = t.rect_px(w, h); + assert!(t.is_within_image(w, h)); + assert!(b.width() <= a.width() + 1e-9 && (a.aspect() - b.aspect()).abs() < 1e-9, "{b:?} from {a:?}"); + assert!(b.center().dist(Point::new(w / 2.0, h / 2.0)) < 1e-9, "centred: {b:?}"); + } + #[test] fn with_angle_survives_a_degenerate_crop() { // zero height: no aspect to keep (width / height = ∞) diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 7bb187811..1c9475cb4 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -247,7 +247,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts). |---|---|---|---|---|---| | LR-CROP-RECT | Crop rectangle | P0 | ✅ | `cmd:crop.set`, `cmd:crop.drag`, `crates/geom/src/crop_drag.rs`, `crates/ui-egui/src/panels/detail.rs` | double-click inside the box applies the crop (like Return) | | LR-CROP-ASPECT | Aspect ratios | P0 | ✅ | `cmd:crop.aspect`, `cmd:crop.rotateAspect`, `crates/ui-egui/src/panels/right.rs` | menu with presets + custom W × H, visible Lock toggle (also A), button names the active ratio; every handle honours the lock, holding Shift while dragging a handle keeps the frame's proportions for that drag (issue #614), and the drag stops at the image edge instead of shifting the frame (issue #295); "Original" is the photo's own shape (no separate As Shot); the last chosen aspect is saved as a crop tool default, resolving Original per photo and preserving existing crops; opening Crop or browsing never edits a photo: the default applies only when a crop gesture starts, within its single undo step | -| LR-CROP-STRAIGHTEN | Straighten tool | P0 | ✅ | `cmd:crop.straighten`, `cmd:crop.autoStraighten`, `crates/ui-egui/src/panels/detail.rs` (`rotate_cursor`, `angle_readout`), `crates/ui-egui/src/panels/right.rs` (Angle field), `crates/ui-egui/src/tests_crop_rotate.rs` | Straighten Tool button: drag along a horizon/vertical; double-click or Auto levels automatically. Dragging outside the crop box rotates it: there the pointer is a curved double arrow that bulges away from the side of the box it is on, diagonally off a corner and following a rotated box (`rotate_cursor_turn`, tested by `rotate_cursor_faces_away_from_the_crop_box`) (with ⌘ held, the straighten crosshair), and while rotating the angle shows next to it, kept on the canvas (issue #534); an exact angle goes in the Angle field under Straighten (`cropAngleField`: applies on Return or leaving it, Esc keeps the old angle, takes a decimal comma; a drag on it is one undo step) or by clicking the Straighten value (its hover says so). Every way of setting the angle (rotate handle, Straighten slider, Angle field, `crop.straighten`, `crop.set {angle}`, and the generic `develop.set` / `develop.adjust` / `develop.quickAdjust` / `develop.resetControl` on `crop.angle`, as MCP, the control channel and `lightcraft-cli render --set` use them) keeps the crop if it still fits, else shrinks it to the largest crop of its aspect inside the photo (`CropGeometry::with_angle`, `crates/engine/src/tests_crop_angle.rs`; before issue #742 the generic path left the box, and the export, past the photo with smeared edge pixels in the corners). The dimming outside the box is the photo minus the (rotated) box, drawn as one mesh (`outside_crop_mesh`), so rotating never dims wedges inside the box or doubles them outside (issue #731) | +| LR-CROP-STRAIGHTEN | Straighten tool | P0 | ✅ | `cmd:crop.straighten`, `cmd:crop.autoStraighten`, `crates/ui-egui/src/panels/detail.rs` (`rotate_cursor`, `angle_readout`), `crates/ui-egui/src/panels/right.rs` (Angle field), `crates/ui-egui/src/tests_crop_rotate.rs` | Straighten Tool button: drag along a horizon/vertical; double-click or Auto levels automatically. Dragging outside the crop box rotates it: there the pointer is a curved double arrow that bulges away from the side of the box it is on, diagonally off a corner and following a rotated box (`rotate_cursor_turn`, tested by `rotate_cursor_faces_away_from_the_crop_box`) (with ⌘ held, the straighten crosshair), and while rotating the angle shows next to it, kept on the canvas (issue #534); an exact angle goes in the Angle field under Straighten (`cropAngleField`: applies on Return or leaving it, Esc keeps the old angle, takes a decimal comma; a drag on it is one undo step) or by clicking the Straighten value (its hover says so). Every way of setting the angle (rotate handle, Straighten slider, Angle field, `crop.straighten`, `crop.set {angle}`, and the generic `develop.set` / `develop.adjust` / `develop.quickAdjust` / `develop.resetControl` on `crop.angle`, as MCP, the control channel and `lightcraft-cli render --set` use them) keeps the crop if it still fits, else shrinks it about its own centre, keeping its aspect, until it fits; the full photo (or a crop an earlier turn shrank to the largest of its aspect) stays the largest, so it grows back towards 0° (issue #1388), and no other crop ever grows (issue #1794: a crop dragged in from a corner turned into nearly the whole photo) (`CropGeometry::with_angle`, `crates/engine/src/tests_crop_angle.rs`; before issue #742 the generic path left the box, and the export, past the photo with smeared edge pixels in the corners). The dimming outside the box is the photo minus the (rotated) box, drawn as one mesh (`outside_crop_mesh`), so rotating never dims wedges inside the box or doubles them outside (issue #731) | | LR-CROP-AUTO | Auto straighten | P1 | ✅ | `cmd:crop.autoStraighten` | crop-angle leveling from detected horizon/plumb lines (consensus required) | | LR-CROP-ANGLE | Angle slider | P0 | ✅ | `ctl:crop.angle`, `crates/ui-egui/src/panels/right.rs` | the Straighten slider; an exact angle can also be typed in the Angle field under it (see LR-CROP-STRAIGHTEN); setting `crop.angle` through `develop.set` fits the crop inside the photo as Straighten does (issue #742) | | LR-CROP-ROTATE90 | Rotate 90° | P0 | ✅ | `cmd:photo.rotateLeft`, `cmd:photo.rotateRight` | |