diff --git a/crates/engine/src/cmd/atn_cmds/steps_other.rs b/crates/engine/src/cmd/atn_cmds/steps_other.rs index 1bdf27cd90..2d3d57266c 100644 --- a/crates/engine/src/cmd/atn_cmds/steps_other.rs +++ b/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 { 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 = 1.0..=9999.0; +const CUSTOM_OFFSET: RangeInclusive = -9999.0..=9999.0; +const CUSTOM_CELL: RangeInclusive = -999.0..=999.0; + +/// A number within `range`. +fn within(range: &RangeInclusive) -> impl Fn(&D) -> Option + '_ { + 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> = 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::>>() 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 { if !pixel_target(p) || !sets_only(p, &["target", "kernel", "scale", "offset"]) { return None; } - let int = |v: &Value| { + let int = |range: &RangeInclusive, 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, default: i32| match p.get(k) { None | Some(Value::Null) => Some(default), - Some(v) => int(v), + Some(v) => int(range, v), }; let cells: Vec = 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::>()?, + Some(v) => { + let a = v.as_array().filter(|a| a.len() == 25)?; + a.iter().map(|c| int(&CUSTOM_CELL, c)).collect::>()? + } }; - 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())), ) } diff --git a/crates/engine/src/cmd/atn_cmds/tests.rs b/crates/engine/src/cmd/atn_cmds/tests.rs index a7c49c24b8..6ee39c6116 100644 --- a/crates/engine/src/cmd/atn_cmds/tests.rs +++ b/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", ¶ms, "S").is_none(), "{params} exported"); + } +}