diff --git a/core/dr-pipeline/src/mask.rs b/core/dr-pipeline/src/mask.rs index 76c7d9c..b35b4f2 100644 --- a/core/dr-pipeline/src/mask.rs +++ b/core/dr-pipeline/src/mask.rs @@ -660,12 +660,37 @@ pub struct MaskLayer { pub ops: Vec>, } +/// The chain a mask layer holds: every point operation, and none of the +/// neighbourhood ones. +/// +/// A layer's adjustments are fused into the colour dispatch and multiplied by +/// the mask afterwards, which is exactly why a layer needs no per-operation +/// support — the composer already knows how to turn a chain into WGSL. A +/// neighbourhood operation cannot go through that path at all: it runs as its +/// own dispatch in [`crate::detail`], after the fused pass and after the masks +/// have already been applied, and there is nowhere in that arrangement for it +/// to be given one layer's mask. +/// +/// Left in, it would be worse than absent. `Operation::wgsl_body` returns an +/// empty string for a detail operation, so the layer would emit an empty block +/// and the panel — which builds itself from [`MaskLayer::capabilities`] and +/// names no operation — would offer a slider that moved and did nothing. +/// Filtering here means a local sharpening or denoise control simply does not +/// appear until there is a stage that can honour it, which is the honest +/// state of affairs. +fn layer_chain() -> Vec> { + ops::chain() + .into_iter() + .filter(|o| o.detail().is_none()) + .collect() +} + impl Clone for MaskLayer { /// Cloned by *value*, not by handle: the ops are trait objects, so this /// rebuilds a fresh chain and copies the parameters across. Needed because /// the UI edits a layer speculatively and the history stores snapshots. fn clone(&self) -> Self { - let mut ops = ops::chain(); + let mut ops = layer_chain(); for (dst, src) in ops.iter_mut().zip(&self.ops) { for p in src.descriptor().params { dst.set_param(p.id, src.param(p.id)); @@ -738,7 +763,7 @@ impl MaskLayer { falloff: Falloff::default(), morphology: Morphology::default(), morph_radius: 0.0, - ops: ops::chain(), + ops: layer_chain(), } } @@ -1276,6 +1301,49 @@ mod tests { assert_eq!(stack.len(), 1, "but they are not deleted"); } + #[test] + fn a_layer_offers_only_the_operations_it_can_actually_apply() { + // A layer's adjustments are fused into the colour dispatch and then + // multiplied by the mask. A neighbourhood operation cannot take that + // route: it is a dispatch of its own, run after the fused pass and + // after the masks are already applied, so there is nowhere to hand it + // one layer's mask. + // + // The panel builds itself from `capabilities()` and names no + // operation, so anything left in this chain becomes a control. One + // that cannot work is worse than one that is missing: it moves, the + // picture does not change, and nothing says why. + let layer = lit_layer("m1", 1.0); + let ids: Vec<&str> = layer.capabilities().iter().map(|c| c.id.0).collect(); + + let global = crate::ops::chain(); + for op in &global { + let id = op.descriptor().id.0; + assert_eq!( + ids.contains(&id), + op.detail().is_none(), + "{id} is offered as a local adjustment but cannot be one, \ + or is a point operation and has gone missing from a layer" + ); + } + assert!( + ids.len() < global.len() || global.iter().all(|o| o.detail().is_none()), + "the filter dropped nothing, so either it is not running or the \ + chain has no neighbourhood operation left to drop" + ); + + // And a clone must rebuild the same chain: it copies parameters across + // by position, so a chain built one way and rebuilt another would + // silently apply each value to the wrong operation. + let cloned: Vec<&str> = layer + .clone() + .capabilities() + .iter() + .map(|c| c.id.0) + .collect(); + assert_eq!(ids, cloned); + } + #[test] fn zero_opacity_is_inactive() { let mut layer = lit_layer("m1", 1.0);