Actions: keep a Custom step outside the command's scale, offset or cell range (FredApps review)

filter.other.custom plays a scale of 1..9999, an offset of -9999..9999
and kernel cells of -999..999, and clamps anything else when it plays,
so Scl 0 played as 1 and saved back as 0. Such a step now keeps the
Photoshop step, and export refuses those values. The matrix length is
checked before its cells are collected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

crates/engine/src/cmd/atn_cmds/steps_other.rs

@@ -6,6 +6,7 @@ use super::events::pixel_target;
 use super::fields::{Mapped, step};
 use super::tables::{lookup, lookup_back};
 use super::{D, Descriptor, Map, Value, json};
+use std::ops::RangeInclusive;
 
 /// Whether every key of `d` is one of `keys` (codes compare without their space padding).
 fn only_keys(d: &Descriptor, keys: &[&str]) -> bool {
@@ -84,18 +85,31 @@ pub(super) fn enums_to(p: &Value, keys: &[EnumKey]) -> Option<Descriptor> {
     Some(d)
 }
 
+/// `filter.other.custom`'s ranges: (scale, offset, kernel cell). The command clamps a value
+/// outside them when it plays, so `Scl 0` would play as 1 and save back as 0.
+const CUSTOM_SCALE: RangeInclusive<f64> = 1.0..=9999.0;
+const CUSTOM_OFFSET: RangeInclusive<f64> = -9999.0..=9999.0;
+const CUSTOM_CELL: RangeInclusive<f64> = -999.0..=999.0;
+
+/// A number within `range`.
+fn within(range: &RangeInclusive<f64>) -> impl Fn(&D) -> Option<f64> + '_ {
+    move |v| num(v).filter(|n| range.contains(n))
+}
+
 /// Custom (`Cstm`): `Mtrx` is the 5 × 5 kernel, row by row. Photoshop takes exactly 25 cells (a
