Keep neighbourhood operations out of a mask layer's chain
A layer holds a full chain and fuses it into the colour dispatch, so the panel - which names no operation - would have offered a noise reduction slider inside a local adjustment. It could not have worked: the detail stage is its own dispatch, running after the masks are already applied, with nowhere to be handed one layer's mask. The control would have moved and done nothing. Filter the layer's chain to the operations that can honour it.
This commit is contained in:
@@ -660,12 +660,37 @@ pub struct MaskLayer {
|
|||||||
pub ops: Vec<Box<dyn Operation>>,
|
pub ops: Vec<Box<dyn Operation>>,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// 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<Box<dyn Operation>> {
|
||||||
|
ops::chain()
|
||||||
|
.into_iter()
|
||||||
|
.filter(|o| o.detail().is_none())
|
||||||
|
.collect()
|
||||||
|
}
|
||||||
|
|
||||||
impl Clone for MaskLayer {
|
impl Clone for MaskLayer {
|
||||||
/// Cloned by *value*, not by handle: the ops are trait objects, so this
|
/// Cloned by *value*, not by handle: the ops are trait objects, so this
|
||||||
/// rebuilds a fresh chain and copies the parameters across. Needed because
|
/// rebuilds a fresh chain and copies the parameters across. Needed because
|
||||||
/// the UI edits a layer speculatively and the history stores snapshots.
|
/// the UI edits a layer speculatively and the history stores snapshots.
|
||||||
fn clone(&self) -> Self {
|
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 (dst, src) in ops.iter_mut().zip(&self.ops) {
|
||||||
for p in src.descriptor().params {
|
for p in src.descriptor().params {
|
||||||
dst.set_param(p.id, src.param(p.id));
|
dst.set_param(p.id, src.param(p.id));
|
||||||
@@ -738,7 +763,7 @@ impl MaskLayer {
|
|||||||
falloff: Falloff::default(),
|
falloff: Falloff::default(),
|
||||||
morphology: Morphology::default(),
|
morphology: Morphology::default(),
|
||||||
morph_radius: 0.0,
|
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");
|
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]
|
#[test]
|
||||||
fn zero_opacity_is_inactive() {
|
fn zero_opacity_is_inactive() {
|
||||||
let mut layer = lit_layer("m1", 1.0);
|
let mut layer = lit_layer("m1", 1.0);
|
||||||
|
|||||||
Reference in New Issue
Block a user