Merged develop settings are kept in range, as XMP import keeps them (fixes #1946)

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<DevelopSettings, serde_json::Error> {
         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();

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)]