-/// 3 × 3 list makes it fail), so any other matrix keeps the Photoshop step, as does a `Scl ` or
-/// `Ofst` that is not a number.
+/// 3 × 3 list makes it fail), so any other matrix keeps the Photoshop step, as does a `Scl `,
+/// `Ofst` or cell that is not a number, or one outside the command's range (it would play
+/// clamped).
 pub(super) fn custom_from(d: &Descriptor) -> Mapped {
     let unsupported = Mapped::Unsupported { harmless: false };
     if !only_keys(d, &["Scl", "Ofst", "Mtrx"]) {
         return unsupported;
     }
-    let Some(cells) = get_list(d, "Mtrx") else { return unsupported };
-    let kernel: Option<Vec<f64>> = cells.iter().map(num).collect();
-    let Some(kernel) = kernel.filter(|k| k.len() == 25) else { return unsupported };
-    let (Some(scale), Some(offset)) = (setting(d, "Scl ", num, 1.0), setting(d, "Ofst", num, 0.0)) else { return unsupported };
+    let Some(cells) = get_list(d, "Mtrx").filter(|c| c.len() == 25) else { return unsupported };
+    let Some(kernel) = cells.iter().map(within(&CUSTOM_CELL)).collect::<Option<Vec<f64>>>() else { return unsupported };
+    let (Some(scale), Some(offset)) = (setting(d, "Scl ", within(&CUSTOM_SCALE), 1.0), setting(d, "Ofst", within(&CUSTOM_OFFSET), 0.0)) else {
+        return unsupported;
+    };
     let mut p = Map::new();
     p.insert("kernel".into(), json!(kernel));
     p.insert("scale".into(), scale.into());
@@ -104,30 +118,31 @@ pub(super) fn custom_from(d: &Descriptor) -> Mapped {
 }
 
 /// `Scl `, `Ofst` and the 25 cells as integers; a kernel left out is the command's identity
-/// (1 in the middle). A fraction anywhere refuses the step: it would replay rounded.
+/// (1 in the middle). A fraction anywhere refuses the step: it would replay rounded; so does a
+/// value outside the command's range, which it doesn't play.
 pub(super) fn custom_to(_: &str, p: &Value) -> Option<Descriptor> {
     if !pixel_target(p) || !sets_only(p, &["target", "kernel", "scale", "offset"]) {
         return None;
     }
-    let int = |v: &Value| {
+    let int = |range: &RangeInclusive<f64>, v: &Value| {
         let n = v.as_f64()?;
-        (n.fract() == 0.0 && (f64::from(i32::MIN)..=f64::from(i32::MAX)).contains(&n)).then_some(n as i32)
+        (n.fract() == 0.0 && range.contains(&n)).then_some(n as i32)
     };
-    let field = |k: &str, default: i32| match p.get(k) {
+    let field = |k: &str, range: &RangeInclusive<f64>, default: i32| match p.get(k) {
         None | Some(Value::Null) => Some(default),
-        Some(v) => int(v),
+        Some(v) => int(range, v),
     };
     let cells: Vec<i32> = match p.get("kernel") {
         None | Some(Value::Null) => (0..25).map(|i| i32::from(i == 12)).collect(),
-        Some(v) => v.as_array()?.iter().map(int).collect::<Option<_>>()?,
+        Some(v) => {
+            let a = v.as_array().filter(|a| a.len() == 25)?;
+            a.iter().map(|c| int(&CUSTOM_CELL, c)).collect::<Option<_>>()?
+        }
     };
-    if cells.len() != 25 {
-        return None;
-    }
     Some(
         desc("null")
-            .with("Scl ", D::Integer(field("scale", 1)?))
-            .with("Ofst", D::Integer(field("offset", 0)?))
+            .with("Scl ", D::Integer(field("scale", &CUSTOM_SCALE, 1)?))
+            .with("Ofst", D::Integer(field("offset", &CUSTOM_OFFSET, 0)?))
             .with("Mtrx", D::List(cells.into_iter().map(D::Integer).collect())),
     )
 }

crates/engine/src/cmd/atn_cmds/tests.rs

@@ -3333,3 +3333,39 @@ fn custom_reads_an_unpadded_scale() {
     let Mapped::Steps(steps) = other_step("Cstm", desc("null").with("Scl", D::Integer(3)).with("Mtrx", identity)) else { panic!("kept") };
     assert_eq!(steps[0].1["scale"], json!(3.0));
 }
+
+/// `filter.other.custom` plays a scale of 1..9999, an offset of -9999..9999 and cells of
+/// -999..999, and clamps anything else: `Scl 0` would play as 1 and save back as 0. Such a step
+/// stays a Photoshop step, as does a matrix of more than 25 cells, and export refuses those
+/// values instead of writing what the command doesn't play (#10607).
+#[test]
+fn custom_outside_the_commands_range_is_kept_and_refused() {
+    let kept = |m: Mapped| matches!(m, Mapped::Unsupported { harmless: false });
+    let kernel = |cell: i32| D::List((0..25).map(|i| D::Integer(if i == 12 { cell } else { 0 })).collect());
+    let identity = kernel(1);
+    for d in [
+        desc("null").with("Mtrx", identity.clone()).with("Scl ", D::Integer(0)),
+        desc("null").with("Mtrx", identity.clone()).with("Scl ", D::Integer(10_000)),
+        desc("null").with("Mtrx", identity.clone()).with("Ofst", D::Integer(-10_000)),
+        desc("null").with("Mtrx", identity.clone()).with("Ofst", D::Integer(10_000)),
+        desc("null").with("Mtrx", kernel(1000)),
+        desc("null").with("Mtrx", kernel(-1000)),
+        desc("null").with("Mtrx", D::List((0..26).map(|_| D::Integer(0)).collect())),
+        desc("null").with("Mtrx", D::List((0..1000).map(|_| D::Integer(0)).collect())),
+    ] {
+        assert!(kept(other_step("Cstm", d.clone())), "{:?} was mapped", d.items);
+    }
+    for (scale, offset, cell) in [(1, -9999, -999), (9999, 9999, 999)] {
+        let d = desc("null").with("Scl ", D::Integer(scale)).with("Ofst", D::Integer(offset)).with("Mtrx", kernel(cell));
+        let Mapped::Steps(steps) = other_step("Cstm", d) else { panic!("{scale} {offset} {cell} kept") };
+        assert_eq!(
+            (&steps[0].1["scale"], &steps[0].1["offset"], &steps[0].1["kernel"][12]),
+            (&json!(f64::from(scale)), &json!(f64::from(offset)), &json!(f64::from(cell)))
+        );
+    }
+    let mut cells = vec![json!(0); 25];
+    cells[12] = json!(1000);
+    for params in [json!({"scale": 0}), json!({"scale": 10_000}), json!({"offset": -10_000}), json!({"offset": 10_000}), json!({"kernel": cells})] {
+        assert!(export_item("filter.other.custom", &params, "S").is_none(), "{params} exported");
+    }
+}