From 646566f1f389ed016b15515e13b4da4460da64fb Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 14:16:25 -0700 Subject: [PATCH] LR-CROP-RECT: sidecar settings kept in range; a turned crop bounded to the photo (#2555) A sidecar's lc:settings became the photo's settings through plain from_json, without the keep_in_range that #1960 gave merged settings, and keep_in_range itself leaves a turned crop unbounded. A crop rect of 0..1000 at any angle made the native size of a 6000 x 4000 photo 6,000,000 x 4,000,000 px, which a full-size export passes its memory check with and allocates. - DevelopSettings::from_untrusted_json (from_json + keep_in_range), used for lc:settings. - keep_in_range keeps the crop angle within +-45 degrees (not finite: 0). - CropGeometry::bounded clips the crop to the bounding box of the photo turned by its angle, which holds every crop that stays on the photo (kept bit for bit); Frame applies it, so every source of settings is covered. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01K93HTyCiwYvbRGaAkjPvXn --- crates/develop/src/lib.rs | 14 +++++++-- crates/engine/src/sidecar.rs | 3 +- crates/engine/src/tests_process.rs | 17 ++++++++++ crates/geom/src/crop.rs | 50 ++++++++++++++++++++++++++++++ crates/pipeline/src/geometry.rs | 15 ++++++++- docs/whats-new.md | 9 ++++++ 6 files changed, 104 insertions(+), 4 deletions(-) diff --git a/crates/develop/src/lib.rs b/crates/develop/src/lib.rs index 4631e5e49..7750afffc 100644 --- a/crates/develop/src/lib.rs +++ b/crates/develop/src/lib.rs @@ -38,6 +38,14 @@ impl DevelopSettings { serde_json::from_value(v.clone()) } + /// [`Self::from_json`] for settings from outside the app, such as a sidecar's `lc:settings` + /// (issue #2555): kept in range as [`Self::merged`] keeps them. + pub fn from_untrusted_json(v: &Value) -> Result { + let mut s = DevelopSettings::from_json(v)?; + s.keep_in_range(&DevelopSettings::default()); + Ok(s) + } + /// Merge a partial JSON object into these settings (fields not mentioned are kept). A white balance that /// names no [`WbScale`] keeps the meaning it always had (`Legacy`), whatever scale these settings were on: a /// partial from before the scale existed, or one that never said, must not become Kelvin by landing on @@ -58,8 +66,9 @@ impl DevelopSettings { /// What crs XMP import does to the values it reads, for settings that arrive as JSON (presets, `develop.merge`, /// `render --settings`; issue #1946): every slider clamped through its control spec, point curves in 0..1, and a - /// crop with no area left as it was in `before`. Unrotated, the crop is clipped to the photo first; a turned crop - /// may reach past 0..1 and is left to the crop tools, which know the photo's size. + /// crop with no area left as it was in `before`. The angle is kept within ±45° (not finite: 0). Unrotated, the crop + /// is clipped to the photo first; a turned crop may reach past 0..1 and is bounded where the photo's size is known + /// (`CropGeometry::bounded`, applied by the pipeline). fn keep_in_range(&mut self, before: &DevelopSettings) { for c in controls::CONTROLS { if let Some(v) = controls::get(self, c.id) { @@ -71,6 +80,7 @@ impl DevelopSettings { *p = lightcraft_geom::Point::new(p.x.clamp(0.0, 1.0), p.y.clamp(0.0, 1.0)); } let g = &mut self.crop.geometry; + g.angle = if g.angle.is_finite() { g.angle.clamp(-45.0, 45.0) } else { 0.0 }; if g.angle == 0.0 { g.rect = g.rect.intersect(&lightcraft_geom::Rect::UNIT); } diff --git a/crates/engine/src/sidecar.rs b/crates/engine/src/sidecar.rs index d13b95125..6a4535f05 100644 --- a/crates/engine/src/sidecar.rs +++ b/crates/engine/src/sidecar.rs @@ -202,7 +202,8 @@ pub fn parse_sidecar(xmp: &str, target: crate::crs::Target) -> std::result::Resu out.label = Some(ColorLabel::parse(l.trim())); out.label_text = Some(l.trim().to_string()); } - let full = d.lc_settings.as_deref().and_then(|j| serde_json::from_str::(j).ok()).and_then(|v| DevelopSettings::from_json(&v).ok()); + let full = + d.lc_settings.as_deref().and_then(|j| serde_json::from_str::(j).ok()).and_then(|v| DevelopSettings::from_untrusted_json(&v).ok()); out.develop = match full { Some(s) => Some(DevelopPatch::Full(Box::new(s))), None if crate::crs::has_adjustments(&d.properties) => { diff --git a/crates/engine/src/tests_process.rs b/crates/engine/src/tests_process.rs index d101073ec..39747b1a1 100644 --- a/crates/engine/src/tests_process.rs +++ b/crates/engine/src/tests_process.rs @@ -187,6 +187,23 @@ fn process_numbers_survive_a_reopen_and_develop_get_reports_them() { let _ = std::fs::remove_dir_all(&dir); } +/// Issue #2555: a sidecar's `lc:settings` became the photo's settings without the range checks +/// presets and `develop.merge` get, so sliders and the crop angle kept any value. +#[test] +fn sidecar_settings_are_kept_in_range() { + use crate::sidecar::{DevelopPatch, parse_sidecar}; + let mut wild = lightcraft_develop::DevelopSettings::default(); + wild.light.exposure = 1e9; + wild.crop.geometry = lightcraft_geom::CropGeometry { rect: lightcraft_geom::Rect::new(0.0, 0.0, 1000.0, 1000.0), angle: 400.0 }; + let json = wild.to_json().to_string(); + let packet = lightcraft_meta::write_xmp(&lightcraft_meta::Metadata::default(), Some(&json)); + let Some(DevelopPatch::Full(d)) = parse_sidecar(&packet, crate::crs::Target::Rendered).unwrap().develop else { panic!("no lc:settings") }; + assert!(d.light.exposure <= 10.0, "exposure {}", d.light.exposure); + assert_eq!(d.crop.geometry.angle, 45.0); + let (w, h) = lightcraft_pipeline::native_output_size(6000, 4000, &d); + assert!(w <= 12_000.0 && h <= 8_000.0, "{w} x {h}"); +} + #[test] fn sidecars_keep_the_process_and_interchange_edits_get_the_latest() { use crate::sidecar::{DevelopPatch, merge_into, parse_sidecar, sidecar_packet}; diff --git a/crates/geom/src/crop.rs b/crates/geom/src/crop.rs index cc26b40fe..a8df07682 100644 --- a/crates/geom/src/crop.rs +++ b/crates/geom/src/crop.rs @@ -44,6 +44,29 @@ impl CropGeometry { Affine::rotate_about(-self.angle.to_radians(), c) * to_rot } + /// This crop with values no crop tool can produce taken out, for settings read from a file or an + /// agent (issue #2555): the angle within ±45° (not finite: 0), and the rect clipped to the + /// bounding box of the image turned by that angle, which holds every crop that stays on the + /// photo, so a real crop is unchanged while one reaching far past the photo can no longer make + /// a full-size output of billions of pixels. A rect with nothing left (or not finite) is the + /// whole frame. + pub fn bounded(&self, w: f64, h: f64) -> CropGeometry { + let angle = if self.angle.is_finite() { self.angle.clamp(-45.0, 45.0) } else { 0.0 }; + let (sin, cos) = angle.to_radians().sin_cos(); + let (bw, bh) = (w * cos.abs() + h * sin.abs(), w * sin.abs() + h * cos.abs()); + let bbox = Rect::from_center(Point::new(w / 2.0, h / 2.0), bw, bh); + let r = self.rect_px(w, h); + let finite = [r.x0, r.y0, r.x1, r.y1].iter().all(|v| v.is_finite()); + let inside = r.x0 >= bbox.x0 && r.y0 >= bbox.y0 && r.x1 <= bbox.x1 && r.y1 <= bbox.y1; + if finite && inside { + // left exactly as stored: a round trip through pixels would change its last bits + return CropGeometry { rect: self.rect, angle }; + } + let rect = if finite { r.intersect(&bbox) } else { Rect::new(0.0, 0.0, 0.0, 0.0) }; + let rect = if rect.is_empty() || w <= 0.0 || h <= 0.0 { Rect::UNIT } else { rect.scale(1.0 / w, 1.0 / h) }; + CropGeometry { rect, angle } + } + /// Whether every crop corner lies within the rotated image. pub fn is_within_image(&self, w: f64, h: f64) -> bool { let r = self.rect_px(w, h); @@ -176,6 +199,33 @@ pub fn crop_fit_angle(w: f64, h: f64, angle_deg: f64, aspect: Option) -> Cr mod tests { use super::*; + /// Issue #2555: `bounded` leaves every crop that stays on the photo exactly as it was, and + /// clips one reaching far past it to the turned photo's bounding box. + #[test] + fn bounded_keeps_real_crops_and_clips_the_rest() { + for (w, h) in [(6000.0, 4000.0), (4000.0, 6000.0), (9000.0, 1000.0)] { + for angle in [-45.0, -12.5, 0.0, 0.1, 7.0, 45.0] { + for aspect in [None, Some(1.0), Some(3.0), Some(0.25)] { + let real = crop_fit_angle(w, h, angle, aspect); + assert_eq!(real.bounded(w, h), real, "{w}x{h} {angle} {aspect:?}"); + } + } + assert_eq!(CropGeometry::default().bounded(w, h), CropGeometry::default()); + } + let (w, h) = (6000.0, 4000.0); + let wild = CropGeometry { rect: Rect::new(0.0, 0.0, 1000.0, 1000.0), angle: 5.0 }; + let b = wild.bounded(w, h).rect_px(w, h); + assert!(b.width() <= w * 1.5 && b.height() <= h * 1.5, "{b:?}"); + // angle beyond the tools' range, or not a number; a rect that is not a number + assert_eq!(CropGeometry { rect: Rect::UNIT, angle: 400.0 }.bounded(w, h).angle, 45.0); + assert_eq!(CropGeometry { rect: Rect::UNIT, angle: f64::NAN }.bounded(w, h).angle, 0.0); + let nan = CropGeometry { rect: Rect::new(f64::NAN, 0.0, 1.0, 1.0), angle: 3.0 }.bounded(w, h); + assert_eq!(nan.rect, Rect::UNIT); + // a crop entirely off the turned photo has nothing left: the whole frame + let off = CropGeometry { rect: Rect::new(50.0, 50.0, 60.0, 60.0), angle: 10.0 }.bounded(w, h); + assert_eq!(off.rect, Rect::UNIT); + } + #[test] fn identity_maps_pixels() { let c = CropGeometry::default(); diff --git a/crates/pipeline/src/geometry.rs b/crates/pipeline/src/geometry.rs index 3f2766750..641d50216 100644 --- a/crates/pipeline/src/geometry.rs +++ b/crates/pipeline/src/geometry.rs @@ -54,7 +54,8 @@ impl Frame { pub fn with_lens(src_w: usize, src_h: usize, s: &DevelopSettings, apply_crop: bool, lens: Option<&EmbeddedLens>) -> Frame { let orient = s.orientation; let (ow, oh) = if orient.swaps_axes() { (src_h as f64, src_w as f64) } else { (src_w as f64, src_h as f64) }; - let crop = if apply_crop { s.crop.geometry } else { CropGeometry::default() }; + // bounded: a crop from a sidecar or an agent can reach far past the photo (issue #2555) + let crop = if apply_crop { s.crop.geometry.bounded(ow, oh) } else { CropGeometry::default() }; let lens = lens.map(|l| reorient_lens(l, orient, src_w as f64, src_h as f64)); let mut warp = Warp::from_settings(ow, oh, s, lens.as_ref()); let persp = perspective(s, ow, oh); @@ -355,6 +356,18 @@ pub const FULL: Rect = Rect::UNIT; mod tests { use super::*; + /// Issue #2555: a turned crop rect of 0..1000 (from a sidecar or an agent) made the native size + /// of a 6000 × 4000 photo ~2.4·10¹³ pixels, which a full-size export allocated. + #[test] + fn a_crop_far_past_the_photo_keeps_the_native_size_near_the_source() { + let mut s = DevelopSettings::default(); + s.crop.geometry = lightcraft_geom::CropGeometry { rect: lightcraft_geom::Rect::new(0.0, 0.0, 1000.0, 1000.0), angle: 5.0 }; + let (w, h) = crate::native_output_size(6000, 4000, &s); + assert!(w <= 12_000.0 && h <= 8_000.0, "{w} x {h}"); + // an untouched photo is its own size + assert_eq!(crate::native_output_size(6000, 4000, &DevelopSettings::default()), (6000.0, 4000.0)); + } + #[test] fn default_frame_is_identity() { let s = DevelopSettings::default(); diff --git a/docs/whats-new.md b/docs/whats-new.md index cffff83e2..54d8dcdce 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -1,5 +1,14 @@ # What's new in LightCraft +## 2026-10-22 + +### Edits read from a sidecar are kept in range +- LightCraft edits read back from an XMP sidecar (Read Metadata from File, or at import) are now checked like presets + and pasted settings: sliders and curves are kept in their ranges and the straighten angle within ±45° (issue + #2555). A turned crop that reaches far past the photo is trimmed to the photo when it is shown or exported. Before, + a damaged or hand-edited sidecar could set a crop that made a full-size export ask for trillions of pixels and + close the app. Crops made with the crop tool are unchanged. + ## 2026-10-21 ### Sharper resampling in HDR and panorama merges