diff --git a/core/dr-pipeline/src/framing.rs b/core/dr-pipeline/src/framing.rs index 2bb5294..accde58 100644 --- a/core/dr-pipeline/src/framing.rs +++ b/core/dr-pipeline/src/framing.rs @@ -41,8 +41,10 @@ use std::f32::consts::PI; use std::fmt::Write as _; -use crate::descriptor::{Attribute, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Presentation, Scale, Unit, - WidgetDemand, WidgetKind,}; +use crate::descriptor::{ + Attribute, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Presentation, Scale, + Unit, WidgetDemand, WidgetKind, +}; use crate::operation::Affects; pub const ID: OpId = OpId("framing"); @@ -597,6 +599,105 @@ impl Framing { .normalised() } + /// TRACES: FR-DEV-3 + /// Where an output point comes from in the source, both in normalised + /// `0..1` coordinates. + /// + /// **This is [`Self::wgsl_prologue`] evaluated on the CPU**, for the one + /// caller that cannot run the shader: an interface hit-testing a control + /// drawn *on the photograph*. A gradient's handles are stored in source + /// coordinates and dragged in output ones, and the two are separated by + /// the crop, the zoom, the pan, the straightening and the turns — so a + /// handle that mapped through anything less would drift off the mask the + /// moment the view moved, which is exactly the fault masks are rasterised + /// in source space to avoid. + /// + /// The two must agree step for step. They are kept together in this file, + /// and `the_cpu_map_matches_the_prologue_step_for_step` below pins the + /// correspondence so a change to one that is not made to the other fails + /// rather than showing up as a mask that is subtly wrong only when + /// straightened. + pub fn source_at(&self, out: (f32, f32), src_w: u32, src_h: u32) -> (f32, f32) { + let (ax, fx) = self.aspects(src_w, src_h); + let rect = self.visible_rect(); + + // Into the crop rect, then into the framed image's own centred space. + let uv = (rect.x + out.0 * rect.width, rect.y + out.1 * rect.height); + let mut p = ((uv.0 - 0.5) * fx, uv.1 - 0.5); + + if self.angle != 0.0 { + let rad = self.angle * PI / 180.0; + let (s, c) = (rad.sin(), rad.cos()); + p = (p.0 * c - p.1 * s, p.0 * s + p.1 * c); + } + + let (turns, flip_h, flip_v) = self.effective(); + p = match turns { + 1 => (p.1 * ax, -p.0 / fx), + 2 => (-p.0, -p.1), + 3 => (-p.1 * ax, p.0 / fx), + _ => p, + }; + if flip_h { + p.0 = -p.0; + } + if flip_v { + p.1 = -p.1; + } + + (p.0 / ax + 0.5, p.1 + 0.5) + } + + /// Where a source point lands on the output — [`Self::source_at`] run + /// backwards. + /// + /// Outside `0..1` when the point is cropped away or panned off screen, + /// which is the honest answer: the caller draws a handle there and clips + /// it, rather than being handed a clamped position that claims the mask is + /// somewhere it is not. + pub fn output_at(&self, src: (f32, f32), src_w: u32, src_h: u32) -> (f32, f32) { + let (ax, fx) = self.aspects(src_w, src_h); + let mut p = ((src.0 - 0.5) * ax, src.1 - 0.5); + + let (turns, flip_h, flip_v) = self.effective(); + if flip_v { + p.1 = -p.1; + } + if flip_h { + p.0 = -p.0; + } + p = match turns { + 1 => (-p.1 * fx, p.0 / ax), + 2 => (-p.0, -p.1), + 3 => (p.1 * fx, -p.0 / ax), + _ => p, + }; + + if self.angle != 0.0 { + let rad = -self.angle * PI / 180.0; + let (s, c) = (rad.sin(), rad.cos()); + p = (p.0 * c - p.1 * s, p.0 * s + p.1 * c); + } + + let uv = (p.0 / fx + 0.5, p.1 + 0.5); + let rect = self.visible_rect(); + ( + (uv.0 - rect.x) / rect.width.max(1e-6), + (uv.1 - rect.y) / rect.height.max(1e-6), + ) + } + + /// The x components of `aspect` and `frame_aspect`, whose y is always 1. + /// + /// The pair the prologue puts in scope, and the distinction that makes a + /// quarter turn exact: `aspect` measures the source, `frame_aspect` + /// measures the frame the user is looking at, and a turn is where the two + /// meet. + fn aspects(&self, src_w: u32, src_h: u32) -> (f32, f32) { + let ax = src_w.max(1) as f32 / src_h.max(1) as f32; + (ax, if self.swaps_axes() { 1.0 / ax } else { ax }) + } + /// Uniform values the generated prologue reads. /// /// A fixed-size block in a fixed slot, like the camera matrix: the @@ -1591,4 +1692,141 @@ mod tests { } } } + + // ---- the CPU coordinate map (source_at / output_at) ------------------- + // + // These matter because the map has no other check on it. The shader's + // version is verified by the picture looking right; this one is read by + // hit-testing, where being wrong means a handle that grabs nothing and + // nothing on screen says why. + + /// A 3:2 frame. Square would hide every aspect fault in here. + const SRC: (u32, u32) = (600, 400); + + fn close(a: (f32, f32), b: (f32, f32), what: &str) { + assert!( + (a.0 - b.0).abs() < 1e-4 && (a.1 - b.1).abs() < 1e-4, + "{what}: {a:?} != {b:?}" + ); + } + + #[test] + fn an_unedited_frame_maps_an_output_point_to_itself() { + // The neutral prologue is `uv_src = uv`, and a map that quietly + // introduced an aspect factor here would put every mask a little off + // on every unedited photograph — the case that is never looked at + // twice. + let f = Framing::new(); + for out in [(0.0, 0.0), (0.5, 0.5), (0.25, 0.8), (1.0, 1.0)] { + close(f.source_at(out, SRC.0, SRC.1), out, "neutral"); + } + } + + #[test] + fn the_map_round_trips_through_every_transform_at_once() { + // Handles are drawn with `output_at` and dragged with `source_at`, so + // a discrepancy between them is a handle that jumps away from the + // pointer on the first press. Every stage is on, because the faults + // that survive are the ones only a composition exposes — an aspect + // applied on one leg and not the other cancels under a bare rotation. + for turns in 0..4 { + let mut f = Framing::new(); + f.set_crop(CropRect { + x: 0.1, + y: 0.2, + width: 0.6, + height: 0.5, + }); + f.set_view(CropRect { + x: 0.3, + y: 0.25, + width: 0.4, + height: 0.4, + }); + f.set_param(ANGLE, -7.5); + f.rotate_quarters(turns); + f.set_param(FLIP_H, 1.0); + f.set_param(FLIP_V, 1.0); + + for out in [(0.0, 0.0), (0.5, 0.5), (0.2, 0.9), (0.95, 0.05)] { + let src = f.source_at(out, SRC.0, SRC.1); + close(f.output_at(src, SRC.0, SRC.1), out, "round trip"); + } + } + } + + #[test] + fn a_stored_orientation_is_part_of_the_map() { + // The baseline reaches the prologue through `effective`, so it has to + // reach this the same way. A portrait frame the camera stored sideways + // is the common case, and a map that ignored the tag would place every + // handle on a photograph that is not the one on screen. + let mut f = Framing::new(); + f.set_baseline(dr_types::Orientation { + quarter_turns: 1, + flip_h: false, + flip_v: false, + }); + + // The output's top-left comes from the source's bottom-left under a + // clockwise quarter turn. + close(f.source_at((0.0, 0.0), SRC.0, SRC.1), (0.0, 1.0), "turned"); + close( + f.output_at((0.0, 1.0), SRC.0, SRC.1), + (0.0, 0.0), + "turned back", + ); + } + + #[test] + fn zooming_in_narrows_what_an_output_point_reaches() { + // The property the handles depend on: the same place on screen is a + // *different* source point once the view moves, so a handle drawn from + // stored geometry has to be re-placed on every frame of a pan. If this + // were independent of the view the handles would sit still while the + // photograph slid under them. + let mut f = Framing::new(); + let wide = f.source_at((0.25, 0.25), SRC.0, SRC.1); + + f.set_view(CropRect { + x: 0.25, + y: 0.25, + width: 0.5, + height: 0.5, + }); + let close_in = f.source_at((0.25, 0.25), SRC.0, SRC.1); + + assert!(close_in.0 > wide.0 && close_in.1 > wide.1, "{close_in:?}"); + close(close_in, (0.375, 0.375), "zoomed"); + } + + #[test] + fn the_cpu_map_matches_the_prologue_step_for_step() { + // The two are the same function written twice, and nothing but this + // stops them drifting apart. It checks the *shape* — that the + // permutation the prologue emits for each turn is the one implemented + // above — because the alternative is running WGSL in a unit test. + let mut f = Framing::new(); + f.rotate_quarters(1); + assert!( + f.wgsl_prologue() + .contains("p = vec2(p.y * aspect.x, -p.x / frame_aspect.x);"), + "the one-turn permutation moved; `source_at` must move with it" + ); + + let mut f = Framing::new(); + f.rotate_quarters(3); + assert!( + f.wgsl_prologue() + .contains("p = vec2(-p.y * aspect.x, p.x / frame_aspect.x);"), + "the three-turn permutation moved; `source_at` must move with it" + ); + + // And the sampler's last step, which lives in `operation.rs` and is + // the half of the map this file does not emit. + assert!( + crate::operation::sample_source(false).contains("p / aspect + vec2(0.5)"), + "the sampler's return to texture coordinates moved" + ); + } } diff --git a/core/dr-pipeline/src/operation.rs b/core/dr-pipeline/src/operation.rs index 3a5df12..c5f7b73 100644 --- a/core/dr-pipeline/src/operation.rs +++ b/core/dr-pipeline/src/operation.rs @@ -562,7 +562,7 @@ fn encode_output(c: vec3) -> vec3 {{ /// Split out because it is the join between the coordinate stage and the /// colour stage, and because the choice it makes — an exact integer load, or /// a filtered sample — is the one thing the free-angle case changes. -fn sample_source(interpolate: bool) -> &'static str { +pub(crate) fn sample_source(interpolate: bool) -> &'static str { if interpolate { " // Back to texture coordinates. let uv_src = p / aspect + vec2(0.5); diff --git a/docs/ui-navigation.md b/docs/ui-navigation.md index bf35cfe..0b1cdcb 100644 --- a/docs/ui-navigation.md +++ b/docs/ui-navigation.md @@ -187,10 +187,14 @@ throughout.** The consequences worth stating: - **No modifier key may be required.** A tablet has no shift. Local masking already lost its shift-click extend for this reason, and nothing should reintroduce one as the only route to a feature. -- **Anything dragged needs a finger-sized target.** This is the live one: - gradient masks have no on-canvas handles yet, and when they arrive their - handles are the first control in the app designed to be dragged on a - photograph rather than in a panel. +- **Anything dragged needs a finger-sized target.** ~~This is the live one: + gradient masks have no on-canvas handles yet~~ — they have them now (N2a), and + their handles are the first control in the app designed to be dragged on a + photograph rather than in a panel. Drawn at 14px so they do not hide the edge + they sit on, with a full touch target centred on the drawing, which is the + split `Button` already establishes. Nothing about them is revealed by hover + and nothing about them is qualified by a modifier: what is drawn is all there + is. ### D-N3 — Collapsible panels, not tool tabs · **OPEN** @@ -226,21 +230,59 @@ gain tabs, or stay a single collapsible stack indefinitely. Numbered N to avoid colliding with `ui-refinement.md`'s A–F. -### N1 — The mode strip +### N1 — The mode strip — **done** -**Deliverable.** One control naming the current mode, replacing the implicit +**Deliverable.** ~~One control naming the current mode, replacing the implicit `crop-mode` boolean: **Photo · Crop · Local**. Top of the canvas in expanded, -bottom in compact. The selected mode is the accent's job — it means *active*, +bottom in compact.~~ The selected mode is the accent's job — it means *active*, which is exactly this. `crop-mode` becomes one value of a mode enum rather than its own flag, so the two modes cannot both be on, which today they can. -**Done when.** Entering crop from the strip does what the crop button did; -Escape and back leave the innermost mode; no two modes are ever active -together. +**Landed as one strip, not two.** The mode control and the existing group strip +(`All · Light · Colour`, derived from operation attributes) were going to sit +beside each other above the same column, which is two controls answering one +question — *what am I working on*. They are now one: -### N2 — Local mode +``` +Crop · Local │ All Light Colour +``` + +Lightroom Mobile's bottom strip mixes Crop and Masking with Light and Colour +for the same reason, and it reads naturally because from the photographer's +side they are the same kind of choice. + +The two halves are **different kinds of state and are drawn differently**: a +mode is a chip that fills with the accent when it is on, a group is a word with +a rule under it. That is what lets both be read at once, and both are on at +once routinely — see below. + +**Mode and group are independent axes.** Picking `Light` while a mask layer is +selected filters *that layer's* chain and does not leave local mode. The +alternative — a group press quietly dropping the scope — would be §1.1's fault +reintroduced from the other end, and it would make `Light` mean two things +depending on where it was pressed. + +**Where it sits.** Pinned above the develop column, where the group strip +already was, rather than at the top of the canvas. The half that filters the +column belongs to the column, and moving it onto the photograph would put it +somewhere the four principles say chrome should not be. The canvas keeps one +button — now *"Done Cropping"* / *"Done Masking"*, naming the mode it leaves — +because the column can be closed on a narrow window and no mode may be +inescapable. + +**Also landed.** `GeometryPanel`'s Crop button is gone: a second control +entering the same mode is a second thing that has to agree about which mode the +view is in. + +**Done when.** ~~Entering crop from the strip does what the crop button did; +Escape and back leave the innermost mode; no two modes are ever active +together.~~ All three, checked on screen as well as in tests — `back_step` has +one `LeaveMode` step covering both modes, and the enum makes "no two at once" +unrepresentable rather than merely untested. + +### N2 — Local mode — **done** **Depends on** N1. @@ -252,12 +294,86 @@ names the layer. Leaving local mode clears the selection so the adjustments are unambiguously global again. -**Done when.** There is no way to have a mask selected without knowing it, and -the two toggles that currently arm the overlay and picking are gone. +**Landed.** The "Overlay" and "Select" buttons are gone; entering the mode does +both, and `region-picking` is now derived from the mode rather than toggled. +The masking panel is no longer a panel among peers in the scrolling column — it +appears only in local mode, which is what takes the column from six panels to +three there. + +**The scope is the adjust panel's own heading**, not a caption in the panel +above it. `ADJUST` becomes the layer's name. That is the difference between +describing the hazard and removing it: the heading of the thing that changed +cannot be skipped on the way to a slider, and a caption in a different panel +routinely was. + +**What local mode drops from the column**: the capture metadata, the framing +controls and copy/paste. None is a property of a region within the photograph, +so all three would be controls in scope of nothing. The histogram stays and +still reports the whole frame — the disagreement §1.1 names is real and is N3's +to close; removing the instrument would be a worse answer than an honest one +that is not yet scoped. + +**Done when.** ~~There is no way to have a mask selected without knowing it, and +the two toggles that currently arm the overlay and picking are gone.~~ Both. + +### N2a — Gradient handles — **done** + +Not a numbered workstream when this was written, and it belongs beside N2: the +canvas half of local mode. + +`MaskSource::Linear` and `MaskSource::Radial` could be created and then not +moved, so a radial sat at the centre of the frame at its default size for ever. +They now carry handles on the photograph — the first controls in the +application designed to be dragged there rather than in a panel, and D-N2's +"the live one". + +Three faults had to be fixed before a handle was worth drawing. + +**A gradient did not render at all until the model had run.** The mask +rasteriser was built on the way out of `segment`, and the array's size was read +*off* the segmentation, so a gradient added to an unsegmented photograph +produced nothing — silently, because the generated shader still emits the +layer's block and the empty placeholder multiplies it by zero. The proxy size +is a property of the photograph; both are now derived from it, deliberately at +the same size because a subject's distance field is sampled against the array. + +**A gradient's geometry was measured in raw `0..1` fractions**, so a 45° ramp +was not at 45° and a radial with equal radii drew an ellipse. Angles and +distances are now in the frame's isotropic units — y spans `0..1`, x spans +`0..aspect` — converted in exactly one place, `frame_delta` in `mask.wgsl`. +Only the *meaning* of the stored numbers changed; the sidecar format did not. + +**Hit-testing has to go through the framing map.** A handle is drawn in output +coordinates and stored in source ones, and the two are separated by the crop, +the zoom, the pan, the straightening and the turns. `Framing::source_at` and +`Framing::output_at` are `wgsl_prologue` evaluated on the CPU, kept in that file +beside it so the correspondence is one file's problem. + +**Handles.** A linear ramp has three — centre, width, angle. A radial has three +— centre and one per semi-axis, the major one carrying the ellipse's angle as +well as its length, because where an axis is put says both. A rotation arm was +tried on the radial and taken out: standing off the shape by a fixed distance, +it began outside the photograph at the size a new radial is created at. + +**A drag is a displacement applied to where the mask was when the press +landed**, not a destination the handle is snapped to. Snapping jerks the handle +by up to half a touch target on the first press, and the target is finger-sized +(FR-UI-3). + +**Two faults found by looking at the screen** rather than by reading the source, +both of the kind `ui-refinement.md`'s verification section warns about. A `1px` +rule with a size and no position is *centred* by Slint, so the develop column's +seam was a hairline down the middle of the panel — twice over, once in +`app.slint` and once in `AdjustPanel`. And handing Slint a new `ModelRc` for the +handles on every pointer event made the repeater rebuild its items, taking the +`TouchArea` holding the gesture with them: the handle jumped once and then went +dead under a finger that was still down. The model is now rewritten in place. ### N3 — Scope-following histogram -**Depends on** N2. +**Depends on** N2, which has landed, so this is next and is the outstanding +half of §1.1: the panel now says *which* chain the sliders edit, and the +instrument beside them still measures the other one. **Deliverable.** The histogram reduction takes an optional mask; in local mode it reduces over the selected layer's coverage. The panel says which it is diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index 7367f3c..dc42399 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -838,26 +838,63 @@ impl DevelopSession { // // It also means the array does not reallocate when the window // resizes, and does not need redrawing when the view moves. - let Some(seg) = self.segmentation.as_ref() else { - return false; - }; - let (pw, ph) = seg.proxy_size(); + let (pw, ph) = self.mask_raster_size(); let subjects = self.subjects.as_ref(); + + // **Built here, not in `segment`.** A gradient needs no segmentation — + // a graduated filter over a sky never had to know what a sky is — but + // the rasteriser was only ever constructed on the way out of one, so + // adding a gradient to a photograph nobody had segmented produced an + // array that was never rasterised and a layer that drew nothing at + // all. Silently: the generated shader still emits the layer's block + // and the empty placeholder multiplies it by zero, which is the same + // failure exports and thumbnails had. + // + // The shader compile this costs is paid once, on the first frame after + // the first mask is added — a button press, not a frame anyone is + // dragging through. `is_neutral` above is what keeps it off the path + // of every photograph that has no local adjustment at all. + if self.masks.is_none() { + let ctx = self.ctx.clone(); + self.masks = dr_gpu::MaskPass::new(&ctx) + .inspect_err(|e| log::warn!("no mask rasteriser on this device: {e}")) + .ok(); + } let Some(pass) = self.masks.as_mut() else { return false; }; // 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( - self.graph.masks(), - None, - subjects, - pw as u32, - ph as u32, + pass.render(self.graph.masks(), None, subjects, pw, ph) + .inspect_err(|e| log::warn!("mask rasterisation failed: {e}")) + .is_ok() + } + + /// The size the mask array is rasterised at, in source space. + /// + /// **A property of the photograph, not of the segmentation.** The two come + /// out the same because both are the source scaled to `SEGMENT_PROXY_EDGE`, + /// and they have to: a subject layer's distance field is built at the + /// segmentation's proxy and sampled against this array, so the two sizes + /// agreeing is a requirement rather than a coincidence. Reading the size + /// *off* the segmentation is what made it look like a dependency, and made + /// a gradient — which indexes nothing — wait for a model to run. + /// + /// The aspect must be the source's either way. A gradient's geometry is + /// measured against the frame's own proportions, so a square array over a + /// 3:2 photograph would stretch every circle it drew. + fn mask_raster_size(&self) -> (u32, u32) { + if let Some(seg) = self.segmentation.as_ref() { + let (pw, ph) = seg.proxy_size(); + return (pw as u32, ph as u32); + } + let (sw, sh) = self.demosaiced.size(); + let scale = (SEGMENT_PROXY_EDGE as f32 / sw.max(sh).max(1) as f32).min(1.0); + ( + ((sw as f32 * scale) as u32).max(1), + ((sh as f32 * scale) as u32).max(1), ) - .inspect_err(|e| log::warn!("mask rasterisation failed: {e}")) - .is_ok() } // ---------------------------------------------------------------------- @@ -1087,6 +1124,71 @@ impl DevelopSession { self.active_mask.as_deref() } + /// TRACES: FR-DEV-3 | FR-UI-3 + /// The selected gradient's handles, in fractions of the shown image. + /// + /// Empty unless a gradient layer is selected, which is what makes this the + /// panel's whole test for "is there anything to draw on the canvas". + /// + /// Recomputed on every redraw rather than cached, because the answer + /// changes with the *view* and not only with the mask: a pan moves every + /// handle and touches no geometry. Four handles through an affine map is + /// not work worth caching, and a cache keyed on the wrong thing is how a + /// handle comes to sit where the mask used to be. + pub fn gradient_handles(&self) -> Vec { + let Some(layer) = self.active_layer() else { + return Vec::new(); + }; + let (sw, sh) = self.demosaiced.size(); + crate::gradient::handles(&layer.source, self.graph.framing(), (sw, sh)) + } + + /// Drag one handle of the selected gradient, from `press` to `now`, both + /// in fractions of the shown image. + /// + /// `origin` is the geometry the gesture started from — see + /// [`crate::gradient::drag`] for why a drag is applied to that rather than + /// accumulated. Returns it, so the caller can hold it for the rest of the + /// gesture; `None` when there is no gradient selected to drag. + pub fn drag_gradient_handle( + &mut self, + role: crate::HandleRole, + origin: Option<&MaskSource>, + press: (f32, f32), + now: (f32, f32), + ) -> Option { + let (sw, sh) = self.demosaiced.size(); + let framing = *self.graph.framing(); + let id = self.active_mask.clone()?; + let start = match origin { + Some(s) => s.clone(), + None => self.graph.masks().get(&id)?.source.clone(), + }; + + let moved = crate::gradient::drag(&start, role, press, now, &framing, (sw, sh)); + self.graph.masks_mut().get_mut(&id)?.source = moved; + // **Nothing recorded here.** A drag delivers a pointer event a frame, + // and a history step per frame would make undo walk a gesture back + // pixel by pixel. `Edit` coalesces by operation id and a mask's shape + // is not an operation, so there is no key to coalesce under — the + // honest answer is to record once, on release. + Some(start) + } + + /// A handle drag finished: one history step for the whole gesture. + /// + /// Called on the pointer's release rather than on each move, which is what + /// makes a drag one decision in the undo stack however many frames it took. + pub fn commit_gradient_drag(&mut self) { + self.history.record(&self.graph, Edit::Discrete); + } + + /// The selected layer's mask rule, for a caller that has to remember what + /// a gesture started from. + pub fn active_mask_source(&self) -> Option { + self.active_layer().map(|l| l.source.clone()) + } + /// Select a layer for editing, or `None` to return the panel to the /// global chain. pub fn set_active_mask(&mut self, id: Option<&str>) { @@ -2035,6 +2137,98 @@ mod tests { pollster::block_on(dr_gpu::GpuContext::new_headless()).ok() } + /// TRACES: FR-DEV-3 + /// A gradient needs no segmentation, and until now it silently got no mask. + /// + /// The rasteriser was built on the way out of `segment`, so a gradient + /// added to a photograph nobody had segmented had nothing to draw it — and + /// the failure was invisible from every side. The generated shader still + /// emits the layer's block, the empty placeholder multiplies it by zero, + /// and the result is a well-formed frame with the local adjustment simply + /// absent. No error, no warning, and nothing on screen to tell it apart + /// from a mask the user had placed badly. + /// + /// A graduated filter over a sky never had to know what a sky is, so the + /// dependency was wrong as well as silent. + #[test] + fn a_gradient_renders_on_a_photograph_nobody_has_segmented() { + let Some(ctx) = headless() else { return }; + + // Mid grey, so a brightening layer is unambiguous either way. + let rgba: Vec = (0..64 * 64).flat_map(|_| [128u8, 128, 128, 255]).collect(); + let mut session = + DevelopSession::open_rgb(&ctx, &rgba, 64, 64, dr_types::Orientation::NORMAL) + .expect("session"); + assert!( + !session.has_segmentation(), + "the point of the test is that there is none" + ); + + let before = read_back(&ctx, &session.render(64, 64).expect("render")); + + // A radial over the middle, brightened hard. Addressed by index, so + // this names no operation (FR-DEV-3a). + session.add_gradient_mask(true).expect("a radial"); + let row = session.rows()[0].clone(); + session.set_param(row.op_index, row.param_index, row.maximum); + + let after = read_back(&ctx, &session.render(64, 64).expect("render")); + + let centre = |px: &[u8]| px[((32 * 64 + 32) * 4) as usize]; + assert!( + centre(&after) > centre(&before) + 20, + "the middle of the frame must brighten: {} against {}", + centre(&after), + centre(&before) + ); + // And only the middle: a mask that failed to rasterise the other way — + // covering everything — would pass the assertion above. + let corner = |px: &[u8]| px[0]; + assert_eq!( + corner(&after), + corner(&before), + "the corner is outside the radial and must not move" + ); + } + + /// TRACES: FR-DEV-3 + /// The mask array and the segmentation proxy are the same size on purpose. + /// + /// They have to be: a subject layer's distance field is built at the + /// segmentation's proxy resolution and sampled against the array, so if the + /// two ever diverged a subject mask would be drawn at the wrong scale — + /// a mask that is confidently in the wrong place, which is worse than none. + /// + /// It used to hold because the array's size was *read off* the + /// segmentation, which also made a gradient wait for a model it does not + /// use. Deriving both from the photograph keeps the agreement and drops the + /// dependency, and this is what stops the agreement being an accident. + #[test] + fn the_mask_array_is_the_size_the_segmentation_will_use() { + let Some(ctx) = headless() else { return }; + + let rgba: Vec = (0..100 * 100) + .flat_map(|_| [128u8, 128, 128, 255]) + .collect(); + let mut session = + DevelopSession::open_rgb(&ctx, &rgba, 100, 100, dr_types::Orientation::NORMAL) + .expect("session"); + + let before = session.mask_raster_size(); + if session + .segment(&ctx, &crate::segmentation::Options::default()) + .is_err() + { + eprintln!("no model; skipping"); + return; + } + assert_eq!( + before, + session.mask_raster_size(), + "a mask rasterised before the model ran must not move when it does" + ); + } + #[test] fn an_unzoomed_overlay_shows_the_whole_frame() { let Some(ctx) = headless() else { return }; diff --git a/ui/dr-ui/src/gradient.rs b/ui/dr-ui/src/gradient.rs new file mode 100644 index 0000000..f84fc3a --- /dev/null +++ b/ui/dr-ui/src/gradient.rs @@ -0,0 +1,765 @@ +//! On-canvas handles for the gradient masks (FR-DEV-3, FR-UI-3). +//! +//! A linear or radial mask could be created and then not moved: it had no +//! handles, so a radial sat at the centre of the frame at its default size for +//! ever. These are the first controls in the application designed to be +//! dragged on the photograph rather than in a panel, and two things follow +//! from that which do not apply to a slider. +//! +//! # The geometry is stored where the mask is, not where the pointer is +//! +//! A gradient's geometry is in **normalised source coordinates**, because that +//! is where the mask is rasterised and it is what makes the mask survive a +//! crop, a zoom, a pan and an export at another size. The pointer arrives in +//! **output** coordinates — fractions of the photograph as it currently sits +//! on screen. Everything here is the journey between those two, and it goes +//! through [`dr_pipeline::Framing::source_at`], which is the same map the +//! shader applies. Anything less — the crop alone, say — would put a handle +//! on the mask at one zoom level and beside it at every other. +//! +//! # A drag is a displacement, not a destination +//! +//! Each handle answers to the *movement* of the pointer since the press, not +//! to where the pointer now is. Snapping the handle to the pointer instead +//! would jerk it by up to half a touch target the instant it was grabbed — +//! and the target is finger-sized (FR-UI-3), so that jerk is about 20 pixels +//! on a first press. The map is affine, so a displacement in output space is +//! exactly a displacement in source space and nothing is lost by working this +//! way. +//! +//! # Frame units +//! +//! Positions are fractions of each axis; **distances and angles are in the +//! frame's isotropic units**, where y spans `0..1` and x spans `0..aspect`. +//! See [`dr_pipeline::mask::MaskSource::Linear`] for why. Converting between +//! the two is [`to_frame`] and [`to_uv`], and they are the only place it +//! happens on this side. + +use std::f32::consts::FRAC_PI_2; + +use dr_pipeline::mask::MaskSource; +use dr_pipeline::Framing; + +use crate::{GradientHandle, HandleRole}; + +/// How far the rotation handle stands off the shape it turns, in frame units. +/// +/// Far enough that turning is a comfortable lever and near enough that it is +/// still on the photograph at a middling zoom. +const ROT_ARM: f32 = 0.22; + +/// The closest a size handle is ever *drawn* to the centre, in frame units. +/// +/// A hard-edged ramp has zero width and a radial can be dragged very small, +/// and a handle drawn at its true position would then sit underneath the +/// centre handle where it could never be grabbed again — the size would be a +/// one-way trip. So the drawn offset has a floor while the stored value does +/// not. The handle lies by at most this much, and only when the shape is +/// already smaller than a fingertip. +const MIN_ARM: f32 = 0.06; + +/// The largest a gradient may be dragged, in frame units. Well past the +/// diagonal of any frame, so it bounds nothing a user would do and does bound +/// a drag flung off the edge of the screen. +const MAX_EXTENT: f32 = 4.0; + +/// The smallest semi-axis a radial may be dragged to. Not zero: a radial with +/// a zero axis covers nothing and looks broken rather than small. +const MIN_RADIUS: f32 = 0.005; + +/// Where every handle for `source` currently sits, in normalised **output** +/// coordinates. +/// +/// Empty for a mask that is not a gradient — a subject's outline is the +/// model's and has nothing to drag. +/// +/// A position outside `0..1` is returned rather than clamped: the caller draws +/// the handles inside the photograph's own rect and lets them clip, so a +/// handle panned off screen is absent instead of pinned to the edge claiming +/// the mask is somewhere it is not. +pub(crate) fn handles( + source: &MaskSource, + framing: &Framing, + src: (u32, u32), +) -> Vec { + let aspect = aspect_of(src); + points(source, aspect) + .into_iter() + .map(|(role, uv)| { + let (x, y) = framing.output_at(uv, src.0, src.1); + GradientHandle { role, x, y } + }) + .collect() +} + +/// The gradient `source` becomes when `role` is dragged from `press` to `now`, +/// both in normalised output coordinates. +/// +/// `source` must be the geometry as it stood **when the press began**, so that +/// a drag is applied once rather than accumulated frame by frame. The caller +/// captures it on the way down for the same reason the crop handles capture +/// the rect they started from. +pub(crate) fn drag( + source: &MaskSource, + role: HandleRole, + press: (f32, f32), + now: (f32, f32), + framing: &Framing, + src: (u32, u32), +) -> MaskSource { + let aspect = aspect_of(src); + let from = framing.source_at(press, src.0, src.1); + let to = framing.source_at(now, src.0, src.1); + let delta = (to.0 - from.0, to.1 - from.1); + + // Where this handle was, moved by what the pointer did. Everything below + // reads the field back out of that one moved point, so a handle cannot + // disagree with the shape it is drawn on. + let Some((_, start)) = points(source, aspect).into_iter().find(|(r, _)| *r == role) else { + return source.clone(); + }; + let moved = (start.0 + delta.0, start.1 + delta.1); + + match source { + MaskSource::Linear { + centre, + angle, + width, + } => { + if role == HandleRole::Centre { + return MaskSource::Linear { + centre: moved, + angle: *angle, + width: *width, + }; + } + let d = frame_delta(moved, *centre, aspect); + match role { + // The projection onto the ramp direction, doubled: `width` is + // the whole distance from full effect to none and the handle + // sits at half of it. Projected rather than measured, so + // dragging sideways changes the width by nothing — the + // rotation handle is what turns a ramp. + HandleRole::Edge => MaskSource::Linear { + centre: *centre, + angle: *angle, + width: (2.0 * dot(d, direction(*angle))).clamp(0.0, MAX_EXTENT), + }, + HandleRole::Rotate => MaskSource::Linear { + centre: *centre, + // The arm lies along the ramp *line*, a quarter turn from + // the direction coverage increases in. + angle: bearing(d).map_or(*angle, |b| b - FRAC_PI_2), + width: *width, + }, + _ => source.clone(), + } + } + + MaskSource::Radial { + centre, + radii, + angle, + feather, + } => { + if role == HandleRole::Centre { + return MaskSource::Radial { + centre: moved, + radii: *radii, + angle: *angle, + feather: *feather, + }; + } + let d = frame_delta(moved, *centre, aspect); + match role { + // The major axis, length *and* direction. Dragging outward + // resizes and dragging round turns, which is the one gesture + // an ellipse's own axis affords — and the reason there is no + // separate rotation handle. + HandleRole::Edge => MaskSource::Radial { + centre: *centre, + radii: (length(d).clamp(MIN_RADIUS, MAX_EXTENT), radii.1), + angle: bearing(d).unwrap_or(*angle), + feather: *feather, + }, + // The minor axis, length only. Projected onto the axis it + // owns, so the ellipse keeps the angle the major handle set + // rather than the two fighting over it. + HandleRole::Cross => { + let minor = { + let major = direction(*angle); + (-major.1, major.0) + }; + MaskSource::Radial { + centre: *centre, + radii: (radii.0, dot(d, minor).abs().clamp(MIN_RADIUS, MAX_EXTENT)), + angle: *angle, + feather: *feather, + } + } + _ => source.clone(), + } + } + + // A subject or a region has no geometry of its own — its outline is + // the model's, and the edge controls in the panel are how it is + // shaped. A painted mask will be the same answer for a different + // reason: its geometry is the strokes, and a stroke is made by + // painting rather than by moving a handle. + _ => source.clone(), + } +} + +/// Every handle's position in normalised **source** coordinates. +/// +/// The single description both directions read: [`handles`] maps these onto +/// the screen, and [`drag`] moves one of them. Two lists would be two things +/// to keep in step, and the symptom of them disagreeing is a handle that +/// grabs at a distance. +fn points(source: &MaskSource, aspect: f32) -> Vec<(HandleRole, (f32, f32))> { + match source { + MaskSource::Linear { + centre, + angle, + width, + } => { + let along = direction(*angle); + let across = (-along.1, along.0); + vec![ + ( + HandleRole::Edge, + offset(*centre, along, (width * 0.5).max(MIN_ARM), aspect), + ), + (HandleRole::Rotate, offset(*centre, across, ROT_ARM, aspect)), + (HandleRole::Centre, *centre), + ] + } + // **Three handles, and no separate rotation.** The major-axis handle + // *is* the major axis, so where it is put says both how long the axis + // is and which way it points — the ellipse needs no fourth control to + // say the same thing twice. + // + // A rotation arm was tried and taken out: standing off the shape by a + // fixed distance, it began outside the photograph at the size a new + // radial is created at, so the first thing the user saw was a handle + // they could not reach without first shrinking the mask. + MaskSource::Radial { + centre, + radii, + angle, + .. + } => { + let major = direction(*angle); + let minor = (-major.1, major.0); + vec![ + ( + HandleRole::Edge, + offset(*centre, major, radii.0.max(MIN_ARM), aspect), + ), + ( + HandleRole::Cross, + offset(*centre, minor, radii.1.max(MIN_ARM), aspect), + ), + (HandleRole::Centre, *centre), + ] + } + _ => Vec::new(), + } +} + +/// The frame's aspect, which is what separates a fraction of the width from a +/// fraction of the height. +fn aspect_of(src: (u32, u32)) -> f32 { + src.0.max(1) as f32 / src.1.max(1) as f32 +} + +fn to_frame(uv: (f32, f32), aspect: f32) -> (f32, f32) { + (uv.0 * aspect, uv.1) +} + +fn to_uv(q: (f32, f32), aspect: f32) -> (f32, f32) { + (q.0 / aspect, q.1) +} + +/// `centre` displaced by `distance` frame units along the unit vector `dir`, +/// returned in normalised coordinates. +fn offset(centre: (f32, f32), dir: (f32, f32), distance: f32, aspect: f32) -> (f32, f32) { + let q = to_frame(centre, aspect); + to_uv((q.0 + dir.0 * distance, q.1 + dir.1 * distance), aspect) +} + +/// The vector from `centre` to `point`, in frame units. +fn frame_delta(point: (f32, f32), centre: (f32, f32), aspect: f32) -> (f32, f32) { + let a = to_frame(point, aspect); + let b = to_frame(centre, aspect); + (a.0 - b.0, a.1 - b.1) +} + +fn direction(angle: f32) -> (f32, f32) { + (angle.cos(), angle.sin()) +} + +fn dot(a: (f32, f32), b: (f32, f32)) -> f32 { + a.0 * b.0 + a.1 * b.1 +} + +fn length(v: (f32, f32)) -> f32 { + (v.0 * v.0 + v.1 * v.1).sqrt() +} + +/// Which way `d` points, or `None` when it is too short to have a direction. +/// +/// A drag that lands on the centre would otherwise send the angle somewhere +/// arbitrary, and a gradient that spins when the pointer passes through its +/// middle is the sort of thing that makes a control feel broken. +fn bearing(d: (f32, f32)) -> Option { + ((d.0 * d.0 + d.1 * d.1) > 1e-8).then(|| d.1.atan2(d.0)) +} + +#[cfg(test)] +mod tests { + use super::*; + use dr_pipeline::framing::CropRect; + + /// A 3:2 sensor. Square would hide every aspect fault in this file. + const SRC: (u32, u32) = (6000, 4000); + + fn linear() -> MaskSource { + MaskSource::Linear { + centre: (0.5, 0.5), + angle: FRAC_PI_2, + width: 0.3, + } + } + + fn radial() -> MaskSource { + MaskSource::Radial { + centre: (0.5, 0.5), + radii: (0.35, 0.25), + angle: 0.0, + feather: 0.5, + } + } + + fn centre_of(source: &MaskSource) -> (f32, f32) { + match source { + MaskSource::Linear { centre, .. } | MaskSource::Radial { centre, .. } => *centre, + _ => panic!("not a gradient"), + } + } + + fn spot(handles: &[GradientHandle], role: HandleRole) -> (f32, f32) { + let h = handles + .iter() + .find(|h| h.role == role) + .unwrap_or_else(|| panic!("no {role:?} handle")); + (h.x, h.y) + } + + fn close(a: (f32, f32), b: (f32, f32), what: &str) { + assert!( + (a.0 - b.0).abs() < 1e-3 && (a.1 - b.1).abs() < 1e-3, + "{what}: {a:?} != {b:?}" + ); + } + + /// A view that is cropped, zoomed, panned, straightened and turned — the + /// composition, because a handle that is only ever tested unedited is + /// tested in the one state where the map is the identity. + fn moved_view() -> Framing { + let mut f = Framing::new(); + f.set_crop(CropRect { + x: 0.1, + y: 0.15, + width: 0.7, + height: 0.6, + }); + f.set_view(CropRect { + x: 0.2, + y: 0.3, + width: 0.5, + height: 0.5, + }); + f.set_param(dr_pipeline::framing::ANGLE, 6.0); + f.rotate_quarters(1); + f + } + + #[test] + fn a_centre_drag_lands_where_the_pointer_did() { + // The whole point of the feature, and the thing that breaks silently: + // the handle must end up under the pointer, not under where the + // pointer would have been at some other zoom level. + let f = Framing::new(); + let dragged = drag( + &linear(), + HandleRole::Centre, + (0.5, 0.5), + (0.3, 0.8), + &f, + SRC, + ); + close(centre_of(&dragged), (0.3, 0.8), "unzoomed"); + close( + f.output_at(centre_of(&dragged), SRC.0, SRC.1), + (0.3, 0.8), + "and reads back to the same place on screen", + ); + } + + #[test] + fn a_centre_drag_lands_where_the_pointer_did_after_the_view_moves() { + // The regression this exists for. With the view moved, an output + // fraction and a source fraction are different numbers, so a drag that + // forgot the framing map would put the mask somewhere the pointer + // never was — and the further the user had zoomed, the further off. + let f = moved_view(); + let start = f.output_at(centre_of(&linear()), SRC.0, SRC.1); + let target = (0.62, 0.28); + + let dragged = drag(&linear(), HandleRole::Centre, start, target, &f, SRC); + + close( + f.output_at(centre_of(&dragged), SRC.0, SRC.1), + target, + "the centre is under the pointer", + ); + assert!( + (centre_of(&dragged).0 - target.0).abs() > 0.05, + "and it is *not* simply the output fraction stored raw, which is \ + the mistake this guards: {:?}", + centre_of(&dragged) + ); + } + + #[test] + fn dragging_a_handle_onto_another_gradients_handle_produces_that_gradient() { + // The contract for every handle at once, and the sharpest way to state + // it: put a handle where some other gradient's matching handle sits and + // the mask must *become* that gradient. + // + // Sharper than "the handle lands under the pointer", which is only + // true of the centre — the size handles project onto the axis they + // control and the rotation handle keeps its arm's length, so all three + // deliberately land somewhere other than the pointer. This holds for + // all of them, and it is what closes the loop between the two + // directions of the map: the position is computed one way, the field + // is recovered the other, and they have to be inverses. + // + // Through a moved view, because that is where a one-legged map hides: + // when the framing is neutral the two directions are both the + // identity and any pair of them agrees. + let f = moved_view(); + + // Each pair differs in exactly the field the named handle controls, so + // a handle that moved something else fails as well as one that landed + // in the wrong place. + let cases: Vec<(HandleRole, MaskSource, MaskSource)> = vec![ + ( + HandleRole::Centre, + linear(), + MaskSource::Linear { + centre: (0.31, 0.62), + angle: FRAC_PI_2, + width: 0.3, + }, + ), + ( + HandleRole::Edge, + linear(), + MaskSource::Linear { + centre: (0.5, 0.5), + angle: FRAC_PI_2, + // Both widths well past twice `MIN_ARM`, or the drawn + // offset is the floor rather than the width and the test + // would be measuring the floor. + width: 0.52, + }, + ), + ( + HandleRole::Rotate, + linear(), + MaskSource::Linear { + centre: (0.5, 0.5), + angle: 0.9, + width: 0.3, + }, + ), + ( + HandleRole::Centre, + radial(), + MaskSource::Radial { + centre: (0.4, 0.34), + radii: (0.35, 0.25), + angle: 0.0, + feather: 0.5, + }, + ), + ( + HandleRole::Edge, + radial(), + MaskSource::Radial { + centre: (0.5, 0.5), + radii: (0.52, 0.25), + angle: 0.0, + feather: 0.5, + }, + ), + ( + HandleRole::Cross, + radial(), + MaskSource::Radial { + centre: (0.5, 0.5), + radii: (0.35, 0.41), + angle: 0.0, + feather: 0.5, + }, + ), + // The major-axis handle carries the angle as well as the length, + // so a turned ellipse is reached through `Edge` and there is no + // `Rotate` case for a radial to check. + ( + HandleRole::Edge, + radial(), + MaskSource::Radial { + centre: (0.5, 0.5), + radii: (0.35, 0.25), + angle: 0.55, + feather: 0.5, + }, + ), + ]; + + for (role, from, want) in cases { + let press = spot(&handles(&from, &f, SRC), role); + let target = spot(&handles(&want, &f, SRC), role); + let got = drag(&from, role, press, target, &f, SRC); + assert_eq!( + describe(&got), + describe(&want), + "{role:?} on a {}", + from.kind() + ); + } + } + + /// A gradient rounded to three places, so two of them can be compared + /// without floating-point noise deciding the outcome. + fn describe(source: &MaskSource) -> String { + let r = |v: f32| (v * 1000.0).round() as i32; + match source { + MaskSource::Linear { + centre, + angle, + width, + } => format!( + "linear c=({},{}) a={} w={}", + r(centre.0), + r(centre.1), + r(*angle), + r(*width) + ), + MaskSource::Radial { + centre, + radii, + angle, + feather, + } => format!( + "radial c=({},{}) r=({},{}) a={} f={}", + r(centre.0), + r(centre.1), + r(radii.0), + r(radii.1), + r(*angle), + r(*feather) + ), + other => other.kind().to_string(), + } + } + + #[test] + fn the_size_handle_reads_the_projection_and_not_the_distance() { + // Dragging a width handle sideways must change nothing. Reading the + // raw distance instead would make every rotation of the pointer widen + // the ramp, so a user turning a gradient would find it growing. + let f = Framing::new(); + let before = handles(&linear(), &f, SRC); + let edge = spot(&before, HandleRole::Edge); + + // The ramp runs down the frame, so sideways is along x. + let sideways = drag( + &linear(), + HandleRole::Edge, + edge, + (edge.0 + 0.2, edge.1), + &f, + SRC, + ); + match sideways { + MaskSource::Linear { width, .. } => { + assert!((width - 0.3).abs() < 1e-3, "width moved to {width}") + } + _ => panic!("kind changed"), + } + } + + #[test] + fn a_hard_edged_ramp_keeps_a_reachable_size_handle() { + // Width zero is a legitimate mask — a hard edge — and its handle sits + // exactly on the centre. Drawn there it would be under the centre + // handle and the width could never be raised again, so the *drawn* + // offset has a floor while the stored width does not. + let hard = MaskSource::Linear { + centre: (0.5, 0.5), + angle: 0.0, + width: 0.0, + }; + let f = Framing::new(); + let hs = handles(&hard, &f, SRC); + let (cx, cy) = spot(&hs, HandleRole::Centre); + let (ex, ey) = spot(&hs, HandleRole::Edge); + let apart = ((ex - cx).powi(2) + (ey - cy).powi(2)).sqrt(); + assert!(apart > 0.02, "the two handles are on top of each other"); + + // And dragging it outward still sets a real width rather than one + // measured from the place the handle was drawn at. + let widened = drag(&hard, HandleRole::Edge, (ex, ey), (ex + 0.1, ey), &f, SRC); + match widened { + MaskSource::Linear { width, .. } => assert!(width > 0.0, "width stayed {width}"), + _ => panic!("kind changed"), + } + } + + #[test] + fn turning_a_ramp_leaves_its_centre_and_width_alone() { + // Three fields, three handles, and each must move only its own — a + // rotation that also nudged the centre would make aiming a gradient a + // negotiation. + let f = Framing::new(); + let rot = spot(&handles(&linear(), &f, SRC), HandleRole::Rotate); + let turned = drag(&linear(), HandleRole::Rotate, rot, (0.9, 0.2), &f, SRC); + match turned { + MaskSource::Linear { + centre, + angle, + width, + } => { + close(centre, (0.5, 0.5), "centre"); + assert!((width - 0.3).abs() < 1e-6, "width moved to {width}"); + assert!( + (angle - FRAC_PI_2).abs() > 0.1, + "the angle did not actually turn: {angle}" + ); + } + _ => panic!("kind changed"), + } + } + + #[test] + fn each_radial_handle_owns_one_semi_axis() { + // Two axes and one handle each. Scaling both together would make an + // ellipse unreachable, which is the shape a face wants. + let f = Framing::new(); + let hs = handles(&radial(), &f, SRC); + let edge = spot(&hs, HandleRole::Edge); + let wider = drag( + &radial(), + HandleRole::Edge, + edge, + (edge.0 + 0.1, edge.1), + &f, + SRC, + ); + match wider { + MaskSource::Radial { radii, .. } => { + assert!(radii.0 > 0.35, "the major axis grew: {radii:?}"); + assert!( + (radii.1 - 0.25).abs() < 1e-6, + "the minor did not: {radii:?}" + ); + } + _ => panic!("kind changed"), + } + + // And the minor handle keeps the angle the major one set, rather than + // the two contradicting each other about which way the ellipse lies. + let turned = MaskSource::Radial { + centre: (0.5, 0.5), + radii: (0.35, 0.25), + angle: 0.7, + feather: 0.5, + }; + let cross = spot(&handles(&turned, &f, SRC), HandleRole::Cross); + let taller = drag( + &turned, + HandleRole::Cross, + cross, + (cross.0 + 0.06, cross.1), + &f, + SRC, + ); + match taller { + MaskSource::Radial { radii, angle, .. } => { + assert!((angle - 0.7).abs() < 1e-6, "the angle moved to {angle}"); + assert!((radii.0 - 0.35).abs() < 1e-6, "the major moved: {radii:?}"); + } + _ => panic!("kind changed"), + } + } + + #[test] + fn a_new_radials_handles_are_all_on_the_photograph() { + // The first thing anyone sees. A handle placed by a fixed standoff + // from the shape starts outside the frame at the size a radial is + // created at, and a control you have to shrink the mask to reach is + // one nobody finds — a tablet has no hover to hint at it and no + // modifier to summon it (FR-UI-7). + let radial = MaskSource::Radial { + // What `add_gradient_mask` creates. + centre: (0.5, 0.5), + radii: (0.35, 0.35), + angle: 0.0, + feather: 0.5, + }; + for h in handles(&radial, &Framing::new(), SRC) { + assert!( + (0.0..=1.0).contains(&h.x) && (0.0..=1.0).contains(&h.y), + "{:?} is off the picture at ({}, {})", + h.role, + h.x, + h.y + ); + } + + // The linear's rotation arm is measured from the centre rather than + // from an edge, so it has the same obligation and much more room. + let linear = MaskSource::Linear { + centre: (0.5, 0.5), + angle: FRAC_PI_2, + width: 0.3, + }; + for h in handles(&linear, &Framing::new(), SRC) { + assert!( + (0.0..=1.0).contains(&h.x) && (0.0..=1.0).contains(&h.y), + "{:?} is off the picture at ({}, {})", + h.role, + h.x, + h.y + ); + } + } + + #[test] + fn a_subject_mask_offers_nothing_to_drag() { + // Its outline is the model's. Offering handles would suggest the + // shape can be moved, and the edge controls in the panel are what + // actually shapes one. + let subject = MaskSource::Subject { + signature: 1, + index: 0, + class: "person".into(), + score: 0.9, + }; + assert!(handles(&subject, &Framing::new(), SRC).is_empty()); + } +} diff --git a/ui/dr-ui/src/lib.rs b/ui/dr-ui/src/lib.rs index c7d45ae..276b8b6 100644 --- a/ui/dr-ui/src/lib.rs +++ b/ui/dr-ui/src/lib.rs @@ -24,6 +24,7 @@ mod collections_ui; mod derived_sync; mod develop; mod export; +mod gradient; mod histogram; mod labels; mod library; @@ -257,11 +258,12 @@ fn is_supported(p: &Path) -> bool { /// Return the view to its opening state for a newly loaded image. /// -/// Zoom and crop mode are properties of *looking at one photograph*, so +/// Zoom and the view mode are properties of *looking at one photograph*, so /// carrying them to the next one would leave the second image cropped to a -/// rect chosen for the first. +/// rect chosen for the first — or, since local masking became a mode, would +/// open the next photograph with a mask stack it does not have. fn reset_view_state(window: &AppWindow) { - window.set_crop_mode(false); + window.set_view_mode(ViewMode::Photo); window.set_zoom(1.0); window.set_zoomed(false); window.set_crop_x(0.0); @@ -1126,7 +1128,7 @@ pub fn run(paths: Vec) -> Result<()> { // Crop mode shows the whole frame, or the area being cropped away // would not be on screen for the handles to drag across. The // overlay draws the rect on top of it. - let rendered = if window.get_crop_mode() { + let rendered = if window.get_view_mode() == ViewMode::Crop { s.render_uncropped(w, h).map(|(image, _, _)| image) } else { s.render(w, h) @@ -1882,25 +1884,52 @@ pub fn run(paths: Vec) -> Result<()> { }); } { - // Entering crop mode drops the zoom: the handles are placed against - // the whole frame, and a zoomed view would put most of that frame off - // screen where it cannot be dragged. + // TRACES: FR-UI-5 + // **Entering a mode is a side effect, which is why Rust owns it** and + // the strip does not simply write the property. Each of the three has + // work to do that the interface cannot see: + // + // *Crop* drops the zoom. The handles are placed against the whole + // frame, and a zoomed view would put most of that frame off screen + // where it cannot be dragged. + // + // *Local* turns the region overlay on. It used to be a button in the + // masking panel, so the mode could be open with the overlay off — + // which is a mode you have entered that is doing nothing. + // + // *Leaving* clears the selection, and that is the fault this whole + // pass exists for: a selected layer silently re-points thirty sliders + // at that layer's chain, so a mode you have left must not leave one + // behind. After this the controls are unambiguously global again, + // which is what the panel's heading then says. let weak = window.as_weak(); let session = session.clone(); let redraw = redraw.clone(); - window.on_crop_mode_toggled(move |on| { + let rows = rows.clone(); + window.on_mode_picked(move |mode| { let Some(w) = weak.upgrade() else { return }; if let Some(s) = session.borrow_mut().as_mut() { - if on { - s.reset_zoom(); - let c = s.crop(); - w.set_crop_x(c.x); - w.set_crop_y(c.y); - w.set_crop_w(c.width); - w.set_crop_h(c.height); + match mode { + ViewMode::Crop => { + s.reset_zoom(); + let c = s.crop(); + w.set_crop_x(c.x); + w.set_crop_y(c.y); + w.set_crop_w(c.width); + w.set_crop_h(c.height); + } + ViewMode::Local => s.set_overlay(true), + ViewMode::Photo => { + s.set_overlay(false); + s.set_active_mask(None); + } } } - w.set_crop_mode(on); + w.set_view_mode(mode); + masks_ui::sync(&w, &session); + // The scope may have just changed, so the panel below is now + // describing a different chain. + sync_rows(&w, &rows, &session); redraw(&w); }); } @@ -2267,7 +2296,7 @@ fn back_one_step(w: &AppWindow) -> bool { launch: w.get_show_launch(), browsing: w.get_launch_browsing(), library: w.get_show_library(), - crop: w.get_crop_mode(), + mode: w.get_view_mode(), zoomed: w.get_zoomed(), // Files named on the command line have no grid behind them — the same // condition the status strip uses to decide whether to offer the way @@ -2283,7 +2312,7 @@ fn back_one_step(w: &AppWindow) -> bool { match step { BackStep::CloseSettings => w.invoke_settings_close(), BackStep::CancelBrowse => w.invoke_launch_browse_cancel(), - BackStep::LeaveCrop => w.invoke_crop_mode_toggled(false), + BackStep::LeaveMode => w.invoke_mode_picked(ViewMode::Photo), BackStep::ResetZoom => w.invoke_zoom_reset(), BackStep::ToLibrary => w.invoke_back_to_library(), BackStep::ClearScope => w.invoke_collection_select(0), @@ -2297,13 +2326,21 @@ fn back_one_step(w: &AppWindow) -> bool { /// stated and tested without a Slint backend: which of two states is left first /// is the whole of this feature, and it is the part that is easy to get subtly /// wrong when it is spelled out in nested `if`s over live properties. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +// No `Eq`: `ViewMode` is generated by Slint and derives only `PartialEq`, +// which is all the comparisons below need. `Default` still derives, and it +// gives `mode` the enum's own first variant — `photo`, which is what "no mode" +// means and what the tests below want as their baseline. +#[derive(Clone, Copy, Debug, Default, PartialEq)] struct NavState { settings: bool, launch: bool, browsing: bool, library: bool, - crop: bool, + /// Which develop mode is on, if any. One field rather than one flag per + /// mode, so "leave the innermost" cannot be asked of two at once — the + /// ordering below would have had to invent an answer for a state the + /// interface can no longer be in. + mode: ViewMode, zoomed: bool, has_grid: bool, scoped: bool, @@ -2314,7 +2351,10 @@ struct NavState { enum BackStep { CloseSettings, CancelBrowse, - LeaveCrop, + /// Leave whichever develop mode is on — crop or local — and return to the + /// whole photograph. One step for both, because there is one mode at a + /// time and "back" means the same thing from either. + LeaveMode, ResetZoom, ToLibrary, ClearScope, @@ -2334,10 +2374,15 @@ fn back_step(s: NavState) -> Option { } if !s.library { - // Develop. Crop is a mode and zoom is a view state; both are left - // before the image is. - if s.crop { - return Some(BackStep::LeaveCrop); + // Develop. Crop and local are modes and zoom is a view state; all are + // left before the image is. + // + // The mode goes before the zoom because that is the order they were + // entered in — a photographer zooms to place a mask, not the other way + // about — and because leaving local mode drops the mask selection, + // which is a bigger step back than returning to fit. + if s.mode != ViewMode::Photo { + return Some(BackStep::LeaveMode); } if s.zoomed { return Some(BackStep::ResetZoom); @@ -2381,7 +2426,7 @@ mod tests { let from_develop = NavState { settings: true, - crop: true, + mode: ViewMode::Crop, ..developing() }; assert_eq!(back_step(from_develop), Some(BackStep::CloseSettings)); @@ -2389,14 +2434,20 @@ mod tests { #[test] fn back_leaves_a_mode_before_it_leaves_the_image() { - // Crop then zoom then the view: innermost first, because that is the - // order they were entered in. - let cropping = NavState { - crop: true, - zoomed: true, - ..developing() - }; - assert_eq!(back_step(cropping), Some(BackStep::LeaveCrop)); + // A mode, then zoom, then the view: innermost first, because that is + // the order they were entered in. + for mode in [ViewMode::Crop, ViewMode::Local] { + let in_mode = NavState { + mode, + zoomed: true, + ..developing() + }; + assert_eq!( + back_step(in_mode), + Some(BackStep::LeaveMode), + "{mode:?} must be left before the zoom is reset" + ); + } let zoomed = NavState { zoomed: true, @@ -2407,6 +2458,22 @@ mod tests { assert_eq!(back_step(developing()), Some(BackStep::ToLibrary)); } + /// TRACES: FR-UI-5 + /// Local masking joins the existing order rather than inventing an exit. + /// + /// It is the point of making it a mode: before this, back and Escape did + /// nothing about a masking session, so the only way out of it was to find + /// the two toggles that had armed it and press them again — and neither + /// was anywhere near the photograph the user was looking at. + #[test] + fn local_masking_is_left_by_the_same_step_crop_is() { + let masking = NavState { + mode: ViewMode::Local, + ..developing() + }; + assert_eq!(back_step(masking), Some(BackStep::LeaveMode)); + } + #[test] fn back_from_an_image_with_no_grid_behind_it_is_the_top_of_the_stack() { // Files named on the command line: there is no library to return to, diff --git a/ui/dr-ui/src/masks_ui.rs b/ui/dr-ui/src/masks_ui.rs index d94f5f2..66a3d25 100644 --- a/ui/dr-ui/src/masks_ui.rs +++ b/ui/dr-ui/src/masks_ui.rs @@ -17,11 +17,40 @@ use std::cell::RefCell; use std::rc::Rc; -use slint::{ComponentHandle as _, ModelRc, VecModel}; +use slint::{ComponentHandle as _, Model as _, ModelRc, VecModel}; use crate::develop::DevelopSession; use crate::segmentation; -use crate::{sync_rows, AppWindow, MaskRow, ParamRow, SubjectRow}; +use crate::{sync_rows, AppWindow, GradientHandle, MaskRow, ParamRow, SubjectRow}; + +/// What the adjust panel's heading says when the controls are global. +/// +/// The panel's own default too, and named because the two have to agree: a +/// literal in both would eventually be a literal in one. +pub(crate) const GLOBAL_SCOPE: &str = "ADJUST"; + +/// TRACES: FR-DEV-3 +/// What the adjust panel is pointed at, for its heading. +/// +/// **The layer's name, not the word "adjust".** Selecting a layer re-points +/// every control in that panel at that layer's chain, and the heading is the +/// one piece of text a photographer cannot avoid reading on the way to a +/// slider. Upper case because the heading style is, and it is the *same* +/// string the row in the stack above shows — one name for one thing, so the +/// selected row and the panel it scopes cannot appear to disagree. +pub(crate) fn scope_label(session: &DevelopSession) -> String { + let Some(id) = session.active_mask() else { + return GLOBAL_SCOPE.to_string(); + }; + session + .mask_layers() + .into_iter() + .find(|(layer_id, ..)| layer_id == id) + .map_or_else( + || GLOBAL_SCOPE.to_string(), + |(_, label, ..)| label.to_uppercase(), + ) +} /// Push every mask-related property from the session into the window. pub(crate) fn sync(window: &AppWindow, session: &Rc>>) { @@ -30,12 +59,12 @@ pub(crate) fn sync(window: &AppWindow, session: &Rc::default())); window.set_subject_rows(ModelRc::new(VecModel::::default())); window.set_segmented(false); - window.set_editing_mask(false); + window.set_adjust_scope(GLOBAL_SCOPE.into()); + clear_handles(window); window.set_overlay_on(false); return; }; - let active = s.active_mask().map(|id| id.to_string()); let rows: Vec = s .mask_layers() .into_iter() @@ -71,7 +100,8 @@ pub(crate) fn sync(window: &AppWindow, session: &Rc next.len() { + model.remove(model.row_count() - 1); + } + for (i, handle) in next.into_iter().enumerate() { + if i < model.row_count() { + // Only where it actually moved: an unchanged row written back is + // still a change notification, and the point of this function is + // to emit as few of those as the truth allows. + if model.row_data(i).as_ref() != Some(&handle) { + model.set_row_data(i, handle); + } + } else { + model.push(handle); + } + } + + window.set_gradient_handles(model.into()); +} + +/// The handles' model, held for the life of the process. +/// +/// One shared identity, for the reason [`sync_handles`] gives. A thread-local +/// because the interface is single-threaded and this is the same shape +/// `develop.rs` uses for its shared empty models. +fn handle_model() -> Rc> { + thread_local! { + static HANDLES: Rc> = Rc::new(VecModel::default()); + } + HANDLES.with(Clone::clone) +} + +/// Take the handles off the canvas, emptying the held model rather than +/// replacing it — see [`sync_handles`] for why the identity is kept. +fn clear_handles(window: &AppWindow) { + let model = handle_model(); + while model.row_count() > 0 { + model.remove(model.row_count() - 1); + } + window.set_gradient_handles(model.into()); +} + /// Push the overlay's clip rectangle and angle. /// /// Separate from [`sync`] because it is called from the render path too: a pan @@ -163,23 +258,61 @@ pub(crate) fn wire( }); } - // --- the overlay and the picking mode --------------------------------- + // --- dragging a gradient on the photograph ---------------------------- + // + // The geometry the gesture started from, held for its duration. + // + // **A drag is applied to where the mask was when the press landed**, not + // to where it was one frame ago. Accumulating frame by frame would let the + // clamps compound — a radius dragged past its limit and back would not + // return to where it started — and would make the result depend on how + // many events the pointer happened to deliver. + let dragging: Rc>> = Rc::new(RefCell::new(None)); { let weak = window.as_weak(); let session = session.clone(); - window.on_overlay_toggled(move |on| { + let redraw = redraw.clone(); + let dragging = dragging.clone(); + window.on_gradient_handle_dragged(move |role, from_x, from_y, to_x, to_y| { let Some(w) = weak.upgrade() else { return }; - if let Some(s) = session.borrow_mut().as_mut() { - s.set_overlay(on); + let origin = dragging.borrow().clone(); + let started = session.borrow_mut().as_mut().and_then(|s| { + s.drag_gradient_handle(role, origin.as_ref(), (from_x, from_y), (to_x, to_y)) + }); + if started.is_none() { + return; } - sync(&w, &session); + *dragging.borrow_mut() = started; + // Only the handles, not the whole panel: nothing in the mask stack + // or the subject list changed, and rewriting those models on every + // frame of a drag is work for no difference. See `sync_handles` for + // the sharper reason — a full `sync` would also take the gesture + // out from under the finger. + if let Some(s) = session.borrow().as_ref() { + sync_handles(&w, s); + } + redraw(&w); }); } { let weak = window.as_weak(); - window.on_region_picking_toggled(move |on| { - let Some(w) = weak.upgrade() else { return }; - w.set_region_picking(on); + let session = session.clone(); + let dragging = dragging.clone(); + window.on_gradient_handle_released(move || { + // Forgotten on release, so the next gesture measures from wherever + // this one left the mask rather than from where this one began. + if dragging.borrow_mut().take().is_none() { + // A press with no movement. Nothing changed, so recording a + // step would put an identical snapshot on the undo stack. + return; + } + if let Some(s) = session.borrow_mut().as_mut() { + s.commit_gradient_drag(); + } + if let Some(w) = weak.upgrade() { + w.set_can_undo(session.borrow().as_ref().is_some_and(|s| s.can_undo())); + w.set_can_redo(session.borrow().as_ref().is_some_and(|s| s.can_redo())); + } }); } @@ -382,15 +515,178 @@ pub(crate) fn wire( /// Clear the panel when the open image changes. /// /// Its own function rather than a call to [`sync`] with an empty session, -/// because the *window* state has to be reset too: picking mode and the -/// overlay are properties of looking at one photograph, and carrying them to -/// the next one leaves a crosshair over an image with no region map behind it. +/// because the *window* state has to be reset too: the overlay and the scope +/// are properties of looking at one photograph, and carrying them to the next +/// one would draw a region map over an image that has none and name a heading +/// after a layer that is not there. Picking is not among them any more — it +/// follows the view mode, which `reset_view_state` returns to `photo`. pub(crate) fn reset(window: &AppWindow) { - window.set_region_picking(false); window.set_overlay_on(false); window.set_segmenting(false); window.set_segmented(false); window.set_mask_rows(ModelRc::new(VecModel::::default())); window.set_subject_rows(ModelRc::new(VecModel::::default())); - window.set_editing_mask(false); + window.set_adjust_scope(GLOBAL_SCOPE.into()); + clear_handles(window); +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A session over a flat frame. No segmentation, which is deliberate: a + /// gradient needs none, and the tests below are about scope rather than + /// about what the model found. + fn session() -> Option { + let ctx = pollster::block_on(dr_gpu::GpuContext::new_headless()).ok()?; + let rgba: Vec = (0..32 * 32).flat_map(|_| [128u8, 128, 128, 255]).collect(); + DevelopSession::open_rgb(&ctx, &rgba, 32, 32, dr_types::Orientation::NORMAL).ok() + } + + /// TRACES: FR-DEV-3 | FR-UI-1 + /// The fault this pass exists for, stated as a test. + /// + /// Selecting a mask layer re-points every control in the adjust panel at + /// that layer's chain. Before this, the only thing that said so was a + /// caption in a *different* panel, which a photographer reaching for the + /// exposure slider has no reason to read. The heading of the panel that + /// changed now carries the answer, so the two cannot be read apart — and + /// this asserts they cannot come apart either. + #[test] + fn the_heading_says_which_chain_the_controls_are_pointed_at() { + let Some(mut s) = session() else { + eprintln!("no adapter; skipping"); + return; + }; + + assert_eq!(scope_label(&s), GLOBAL_SCOPE, "nothing selected"); + + let id = s + .add_gradient_mask(false) + .expect("a gradient needs no model"); + assert_ne!( + scope_label(&s), + GLOBAL_SCOPE, + "adding a layer selects it, so the panel is already scoped to it \ + and must already say so" + ); + + // And the name is the one the row in the stack shows. Two names for + // one layer would let the selected row and the panel it scopes appear + // to disagree. + let row = s + .mask_layers() + .into_iter() + .find(|(layer_id, ..)| *layer_id == id) + .expect("the layer is in the stack"); + assert_eq!(scope_label(&s), row.1.to_uppercase()); + + s.set_active_mask(None); + assert_eq!( + scope_label(&s), + GLOBAL_SCOPE, + "clearing the selection must put the heading back, or the panel \ + would go on naming a layer it is no longer editing" + ); + } + + /// TRACES: FR-UI-5 + /// Leaving local mode is what clears the selection, and this is the half + /// of it that can be tested without a window. + /// + /// The mode handler in `lib.rs` calls `set_active_mask(None)`; what has to + /// be true afterwards is that the controls are global *and say so*. A mode + /// that was left with a layer still selected would leave thirty sliders + /// pointed at a region of the photograph with nothing on screen saying it. + #[test] + fn clearing_the_selection_returns_the_rows_to_the_whole_photograph() { + let Some(mut s) = session() else { + eprintln!("no adapter; skipping"); + return; + }; + + // The scope is invisible in the *shape* of the panel — a layer holds + // the same chain the frame does, so both produce the same rows in the + // same order. It is only visible in what those rows read, which is + // precisely why the fault was silent: the panel looks identical either + // way and means something different. + // + // Addressed by index rather than by name, because no part of the + // frontend may route by a parameter's identity (FR-DEV-3a). + let first = s.rows()[0].clone(); + s.set_param(first.op_index, first.param_index, first.maximum); + assert_eq!(s.rows()[0].value, first.maximum, "the global chain moved"); + + // Adding a layer selects it, so the same row is now the layer's. + s.add_gradient_mask(true).expect("gradient"); + assert_eq!( + s.rows()[0].value, + first.default_value, + "the same control, pointed somewhere else and reading its own \ + value — the whole hazard, in one row" + ); + + s.set_active_mask(None); + assert_eq!( + s.rows()[0].value, + first.maximum, + "and leaving the layer puts the frame's value back" + ); + assert_eq!(scope_label(&s), GLOBAL_SCOPE); + } + + /// TRACES: FR-UI-3 + /// The handles' model keeps one identity for the life of the process. + /// + /// **This is what makes a drag last longer than one frame.** The handles + /// are a repeater over this model, and a *new* `ModelRc` makes Slint throw + /// the repeated items away and build fresh ones — taking the `TouchArea` + /// that holds the pointer with them. The symptom is precise and was seen + /// on screen before it was understood: the handle jumps once, on the first + /// pointer event, and then sits dead under a finger that is still down. + /// + /// It cannot be asserted through a window without a Slint backend, so it is + /// asserted where it is decided. Every path that touches the handles — + /// `sync_handles` and `clear_handles` — goes through this one model. + #[test] + fn the_handles_are_one_model_rewritten_rather_than_a_new_one_each_time() { + assert!( + Rc::ptr_eq(&handle_model(), &handle_model()), + "a fresh model per sync destroys the gesture that is moving the \ + handles" + ); + } + + /// TRACES: FR-DEV-3 | FR-UI-3 + /// A gradient offers handles; a mask with nothing to drag offers none. + /// + /// This is the panel's whole test for whether to draw anything on the + /// canvas, so it is worth pinning: handles over a subject mask would + /// suggest an outline that cannot be moved can be. + #[test] + fn only_a_selected_gradient_puts_handles_on_the_canvas() { + let Some(mut s) = session() else { + eprintln!("no adapter; skipping"); + return; + }; + + assert!(s.gradient_handles().is_empty(), "nothing selected"); + + s.add_gradient_mask(false).expect("linear"); + assert_eq!(s.gradient_handles().len(), 3, "centre, width and rotation"); + + s.add_gradient_mask(true).expect("radial"); + assert_eq!( + s.gradient_handles().len(), + 3, + "centre and two semi-axes — the major one carries the angle, so \ + an ellipse needs no fourth handle to say it twice" + ); + + s.set_active_mask(None); + assert!( + s.gradient_handles().is_empty(), + "a gradient nobody has selected is not being edited" + ); + } } diff --git a/ui/dr-ui/ui/adjust.slint b/ui/dr-ui/ui/adjust.slint index 8d81154..b73adb1 100644 --- a/ui/dr-ui/ui/adjust.slint +++ b/ui/dr-ui/ui/adjust.slint @@ -453,10 +453,6 @@ component ParamControl inherits Rectangle { // what a bespoke widget for a known stage is entitled to do. export component GeometryPanel inherits Rectangle { in property enabled: true; - /// Whether the crop overlay is up. The button is a toggle, not an action: - /// crop mode is sustained state, and the canvas looks different while it - /// is on. - in property crop-mode: false; in property angle: 0.0; in property max-straighten: 45.0; in property flip-h: false; @@ -464,7 +460,6 @@ export component GeometryPanel inherits Rectangle { /// Any of crop, angle, rotation or flips differs from neutral. in property modified: false; - callback crop-toggled(bool); callback rotate(int); callback flip-h-toggled(); callback flip-v-toggled(); @@ -497,13 +492,11 @@ export component GeometryPanel inherits Rectangle { spacing: Theme.gap-sm; padding-bottom: Theme.gap-sm; - // Crop first: it is the framing decision the others serve. - Button { - text: root.crop-mode ? "Done Cropping" : "Crop"; - active: root.crop-mode; - enabled: root.enabled; - clicked => { root.crop-toggled(!root.crop-mode); } - } + // No crop button. Crop is one value of the view mode now, entered + // and left from the strip pinned at the top of this column — a + // second control that entered the same mode would be a second + // thing that has to agree about which mode the view is in, and + // the whole point of the enum is that there is one answer. // Rotation and flips. Icons rather than labels: four controls // named in words would wrap the 280px column, and each of @@ -629,7 +622,41 @@ export component TransferPanel inherits VerticalLayout { } -/// The group strip: which kind of adjustment the panel is showing. +/// Which mode the develop view is in. +/// +/// `crop` was a bare `bool` on the window and local masking was a panel with +/// two toggles, so nothing stopped both being on at once — and nothing on +/// screen said which of them the canvas and the column were obeying. One value +/// with three states cannot be in two of them, which is the whole reason this +/// is an enum rather than a tidier pair of flags. +export enum ViewMode { + /// The whole photograph. Sliders are global, the canvas pans and zooms. + photo, + /// The crop overlay is up and the canvas shows the uncropped frame. + crop, + /// The region map is drawn, a click on the photograph selects, and the + /// column is the mask stack and the selected layer's adjustments. + local, +} + +/// The strip: what the photographer is working on. +/// +/// Two kinds of entry, deliberately together. +/// +/// **Modes** — crop and local — change the canvas as well as the column. They +/// are drawn as chips and lit with the accent, which means *active* everywhere +/// else in this interface and means exactly that here. +/// +/// **Groups** — the rest — filter the adjustments to one kind. They are +/// underlined instead, because the accent is already spoken for and because +/// they are a different sort of state: a mode is something you are *in*, a +/// group is something you are *looking at*. +/// +/// Keeping them apart visually is what lets them be independent. Picking a +/// group in local mode filters the selected layer's chain and does not leave +/// the mode, so "Light" means the same thing wherever it is pressed — the +/// alternative, where a group press silently dropped the scope, would be the +/// §1.1 fault reintroduced from the other end. /// /// **Pinned above the scrolling column, not inside a panel.** It began inside /// `AdjustPanel`, which put it below five other panels and off the bottom of a @@ -639,25 +666,100 @@ export component TransferPanel inherits VerticalLayout { /// /// **This file names no group.** The strings arrive already resolved from /// whatever the operations declared themselves to be about, so a new operation -/// joins a group without an edit here (FR-DEV-3a). -export component GroupStrip inherits Rectangle { +/// joins a group without an edit here (FR-DEV-3a). The two modes are not +/// operations — crop is a gesture on the canvas and local is a scope — so +/// naming them breaks nothing. +export component ModeStrip inherits Rectangle { in property <[string]> tabs; /// Index into `tabs`, or -1 for "everything". in property active-tab: -1; + in property mode: ViewMode.photo; in property enabled: true; callback picked(int); + callback mode-picked(ViewMode); background: Theme.surface; - height: root.enabled && root.tabs.length > 1 ? layout.preferred-height : 0px; - visible: root.enabled && root.tabs.length > 1; + height: root.enabled ? layout.preferred-height : 0px; + visible: root.enabled; + + // Scrolls rather than overflowing, exactly as the develop status strip + // does and for the same reason: a `HorizontalLayout` given less width than + // its children need does not shrink them, it runs off the end. Two modes + // plus however many groups the operation set declares is already more than + // a 280px column holds, and the column is where this is pinned. + Flickable { + width: 100%; + height: 100%; + viewport-height: self.height; + viewport-width: max(self.width, layout.preferred-width); layout := HorizontalLayout { + width: parent.viewport-width; padding-left: Theme.gap; padding-right: Theme.gap; spacing: Theme.gap-sm; alignment: start; + // A chip per mode. Drawn as an outline that fills when active rather + // than as an underline, so at a glance the strip reads as two runs + // and it is never ambiguous which half a lit entry belongs to. + // + // **Where a brush goes.** Painting a mask is a third mode of exactly + // this shape — it arms a canvas gesture and scopes the column — so it + // joins this list and the `ViewMode` enum, and needs nothing else here. + // `MaskSource::Brush` and the stroke calls on `MaskLayer` land in the + // core separately; what is missing on this side is only the canvas + // interaction, which is the gradient handles' neighbour. + for entry in [ + { label: "Crop", value: ViewMode.crop }, + { label: "Local", value: ViewMode.local }, + ]: mode-chip := TouchArea { + width: mode-name.preferred-width + 2 * Theme.gap-sm; + height: Theme.touch-target; + mouse-cursor: pointer; + + property on: root.mode == entry.value; + + // Pressing the mode you are already in leaves it, which is what + // makes the strip the way out as well as the way in — the same + // control both directions, as the crop button was. + clicked => { + root.mode-picked(self.on ? ViewMode.photo : entry.value); + } + + Rectangle { + y: (parent.height - self.height) / 2; + height: Theme.control-height; + width: parent.width; + border-radius: Theme.radius; + border-width: 1px; + border-color: mode-chip.on ? Theme.active : Theme.rule; + background: mode-chip.on + ? Theme.active-dim + : (mode-chip.has-hover ? Theme.hover : transparent); + } + + mode-name := Text { + text: entry.label; + // Dark on the lit fill, which is near-white: the same + // inversion `Button`'s primary state makes. + color: mode-chip.on ? Theme.ground : Theme.ink-dim; + font-size: Theme.text-sm; + font-weight: 600; + vertical-alignment: center; + horizontal-alignment: center; + } + } + + // The divider between the two kinds. One pixel, and it is what stops + // the strip reading as one undifferentiated row of five words. + Rectangle { + width: 1px; + height: Theme.touch-target; + background: Theme.rule; + } + all := TouchArea { width: 34px; height: Theme.touch-target; @@ -691,9 +793,8 @@ export component GroupStrip inherits Rectangle { horizontal-alignment: center; } - // Underlined rather than filled: the accent means *modified* - // everywhere else here, and spending it on "which group" would - // blunt the one signal the panel has. + // Underlined rather than filled: the accent is the mode chips' + // now, and spending it on "which group" as well would blunt both. Rectangle { y: parent.height - 2px; height: 2px; @@ -702,11 +803,25 @@ export component GroupStrip inherits Rectangle { } } } + } } export component AdjustPanel inherits Rectangle { in property <[ParamRow]> rows; in property enabled: true; + /// What these controls are pointed at — the whole photograph, or one mask + /// layer by name. + /// + /// **The heading, not a caption beside it.** Selecting a layer re-points + /// every one of these controls at that layer's chain, and until now the + /// only sign of it was a sentence in the panel above. Putting the answer + /// in the heading of the thing that changed means the scope cannot be read + /// without also reading what it applies to. + /// + /// Supplied already resolved: whether a layer is selected and what it is + /// called are session facts, and deriving them here would need this file + /// to reason about the mask stack. + in property scope: "ADJUST"; /// The tone curve's sampled shape, evaluated in Rust by the same spline /// the shader runs so the drawn line cannot disagree with the applied one. in property <[float]> curve-samples; @@ -737,8 +852,18 @@ export component AdjustPanel inherits Rectangle { alignment: start; HorizontalLayout { - PanelHeading { text: "ADJUST"; } - Rectangle { horizontal-stretch: 1; } + PanelHeading { + text: root.scope; + // Elided rather than wrapped. A layer's name is the user's and + // can be any length; a heading that wrapped would change the + // panel's height as the selection moved, and an unwrapped one + // would set the 280px column's minimum width from it. + overflow: elide; + horizontal-stretch: 1; + } + // No spacer: the heading takes the slack itself, so a long layer + // name elides against the reset rather than pushing it off the + // 280px column. reset := TouchArea { width: 44px; height: 20px; @@ -873,8 +998,12 @@ export component AdjustPanel inherits Rectangle { } } - Rectangle { - width: 1px; - background: Theme.rule; - } + // No seam of its own. + // + // There was one — a 1px `rule` rectangle — and being a sized child of a + // plain Rectangle with no position, Slint *centred* it: a hairline drawn + // straight down the middle of the panel, through every slider in it. The + // column that hosts this panel already draws the seam between itself and + // the photograph, so the fix is one rule in one place rather than two that + // were never both wanted. } diff --git a/ui/dr-ui/ui/app.slint b/ui/dr-ui/ui/app.slint index 8460f8e..ce62ec6 100644 --- a/ui/dr-ui/ui/app.slint +++ b/ui/dr-ui/ui/app.slint @@ -1,6 +1,6 @@ import { Theme } from "theme.slint"; -import { AdjustPanel, GeometryPanel, GroupStrip, ParamRow, TransferPanel } from "adjust.slint"; -import { MaskPanel, MaskRow, SubjectRow } from "masks.slint"; +import { AdjustPanel, GeometryPanel, ModeStrip, ParamRow, TransferPanel, ViewMode } from "adjust.slint"; +import { GradientHandle, HandleRole, MaskPanel, MaskRow, SubjectRow } from "masks.slint"; import { LaunchScreen } from "launch.slint"; import { LibraryGrid, LibraryCell, TimelineBar, PhotoRoll } from "library.slint"; import { Button, PanelHeading, Label, Value, Caption, Panel, EmptyState, ProgressBar, ActivityRow } from "widgets.slint"; @@ -9,6 +9,7 @@ import { HistogramPanel, HistogramView } from "histogram.slint"; import { SettingsPage } from "settings.slint"; export { LibraryCell, TimelineBar, CollectionRow, ActivityRow, HistogramView } +export { ViewMode, GradientHandle, HandleRole } // Status strip — surfaces the GPU backend and adapter, which matters during // v0.1 because assumption A1 is exactly "does this compositing path work on @@ -321,10 +322,25 @@ export component AppWindow inherits Window { /// resolution against the viewport's, which Slint does not know. in property magnified: false; - /// Whether the crop overlay is active. While it is, the canvas shows the - /// *whole* frame — otherwise the area being cropped away would not be on - /// screen to drag across — and the surround is greyed. - in-out property crop-mode: false; + /// TRACES: FR-UI-5 + /// What the develop view is currently doing (see `ViewMode`). + /// + /// One value rather than a flag per mode, so two modes cannot be on at + /// once — which, with `crop-mode` a bare bool beside a masking panel that + /// armed the canvas through two toggles of its own, they could be. Rust + /// owns it: entering a mode has side effects on the session (crop drops + /// the zoom, local clears nothing and leaving it clears the selection), + /// and a property the interface could set directly would let the view + /// change without them. + /// + /// In `crop` the canvas shows the *whole* frame — otherwise the area being + /// cropped away would not be on screen to drag across — and the surround + /// is greyed. + in property view-mode: ViewMode.photo; + /// Shorthand for the two conditions half this file tests, so a mode + /// comparison is written once rather than at each of a dozen call sites. + property cropping: root.view-mode == ViewMode.crop; + property local-mode: root.view-mode == ViewMode.local; /// The crop rect in fractions of the frame, mirrored from Rust so the /// overlay draws exactly what the pipeline holds. @@ -333,7 +349,8 @@ export component AppWindow inherits Window { in-out property crop-w: 1.0; in-out property crop-h: 1.0; - callback crop-mode-toggled(bool); + /// Enter a mode, or `photo` to leave whichever one is current. + callback mode-picked(ViewMode); /// A dragged crop rect, in fractions of the frame. callback crop-changed(float, float, float, float); @@ -818,8 +835,15 @@ export component AppWindow inherits Window { in property overlay-clip-h; /// The straightening angle, so the overlay turns with the frame. in property overlay-angle: 0.0; - /// Clicking the canvas selects a region instead of panning. - in property region-picking: false; + + /// Whether a click on the canvas selects a region instead of panning. + /// + /// **Derived, not toggled.** It used to be a button in the masking panel, + /// which meant local masking could be open with picking off — a mode + /// entered and doing nothing. Arming a click is part of what local mode + /// *is*, so it follows the mode and the existence of something to pick. + property region-picking: root.local-mode && root.segmented; + /// Whether shift is down, tracked by the develop key scope below. /// /// A `TouchArea`'s `clicked` carries no modifiers, so the state has to be @@ -831,11 +855,26 @@ export component AppWindow inherits Window { in property <[SubjectRow]> subject-rows; in property segmented: false; in property segmenting: false; - in property editing-mask: false; + /// The selected layer's name, already resolved, or "ADJUST" when the + /// controls are global. Names the adjust panel's heading. + in property adjust-scope: "ADJUST"; + + /// The selected gradient's handles, in fractions of the shown image. + /// Empty unless a gradient layer is selected in local mode. + in property <[GradientHandle]> gradient-handles; + /// A handle dragged: which one, where the press was and where the pointer + /// is now, both in fractions of the shown image. + /// + /// The press is carried rather than a running delta because the geometry + /// is re-derived from the position it had when the drag began — which is + /// what stops a drag accumulating rounding error along its length, the + /// same reason the crop handles capture the rect they started from. + callback gradient-handle-dragged(HandleRole, float, float, float, float); + /// A drag finished, so the edit can be recorded as one history step + /// rather than as one per frame of the gesture. + callback gradient-handle-released(); callback segment-image(); - callback overlay-toggled(bool); - callback region-picking-toggled(bool); /// A click on the photograph, in fractions of the shown image, plus /// whether it should extend the selection rather than replace it. callback region-picked(float, float, bool); @@ -1417,7 +1456,7 @@ in property panel-visible: true; x: 0; y: 0; width: 100%; height: 100%; - enabled: !root.crop-mode; + enabled: !root.cropping; property last-scale: 1.0; @@ -1448,7 +1487,7 @@ in property panel-visible: true; width: parent.shown-w; height: parent.shown-h; mouse-cursor: MouseCursor.crosshair; - enabled: !root.crop-mode; + enabled: !root.cropping; clicked => { root.region-picked( @@ -1468,7 +1507,7 @@ in property panel-visible: true; // A pan is only meaningful once there is something outside // the viewport to reach. mouse-cursor: root.zoomed ? MouseCursor.grab : MouseCursor.default; - enabled: !root.crop-mode; + enabled: !root.cropping; property last-x; property last-y; @@ -1517,7 +1556,7 @@ in property panel-visible: true; // Four dimmed, desaturated panels around the crop, then the // rect itself with handles. Panels rather than one shape with // a hole: Slint has no cut-out, and four rectangles are exact. - if root.crop-mode && root.total > 0 && root.load-error == "": crop-overlay := Rectangle { + if root.cropping && root.total > 0 && root.load-error == "": crop-overlay := Rectangle { x: parent.shown-x; y: parent.shown-y; width: parent.shown-w; @@ -1700,6 +1739,106 @@ in property panel-visible: true; } } + // --- gradient handles --------------------------------------- + // + // TRACES: FR-DEV-3 | FR-UI-3 + // A linear or radial mask could be made and then never + // moved: there was nothing to grab, so a radial sat at the + // centre of the frame at its default size for ever. + // + // **Positioned against `shown-*`, like the crop overlay + // and the region picker.** The photograph is letterboxed + // inside this box and Slint does not report the fitted + // rect, so anything that has to land *on* the picture is + // placed against the derived one. Rust supplies the + // positions already mapped through the framing — the same + // map the shader applies — so a handle follows the mask + // through a zoom, a pan, a crop and a straightening rather + // than sitting where the mask used to be. + // + // **Drawn small, grabbed large.** The visible dot is 14px + // because a bigger one would hide the edge it is placed + // on, and the `TouchArea` is a full touch target, centred + // on it — `Button` establishes the same split. These are + // the first controls in the application meant to be + // dragged on the photograph, and a 12-inch tablet has no + // hover to reveal them with and no modifier to qualify + // them by, so what is drawn is all there is (FR-UI-7). + for handle in root.gradient-handles: Rectangle { + x: parent.shown-x + handle.x * parent.shown-w - self.width / 2; + y: parent.shown-y + handle.y * parent.shown-h - self.height / 2; + width: Theme.touch-target; + height: Theme.touch-target; + + // The centre moves the whole mask, so it is filled; + // the others shape it and are rings. Shape rather than + // colour, because the handles sit on a photograph and + // any colour they carried would be read as part of it. + property solid: handle.role == HandleRole.centre; + + Rectangle { + width: handle.role == HandleRole.rotate ? 12px : 14px; + height: self.width; + x: (parent.width - self.width) / 2; + y: (parent.height - self.height) / 2; + border-radius: self.width / 2; + border-width: 2px; + // White with a dark ring, so the handle is visible + // against a blown sky and against a black frame — + // one treatment, both extremes, no theme token + // because this is drawn over the image and not over + // the interface. + border-color: #00000099; + background: parent.solid ? #ffffff : #ffffff44; + } + + drag := TouchArea { + width: 100%; + height: 100%; + mouse-cursor: parent.solid + ? MouseCursor.move + : MouseCursor.crosshair; + + // Where the press landed, in the same fractions the + // callback reports in. Captured on the way down so + // the whole gesture is measured from one origin. + property from-x; + property from-y; + + function fraction-x(local-x: length) -> float { + return (parent.x + local-x - canvas-area.shown-x) + / max(canvas-area.shown-w, 1px); + } + function fraction-y(local-y: length) -> float { + return (parent.y + local-y - canvas-area.shown-y) + / max(canvas-area.shown-h, 1px); + } + + pointer-event(ev) => { + if (ev.kind == PointerEventKind.down) { + self.from-x = self.fraction-x(self.pressed-x); + self.from-y = self.fraction-y(self.pressed-y); + } + // One history step per gesture, not per frame. + if (ev.kind == PointerEventKind.up) { + root.gradient-handle-released(); + } + } + + moved => { + if (self.pressed) { + root.gradient-handle-dragged( + handle.role, + self.from-x, + self.from-y, + self.fraction-x(self.mouse-x), + self.fraction-y(self.mouse-y), + ); + } + } + } + } + // Arrow keys and space step through the folder. // // Focused on show rather than waiting for a click, exactly @@ -1757,19 +1896,31 @@ in property panel-visible: true; // beside what it is reporting on. Crop moved to the panel // because it is an edit, and edits live with the other edits. // - // A crop exit is still reachable from here while cropping — - // the panel may be collapsed on a narrow window, and stranding - // the user in a mode with no visible way out is worse than one - // duplicated control. + // A way out of whichever mode is on is still reachable from + // here — the develop column may be closed on a narrow + // window, and the strip that enters a mode is pinned inside + // it. Stranding the user in a mode with no visible way out + // is worse than one duplicated control, and it is worse + // still now that there are two modes to be stranded in. if root.total > 0 && root.load-error == "": HorizontalLayout { x: 12px; y: parent.height - self.preferred-height - 12px; + // Bounded, or the row takes the canvas's whole width + // and stretches its children across it. With one + // button that read as an odd-looking "Fit"; with two it + // reads as a toolbar laid over the photograph, which is + // the opposite of what a floating control should do. + width: self.preferred-width; spacing: 6px; + alignment: start; - if root.crop-mode: Button { - text: "Done"; + if root.view-mode != ViewMode.photo: Button { + // Names the mode being left rather than saying + // "Done", which was unambiguous while there was + // one mode and would not be with two. + text: root.cropping ? "Done Cropping" : "Done Masking"; active: true; - clicked => { root.crop-mode-toggled(false); } + clicked => { root.mode-picked(ViewMode.photo); } } // Zoom is a view state, so its readout doubles as the @@ -1843,25 +1994,45 @@ in property panel-visible: true; // would both sit at its origin and overlap. The strip is pinned by // being outside the Flickable rather than by any coordinate. VerticalLayout { - GroupStrip { + ModeStrip { enabled: root.adjust-enabled; + mode: root.view-mode; tabs: root.adjust-tabs; active-tab: root.adjust-active-tab; picked(i) => { root.adjust-tab-picked(i); } + mode-picked(m) => { root.mode-picked(m); } } Flickable { viewport-height: column.preferred-height; interactive: !adjust.slider-dragging; + // **What the column holds is the mode's answer.** + // + // In local mode it is the mask stack and then the + // selected layer's adjustments, and nothing else: + // the capture metadata describes the file, copy and + // paste move a whole edit between photographs, and + // framing is a decision about the picture's shape — + // none of the three is a property of a region + // within it, so all three would be controls in + // scope of nothing. + // + // **Each child carries its own condition** rather + // than the three being grouped inside one `if`. + // A nested layout under-reports its height here and + // the panels below it get drawn on top of each + // other — invisible in this file and obvious on + // screen. `masks.slint` carries the same note for + // the same reason. column := VerticalLayout { - InfoPanel { + if !root.local-mode: InfoPanel { camera: root.camera; exposure: root.exposure; dimensions: root.dimensions; } - Rectangle { + if !root.local-mode: Rectangle { height: 1px; background: Theme.rule; } @@ -1872,6 +2043,12 @@ in property panel-visible: true; // set by watching this move (FR-DSP-7). An instrument // below the sliders it reports on would have the // photographer looking away from it to use it. + // + // Kept in local mode, where it still reports the + // whole frame while the sliders edit a layer. That + // disagreement is real and is N3's to close; taking + // the instrument away instead would be a worse + // answer than an honest one that is not yet scoped. HistogramPanel { data: root.histogram; } @@ -1885,16 +2062,14 @@ in property panel-visible: true; // made rather than how it is applied: the frame is decided // by eye first and the pipeline runs it last (see // `dr_pipeline::framing` on the coordinate order). - GeometryPanel { + if !root.local-mode: GeometryPanel { enabled: root.adjust-enabled; - crop-mode: root.crop-mode; angle: root.straighten; max-straighten: root.max-straighten; flip-h: root.flip-h; flip-v: root.flip-v; modified: root.framing-modified; - crop-toggled(on) => { root.crop-mode-toggled(on); } rotate(turns) => { root.rotate-quarters(turns); } flip-h-toggled => { root.flip-h-toggled(); } flip-v-toggled => { root.flip-v-toggled(); } @@ -1903,7 +2078,7 @@ in property panel-visible: true; reset => { root.framing-reset(); } } - Rectangle { + if !root.local-mode: Rectangle { height: 1px; background: Theme.rule; } @@ -1913,7 +2088,7 @@ in property panel-visible: true; // burying it under thirty sliders would put the one // control that operates on all of them below all of // them. - TransferPanel { + if !root.local-mode: TransferPanel { enabled: root.adjust-enabled; armed: root.settings-armed; summary: root.settings-summary; @@ -1931,19 +2106,19 @@ in property panel-visible: true; // those sliders act on. Below it, the photographer // would set an exposure and only then discover which // scope it landed in. - MaskPanel { + // + // Only in local mode: it is no longer a panel among + // peers that silently re-points another panel, it is + // what that mode's column *is*. That also takes one + // panel out of a scrolling column that had six. + if root.local-mode: MaskPanel { enabled: root.adjust-enabled; masks: root.mask-rows; subjects: root.subject-rows; segmented: root.segmented; segmenting: root.segmenting; - overlay: root.overlay-on; - picking: root.region-picking; - editing-mask: root.editing-mask; segment => { root.segment-image(); } - overlay-toggled(on) => { root.overlay-toggled(on); } - picking-toggled(on) => { root.region-picking-toggled(on); } mask-selected(id) => { root.mask-selected(id); } mask-removed(id) => { root.mask-removed(id); } mask-toggled(id, on) => { root.mask-toggled(id, on); } @@ -1969,7 +2144,7 @@ in property panel-visible: true; add-subject(i) => { root.add-subject-mask(i); } } - Rectangle { + if root.local-mode: Rectangle { height: 1px; background: Theme.rule; } @@ -1977,6 +2152,7 @@ in property panel-visible: true; adjust := AdjustPanel { rows: root.adjust-rows; enabled: root.adjust-enabled; + scope: root.adjust-scope; curve-samples: root.curve-samples; param-changed(op, param, value) => { root.param-changed(op, param, value); @@ -1997,8 +2173,20 @@ in property panel-visible: true; } } + // The seam between the photograph and the column. + // + // **`x: 0` is load-bearing.** A child of a plain Rectangle + // that declares a size and no position is *centred* in it, + // so this hairline was being drawn straight down the middle + // of the develop column — over the histogram, the geometry + // controls and every slider below them. Invisible in the + // source and, at one pixel of `rule` grey, quiet enough on + // screen to be read as a divider that was meant to be + // there. Rectangle { + x: 0; width: 1px; + height: 100%; background: Theme.rule; } } diff --git a/ui/dr-ui/ui/masks.slint b/ui/dr-ui/ui/masks.slint index 715efd7..e606596 100644 --- a/ui/dr-ui/ui/masks.slint +++ b/ui/dr-ui/ui/masks.slint @@ -41,6 +41,25 @@ export struct MaskRow { adjusted: bool, } +/// Which part of a gradient a canvas handle drags. +/// +/// Named by the job rather than by the field, because the two gradients keep +/// different things in the same place: `edge` is a linear ramp's width and a +/// radial's major semi-axis, and `cross` is the radial's minor one. +export enum HandleRole { centre, edge, cross, rotate } + +/// One draggable point on the photograph. +/// +/// Positioned in fractions of the image **as it is currently shown** — after +/// the crop, the zoom and the pan — because that is the only space this file +/// can draw in. Rust maps the mask's own source-space geometry into it on +/// every frame, so the handle sits on the mask rather than beside it. +export struct GradientHandle { + role: HandleRole, + x: float, + y: float, +} + /// Something the model found. export struct SubjectRow { index: int, @@ -224,19 +243,8 @@ export component MaskPanel inherits Rectangle { /// One is being computed now. in property segmenting: false; - /// Draw the false-coloured region map over the photograph. - in property overlay: false; - /// Clicking the photograph selects a region rather than panning. - in property picking: false; - /// A layer is selected, so the adjust panel below is scoped to it. - /// - /// Supplied rather than derived: Slint has no `any` over a model, and the - /// core already knows the answer. - in property editing-mask: false; callback segment(); - callback overlay-toggled(bool); - callback picking-toggled(bool); callback mask-selected(string); callback mask-removed(string); @@ -266,7 +274,7 @@ export component MaskPanel inherits Rectangle { alignment: start; HorizontalLayout { - PanelHeading { text: "LOCAL"; } + PanelHeading { text: "MASKS"; } Rectangle { horizontal-stretch: 1; } if root.segmented: Value { text: root.subjects.length + (root.subjects.length == 1 ? " subject" : " subjects"); } @@ -294,25 +302,12 @@ export component MaskPanel inherits Rectangle { clicked => { root.segment(); } } - // The overlay the whole map is judged by, and the picking mode that - // makes a click mean "select" instead of "pan". - if root.enabled && root.segmented: HorizontalLayout { - spacing: Theme.gap-sm; - - Button { - text: "Overlay"; - active: root.overlay; - clicked => { root.overlay-toggled(!root.overlay); } - } - - Button { - text: "Select"; - active: root.picking; - clicked => { root.picking-toggled(!root.picking); } - } - } - - if root.enabled && root.segmented && root.picking: Caption { + // No "Overlay" button and no "Select" button. Both switched on things + // that are now simply what local mode *is*: entering it draws the + // region map and makes a click on the photograph mean "select". A mode + // whose behaviour has to be armed separately is a mode that can be + // entered and still do nothing, which is what these two allowed. + if root.enabled && root.segmented: Caption { text: "Click a subject in the photograph to mask it."; wrap: word-wrap; } @@ -402,14 +397,12 @@ export component MaskPanel inherits Rectangle { morph-radius-changed(v) => { root.mask-morph-radius-changed(mask.id, v); } } - // The one thing this panel has to say about the adjust panel below it, - // because otherwise selecting a layer silently changes what those - // sliders mean — the most confusing thing a scoped panel can do. - if root.enabled && root.masks.length > 0: Caption { - text: root.editing-mask - ? "The controls below adjust the selected mask." - : "The controls below adjust the whole photograph."; - wrap: word-wrap; - } + // No caption saying which chain the sliders below are pointed at. + // + // That sentence used to be the *only* indication that selecting a + // layer had silently re-scoped thirty controls, and describing a + // hazard in a caption is not the same as removing it. The adjust + // panel's own heading names the layer it is editing now, which puts + // the answer on the thing that changed rather than above it. } }