From c045702a47f09b9644712f1b8a2604320f96bdbc Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Thu, 10 Sep 2026 20:28:06 +0200 Subject: [PATCH] Show the photographer the mask they are shaping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nobody can refine an edge they are not being shown. The only thing drawn on the canvas was the region overlay — a false-coloured picture of what the model *detected* — which knows nothing of a layer's feather, its falloff, its morphology, its invert or its opacity, and nothing at all about a gradient, a range or a stroke. Every control added for mask editing therefore acted on something invisible, which is why the whole feature reads as absent rather than as unfinished. A layer's finished mask now draws over the photograph in one of three styles: a tint for whether the right thing is selected, an alpha for where the edge is, an outline for whether that edge is registered against the detail the other two hide. The hard part is not the shader. A selection with no adjustment on it changes no pixel, so it is not active, so it holds no slice of the mask array and is never rasterised — and that is exactly the layer somebody wants to look at, for the whole of the time between choosing a subject and deciding what to do to it. So `MaskStack::rendered` is `active()` plus the layer being looked at, and the rasteriser, the composer and the distance-field builder all index by position in it. Which is also why the design's "two uniforms, no recompile" is not available: a uniform can select a slot, it cannot conjure one. The reveal is never on the graph. It reaches the pipeline as an argument to `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 shorter, would have type-checked, and would have been one forgotten reset away from a red tint baked into an exported file. And the tools that shape a mask now arm. `Masking.tool` is an `in` property only Rust may write, and the handler wrote nothing back, so the strip reported "Select" however many times Paint was pressed and the paint area was never enabled — the brush, the parts and the whole of FR-DEV-19b reachable from no control in the application. The region overlay stands down while a mask is being shown, and its button now says what it hides: two overlays that look alike and mean different things is worse than either. --- core/dr-gpu/src/mask.rs | 30 ++- core/dr-gpu/tests/local_adjustments.rs | 199 +++++++++++++++++++- core/dr-pipeline/src/graph.rs | 29 +++ core/dr-pipeline/src/mask.rs | 241 ++++++++++++++++++++++++- core/dr-pipeline/src/operation.rs | 82 ++++++++- docs/mask-editing.md | 115 ++++++++++-- docs/requirements.md | 14 ++ ui/dr-ui/src/develop.rs | 116 +++++++++++- ui/dr-ui/src/masks_ui.rs | 92 +++++++++- ui/dr-ui/ui/masks.slint | 60 +++++- 10 files changed, 927 insertions(+), 51 deletions(-) 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; } }