From 54c6c43b07eb6944fcd19d2741a2ad4b30652d11 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 23:30:07 +0000 Subject: [PATCH 1/3] #1927: Constrain Crop tightens the stored crop Constrain Crop only applied at render time, and the Crop tool shows the uncropped view with the stored crop, so in the Crop panel (where the checkbox is) ticking it or moving a Geometry slider changed nothing. Session::set_develop now shrinks the stored crop about its centre until it fits the warped photo when Constrain Crop is turned on, or when an edit changes the warp (Geometry, Upright, lens corrections, section eyes) while it is on. A crop edit itself is kept as drawn. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AbegtYaPuwqZ81pGfELsFK --- crates/engine/src/lib.rs | 35 ++++++++++- crates/engine/src/tests_constrain_crop.rs | 72 +++++++++++++++++++++++ docs/parity-checklist.md | 2 +- docs/whats-new.md | 9 +++ 4 files changed, 116 insertions(+), 2 deletions(-) create mode 100644 crates/engine/src/tests_constrain_crop.rs diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 8a282ce3e..f472d7b60 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -874,7 +874,7 @@ impl Session { /// undo step; otherwise it is committed with a history entry. pub fn set_develop(&mut self, id: PhotoId, settings: DevelopSettings, label: &str) -> Result<()> { let now = (self.clock)(); - let settings = Arc::new(settings); + let settings = Arc::new(self.fit_constrained_crop(id, settings)); if let Some(i) = &self.interaction && i.photo == id { @@ -886,6 +886,37 @@ impl Session { self.commit(label, op) } + /// "Constrain Crop" keeps the stored crop inside the warped photo, as Lightroom does (issue + /// #1927): turning it on, or an edit that changes the warp (Geometry, Upright, lens corrections, + /// their sections' eyes) while it is on, shrinks the crop about its centre until no blank corner + /// is left, so the Crop tool shows what the render keeps. An edit of the crop itself is left as + /// drawn (the render still constrains it). + fn fit_constrained_crop(&self, id: PhotoId, mut d: DevelopSettings) -> DevelopSettings { + if !d.geometry.constrain_crop { + return d; + } + let Some(photo) = self.catalog.photo(id) else { return d }; + let before = &photo.develop; + let turned_on = !before.geometry.constrain_crop; + let warp_changed = before.geometry != d.geometry + || before.optics != d.optics + || before.orientation != d.orientation + || before.disabled_sections != d.disabled_sections; + let crop_edited = before.crop.geometry != d.crop.geometry; + if !(turned_on || (warp_changed && !crop_edited)) { + return d; + } + let frame = lightcraft_pipeline::geometry::Frame::with_lens( + photo.width.max(1) as usize, + photo.height.max(1) as usize, + &d, + true, + photo.embedded_lens.as_ref(), + ); + d.crop.geometry.rect = frame.crop.rect; + d + } + /// With Auto Sync on, the ops that carry an edit of the active photo `id` (to `new`) over to the /// other selected photos: only the settings that changed; never spot removal or red eye (they /// belong to one photo's pixels), nor the rendering process (Sync doesn't carry it either), nor @@ -1242,6 +1273,8 @@ mod tests_albums_local; #[cfg(test)] mod tests_color; #[cfg(test)] +mod tests_constrain_crop; +#[cfg(test)] mod tests_crop_angle; #[cfg(test)] mod tests_denoise; diff --git a/crates/engine/src/tests_constrain_crop.rs b/crates/engine/src/tests_constrain_crop.rs new file mode 100644 index 000000000..37c6d734d --- /dev/null +++ b/crates/engine/src/tests_constrain_crop.rs @@ -0,0 +1,72 @@ +//! Constrain Crop changes the stored crop, as in Lightroom (issue #1927): with it on, the crop the +//! Crop tool shows (the stored one) never takes in the blank corners a Geometry warp leaves, so +//! ticking it, or moving a Geometry slider while it is ticked, visibly tightens the crop. + +use lightcraft_geom::{CropGeometry, Rect}; +use lightcraft_pipeline::geometry::Frame; +use serde_json::json; + +use crate::Session; + +fn crop(s: &Session) -> CropGeometry { + s.develop_of(s.active().unwrap()).unwrap().crop.geometry +} + +/// Does the stored crop already fit the warped photo (the render's constraint would leave it alone)? +fn fits(s: &Session) -> bool { + let p = s.catalog.photo(s.active().unwrap()).unwrap(); + let mut d = (*p.develop).clone(); + d.geometry.constrain_crop = true; + let f = Frame::with_lens(p.width as usize, p.height as usize, &d, true, p.embedded_lens.as_ref()); + let (a, b) = (f.crop.rect, p.develop.crop.geometry.rect); + [a.x0 - b.x0, a.y0 - b.y0, a.x1 - b.x1, a.y1 - b.y1].iter().all(|d| d.abs() < 1e-9) +} + +fn smaller(a: Rect, b: Rect) -> bool { + a.width() < b.width() - 1e-3 && a.height() < b.height() - 1e-3 +} + +fn constrain(s: &mut Session, on: bool) { + s.execute("develop.merge", &json!({"settings": {"geometry": {"constrain_crop": on}}, "label": "Constrain Crop"})).unwrap(); +} + +fn vertical(s: &mut Session, v: f64) { + s.execute("develop.set", &json!({"control": "geometry.vertical", "value": v})).unwrap(); +} + +#[test] +fn a_geometry_slider_tightens_the_crop_while_constrained() { + let mut s = Session::with_demo(); + let full = crop(&s).rect; + constrain(&mut s, true); + assert_eq!(crop(&s).rect, full, "no warp yet: nothing to cut off"); + vertical(&mut s, -60.0); + assert!(smaller(crop(&s).rect, full), "the stored crop shrank: {:?}", crop(&s).rect); + assert!(fits(&s), "and holds no blank corner"); + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(crop(&s).rect, full, "undo puts the crop back with the slider"); +} + +#[test] +fn ticking_constrain_after_the_warp_tightens_the_crop() { + let mut s = Session::with_demo(); + let full = crop(&s).rect; + vertical(&mut s, -60.0); + assert_eq!(crop(&s).rect, full, "unconstrained: the crop keeps the blank corners"); + assert!(!fits(&s)); + constrain(&mut s, true); + assert!(smaller(crop(&s).rect, full) && fits(&s), "{:?}", crop(&s).rect); + // unticking keeps the tighter crop, as in Lightroom + let tight = crop(&s).rect; + constrain(&mut s, false); + assert_eq!(crop(&s).rect, tight); +} + +#[test] +fn a_crop_drawn_by_hand_is_kept_as_drawn() { + let mut s = Session::with_demo(); + constrain(&mut s, true); + vertical(&mut s, -60.0); + s.execute("crop.set", &json!({"rect": [0.0, 0.0, 1.0, 1.0]})).unwrap(); + assert_eq!(crop(&s).rect, Rect::new(0.0, 0.0, 1.0, 1.0), "the crop edit itself is not refitted"); +} diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 95dcd2aef..3504eb77d 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -220,7 +220,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts). | LR-EDIT-OPTICS-MANUAL | Manual distortion / vignetting | P1 | ✅ | `ctl:optics.distortion`, `ctl:optics.vignetting`, `ctl:optics.vignettingMidpoint` | | | LR-EDIT-GEOM-UPRIGHT | Upright | P1 | ✅ | `cmd:geometry.upright`, `cmd:geometry.guides` | | | LR-EDIT-GEOM-MANUAL | Manual transform | P1 | ✅ | `ctl:geometry.*` | | -| LR-EDIT-GEOM-CONSTRAIN | Constrain crop | P1 | ✅ | `crates/ui-egui/src/panels/right.rs` (checkbox → `cmd:develop.merge`) | | +| LR-EDIT-GEOM-CONSTRAIN | Constrain crop | P1 | ✅ | `crates/ui-egui/src/panels/right.rs` (checkbox → `cmd:develop.merge`), `crates/engine/src/lib.rs` (`fit_constrained_crop`) | Ticking it, or changing Geometry, Upright or lens corrections while it is ticked, shrinks the stored crop about its centre until no blank corner is left, so the Crop tool shows it (it used to apply at render time only, outside the Crop tool: issue #1927; tests in `crates/engine/src/tests_constrain_crop.rs`). A crop drawn by hand is kept as drawn; unticking keeps the tighter crop | | LR-EDIT-GEOM-GRID | Grid while transforming | P2 | ✅ | `crates/ui-egui/src/panels/detail.rs` | a fine grid over the photo while a geometry slider is dragged | | LR-EDIT-LENSBLUR | Lens blur | P2 | ⬜ | | settings field reserved, not rendered | | LR-EDIT-CALIB | Calibration [Classic] | P1 | ✅ | `ctl:calibration.*` | shadows tint, red/green/blue primary hue and saturation; read/written in XMP | diff --git a/docs/whats-new.md b/docs/whats-new.md index 97e1367a8..0eabf447b 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -1,5 +1,14 @@ # What's new in LightCraft +## 2026-10-15 + +### Constrain Crop tightens the crop you see +- Geometry ▸ Constrain Crop now changes the crop itself, as in Lightroom (issue #1927). Ticking it, or moving a + Geometry slider, Upright or a lens correction while it is ticked, shrinks the crop about its centre until the blank + corners left by the correction are gone, and the Crop tool shows the tighter crop. Before, it only applied when the + photo was shown outside the Crop tool, so in the Crop panel, where the checkbox is, nothing seemed to change. A crop + you draw by hand is kept as drawn, and unticking keeps the tighter crop. + ## 2026-10-14 ### Lightroom radial masks on the right side From 23f00822eb87b047ff9d2adc2dfb1339d945d158 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 18 Oct 2026 12:37:56 +0000 Subject: [PATCH 2/3] #1927: Auto Sync fits each photo's own crop under Constrain Crop The crop fit ran before the Auto Sync delta, so a Geometry edit carried the active photo's fitted crop rect into the delta and onto every other selected photo, replacing (and could enlarge) their own crops. The delta is now taken from the edit as asked, and each target fits its own crop. Review on #1990. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AbegtYaPuwqZ81pGfELsFK --- crates/engine/src/lib.rs | 17 ++++++++------ crates/engine/src/tests_constrain_crop.rs | 28 +++++++++++++++++++++++ 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index f472d7b60..e60cf5cf8 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -874,14 +874,16 @@ impl Session { /// undo step; otherwise it is committed with a history entry. pub fn set_develop(&mut self, id: PhotoId, settings: DevelopSettings, label: &str) -> Result<()> { let now = (self.clock)(); - let settings = Arc::new(self.fit_constrained_crop(id, settings)); + // Auto Sync carries the edit as asked; each photo then fits its own crop (review of #1990) + let requested = settings; + let settings = Arc::new(self.fit_constrained_crop(id, requested.clone())); if let Some(i) = &self.interaction && i.photo == id { return self.apply_silent(Op::SetDevelop { id, settings, label: label.into(), edited: Some(now) }); } let mut ops = vec![self.develop_op(id, (*settings).clone(), label).ok_or(lightcraft_catalog::CatalogError::NoPhoto(id))?]; - ops.extend(self.auto_sync_ops(id, &settings, label)); + ops.extend(self.auto_sync_ops(id, &requested, label)); let op = if ops.len() == 1 { ops.remove(0) } else { Op::Batch { ops } }; self.commit(label, op) } @@ -917,10 +919,11 @@ impl Session { d } - /// With Auto Sync on, the ops that carry an edit of the active photo `id` (to `new`) over to the - /// other selected photos: only the settings that changed; never spot removal or red eye (they - /// belong to one photo's pixels), nor the rendering process (Sync doesn't carry it either), nor - /// history / snapshot restores. + /// With Auto Sync on, the ops that carry an edit of the active photo `id` (to `new`, as asked, + /// before its crop is fitted) over to the other selected photos: only the settings that changed; + /// never spot removal or red eye (they belong to one photo's pixels), nor the rendering process + /// (Sync doesn't carry it either), nor history / snapshot restores. Constrain Crop then fits each + /// photo's own crop: the active photo's fitted crop is never copied over another's. fn auto_sync_ops(&self, id: PhotoId, new: &DevelopSettings, label: &str) -> Vec { if !self.auto_sync || self.active() != Some(id) @@ -946,7 +949,7 @@ impl Session { .filter(|x| **x != id) .filter_map(|x| self.develop_of(*x).map(|d| (*x, d))) .filter_map(|(x, d)| { - let synced = lightcraft_develop::apply_partial(&d, &delta, 1.0); + let synced = self.fit_constrained_crop(x, lightcraft_develop::apply_partial(&d, &delta, 1.0)); (synced != *d).then(|| self.develop_op(x, synced, label)).flatten() }) .collect() diff --git a/crates/engine/src/tests_constrain_crop.rs b/crates/engine/src/tests_constrain_crop.rs index 37c6d734d..51dcee1c0 100644 --- a/crates/engine/src/tests_constrain_crop.rs +++ b/crates/engine/src/tests_constrain_crop.rs @@ -70,3 +70,31 @@ fn a_crop_drawn_by_hand_is_kept_as_drawn() { s.execute("crop.set", &json!({"rect": [0.0, 0.0, 1.0, 1.0]})).unwrap(); assert_eq!(crop(&s).rect, Rect::new(0.0, 0.0, 1.0, 1.0), "the crop edit itself is not refitted"); } + +/// Review of #1990: with Auto Sync, a Geometry edit on the active photo fits each selected photo's +/// own crop; the active photo's fitted crop is never copied over another photo's (smaller) crop. +#[test] +fn auto_sync_fits_each_photos_own_crop() { + let mut s = Session::with_demo(); + let ids: Vec<_> = s.catalog.photos().take(2).map(|p| p.id).collect(); + let (a, b) = (ids[0], ids[1]); + // B: a small crop off-centre; both constrained + s.selection = crate::Selection { ids: vec![b], active: Some(b) }; + s.execute("crop.set", &json!({"rect": [0.05, 0.1, 0.45, 0.5]})).unwrap(); + constrain(&mut s, true); + s.selection = crate::Selection { ids: vec![a], active: Some(a) }; + constrain(&mut s, true); + let b_before = s.develop_of(b).unwrap().crop.geometry.rect; + // A active, both selected, Auto Sync on: a Geometry edit on A + s.selection = crate::Selection { ids: vec![a, b], active: Some(a) }; + s.execute("develop.autoSync", &json!({"on": true})).unwrap(); + vertical(&mut s, -60.0); + let (da, db) = (s.develop_of(a).unwrap(), s.develop_of(b).unwrap()); + assert_eq!(db.geometry.vertical, -60.0, "the edit reached B"); + let b_after = db.crop.geometry.rect; + assert_ne!(b_after, da.crop.geometry.rect, "B keeps a crop of its own"); + let inside = |r: Rect, o: Rect| r.x0 >= o.x0 - 1e-9 && r.y0 >= o.y0 - 1e-9 && r.x1 <= o.x1 + 1e-9 && r.y1 <= o.y1 + 1e-9; + assert!(inside(b_after, b_before), "B's crop only shrinks within itself: {b_before:?} -> {b_after:?}"); + s.selection = crate::Selection { ids: vec![b], active: Some(b) }; + assert!(fits(&s), "and fits B's warp"); +} From debfa644ec06a53c66f5c21219044f913d57ef61 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 18 Oct 2026 16:48:52 +0000 Subject: [PATCH 3/3] #1927: a slider drag and unrelated section eyes don't misplace crops Two more cases from the review of #1990: the end of a slider drag replayed the crop Constrain Crop had fitted during the drag, so Auto Sync still copied the active photo's fitted crop to the others; the end of an interaction now starts again from the original crop when the drag didn't edit the crop. And any section eye counted as a warp change, so turning Light off could refit a hand-drawn crop: only the Geometry and Optics eyes do now. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AbegtYaPuwqZ81pGfELsFK --- crates/engine/src/lib.rs | 22 ++++++++--- crates/engine/src/tests_constrain_crop.rs | 47 +++++++++++++++++++++++ 2 files changed, 64 insertions(+), 5 deletions(-) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index e60cf5cf8..da435b7d7 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -184,6 +184,10 @@ pub struct Interaction { pub label: String, pub photo: PhotoId, pub original: Arc, + /// Whether an edit during the interaction changed the crop itself. If none did, the crop the + /// photo shows was fitted by Constrain Crop, and the final commit starts again from the original + /// crop, so Auto Sync carries the edit and not the active photo's fitted crop. + pub crop_edited: bool, } /// The most photos an edit may touch for the view to follow it photo by photo; a larger edit is worked out whole. @@ -877,9 +881,11 @@ impl Session { // Auto Sync carries the edit as asked; each photo then fits its own crop (review of #1990) let requested = settings; let settings = Arc::new(self.fit_constrained_crop(id, requested.clone())); - if let Some(i) = &self.interaction + let crop_edited = self.develop_of(id).is_some_and(|d| d.crop.geometry != requested.crop.geometry); + if let Some(i) = &mut self.interaction && i.photo == id { + i.crop_edited |= crop_edited; return self.apply_silent(Op::SetDevelop { id, settings, label: label.into(), edited: Some(now) }); } let mut ops = vec![self.develop_op(id, (*settings).clone(), label).ok_or(lightcraft_catalog::CatalogError::NoPhoto(id))?]; @@ -903,7 +909,7 @@ impl Session { let warp_changed = before.geometry != d.geometry || before.optics != d.optics || before.orientation != d.orientation - || before.disabled_sections != d.disabled_sections; + || ["geometry", "optics"].iter().any(|s| before.section_enabled(s) != d.section_enabled(s)); let crop_edited = before.crop.geometry != d.crop.geometry; if !(turned_on || (warp_changed && !crop_edited)) { return d; @@ -971,7 +977,7 @@ impl Session { } let id = self.active().ok_or_else(|| EngineError::Other("no active photo".into()))?; let original = self.develop_of(id).unwrap_or_default(); - self.interaction = Some(Interaction { label: label.into(), photo: id, original }); + self.interaction = Some(Interaction { label: label.into(), photo: id, original, crop_edited: false }); Ok(()) } @@ -982,9 +988,15 @@ impl Session { if *cur == *i.original { return Ok(()); } - // Restore the original silently, then commit the final value as one step. + // Restore the original silently, then commit the final value as one step. A crop nobody + // edited goes back to the original's: Constrain Crop fits it again for the final warp, and + // Auto Sync carries the edit, not the active photo's fitted crop (review of #1990). self.apply_silent(Op::SetDevelop { id: i.photo, settings: i.original.clone(), label: i.label.clone(), edited: None })?; - self.set_develop(i.photo, (*cur).clone(), &i.label) + let mut last = (*cur).clone(); + if !i.crop_edited { + last.crop.geometry = i.original.crop.geometry; + } + self.set_develop(i.photo, last, &i.label) } pub fn cancel_interaction(&mut self) -> Result<()> { diff --git a/crates/engine/src/tests_constrain_crop.rs b/crates/engine/src/tests_constrain_crop.rs index 51dcee1c0..9f01ed05d 100644 --- a/crates/engine/src/tests_constrain_crop.rs +++ b/crates/engine/src/tests_constrain_crop.rs @@ -98,3 +98,50 @@ fn auto_sync_fits_each_photos_own_crop() { s.selection = crate::Selection { ids: vec![b], active: Some(b) }; assert!(fits(&s), "and fits B's warp"); } + +/// Review of #1990 (CodeRabbit): the same with a slider drag (an interaction), the path the +/// window uses: the drag fits A's crop live, and its end must not carry that crop over to B. +#[test] +fn auto_sync_after_a_slider_drag_fits_each_photos_own_crop() { + let mut s = Session::with_demo(); + let ids: Vec<_> = s.catalog.photos().take(2).map(|p| p.id).collect(); + let (a, b) = (ids[0], ids[1]); + s.selection = crate::Selection { ids: vec![b], active: Some(b) }; + s.execute("crop.set", &json!({"rect": [0.05, 0.1, 0.45, 0.5]})).unwrap(); + constrain(&mut s, true); + s.selection = crate::Selection { ids: vec![a], active: Some(a) }; + constrain(&mut s, true); + let b_before = s.develop_of(b).unwrap().crop.geometry.rect; + s.selection = crate::Selection { ids: vec![a, b], active: Some(a) }; + s.execute("develop.autoSync", &json!({"on": true})).unwrap(); + s.execute("develop.beginInteraction", &json!({"label": "Vertical"})).unwrap(); + for v in [-20.0, -40.0, -60.0] { + vertical(&mut s, v); + } + s.execute("develop.endInteraction", &json!({})).unwrap(); + let (da, db) = (s.develop_of(a).unwrap(), s.develop_of(b).unwrap()); + assert_eq!((da.geometry.vertical, db.geometry.vertical), (-60.0, -60.0)); + let b_after = db.crop.geometry.rect; + assert_ne!(b_after, da.crop.geometry.rect, "B keeps a crop of its own"); + let inside = |r: Rect, o: Rect| r.x0 >= o.x0 - 1e-9 && r.y0 >= o.y0 - 1e-9 && r.x1 <= o.x1 + 1e-9 && r.y1 <= o.y1 + 1e-9; + assert!(inside(b_after, b_before), "{b_before:?} -> {b_after:?}"); + for photo in [a, b] { + s.selection = crate::Selection { ids: vec![photo], active: Some(photo) }; + assert!(fits(&s), "each crop fits its photo's warp"); + } +} + +/// Review of #1990 (CodeRabbit): only the Geometry and Optics eyes change the warp; turning +/// another section's eye off leaves a crop drawn by hand as it is. +#[test] +fn an_unrelated_section_eye_leaves_a_hand_drawn_crop() { + let mut s = Session::with_demo(); + constrain(&mut s, true); + vertical(&mut s, -60.0); + s.execute("crop.set", &json!({"rect": [0.0, 0.0, 1.0, 1.0]})).unwrap(); + s.execute("develop.sectionEnabled", &json!({"section": "light", "enabled": false})).unwrap(); + assert_eq!(crop(&s).rect, Rect::new(0.0, 0.0, 1.0, 1.0), "the Light eye is no warp change"); + s.execute("develop.sectionEnabled", &json!({"section": "geometry", "enabled": false})).unwrap(); + s.execute("develop.sectionEnabled", &json!({"section": "geometry", "enabled": true})).unwrap(); + assert!(fits(&s), "the Geometry eye is one: the crop fits again"); +}