diff --git a/core/dr-gpu/src/mask.rs b/core/dr-gpu/src/mask.rs index 7e0e001..9d76e08 100644 --- a/core/dr-gpu/src/mask.rs +++ b/core/dr-gpu/src/mask.rs @@ -708,10 +708,36 @@ impl MaskPass { source: Option<&DemosaicedImage>, width: u32, height: u32, + ) -> Result<&MaskArray, GpuError> { + self.render_revealing(stack, labels, subjects, source, width, height, None) + } + + /// TRACES: FR-DEV-19c + /// [`Self::render`], also drawing the layer being looked at. + /// + /// A selection with no adjustment on it changes no pixel, so it is not + /// active and has no slice — which is right until somebody asks to *see* + /// it, and that is the state a photographer is in from choosing a subject + /// until deciding what to do to it. + /// + /// `reveal` has to be the same one the shader was composed with and the + /// same one the distance fields were built for: all three index this array + /// by position in [`MaskStack::rendered`], and two of them disagreeing + /// shows as an adjustment applied through another layer's mask. + #[allow(clippy::too_many_arguments)] + pub fn render_revealing( + &mut self, + stack: &MaskStack, + labels: Option<&LabelField>, + subjects: Option<&SubjectMasks>, + source: Option<&DemosaicedImage>, + width: u32, + height: u32, + reveal: Option<&dr_pipeline::mask::Reveal>, ) -> Result<&MaskArray, GpuError> { // At least one layer, because a zero-layer texture array is invalid // and the shader binds this slot unconditionally. - let active = stack.active_count().clamp(1, MAX_LAYERS) as u32; + let active = stack.rendered_count(reveal).clamp(1, MAX_LAYERS) as u32; self.ensure_array(width, height, active)?; let mut encoder = self @@ -721,7 +747,7 @@ impl MaskPass { label: Some("mask-encoder"), }); - for (slot, layer) in stack.active().enumerate().take(MAX_LAYERS) { + for (slot, layer) in stack.rendered(reveal).enumerate().take(MAX_LAYERS) { // **The path a mask with one part takes is the path every mask // took before parts existed**: drawn straight into the layer's // slice, cleared by the draw itself. Nothing about an unedited diff --git a/core/dr-gpu/tests/local_adjustments.rs b/core/dr-gpu/tests/local_adjustments.rs index 923452b..7110f28 100644 --- a/core/dr-gpu/tests/local_adjustments.rs +++ b/core/dr-gpu/tests/local_adjustments.rs @@ -12,8 +12,8 @@ use dr_gpu::{AdjustPass, DemosaicedImage, GpuContext, LabelField, MaskPass}; use dr_pipeline::descriptor::ParamId; -use dr_pipeline::mask::{Join, MaskLayer, MaskPart, MaskSource, MaskStack}; -use dr_pipeline::operation::compose_full; +use dr_pipeline::mask::{Join, MaskLayer, MaskPart, MaskSource, MaskStack, Reveal, RevealStyle}; +use dr_pipeline::operation::{compose_full, compose_full_revealing}; use dr_pipeline::spot::SpotSet; use dr_pipeline::{ops, EditGraph, Framing}; use dr_types::ColourSpace; @@ -789,3 +789,198 @@ fn the_order_parts_are_joined_in_is_the_mask() { "and subtracting after an addition takes it away again" ); } + +// --- seeing the mask (FR-DEV-19c) ------------------------------------------ + +/// A radial that covers the middle of the frame and nothing near the corners. +fn middle() -> MaskSource { + MaskSource::Radial { + centre: (0.5, 0.5), + radii: (0.3, 0.3), + angle: 0.0, + feather: 0.05, + } +} + +/// [`render`], with one layer's mask drawn over the result. +fn render_revealing(ctx: &GpuContext, stack: &MaskStack, reveal: &Reveal) -> Vec { + let source = grey(ctx); + let shader = compose_full_revealing( + &ops::chain(), + &Framing::new(), + ColourSpace::Srgb, + stack, + &SpotSet::new(), + &[], + Some(reveal), + ); + + let mut masks = MaskPass::new(ctx).expect("mask pass"); + let array = masks + .render_revealing(stack, None, None, None, SIZE, SIZE, Some(reveal)) + .expect("rasterise"); + + let mut adjust = AdjustPass::new(ctx); + adjust + .render_masked(&source, &shader, SIZE, SIZE, Some(array)) + .expect("render"); + adjust.export_pixels().expect("readback").0 +} + +/// TRACES: FR-DEV-19c +/// The state every mask is in for its first few seconds: chosen, and not yet +/// used for anything. +/// +/// Such a layer changes no pixel, so it is not active, so it occupied no mask +/// slot and was never rasterised — and the reveal drew nothing. That is the +/// whole of "I clicked the category and nothing happened": there was a mask, +/// and no way to see that there was. +#[test] +fn a_selection_with_no_adjustment_can_still_be_seen() { + let Some(ctx) = ctx() else { + eprintln!("no adapter; skipping"); + return; + }; + + let mut stack = MaskStack::new(); + stack.push(MaskLayer::new("m1", middle())); + assert!( + stack.is_neutral(), + "the fixture must be a selection with nothing done to it" + ); + + let pixels = render_revealing( + &ctx, + &stack, + &Reveal { + layer: "m1".into(), + style: RevealStyle::Alpha, + }, + ); + + assert!( + luma_at(&pixels, SIZE / 2, SIZE / 2) > 200, + "the middle is inside the mask and should read white" + ); + assert!( + luma_at(&pixels, 1, 1) < 40, + "the corner is outside it and should read black" + ); +} + +/// And with nobody looking, the same stack changes nothing at all. +/// +/// The other half of the property above: a layer renders *because* it is being +/// revealed, so it must stop when the reveal does — otherwise a selection with +/// no adjustment would leave a slice in the array for ever. +#[test] +fn a_mask_nobody_is_looking_at_draws_nothing() { + let Some(ctx) = ctx() else { + eprintln!("no adapter; skipping"); + return; + }; + + let mut stack = MaskStack::new(); + stack.push(MaskLayer::new("m1", middle())); + + let pixels = render(&ctx, &stack, None); + assert_eq!( + luma_at(&pixels, SIZE / 2, SIZE / 2), + 128, + "flat grey, exactly as it went in" + ); +} + +/// TRACES: FR-DEV-19c +/// A tint has to leave the photograph visible, or it cannot be judged against +/// it — which is the one thing an overlay exists for. +#[test] +fn a_tint_colours_the_mask_and_leaves_the_rest_alone() { + let Some(ctx) = ctx() else { + eprintln!("no adapter; skipping"); + return; + }; + + let mut stack = MaskStack::new(); + stack.push(MaskLayer::new("m1", middle())); + + let pixels = render_revealing( + &ctx, + &stack, + &Reveal { + layer: "m1".into(), + style: RevealStyle::Tint, + }, + ); + + let at = |x: u32, y: u32| { + let i = ((y * SIZE + x) * 4) as usize; + (pixels[i], pixels[i + 1], pixels[i + 2]) + }; + + let (r, g, _) = at(SIZE / 2, SIZE / 2); + assert!(r > g + 40, "the mask should read red, got r={r} g={g}"); + assert!( + g > 20, + "and not opaque — the photograph under it is what the tint is judged \ + against, got g={g}" + ); + + let (r, g, b) = at(1, 1); + assert!( + (120..=136).contains(&r) && r == g && g == b, + "outside the mask the photograph is untouched, got ({r}, {g}, {b})" + ); +} + +/// TRACES: FR-DEV-19c +/// An outline draws where the mask stops and nowhere else — which is the +/// point of it, since the other two styles cover the detail the boundary has +/// to be judged against. +#[test] +fn an_outline_draws_the_boundary_and_not_the_interior() { + let Some(ctx) = ctx() else { + eprintln!("no adapter; skipping"); + return; + }; + + let mut stack = MaskStack::new(); + stack.push(MaskLayer::new("m1", middle())); + + let pixels = render_revealing( + &ctx, + &stack, + &Reveal { + layer: "m1".into(), + style: RevealStyle::Edge, + }, + ); + + // Where the line landed, along the row through the centre. Searched + // rather than sampled at one place: the radial's edge crosses this row + // about 9.6 pixels out from the middle on a 32px frame, and asserting a + // particular pixel would be asserting the rounding. + let (at, brightest) = (SIZE / 2..SIZE) + .map(|x| (x, luma_at(&pixels, x, SIZE / 2))) + .max_by_key(|&(_, v)| v) + .expect("the row is not empty"); + + assert!( + brightest > 160, + "there should be a line somewhere on this row, brightest was {brightest}" + ); + assert!( + (SIZE / 2 + 7..=SIZE / 2 + 12).contains(&at), + "and it should be on the mask's boundary, not somewhere else: x={at}" + ); + assert_eq!( + luma_at(&pixels, SIZE / 2, SIZE / 2), + 128, + "the picture inside the mask is untouched" + ); + assert_eq!( + luma_at(&pixels, 1, 1), + 128, + "and so is the picture outside it" + ); +} diff --git a/core/dr-pipeline/src/graph.rs b/core/dr-pipeline/src/graph.rs index 3f5659c..7d86515 100644 --- a/core/dr-pipeline/src/graph.rs +++ b/core/dr-pipeline/src/graph.rs @@ -849,6 +849,35 @@ impl EditGraph { ) } + /// TRACES: FR-DEV-19c + /// [`Self::compose_for`], with one layer's mask drawn over the picture. + /// + /// **The screen's composition, and only the screen's.** The reveal is not + /// on the graph and cannot be: it is how a photographer is looking at an + /// edit, not part of one, so it arrives as an argument to the one call + /// that draws the canvas. Every other path through this type composes + /// without it and could not ask for it if it wanted to. + /// + /// The revealed layer renders whether or not it carries an adjustment — + /// which is the whole point, since a fresh selection carries none — so the + /// mask array must be rasterised for the same `reveal`. See + /// [`crate::mask::MaskStack::rendered`] for what the two have to agree on. + pub fn compose_revealing( + &self, + output: dr_types::ColourSpace, + reveal: Option<&crate::mask::Reveal>, + ) -> ComposedShader { + crate::operation::compose_full_revealing( + &self.ops, + &self.framing, + output, + &self.masks, + &self.spots, + &self.warps, + reveal, + ) + } + /// TRACES: FR-DSP-1 /// How this render relates to the file it stands for. /// diff --git a/core/dr-pipeline/src/mask.rs b/core/dr-pipeline/src/mask.rs index fef91bb..9a913b1 100644 --- a/core/dr-pipeline/src/mask.rs +++ b/core/dr-pipeline/src/mask.rs @@ -1760,6 +1760,49 @@ impl MaskLayer { } } +/// TRACES: FR-DEV-19c +/// How a mask is drawn when the photographer asks to see it. +/// +/// Three, because they answer three different questions and no one of them +/// answers all three — which is the argument for offering a choice rather than +/// picking the best one. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RevealStyle { + /// The mask over the photograph in a flat colour. What every editor's + /// photographers already expect, and the only style that answers "is this + /// selecting the right thing" while the picture is still visible. + Tint, + /// The mask alone, white on black. For judging an edge, which a tint over + /// a busy photograph cannot be read against. + Alpha, + /// The boundary outlined over the untouched picture. For checking + /// registration against detail the other two hide — the same reasoning the + /// region overlay's white outline already carries. + Edge, +} + +impl RevealStyle { + /// In the order the interface offers them. + pub const ALL: [RevealStyle; 3] = [Self::Tint, Self::Alpha, Self::Edge]; +} + +/// TRACES: FR-DEV-19c +/// The layer whose mask is being shown, and how. +/// +/// **Never part of an edit.** It is not stored on [`MaskStack`] and it does +/// not travel with the graph: it is passed to the one composition that draws +/// the screen, so an export, a thumbnail and the neutral probe are +/// structurally unable to reveal anything. A flag on the stack would have been +/// fewer parameters and would have tinted every exported file red. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Reveal { + /// Which layer, by id. By id rather than by slot because the slot is + /// derived from which layers render, and that is decided *by* this — see + /// [`MaskStack::rendered`]. + pub layer: String, + pub style: RevealStyle, +} + /// The ordered stack of local adjustments. #[derive(Debug, Clone, Default, PartialEq)] pub struct MaskStack { @@ -1838,6 +1881,34 @@ impl MaskStack { self.active().count() } + /// TRACES: FR-DEV-19c + /// [`Self::active`], plus the layer being looked at. + /// + /// A selection with no adjustment on it yet is not active — it changes no + /// pixel, so it occupies no mask slot and the rasteriser never draws it. + /// That is right for rendering and exactly wrong for *showing* the mask, + /// which is the state a photographer is in for the whole of the time + /// between choosing a subject and deciding what to do to it. + /// + /// So this is the sequence both halves walk whenever a reveal is in play, + /// and the index within it is the texture-array slot — the same contract + /// [`Self::active`] carries, and the reason the rasteriser, the composer + /// and the field builder must all be given the same `reveal` or none of + /// them. Two of them disagreeing shows as an adjustment applied through + /// another layer's mask. + pub fn rendered<'a>( + &'a self, + reveal: Option<&'a Reveal>, + ) -> impl Iterator { + self.layers + .iter() + .filter(move |l| l.is_active() || reveal.is_some_and(|r| r.layer == l.id)) + } + + pub fn rendered_count(&self, reveal: Option<&Reveal>) -> usize { + self.rendered(reveal).count() + } + /// Whether any layer changes any pixel. pub fn is_neutral(&self) -> bool { self.active_count() == 0 @@ -1858,25 +1929,45 @@ pub(crate) struct LayerShader { pub uniform_values: Vec, pub body: String, pub helpers: Vec, + /// TRACES: FR-DEV-19c + /// The block that draws one layer's mask over the finished picture, empty + /// when nothing is being revealed. + /// + /// Kept apart from `body` because it belongs at the other end of the + /// shader. Everything in `body` runs on scene-referred colour in the + /// working space, where a flat tint would then be pushed through the base + /// curve and the camera matrix and arrive as some other colour, and a + /// white-on-black alpha would arrive as neither. This runs after the + /// output transform, so what is written is what is seen. + pub reveal: String, } -/// Emit the WGSL for every active layer. +/// Emit the WGSL for every layer that renders, and for the mask being looked +/// at. /// /// `slot` is the layer's index in the mask texture array, matching -/// [`MaskStack::active`]. -pub(crate) fn compose_layers(stack: &MaskStack) -> LayerShader { +/// [`MaskStack::rendered`] — the revealed layer renders whether or not it has +/// an adjustment on it, which is why the two are one sequence and why every +/// other half of the pipeline has to be given the same `reveal` for the slots +/// to mean the same thing. +pub(crate) fn compose_layers_revealing(stack: &MaskStack, reveal: Option<&Reveal>) -> LayerShader { let mut out = LayerShader { uniform_fields: String::new(), uniform_values: Vec::new(), body: String::new(), helpers: Vec::new(), + reveal: String::new(), }; - if stack.active().next().is_some() { + if stack.rendered(reveal).next().is_some() { out.helpers.push(MASK_SAMPLER); } - for (slot, layer) in stack.active().enumerate() { + for (slot, layer) in stack.rendered(reveal).enumerate() { + if reveal.is_some_and(|r| r.layer == layer.id) { + out.reveal = reveal_block(slot, layer, reveal.expect("just matched").style); + } + let prefix = format!("mask{slot}"); let _ = writeln!( @@ -1973,6 +2064,75 @@ pub(crate) fn compose_layers(stack: &MaskStack) -> LayerShader { out } +/// TRACES: FR-DEV-19c +/// The WGSL that draws one layer's mask over the finished picture. +/// +/// # Why this is not two uniforms +/// +/// The slot and the style are written into the source, so turning the reveal +/// on, off, or onto another layer recompiles the fused shader. That is a +/// button press rather than a frame — and the alternative costs more than it +/// saves: a uniform can select a slot, but it cannot conjure one for a layer +/// that is not rendering, and the whole reason this exists is that a selection +/// with no adjustment on it yet is exactly that layer. So the composition +/// changes either way, and a uniform would only have added a branch per pixel +/// on top of it. +/// +/// **Everything below runs after the output transform.** `c` is already in the +/// output space's primaries and still linear — the clip and the encode come +/// after — which is what makes a stated colour arrive as itself. +fn reveal_block(slot: usize, layer: &MaskLayer, style: RevealStyle) -> String { + let prefix = format!("mask{slot}"); + let mut out = format!( + "\n // ==== showing mask {slot}: {} ====\n //\n // Not part of the\ + \n // photograph: this is the mask itself, drawn because someone asked to\ + \n // see it. Nothing downstream of the screen composes this shader.\n {{\n", + layer.display_name() + ); + // The shaped mask, exactly as the layer above applied it — the same two + // uniforms, in the same order. A reveal that showed the raw slice would + // draw a different mask from the one doing the work, which is worse than + // showing none: it would send a photographer to fix an edge that is + // already where they want it. + let _ = writeln!(out, " var m = sample_mask(uv_src, {slot});"); + let _ = writeln!( + out, + " m = select(m, 1.0 - m, u.{prefix}_invert > 0.5);" + ); + let _ = writeln!(out, " m = clamp(m * u.{prefix}_opacity, 0.0, 1.0);"); + + let body = match style { + // Red at a bit over half strength. Half is the strength every editor + // settled on for the same reason: past it the tint is opaque enough to + // hide the thing being judged, and below it a mask over a bright sky + // cannot be seen at all. + RevealStyle::Tint => " c = mix(c, vec3(0.85, 0.10, 0.15), m * 0.55);", + // Neutral, which means the same thing in every output space that + // shares a white point — so this one style needs no correction for the + // panel the window happens to be on. + RevealStyle::Alpha => " c = vec3(m);", + // The gradient's magnitude, over the untouched picture. Central + // differences one texel apart in the *mask's* own grid, so the outline + // is one mask texel wide however far the view is zoomed in — the + // boundary's position is the thing being checked, and a line that grew + // with the zoom would hide it. + RevealStyle::Edge => { + " let texel = 1.0 / vec2(textureDimensions(masks));\n\ + \x20 let dx = sample_mask(uv_src + vec2(texel.x, 0.0), SLOT)\n\ + \x20 - sample_mask(uv_src - vec2(texel.x, 0.0), SLOT);\n\ + \x20 let dy = sample_mask(uv_src + vec2(0.0, texel.y), SLOT)\n\ + \x20 - sample_mask(uv_src - vec2(0.0, texel.y), SLOT);\n\ + \x20 // Doubled so a soft edge, whose gradient is spread over many\n\ + \x20 // texels and therefore shallow everywhere, still draws a line.\n\ + \x20 let edge = clamp(2.0 * sqrt(dx * dx + dy * dy), 0.0, 1.0);\n\ + \x20 c = mix(c, vec3(1.0), edge);" + } + }; + let _ = writeln!(out, "{}", body.replace("SLOT", &slot.to_string())); + let _ = writeln!(out, " }}"); + out +} + /// A stable fingerprint of a segmentation, for [`MaskSource::Regions`]. /// /// Built from the things that change what a region id *means* — the proxy @@ -2021,7 +2181,7 @@ mod tests { let mut stack = MaskStack::new(); stack.push(layer); assert!(stack.is_neutral()); - assert_eq!(compose_layers(&stack).body, ""); + assert_eq!(compose_layers_revealing(&stack, None).body, ""); } #[test] @@ -2107,7 +2267,7 @@ mod tests { stack.push(lit_layer("m1", 1.0)); stack.push(lit_layer("m2", -1.0)); - let shader = compose_layers(&stack); + let shader = compose_layers_revealing(&stack, None); assert!(shader.body.contains("sample_mask(uv_src, 0)")); assert!(shader.body.contains("sample_mask(uv_src, 1)")); assert!(shader.body.contains("u.mask0_opacity")); @@ -2124,7 +2284,7 @@ mod tests { stack.push(off); stack.push(lit_layer("m2", -1.0)); - let shader = compose_layers(&stack); + let shader = compose_layers_revealing(&stack, None); assert!( shader.body.contains("sample_mask(uv_src, 0)"), "the one active layer must use slot 0, not slot 1" @@ -2132,13 +2292,74 @@ mod tests { assert!(!shader.body.contains("sample_mask(uv_src, 1)")); } + /// TRACES: FR-DEV-19c + /// The slot the reveal is given has to be the slot the layer renders + /// through, and a revealed layer renders even with nothing done to it. + /// + /// Both halves in one assertion because the failure is the pair coming + /// apart: a reveal pointed at a slot the rasteriser did not draw shows + /// whatever was last in that slice, which reads as the mask being wrong + /// rather than as the reveal being wrong. + #[test] + fn a_revealed_layer_takes_a_slot_of_its_own() { + let mut stack = MaskStack::new(); + stack.push(lit_layer("m1", 1.0)); + // No adjustment, so this changes no pixel and would ordinarily render + // through no slot at all. + stack.push(MaskLayer::new("m2", MaskSource::brush())); + + let reveal = Reveal { + layer: "m2".into(), + style: RevealStyle::Alpha, + }; + let shader = compose_layers_revealing(&stack, Some(&reveal)); + + assert_eq!( + stack.rendered_count(Some(&reveal)), + 2, + "the layer being looked at renders alongside the active one" + ); + assert!( + shader.reveal.contains("sample_mask(uv_src, 1)"), + "the reveal must read slot 1, which is where m2 renders" + ); + assert!( + shader.reveal.contains("u.mask1_opacity"), + "and shape it with that layer's own uniforms, not another's" + ); + } + + /// A composition nobody asked to see a mask through draws none. + #[test] + fn nothing_is_revealed_unless_it_was_asked_for() { + let mut stack = MaskStack::new(); + stack.push(lit_layer("m1", 1.0)); + assert!(compose_layers_revealing(&stack, None).reveal.is_empty()); + } + + /// A reveal aimed at a layer that is not in the stack is not a slot, and + /// must not become one. + #[test] + fn a_reveal_naming_no_layer_reveals_nothing() { + let mut stack = MaskStack::new(); + stack.push(lit_layer("m1", 1.0)); + let reveal = Reveal { + layer: "gone".into(), + style: RevealStyle::Tint, + }; + assert_eq!(stack.rendered_count(Some(&reveal)), 1); + assert!(compose_layers_revealing(&stack, Some(&reveal)) + .reveal + .is_empty()); + } + #[test] fn each_layer_gets_its_own_uniforms() { let mut stack = MaskStack::new(); stack.push(lit_layer("m1", 1.0)); stack.push(lit_layer("m2", -1.0)); - let shader = compose_layers(&stack); + let shader = compose_layers_revealing(&stack, None); assert!(shader.uniform_fields.contains("mask0_exposure_")); assert!(shader.uniform_fields.contains("mask1_exposure_")); assert_eq!( @@ -2156,7 +2377,7 @@ mod tests { fn the_inner_block_shadows_c_and_copies_back() { let mut stack = MaskStack::new(); stack.push(lit_layer("m1", 1.0)); - let body = compose_layers(&stack).body; + let body = compose_layers_revealing(&stack, None).body; assert!(body.contains("var masked = c;")); assert!(body.contains("var c = masked;")); diff --git a/core/dr-pipeline/src/operation.rs b/core/dr-pipeline/src/operation.rs index 03d2fc0..3eaf4d3 100644 --- a/core/dr-pipeline/src/operation.rs +++ b/core/dr-pipeline/src/operation.rs @@ -551,6 +551,28 @@ pub fn compose_full( masks: &MaskStack, spots: &crate::spot::SpotSet, warps: &[Box], +) -> ComposedShader { + compose_full_revealing(ops, framing, output, masks, spots, warps, None) +} + +/// TRACES: FR-DEV-19c +/// [`compose_full`], with one layer's mask drawn over the finished picture. +/// +/// Separate from [`compose_full`] rather than an argument on it, and that is +/// the safety property rather than a convenience: the reveal is a thing the +/// screen does, and every other consumer of the pipeline — the exporter, the +/// thumbnail, the neutral probe — calls the function that has no way to ask +/// for it. A flag reachable from the graph would have been one forgotten reset +/// away from a red tint baked into an exported file. +#[allow(clippy::too_many_arguments)] +pub fn compose_full_revealing( + ops: &[Box], + framing: &Framing, + output: ColourSpace, + masks: &MaskStack, + spots: &crate::spot::SpotSet, + warps: &[Box], + reveal: Option<&crate::mask::Reveal>, ) -> ComposedShader { // The lens corrections, composed into one coordinate transform. Beside // `framing` because they are the other half of the same stage: framing @@ -715,7 +737,13 @@ pub fn compose_full( // from. Their uniforms follow the global ops' in the block for the same // reason those follow framing's — slot order is emission order, and // nothing addresses a slot by number. - let layers = crate::mask::compose_layers(masks); + let layers = crate::mask::compose_layers_revealing(masks, reveal); + // TRACES: FR-DEV-19c + // Held apart from the body, because it belongs after the output transform + // rather than among the operations — see `mask::LayerShader::reveal`. + // Empty for every composition nobody is looking at a mask through, which + // is all of them but the screen's. + let reveal_block = layers.reveal.clone(); uniform_fields.push_str(&layers.uniform_fields); uniform_values.extend_from_slice(&layers.uniform_values); body.push_str(&layers.body); @@ -975,7 +1003,7 @@ fn main(@builtin(global_invocation_id) gid: vec3) {{ c = mix(c, neutral, clipped); }} {body} -{rendering_tail}{to_output} +{rendering_tail}{to_output}{reveal_block} {store} }} ", @@ -1569,6 +1597,56 @@ mod tests { ); } + /// TRACES: FR-DEV-19c + /// **The property that keeps a reveal off an exported file.** + /// + /// `compose_full` is the entry point the exporter, the thumbnail and the + /// neutral probe all use, and it has no argument that could ask for a + /// mask overlay. Only `compose_full_revealing` does, and only the canvas + /// calls it. Asserted rather than left to the type signature because the + /// tempting simplification — a flag on the graph — would type-check, be + /// shorter, and bake a red tint into every file the photographer sold. + #[test] + fn an_ordinary_composition_cannot_draw_a_mask_over_the_picture() { + use crate::mask::{MaskLayer, MaskSource, Reveal, RevealStyle}; + + let mut stack = MaskStack::new(); + let mut layer = MaskLayer::new("m1", MaskSource::brush()); + layer.set_param("exposure", crate::descriptor::ParamId("exposure"), 1.0); + stack.push(layer); + + let plain = compose_full( + &crate::ops::chain(), + &Framing::new(), + ColourSpace::Srgb, + &stack, + &crate::spot::SpotSet::new(), + &[], + ); + assert!( + !plain.source.contains("==== showing mask"), + "an export must never carry the overlay" + ); + + let shown = compose_full_revealing( + &crate::ops::chain(), + &Framing::new(), + ColourSpace::Srgb, + &stack, + &crate::spot::SpotSet::new(), + &[], + Some(&Reveal { + layer: "m1".into(), + style: RevealStyle::Tint, + }), + ); + assert!(shown.source.contains("==== showing mask")); + assert_ne!( + plain.structure_hash, shown.structure_hash, + "two different shaders must not share a pipeline cache entry" + ); + } + /// Distortion alone samples once; chromatic aberration samples three times. /// /// `splits_channels` is the whole reason for this test. Lateral CA fetches diff --git a/docs/mask-editing.md b/docs/mask-editing.md index 9c5953a..147b24f 100644 --- a/docs/mask-editing.md +++ b/docs/mask-editing.md @@ -36,12 +36,18 @@ the *whole* boundary. When the model's coverage stops two pixels inside the shoulder and leaks four pixels into the hair, no global number fixes both, and that is the ordinary case rather than a corner one. -**And you cannot see the mask.** The overlay on the canvas is -[`overlay_rgba`](../ui/dr-ui/src/segmentation.rs) — a CPU-built false-colour -picture of *what the model detected*, at proxy resolution. It is not the -layer's alpha: it knows nothing of the layer's feather, its falloff, its -morphology, its invert, or its opacity. Nobody can refine an edge they are not -being shown. +**And you cannot see the mask.** *(Built — see §6.)* The overlay on the canvas +was [`overlay_rgba`](../ui/dr-ui/src/segmentation.rs) and nothing else — a +CPU-built false-colour picture of *what the model detected*, at proxy +resolution. It is not the layer's alpha: it knows nothing of the layer's +feather, its falloff, its morphology, its invert, or its opacity. Nobody can +refine an edge they are not being shown. + +That one turned out to be load-bearing for the other three rather than the +last of four. With no way to see a mask, choosing a category produced a layer +whose extent was invisible and whose adjustment had not been touched yet — so +the correct behaviour and the broken one look identical, and "the segmentation +does not make masks" is what it reads as from the outside. Those four are one feature, and this is its specification. @@ -390,31 +396,89 @@ usable along hair. ## 6. Seeing the mask -The mask array is **already bound to the composed adjust shader** — this is -almost free, and it is the first thing to build, because every other tool here +**Status: built.** `MaskStack::rendered`, `RevealStyle`, +`EditGraph::compose_revealing`, `MaskPass::render_revealing`, and the "Show +mask" strip in the Local panel. + +The mask array is **already bound to the composed adjust shader**, so this is +almost free, and it is the first thing to build because every other tool here is unusable without it. -Two uniforms in the composed shader: which layer to reveal (−1 for none) and -which style. The block is always emitted, guarded by the uniform, so switching -the overlay on is a uniform write rather than a shader recompile. +### 6.1 One correction to this section, found in the building + +The draft said "two uniforms — which layer to reveal (−1 for none) and which +style — always emitted and guarded by the uniform, so switching the overlay on +is a uniform write rather than a shader recompile". The second half of that is +not available, and the reason is the case the feature exists for. + +A uniform can *select* a slot. It cannot conjure one. A layer with no +adjustment on it changes no pixel, so it is not `is_active`, so it occupies no +slice of the mask array and the rasteriser never draws it — and that is +precisely the layer a photographer wants to look at, for the whole of the time +between choosing a subject and deciding what to do to it. Revealing it means +*rendering* it, which changes the sequence of layers, which changes the +uniform block. The composition moves either way. + +So the slot and the style are written into the source, and turning the reveal +on, off, or onto another layer recompiles the fused shader. That is a button +press rather than a frame, and the uniform would only have added a branch per +pixel on top of a recomposition that was happening anyway. + +`MaskStack::rendered(reveal)` is the one sequence this rests on: `active()` +plus the layer being looked at. The rasteriser, the composer and the distance +field builder all index by position in it, so all three must be given the same +`reveal` — two of them disagreeing shows as an adjustment applied through +another layer's mask, which is why they take it as an argument rather than +reading a flag. + +### 6.2 Where the block runs, and why not with the others + +After the output transform, immediately before the clip and the encode — not +among the layer blocks. Everything there runs on scene-referred colour in the +working space, where a flat tint would be pushed through the base curve and +the camera matrix and arrive as some other colour, and an alpha's white on +black would arrive as neither. + +### 6.3 Not on the graph + +The reveal is an argument to `EditGraph::compose_revealing`, and +`compose_for` — which the exporter, the thumbnail and the neutral probe all +call — has no way to ask for one. A flag on the graph would have been fewer +parameters, would have type-checked, and would have been one forgotten reset +away from a red tint baked into an exported file. Three styles, all read from the same alpha: - **Tint** — the mask over the picture in a flat colour at ~50%. The default, - and what every editor's photographers already expect. Red by default and - configurable, since a red tint over a red dress shows nothing. + and what every editor's photographers already expect. Red, and *not yet* + configurable: the case for choosing the colour is a red tint over a red + dress, and the answer to that is the Alpha style beside it until somebody + finds a picture where neither works. - **Alpha** — the mask alone, white on black. For judging an edge, where a tint over a busy picture cannot be read. - **Edge** — the boundary outlined over the untouched picture. For checking registration against detail the other two hide, and the same reasoning the region overlay's white outline already carries. -**When it appears.** Automatically while a mask tool is armed, while a part -row is selected, and for ~1s after a shaping slider is released; manually from -a control in the Local panel. The existing `overlay-hidden` property is the -precedent and the trap it documents applies unchanged: the photographer's -switch and the automatic reveal are separate questions, and an automatic -reveal must never silently re-arm a switch the photographer turned off. +**When it appears.** From the "Show mask" strip in the Local panel, which sits +beside the tool strip and under the same condition — both describe one +selected mask. Off is the resting state, so entering local mode does not paint +a photograph red. + +Automatic in exactly one place: arming Paint or Erase turns the tint on if +nothing was showing the mask, which is the same nudge `on_part_added` makes and +on the same argument — a stroke into an invisible mask is indistinguishable +from a tool that did nothing. + +A nudge on an explicit action, never a standing rule. Turning the view off and +then picking the eraser leaves it off. The existing `overlay-hidden` property +is the precedent and the trap it documents applies unchanged: an automatic +reveal that re-arms a switch somebody turned off is worse than no automatic +reveal at all. + +The two remaining triggers in the draft — while a part row is selected, and for +~1s after a shaping slider is released — are not built. The second is the one +worth returning to, and it is a timer rather than a decision. The region overlay stays exactly what it is — a picture of what the model detected — and gains a name in the interface that says so, because two @@ -597,6 +661,19 @@ the mask pass beyond the blend variants; no change to distance fields, because a painted part needs none. *This is the whole of the user-visible ask except push and intersect,* and it is deliberately the milestone that stands alone. +Done: parts with `Union` and `Subtract`, painted parts, the tool strip, the +canvas gesture, and the overlay (§6). Outstanding: the brush HUD and cursor — +radius, hardness and flow are sliders with no ring drawn on the photograph, +so the size of the brush is a number rather than a thing you can see — and +the incremental raster of §5.4, without which a layer's whole stroke history +is redrawn per dab. + +One lesson from the order it was actually built in, since §6 said it and the +build did not listen: the overlay is not the last quarter of M1, it is the +first. Parts and painting shipped without it and the result was a feature +nobody could tell was working — the panel listed a mask, the photograph showed +nothing, and every report of it came back as "the masks do not work". + **M2 — Combine properly.** `Intersect`, model and gradient and range parts, per-part distance fields (§5.5), the part list with its join chips, folding two layers. diff --git a/docs/requirements.md b/docs/requirements.md index b8ef165..1996bde 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -517,6 +517,20 @@ hardness and flow the photographer sets. Strokes are stored as normalised source rasterised on the device, so a correction stays on what it was painted on through a crop, a zoom and an export at any size. A whole stroke is one step in the history. +**FR-DEV-19c — Seeing the mask.** The mask a selected layer actually resolves to shall be drawable +over the photograph, in a tint, as an alpha, or as an outline. What is shown is the finished mask — +every part folded, with the layer's feather, falloff, morphology, invert and opacity applied — and +it is shown for a layer that carries no adjustment yet, which is the state every mask is in for its +first few seconds. No rendered output ever carries it: the reveal is a property of looking at an +edit, not of the edit, so it reaches the pipeline through the composition that draws the canvas and +through no other. + +Nobody can refine an edge they are not being shown. Before this the only thing drawn on the +photograph was FR-DEV-3's region overlay — a false-coloured picture of what the *model detected*, +which knows nothing of a layer's shaping and nothing at all about a gradient, a range or a stroke — +so choosing a subject or a category produced a layer whose extent was invisible, and every control +in FR-DEV-19a and FR-DEV-19b acted on something the photographer could not see. + ### 3.4 Display and interaction **FR-DSP-1 — Proxy-resolution rendering.** The develop view renders at the resolution actually diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index 857f829..bd21c9d 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -865,6 +865,20 @@ pub struct DevelopSession { selected_spot: Option, /// Whether to draw the false-coloured region overlay. show_overlay: bool, + /// TRACES: FR-DEV-19c + /// How the selected layer's mask is being shown, or `None` for not at all. + /// + /// Interface state, like `show_overlay` beside it and `active_masks` above + /// — it changes no pixel of the photograph, it is not in the sidecar and + /// it is not on the undo stack. It reaches the pipeline as an argument to + /// the one composition that draws the canvas, which is what makes an + /// export structurally unable to carry it (`EditGraph::compose_revealing`). + /// + /// The *style* is stored and not the layer, because the layer is always + /// the selected one: a reveal that stayed pointed at a mask nobody was + /// working on would be describing the wrong thing every time the selection + /// moved, and there is no gesture that wants it. + reveal_style: Option, /// Which attribute the panel is filtered to, or all of them. /// /// `None` is "show everything" and is what a frontend that ignores @@ -1020,6 +1034,7 @@ impl DevelopSession { ), selected_spot: None, show_overlay: false, + reveal_style: None, active_tab: None, curve_channel: 0, display_space: dr_types::ColourSpace::Srgb, @@ -1958,7 +1973,22 @@ impl DevelopSession { } }; - for layer in self.graph.masks().active() { + // TRACES: FR-DEV-19c + // The reveal is in the key because it is in the *sequence*: revealing + // a layer with no adjustment on it gives that layer a slot, which + // renumbers every field after it. Leaving it out is the bug where + // clicking a subject shows the mask of whichever layer happened to be + // beneath it. + let reveal = self.reveal(); + mix(reveal.as_ref().map_or(0, |r| { + let mut h: u64 = 1; + for b in r.layer.as_bytes() { + h = h.wrapping_mul(31).wrapping_add(*b as u64); + } + h + })); + + for layer in self.graph.masks().rendered(reveal.as_ref()) { match &layer.base().source { MaskSource::Subject { index, .. } => { mix(1); @@ -2147,10 +2177,13 @@ impl DevelopSession { } }; - // In `active()` order, because that is the order the rasteriser walks - // and the order it indexes these by. + // In `rendered()` order, because that is the order the rasteriser + // walks and the order it indexes these by — the revealed layer + // included, which is why the reveal is asked for here and folded into + // the key above. + let reveal = self.reveal(); let mut fields: Vec> = Vec::new(); - for layer in self.graph.masks().active() { + for layer in self.graph.masks().rendered(reveal.as_ref()) { let field = match self.layer_coverage(layer, pw, ph) { Some(coverage) => { dr_segment::Shaped::build( @@ -2193,7 +2226,16 @@ impl DevelopSession { } fn rasterise_masks(&mut self) -> bool { - if self.graph.masks().is_neutral() { + // TRACES: FR-DEV-19c + // A stack that changes no pixel is normally not worth a pass — except + // when one of its layers is being looked at, which is exactly the + // state a fresh selection is in. Asking `rendered_count` rather than + // `is_neutral` is what makes "click a category, see its mask" work at + // all: before it, the array was never rasterised, the slice the reveal + // samples held whatever was last in it, and the answer was a blank + // photograph. + let reveal = self.reveal(); + if self.graph.masks().rendered_count(reveal.as_ref()) == 0 { return false; } // **Source space, at the segmentation's proxy size** — not the @@ -2239,13 +2281,14 @@ impl DevelopSession { // No label field: region masks were the watershed's, and nothing // produces one any more. A stored layer that still names regions is // skipped by the rasteriser rather than drawn wrong. - pass.render( + pass.render_revealing( self.graph.masks(), None, subjects, Some(source.as_ref()), pw, ph, + reveal.as_ref(), ) .inspect_err(|e| log::warn!("mask rasterisation failed: {e}")) .is_ok() @@ -2426,6 +2469,60 @@ impl DevelopSession { .unwrap_or_default() } + // ---------------------------------------------------------------------- + // Seeing the mask (FR-DEV-19c) + // ---------------------------------------------------------------------- + + /// TRACES: FR-DEV-19c + /// What the canvas should draw over the photograph, if anything. + /// + /// **Only when exactly one layer is selected**, on the same rule the part + /// list and the brush follow: a mask is one thing, and drawing the union + /// of three because three rows were control-clicked would answer a + /// question nobody asked. `None` there rather than picking the first, so + /// the reveal disappearing is itself the signal that the selection is not + /// what it needs to be. + /// + /// Rebuilt per call rather than kept in step with the selection, because + /// it is two fields and a clone of one id — cheaper than the invalidation + /// a cached copy would need every time a layer is added, removed, renamed + /// or reordered. + pub(crate) fn reveal(&self) -> Option { + let style = self.reveal_style?; + let [id] = self.active_masks.as_slice() else { + return None; + }; + Some(dr_pipeline::mask::Reveal { + layer: id.clone(), + style, + }) + } + + /// How the selected layer's mask is being shown, as an index into + /// [`dr_pipeline::mask::RevealStyle::ALL`], or `0` for not at all. + /// + /// An index because the panel offers it as a strip of chips and an index + /// is what a strip of chips reports. The enum stays the thing that is + /// stored, so a fourth style is a variant and a label rather than a number + /// two files have to agree on. + pub fn mask_view(&self) -> usize { + use dr_pipeline::mask::RevealStyle; + self.reveal_style + .and_then(|s| RevealStyle::ALL.iter().position(|&a| a == s)) + .map_or(0, |i| i + 1) + } + + /// Show the selected layer's mask, or stop. + /// + /// Takes no history step and marks nothing dirty: this is how the + /// photograph is being *looked at*, not an edit to it. + pub fn set_mask_view(&mut self, view: usize) { + use dr_pipeline::mask::RevealStyle; + self.reveal_style = view + .checked_sub(1) + .and_then(|i| RevealStyle::ALL.get(i).copied()); + } + // ---------------------------------------------------------------------- // The region overlay // ---------------------------------------------------------------------- @@ -3738,7 +3835,12 @@ impl DevelopSession { // always entered the structure hash. Moving the window to a P3 panel // therefore costs one shader compile and no pipeline change at all. let space = self.display_space; - let shader = self.graph.compose_for(space); + // TRACES: FR-DEV-19c + // **The one composition that may show a mask.** Every other caller of + // the graph — `render_the_file`, the thumbnail, `sample_as_shot` — + // goes through `compose_for`, which cannot ask for a reveal, so no + // exported file can carry one. + let shader = self.graph.compose_revealing(space, self.reveal().as_ref()); // Rasterise the masks first: the shader addresses array slices by // index, so the array has to describe *this* stack before it is bound. diff --git a/ui/dr-ui/src/masks_ui.rs b/ui/dr-ui/src/masks_ui.rs index c1a15ae..c777b7b 100644 --- a/ui/dr-ui/src/masks_ui.rs +++ b/ui/dr-ui/src/masks_ui.rs @@ -264,6 +264,12 @@ pub(crate) fn sync(window: &AppWindow, session: &Rc { window.set_region_overlay(image); window.set_overlay_on(true); @@ -558,6 +573,7 @@ pub(crate) fn wire( { let weak = window.as_weak(); let session = session.clone(); + let redraw = redraw.clone(); let rows = rows.clone(); window .global::() @@ -587,6 +603,11 @@ pub(crate) fn wire( // The scope changed, so the adjust panel below is now describing a // different chain. sync_rows(&w, &rows, &session); + // TRACES: FR-DEV-19c + // And so has the picture, when a mask is being shown: the + // reveal follows the selection, so choosing another layer + // draws another mask. + redraw(&w); }); } { @@ -841,13 +862,66 @@ pub(crate) fn wire( { let weak = window.as_weak(); let session = session.clone(); - window.global::().on_tool_picked(move |_tool| { - // The tool itself lives in the interface — it arms a gesture and - // changes no pixel — so nothing is set here. What this does is - // re-sync, because arming the brush is what makes the parts of a - // mask worth showing. + let redraw = redraw.clone(); + window.global::().on_tool_picked(move |tool| { + // **The tool is written back here, and that is the whole of this + // handler.** `Masking.tool` is an `in` property: the panel reads + // it to light the right chip and `app.slint` reads it to decide + // whether a drag on the photograph paints, but only Rust may write + // it. So a version of this that recorded nothing left the strip + // reporting "Select" however many times "Paint" was pressed, and + // the paint area was never armed — the brush, the parts, the whole + // of FR-DEV-19b, reachable from no control in the application. + // + // It still changes no pixel, which is what the previous note was + // getting at: the tool arms a gesture and belongs to the + // interface, not to the edit, so it takes no history step and is + // not stored. let Some(w) = weak.upgrade() else { return }; + let tool = tool.clamp(0, 2); + w.global::().set_tool(tool); + + // TRACES: FR-DEV-19c + // Arming a brush shows the mask, if nothing was showing it. The + // same nudge `on_part_added` makes and on the same argument: a + // photographer about to correct an edge by hand needs to see the + // edge, and "the tool did nothing" is what a stroke into an + // invisible mask looks like. + // + // A nudge on an explicit action, never a standing rule. Turning + // the view off and then picking the eraser leaves it off — the + // trap `overlay-hidden` documents is that an automatic reveal + // which re-arms a switch somebody turned off is worse than no + // automatic reveal at all. + if tool > 0 { + if let Some(s) = session.borrow_mut().as_mut() { + if s.mask_view() == 0 { + s.set_mask_view(1); + } + } + } + + // Re-synced because arming the brush is what makes the parts of a + // mask worth showing. sync(&w, &session); + redraw(&w); + }); + } + // TRACES: FR-DEV-19c + { + let weak = window.as_weak(); + let session = session.clone(); + let redraw = redraw.clone(); + window.global::().on_mask_view_picked(move |view| { + let Some(w) = weak.upgrade() else { return }; + if let Some(s) = session.borrow_mut().as_mut() { + s.set_mask_view(view.max(0) as usize); + } + // A redraw and not only a sync: the reveal is in the composed + // shader, so what changed is the picture rather than the + // panel. + sync(&w, &session); + redraw(&w); }); } // TRACES: FR-DEV-19a @@ -1127,6 +1201,12 @@ pub(crate) fn reset(window: &AppWindow) { masking.set_segmenting(false); masking.set_refining(false); masking.set_segmented(false); + // TRACES: FR-DEV-19b | FR-DEV-19c + // The tool and the mask view are both about one selected layer, and the + // next photograph has none. Left standing, they would arm a brush over a + // photograph with nothing to paint into and claim a mask was being shown. + masking.set_tool(0); + masking.set_mask_view(0); masking.set_masks(ModelRc::new(VecModel::::default())); masking.set_subjects(ModelRc::new(VecModel::::default())); masking.set_categories(ModelRc::new(VecModel::::default())); diff --git a/ui/dr-ui/ui/masks.slint b/ui/dr-ui/ui/masks.slint index f236fda..f896611 100644 --- a/ui/dr-ui/ui/masks.slint +++ b/ui/dr-ui/ui/masks.slint @@ -582,7 +582,23 @@ export global Masking { in property <[PartRow]> parts; /// TRACES: FR-DEV-19b /// What a drag on the photograph does: 0 selects, 1 paints, 2 erases. + /// + /// Written by Rust and read here, like every other `in` property on this + /// global: `tool-picked` is what the strip reports, and the answer comes + /// back through this. A version of the handler that recorded nothing left + /// the strip stuck on "Select" and the brush reachable from no control. in property tool: 0; + /// TRACES: FR-DEV-19c + /// How the selected layer's mask is drawn over the photograph: 0 not at + /// all, then tint, alpha and edge — `dr_pipeline::mask::RevealStyle::ALL` + /// shifted by one so that zero can mean off. + /// + /// **Not `overlay-hidden`.** That switch belongs to the region overlay, + /// which is a picture of what the *model detected*; this is the mask a + /// layer actually resolves to, feather, morphology, invert and all. Two + /// overlays that look alike and mean different things is worse than + /// either, so they are named apart and switched apart. + in property mask-view: 0; /// The brush: radius as a fraction of the frame's shorter edge, hardness, /// and flow. in property brush-radius: 0.05; @@ -630,6 +646,8 @@ export global Masking { /// TRACES: FR-DEV-19b callback tool-picked(int); + /// TRACES: FR-DEV-19c + callback mask-view-picked(int); callback brush-changed(float, float, float); /// TRACES: FR-DEV-19a callback part-selected(string, int); @@ -638,6 +656,8 @@ export global Masking { callback part-added(string, int); callback add-gradient(bool); + /// TRACES: FR-DEV-19b + callback add-brush(); /// `true` asks for the colour range rather than the brightness one. callback add-range(bool); callback add-subject(int); @@ -680,6 +700,32 @@ export component MaskPanel inherits Rectangle { picked(i) => { Masking.tool-picked(i); } } + // TRACES: FR-DEV-19c + // **Seeing the mask, which every tool above is unusable without.** + // Nobody can refine an edge they are not being shown, and until this + // existed the only thing drawn on the photograph was the model's + // detections — which know nothing of a layer's feather, its falloff, + // its morphology, its invert or its opacity, and nothing at all about + // a gradient or a stroke. + // + // Four chips rather than a toggle because the three styles answer + // three different questions and no one of them answers all three: a + // tint says whether the right thing is selected, an alpha says where + // the edge is, an outline says whether that edge is registered against + // detail the other two hide. + // + // Beside the tool strip and under the same condition, because both + // describe one selected mask — and `columns: 2` for the reason + // `ChipGrid` exists: four chips in a row would set the width of the + // develop column and take every other panel's controls off the edge. + if Develop.enabled && Masking.parts.length > 0: Segmented { + label: "Show mask"; + options: ["Off", "Tint", "Alpha", "Edge"]; + columns: 2; + selected: Masking.mask-view; + picked(i) => { Masking.mask-view-picked(i); } + } + if Develop.enabled && Masking.tool > 0: SliderRow { label: "Size"; value: Masking.brush-radius; @@ -752,10 +798,18 @@ export component MaskPanel inherits Rectangle { // a mask is judged against the photograph under it, and you cannot see // that photograph through the thing describing it. // - // Reads as its own action rather than its state: "Show the mask" is - // what pressing it will do, not what is currently true. + // Reads as its own action rather than its state: "Show what was found" + // is what pressing it will do, not what is currently true. + // + // TRACES: FR-DEV-19c + // **Named for what it actually hides**, which is not the mask. This + // said "Show the mask" / "Hide the mask" while switching the + // false-coloured picture of what the *model detected* — and now that + // there is a control which really does show a mask ("Show mask", + // above), two things called the same thing and meaning different ones + // would be worse than either. if Develop.enabled && Masking.segmented: Button { - text: Masking.overlay-hidden ? "Show the mask" : "Hide the mask"; + text: Masking.overlay-hidden ? "Show what was found" : "Hide what was found"; clicked => { Masking.overlay-hidden = !Masking.overlay-hidden; } }