diff --git a/crates/develop/src/lib.rs b/crates/develop/src/lib.rs index f376b3c67..4631e5e49 100644 --- a/crates/develop/src/lib.rs +++ b/crates/develop/src/lib.rs @@ -41,7 +41,7 @@ impl DevelopSettings { /// 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 - /// Kelvin settings. + /// Kelvin settings. Out-of-range values are brought back into range ([`DevelopSettings::keep_in_range`]). pub fn merged(&self, partial: &Value) -> Result { let mut v = self.to_json(); presets::deep_merge(&mut v, partial); @@ -52,9 +52,34 @@ impl DevelopSettings { { out.wb.scale = WbScale::Legacy; } + out.keep_in_range(self); Ok(out) } + /// 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. + fn keep_in_range(&mut self, before: &DevelopSettings) { + for c in controls::CONTROLS { + if let Some(v) = controls::get(self, c.id) { + controls::set(self, c.id, v); + } + } + let c = &mut self.curve; + for p in [&mut c.master, &mut c.red, &mut c.green, &mut c.blue].into_iter().flatten() { + *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; + if g.angle == 0.0 { + g.rect = g.rect.intersect(&lightcraft_geom::Rect::UNIT); + } + if g.rect.is_empty() { + let old = before.crop.geometry.rect; + g.rect = if old.is_empty() { lightcraft_geom::Rect::UNIT } else { old }; + } + } + /// Stable 64-bit hash of the settings (FNV-1a over canonical JSON), for preview cache keys. pub fn hash64(&self) -> u64 { let s = serde_json::to_string(self).unwrap_or_default(); @@ -255,6 +280,30 @@ mod tests { assert_eq!(m.light.exposure, 0.0); } + #[test] + fn merged_settings_are_kept_in_range() { + // issue #1946: `develop.merge`, `render --settings` and presets stored whatever the JSON said + let mut base = DevelopSettings::default(); + base.crop.geometry.rect = lightcraft_geom::Rect::new(0.1, 0.2, 0.8, 0.9); + let m = base + .merged(&json!({"light": {"exposure": 1e308}, "vignette": {"amount": -1e308}, "geometry": {"scale": 0}, "optics": {"distortion": 1e308}})) + .unwrap(); + assert_eq!((m.light.exposure, m.vignette.amount, m.geometry.scale, m.optics.distortion), (5.0, -100.0, 50.0, 100.0)); + // point curves stay in 0..1, as an imported XMP curve does + let m = base.merged(&json!({"curve": {"master": [{"x": -3, "y": 0}, {"x": 1, "y": 1e308}]}})).unwrap(); + assert_eq!(m.curve.master, vec![lightcraft_geom::Point::new(0.0, 0.0), lightcraft_geom::Point::new(1.0, 1.0)]); + // a crop with no area is left out, as XMP import leaves out one whose edges are inverted + for rect in + [json!({"x0": 0.9, "x1": 0.1, "y0": 0, "y1": 1}), json!({"x0": 1e308, "x1": -1e308, "y0": 0, "y1": 1}), json!({"x0": 0.5, "x1": 0.5})] + { + let m = base.merged(&json!({"crop": {"geometry": {"rect": rect}}})).unwrap(); + assert_eq!(m.crop.geometry.rect, base.crop.geometry.rect, "{rect}"); + } + // unrotated, the crop stays inside the photo + let m = base.merged(&json!({"crop": {"geometry": {"rect": {"x0": -1e308, "y0": 0.25, "x1": 1e308, "y1": 0.75}}}})).unwrap(); + assert_eq!(m.crop.geometry.rect, lightcraft_geom::Rect::new(0.0, 0.25, 1.0, 0.75)); + } + #[test] fn unedited_and_reset() { let mut s = DevelopSettings::default(); diff --git a/crates/develop/src/presets.rs b/crates/develop/src/presets.rs index 85f27d321..b3edc801c 100644 --- a/crates/develop/src/presets.rs +++ b/crates/develop/src/presets.rs @@ -184,17 +184,11 @@ fn scale_patch(base: &Value, patch: &Value, t: f64) -> Value { } /// Apply a partial settings object with an amount (1.0 = 100 %). Out-of-range values are clamped -/// through the control specs. +/// through the control specs, as every merge does ([`DevelopSettings::merged`]). pub fn apply_partial(s: &DevelopSettings, partial: &Value, amount: f64) -> DevelopSettings { let patch = if (amount - 1.0).abs() < 1e-9 { Cow::Borrowed(partial) } else { Cow::Owned(scale_patch(&s.to_json(), partial, amount)) }; // (a white balance without a scale keeps its old meaning: `DevelopSettings::merged`) - let mut out = s.merged(&patch).unwrap_or_else(|_| s.clone()); - for c in crate::controls::CONTROLS { - if let Some(v) = crate::controls::get(&out, c.id) { - crate::controls::set(&mut out, c.id, v); - } - } - out + s.merged(&patch).unwrap_or_else(|_| s.clone()) } #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]