diff --git a/core/dr-gpu/src/shaders/mask.wgsl b/core/dr-gpu/src/shaders/mask.wgsl index a108b4d..8cbe3ba 100644 --- a/core/dr-gpu/src/shaders/mask.wgsl +++ b/core/dr-gpu/src/shaders/mask.wgsl @@ -39,7 +39,9 @@ struct MaskParams { // `falloff_code` on the Rust side. falloff: u32, - // Geometry, in normalised output coordinates. Meaning depends on `mode`. + // Geometry. Meaning depends on `mode`. The centre is in normalised 0..1 + // coordinates; every distance below it is in the isotropic frame units + // `frame_delta` establishes. centre: vec2, // Linear: (cos, sin) of the ramp direction. Radial: semi-axes. axis: vec2, @@ -124,9 +126,23 @@ fn region_mask(px: vec2) -> f32 { return total / n; } +// Offset from a gradient's centre, in the frame's own **isotropic** units: +// y spans 0..1 and x spans 0..aspect, so a step of the same length means the +// same distance whichever way it points. +// +// Without this the geometry lives in raw 0..1, where one axis is compressed +// against the other by the aspect ratio — so a 45° ramp is not at 45° on +// anything but a square frame, and a radial with equal radii draws an ellipse. +// Both faults are invisible in the stored numbers and obvious the moment a +// handle is dragged on a photograph, which is what this exists for. +fn frame_delta(uv: vec2) -> vec2 { + let aspect = vec2(f32(p.width) / f32(max(p.height, 1u)), 1.0); + return (uv - p.centre) * aspect; +} + fn linear_mask(uv: vec2) -> f32 { // Signed distance along the ramp direction, from the centre. - let d = dot(uv - p.centre, p.axis); + let d = dot(frame_delta(uv), p.axis); if (p.softness <= 0.0) { return select(0.0, 1.0, d >= 0.0); } @@ -136,7 +152,7 @@ fn linear_mask(uv: vec2) -> f32 { fn radial_mask(uv: vec2) -> f32 { let ca = cos(-p.angle); let sa = sin(-p.angle); - let d = uv - p.centre; + let d = frame_delta(uv); // Into the ellipse's own frame, then normalised by its semi-axes so the // problem becomes a unit circle. let local = vec2(d.x * ca - d.y * sa, d.x * sa + d.y * ca); diff --git a/core/dr-gpu/tests/masked_outputs.rs b/core/dr-gpu/tests/masked_outputs.rs index 33c6d1e..1dad59c 100644 --- a/core/dr-gpu/tests/masked_outputs.rs +++ b/core/dr-gpu/tests/masked_outputs.rs @@ -189,3 +189,135 @@ fn the_mask_covers_the_same_fraction_at_every_size() { "and that share is the left half: {small:.3}" ); } + +// --------------------------------------------------------------------------- +// Gradient geometry +// +// The mask is rasterised over the source frame, whose two axes are not the +// same length. A gradient measured in raw 0..1 fractions therefore means a +// different distance horizontally than vertically — so a circle comes out an +// ellipse and an angle is not the angle asked for. Neither shows in the stored +// numbers, and both are what a photographer is looking straight at while +// dragging a handle. +// --------------------------------------------------------------------------- + +/// A flat grey frame at an arbitrary shape, brightened wherever `stack` covers. +fn render_gradient(ctx: &GpuContext, stack: &MaskStack, w: u32, h: u32) -> Vec { + let data: Vec = (0..w * h).flat_map(|_| [128u8, 128, 128, 255]).collect(); + let source = DemosaicedImage::from_rgba8(ctx, &data, w, h).expect("upload"); + + let mut masks = MaskPass::new(ctx).expect("mask pass"); + let array = masks.render(stack, None, None, w, h).expect("rasterise"); + + let shader = compose_full(&ops::chain(), &Framing::new(), ColourSpace::Srgb, stack); + let mut adjust = AdjustPass::new(ctx); + adjust + .render_masked(&source, &shader, w, h, Some(array)) + .expect("render"); + adjust.export_pixels().expect("readback").0 +} + +fn brightened(pixels: &[u8], w: u32, x: u32, y: u32) -> bool { + pixels[((y * w + x) * 4) as usize] > 160 +} + +fn gradient(source: MaskSource) -> MaskStack { + let mut stack = MaskStack::new(); + let mut layer = MaskLayer::new("g1", source); + layer.set_param("exposure", ParamId("exposure"), 2.0); + stack.push(layer); + stack +} + +/// A radial with equal radii must be round on the screen, not on the numbers. +#[test] +fn a_radial_with_equal_radii_is_a_circle_on_a_wide_frame() { + let Some(ctx) = ctx() else { + eprintln!("no adapter; skipping"); + return; + }; + // 3:2. On a square frame this test cannot fail, which is why it is not one. + const W: u32 = 96; + const H: u32 = 64; + + // 0.3 of the frame's height. In pixels that is 19 either way — the x axis + // spans 0..1.5 in the same units, so the fraction is smaller and the + // distance is the same. + let pixels = render_gradient( + &ctx, + &gradient(MaskSource::Radial { + centre: (0.5, 0.5), + radii: (0.3, 0.3), + angle: 0.0, + // Hard, so "covered" is a question with an answer rather than a + // ramp to pick a threshold out of. + feather: 0.0, + }), + W, + H, + ); + + let (cx, cy) = (W / 2, H / 2); + assert!(brightened(&pixels, W, cx, cy), "the centre must be covered"); + + for (dx, dy, what) in [(15u32, 0u32, "right"), (0, 15, "down")] { + assert!( + brightened(&pixels, W, cx + dx, cy + dy), + "15px {what} of centre is inside a 19px radius" + ); + } + for (dx, dy, what) in [(24u32, 0u32, "right"), (0, 24, "down")] { + assert!( + !brightened(&pixels, W, cx + dx, cy + dy), + "24px {what} of centre is outside it — before the aspect \ + correction the horizontal reach was 28px and this passed only \ + downwards" + ); + } +} + +/// And a ramp at 45° must be at 45° where the photographer sees it. +#[test] +fn a_diagonal_ramp_runs_at_the_angle_it_was_given() { + let Some(ctx) = ctx() else { + eprintln!("no adapter; skipping"); + return; + }; + const W: u32 = 96; + const H: u32 = 64; + + let pixels = render_gradient( + &ctx, + &gradient(MaskSource::Linear { + centre: (0.5, 0.5), + angle: std::f32::consts::FRAC_PI_4, + width: 0.0, + }), + W, + H, + ); + + // Coverage increases along (cos 45°, sin 45°), so the half-way line runs + // from lower-left to upper-right through the centre: equal steps right and + // down stay on the covered side, and the two sides of it disagree. + let (cx, cy) = (W / 2, H / 2); + assert!( + brightened(&pixels, W, cx + 16, cy + 16), + "down and to the right of the line is inside" + ); + assert!( + !brightened(&pixels, W, cx - 16, cy - 16), + "up and to the left of it is outside" + ); + // The diagonal itself, sampled a few pixels either side. Without the + // aspect correction the line comes out at 34 degrees and both of these + // land on the same side of it. + assert!( + brightened(&pixels, W, cx + 18, cy - 14), + "just below the 45 degree line is inside" + ); + assert!( + !brightened(&pixels, W, cx + 14, cy - 18), + "just above it is not" + ); +} diff --git a/core/dr-pipeline/src/mask.rs b/core/dr-pipeline/src/mask.rs index 3e44ac3..3aaeb78 100644 --- a/core/dr-pipeline/src/mask.rs +++ b/core/dr-pipeline/src/mask.rs @@ -283,25 +283,44 @@ pub enum MaskSource { /// A linear gradient — the graduated-filter mask. /// - /// Geometry is in **normalised output coordinates**, so it survives a crop - /// or an export at another size. Storing pixels would make a mask that - /// silently moves when the frame changes. + /// Geometry is in **normalised source coordinates**, so it survives a crop, + /// a zoom or an export at another size: the mask is rasterised in source + /// space and sampled through the framing map, exactly as a subject mask is. + /// Storing pixels would make a mask that silently moves when the frame + /// changes. + /// + /// # Two spaces, and why both are here + /// + /// The **centre** is a point, so it is a plain `0.0..=1.0` fraction of each + /// axis — the same coordinates a click carries. + /// + /// The **angle and the width are measurements**, and a fraction of the + /// width is not the same length as a fraction of the height on any frame + /// that is not square. So they are in the frame's *isotropic* units: y + /// spans `0..1` and x spans `0..aspect`, which is what makes 45° actually + /// 45° and a circle actually round. `frame_delta` in `mask.wgsl` is the + /// one place that conversion happens, and it must stay the only one. Linear { /// Midpoint of the ramp, `0.0..=1.0` in each axis. centre: (f32, f32), - /// Radians, measured from the +x axis. + /// Radians, measured clockwise from the +x axis in frame units. + /// Coverage increases in this direction. angle: f32, - /// Distance from full effect to none, in normalised units. Zero is a - /// hard edge. + /// Distance from full effect to none, in frame units. Zero is a hard + /// edge. width: f32, }, /// A radial gradient — the classic vignette-shaped local adjustment. + /// + /// The same two spaces as [`Self::Linear`]: a `0..1` centre, and semi-axes + /// measured in the frame's isotropic units so equal radii draw a circle. Radial { centre: (f32, f32), - /// Semi-axes, normalised. Two of them, because a face is an ellipse - /// and forcing a circle makes the user compensate with a crop. + /// Semi-axes, in frame units. Two of them, because a face is an + /// ellipse and forcing a circle makes the user compensate with a crop. radii: (f32, f32), + /// Rotation of the ellipse, radians. angle: f32, /// Fraction of the radius over which the edge falls off. feather: f32, diff --git a/core/dr-pipeline/tests/mask_sidecar.rs b/core/dr-pipeline/tests/mask_sidecar.rs index fcc02cd..3b25ef9 100644 --- a/core/dr-pipeline/tests/mask_sidecar.rs +++ b/core/dr-pipeline/tests/mask_sidecar.rs @@ -421,7 +421,10 @@ fn show_a_sidecar() { "m2", MaskSource::Linear { centre: (0.5, 0.25), - angle: 1.5708, + // The constant rather than four digits of it: clippy rejects the + // literal, and a quarter turn written as a number is a quarter + // turn nobody can see at a glance. + angle: std::f32::consts::FRAC_PI_2, width: 0.4, }, );