mirror of
https://github.com/storytold/lightcraft.git
synced 2026-11-01 05:57:09 -06:00
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K93HTyCiwYvbRGaAkjPvXn
This commit is contained in:
@@ -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<DevelopSettings, serde_json::Error> {
|
||||
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);
|
||||
}
|
||||
|
||||
@@ -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::<Value>(j).ok()).and_then(|v| DevelopSettings::from_json(&v).ok());
|
||||
let full =
|
||||
d.lc_settings.as_deref().and_then(|j| serde_json::from_str::<Value>(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) => {
|
||||
|
||||
@@ -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};
|
||||
|
||||
@@ -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<f64>) -> 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();
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user