diff --git a/core/dr-gpu/tests/tone_curve.rs b/core/dr-gpu/tests/tone_curve.rs new file mode 100644 index 0000000..b12eea0 --- /dev/null +++ b/core/dr-gpu/tests/tone_curve.rs @@ -0,0 +1,139 @@ +//! TRACES: FR-DEV-3 +//! The tone curve's four curves, on a device. +//! +//! `dr-pipeline` asserts that the right WGSL is generated and `dr-gpu`'s other +//! tests assert that a shader runs; neither notices a fragment that says +//! exactly what it should and does not compile, or one that compiles and puts +//! the red curve's uniforms into the blue slot. So this renders flat grey +//! through each curve and looks at what came out. +//! +//! Flat grey because it makes every assertion a comparison between the three +//! components of one pixel: a curve that is meant to be chromatic must move +//! them apart, and one that is meant to be tonal must not. + +use dr_gpu::{AdjustPass, DemosaicedImage, GpuContext}; +use dr_pipeline::ops::curve::{self, Axis, Channel}; +use dr_pipeline::EditGraph; + +const SIZE: u32 = 8; + +fn ctx() -> Option { + pollster::block_on(GpuContext::new_headless()).ok() +} + +/// The centre pixel's red, green and blue, after `graph` has run over flat +/// mid-grey. +fn rendered(ctx: &GpuContext, graph: &EditGraph) -> (u8, u8, u8) { + let data: Vec = (0..SIZE * SIZE).flat_map(|_| [128, 128, 128, 255]).collect(); + let source = DemosaicedImage::from_rgba8(ctx, &data, SIZE, SIZE).expect("upload"); + + // Composed the way the display path composes it. A curve that generates + // invalid WGSL fails at `render` below, which is the point of running this + // on a device at all. + let shader = graph.compose(); + + let mut adjust = AdjustPass::new(ctx); + adjust.render(&source, &shader, SIZE, SIZE).expect("render"); + let pixels = adjust.export_pixels().expect("readback").0; + + let at = ((SIZE / 2 * SIZE + SIZE / 2) * 4) as usize; + (pixels[at], pixels[at + 1], pixels[at + 2]) +} + +/// A curve with its mid-point lifted — the simplest edit that is unmistakably +/// an edit. +fn lifted(channel: Channel) -> EditGraph { + let mut graph = EditGraph::default_chain(); + graph.set_param( + curve::ID, + curve::coordinate(channel, 2, Axis::Y), + 0.75, + ); + graph +} + +#[test] +fn the_master_curve_lifts_every_component_together() { + let Some(ctx) = ctx() else { + eprintln!("no adapter; skipping"); + return; + }; + + let (r0, g0, b0) = rendered(&ctx, &EditGraph::default_chain()); + let (r, g, b) = rendered(&ctx, &lifted(Channel::Master)); + + assert!(r > r0, "the master curve did not lift the image: {r} vs {r0}"); + // Grey in, grey out: the master curve is applied as a ratio over + // luminance, so it changes tone and not hue. A tolerance of one code + // value, because the components travel through the ratio separately and + // the result is quantised to eight bits. + assert!( + r.abs_diff(g) <= 1 && g.abs_diff(b) <= 1, + "the master curve tinted a neutral pixel: {r},{g},{b}" + ); + assert_eq!((g0, b0), (r0, r0), "the unedited image is neutral"); +} + +#[test] +fn a_channel_curve_lifts_only_its_own_component() { + let Some(ctx) = ctx() else { + eprintln!("no adapter; skipping"); + return; + }; + + let (r0, g0, b0) = rendered(&ctx, &EditGraph::default_chain()); + for (channel, name) in [ + (Channel::Red, "red"), + (Channel::Green, "green"), + (Channel::Blue, "blue"), + ] { + let (r, g, b) = rendered(&ctx, &lifted(channel)); + // The component the curve names moves; the other two stay exactly + // where they were. This is what catches a fragment whose uniforms are + // wired to the wrong curve — it would still lift *something*. + let (moved, still) = match channel { + Channel::Red => (r > r0, g == g0 && b == b0), + Channel::Green => (g > g0, r == r0 && b == b0), + _ => (b > b0, r == r0 && g == g0), + }; + assert!(moved, "the {name} curve changed nothing: {r},{g},{b}"); + assert!( + still, + "the {name} curve moved a component that was not its own: \ + {r},{g},{b} from {r0},{g0},{b0}" + ); + } +} + +#[test] +fn the_master_and_the_channels_compose_in_one_pass() { + // All four curves at once: the case where the generated fragment is + // longest, every helper is present, and forty uniforms are in the block. + // Mostly a compile check, which is why the assertion is only that the + // result is a colour and not the one we started with. + let Some(ctx) = ctx() else { + eprintln!("no adapter; skipping"); + return; + }; + + let mut graph = EditGraph::default_chain(); + graph.set_param(curve::ID, curve::P1_Y, 0.15); + graph.set_param(curve::ID, curve::P3_Y, 0.85); + for (channel, y) in [ + (Channel::Red, 0.55), + (Channel::Green, 0.5), + (Channel::Blue, 0.62), + ] { + graph.set_param(curve::ID, curve::coordinate(channel, 2, Axis::Y), y); + } + + let (r0, _, _) = rendered(&ctx, &EditGraph::default_chain()); + let (r, g, b) = rendered(&ctx, &graph); + assert!( + (r, g, b) != (r0, r0, r0), + "four active curves left the image untouched" + ); + // Red and blue were pushed apart from green, which is the chromatic half + // doing its work on top of the tonal one. + assert!(b > g, "blue was lifted above green: {r},{g},{b}"); +} diff --git a/core/dr-pipeline/ops/README.md b/core/dr-pipeline/ops/README.md index dfb8ec8..aa27c1a 100644 --- a/core/dr-pipeline/ops/README.md +++ b/core/dr-pipeline/ops/README.md @@ -209,9 +209,10 @@ They still belong in this directory, because the pipeline's **order** is the one thing a reader comes here to learn, and an order written half in YAML and half in Rust would be worse than either alone. -Currently hand-written: `tone_curve` (a curve widget over five interpolated -points), `colour_mixer` (thirty-six faceted parameters from twelve computed hue -bands). `vignetting` is hand-written too but is not in the develop chain — it +Currently hand-written: `tone_curve` (one widget over four curves of five +interpolated points — master, red, green, blue — each reaching the shader only +when it has been moved), `colour_mixer` (thirty-six faceted parameters from +twelve computed hue bands). `vignetting` is hand-written too but is not in the develop chain — it carries lens-profile coefficients that are not parameters. `distortion` and `aberration` are `Warp`s rather than operations: they rewrite coordinates before sampling rather than transforming a colour after it. diff --git a/core/dr-pipeline/ops/tone_curve.yaml b/core/dr-pipeline/ops/tone_curve.yaml index 91f9e3b..8f14477 100644 --- a/core/dr-pipeline/ops/tone_curve.yaml +++ b/core/dr-pipeline/ops/tone_curve.yaml @@ -16,13 +16,26 @@ attributes: [tone, colour] rust: ToneCurve why_rust: | - Five control points presented as one curve widget, with an interpolator and - a monotonicity guarantee behind it. Its neutral is a *relationship* between - parameters rather than a set of values — the identity diagonal — which is - not something the declarative `active:` rule can express, and its - `presentation()` spans parameters rather than describing one. + Four curves — master, red, green, blue — of five control points each, + presented as one widget, with an interpolator and a monotonicity guarantee + behind them. Its neutral is a *relationship* between parameters rather than + a set of values — the identity diagonal, on every channel — which is not + something the declarative `active:` rule can express; its `presentation()` + spans forty parameters rather than describing one; and its fragment is + *assembled* rather than written, because each curve reaches the shader only + when it has been moved off the diagonal. A declared node's `wgsl:` is one + fixed block of text, which is the right shape for nearly everything here and + the wrong one for a node whose cost has to follow what the photographer + actually touched. placement: | After the fixed-weight region controls, so the curve is the final word on tone: a photographer reaches for it to fix what those controls could not place exactly. + + Within the node, the master curve runs before the per-channel ones. Both + orders are visibly different images and the reasons for this one are written + out in `src/ops/curve.rs`: the master is tonal and hue-preserving, the + per-channel curves are the chromatic grade over the tones it produced, and a + point placed on a channel curve should act on the tone the photographer can + see rather than on the one the master is about to move. diff --git a/core/dr-pipeline/src/ops/curve.rs b/core/dr-pipeline/src/ops/curve.rs index 6bc0bd3..c2c6782 100644 --- a/core/dr-pipeline/src/ops/curve.rs +++ b/core/dr-pipeline/src/ops/curve.rs @@ -1,9 +1,13 @@ -//! The tone curve — a monotonic spline through five movable points. +//! TRACES: FR-DEV-3 +//! The tone curve — four monotonic splines through five movable points each: +//! a master curve over tone, and one per colour channel. //! //! The control every other tonal adjustment is a preset of. Highlights, //! shadows, blacks and whites each shape one region with a fixed weight; the //! curve lets the photographer put the inflection exactly where the image -//! needs it. +//! needs it. The per-channel curves are the same instrument pointed at colour: +//! a lifted blue black point is the faded shadow every film emulation is built +//! out of, and there is no way to ask for it with a saturation slider. //! //! # Why the points are ordinary scalars //! @@ -14,7 +18,7 @@ //! //! - The parameter API stays `f32`-only, so nothing else in the pipeline, //! the graph or the sidecar had to change to accommodate a curve. -//! - A UI that has not implemented the curve widget renders ten sliders and +//! - A UI that has not implemented the curve widget renders forty sliders and //! remains completely functional. //! - Undo, clamping and sidecar serialisation work already, because the //! points are the same kind of thing as every other parameter. @@ -23,28 +27,223 @@ //! need a variable-length value type, which is a much larger change for a //! control that rarely needs more than five. //! +//! [`ParamKind::Scalar`]: crate::descriptor::ParamKind::Scalar +//! //! # Why monotonic //! //! A plain cubic spline through user-placed points overshoots: drag one point //! and the curve can dip *below* its neighbour, which inverts tones locally //! and shows up as a dark halo in a smooth gradient. The Fritsch-Carlson //! filter constrains the tangents so the interpolant is monotone wherever the -//! data is, which is exactly the guarantee a tone curve needs. +//! data is, which is exactly the guarantee a tone curve needs. It is enforced +//! per curve, because "the master's points are in order" says nothing whatever +//! about the blue one's. +//! +//! # Four curves, and the order they run in +//! +//! The master curve runs **first**, and the per-channel curves run on the +//! colour it produced. The two orders are not cosmetically different — an +//! S-curve followed by a lifted blue black point is a visibly different image +//! from the blue lift followed by the S-curve — so the choice has to be made +//! here and stated, rather than left to whichever loop was written first. +//! +//! It is made this way for two reasons. +//! +//! **A control point's x coordinate should mean the tone the photographer can +//! see.** The channel curves are the finishing grade — warm the shadows, cool +//! the highlights — and the tones being graded are the ones on screen, which +//! are the master curve's output. Running the channels first would anchor them +//! to the tones the master is *about to move*: place a warm shadow, then reach +//! for contrast, and the warmth migrates up into the midtones as the master +//! lifts the region the channel curve was pinned to. In this order the master +//! reshapes what reaches the grade, and the grade stays where it was put on +//! the axis the widget draws. +//! +//! **Tone before colour is the order the rest of the chain already runs in.** +//! The master curve is hue-preserving by construction: it curves *luminance* +//! and reapplies the result as a ratio, exactly as contrast does, so it is a +//! tonal operation and nothing else. The per-channel curves deliberately break +//! that ratio — they are the only part of this operation that can change a +//! hue. Putting the chromatic half last keeps this node in step with the chain +//! around it, where the colour mixer is the finishing control and acts on the +//! tones the tonal operations have already settled (`ops/README.md`). +//! +//! # What four curves cost when three of them are untouched +//! +//! Nothing. Each curve is emitted into the fragment and into the uniform block +//! only when it differs from the identity, so the overwhelmingly common edit — +//! an S-curve on the master and no per-channel work at all — generates exactly +//! the shader it generated when this file held one curve, down to the uniform +//! names. An operation whose four curves are all identity is inactive and +//! contributes no code, no uniform and no branch, which is the property the +//! whole composition scheme rests on (ARCH §5.6). -use crate::descriptor::{Attribute, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Presentation, Scale, Unit, - WidgetDemand, WidgetKind,}; +use std::fmt::Write as _; + +use crate::descriptor::{ + Attribute, Facet, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Presentation, + Scale, Unit, WidgetDemand, WidgetKind, +}; use crate::operation::{Helper, Operation, Uniform}; use crate::ops::helpers; pub const ID: OpId = OpId("tone_curve"); -/// How many movable points the curve has. +/// How many movable points a curve has. /// /// Five: the two endpoints, a mid-tone, and one either side. Enough for the /// S-curves and shoulder rolls that make up nearly every tonal edit, few /// enough that the shader can evaluate them without a loop over storage. pub const POINTS: usize = 5; +/// TRACES: FR-DEV-3 +/// Which of the four curves a point belongs to. +/// +/// `Master` is the curve that existed before the other three, and it keeps +/// that position in every list here: it is the one a photographer reaches for +/// first, it is the one that runs first, and — see [`Channel::prefix`] — it is +/// the one whose parameter ids may not change. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Channel { + /// Tone, applied to all three components through a luminance ratio. + Master, + Red, + Green, + Blue, +} + +/// How many curves the operation carries. +pub const CHANNELS: usize = Channel::ALL.len(); + +impl Channel { + /// Every curve, in the order they are applied and presented. + /// + /// Master first because it runs first — see the module documentation for + /// why that is the composition order — and because a list showing the + /// grade before the tone would be describing a different operation. + pub const ALL: [Channel; 4] = [ + Channel::Master, + Channel::Red, + Channel::Green, + Channel::Blue, + ]; + + /// Position in [`Self::ALL`], and so in every array keyed by channel. + pub const fn index(self) -> usize { + match self { + Channel::Master => 0, + Channel::Red => 1, + Channel::Green => 2, + Channel::Blue => 3, + } + } + + /// TRACES: FR-CAT-8 + /// What this channel's parameter ids are prefixed with. + /// + /// **The master's prefix is empty, and that is a compatibility guarantee + /// rather than a saving of two characters.** A sidecar is the + /// authoritative store of an edit (ARCH §6.12) and it keys parameters by + /// `op.param` text, so `tone_curve.p2_y`, written by a build that had only + /// one curve, has to keep meaning the master's third point for as long as + /// those files exist. Every id that existed before the channels did is + /// therefore still spelled exactly as it was, and the new ones are spelled + /// differently — rather than the old ones being renamed into a scheme that + /// reads more evenly and loses every edit in the field. + /// + /// It is also why the channel is a *prefix*. A suffix would collide with + /// the axis — `p2_y_r` is one underscore from a point called `y_r` — and + /// the parse in [`ToneCurve::index_of`] would have to read the end of the + /// string to know how to read the beginning of it. + pub const fn prefix(self) -> &'static str { + match self { + Channel::Master => "", + Channel::Red => "r_", + Channel::Green => "g_", + Channel::Blue => "b_", + } + } + + /// The WGSL vector component this curve is applied to, or `None` for the + /// master, which acts on all three through luminance. + const fn component(self) -> Option<&'static str> { + match self { + Channel::Master => None, + Channel::Red => Some("r"), + Channel::Green => Some("g"), + Channel::Blue => Some("b"), + } + } + + /// The localisation key naming this curve. + /// + /// A key, not a word: resolving one needs a localiser and `core/` must not + /// depend on one (NFR-A11Y-1). It reaches the interface as a + /// [`Facet::subject`] — see [`facet_of`] — which is how a panel comes to + /// draw a channel selector without this file knowing that selectors exist. + pub const fn subject(self) -> LocalizedKey { + LocalizedKey(match self { + Channel::Master => "channel.rgb", + Channel::Red => "channel.red", + Channel::Green => "channel.green", + Channel::Blue => "channel.blue", + }) + } + + /// Where this channel sits on the hue wheel, in degrees. + /// + /// Data about the operation, not a decision about appearance: the red + /// curve genuinely acts on the primary at 0°. Whether a frontend draws a + /// swatch from it, and in what shade, is the frontend's to decide + /// (ARCH §4.3a) — which is why this is a number and not a colour. `None` + /// for the master, whose subject is tone rather than a colour. + pub const fn hue(self) -> Option { + match self { + Channel::Master => None, + Channel::Red => Some(0.0), + Channel::Green => Some(120.0), + Channel::Blue => Some(240.0), + } + } + + /// This channel's ten point parameters, x and y interleaved. + pub fn params(self) -> &'static [ParamId] { + let base = self.index() * POINTS * 2; + &CURVE_PARAMS[base..base + POINTS * 2] + } +} + +/// Which coordinate of a point, for [`coordinate`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Axis { + X, + Y, +} + +/// TRACES: FR-DEV-3 +/// The parameter id for one coordinate of one channel's curve. +/// +/// How a caller outside this module addresses a point. The alternative — forty +/// public constants — would write the layout of the parameter list into every +/// call site, where it would then have to agree forever; a caller that wants +/// point 2 of the blue curve says so. +/// +/// Panics if `point` is out of range, which is a programming error rather than +/// anything a file or a user can cause: ids arriving from a sidecar go through +/// [`ToneCurve::index_of`], which returns an option. +pub fn coordinate(channel: Channel, point: usize, axis: Axis) -> ParamId { + assert!(point < POINTS, "the curve has {POINTS} points"); + let axis = match axis { + Axis::X => 0, + Axis::Y => 1, + }; + CURVE_PARAMS[channel.index() * POINTS * 2 + point * 2 + axis] +} + +// The master curve's parameter ids, named because they were named before the +// channels existed: the tests, the develop example and the history's +// coalescing test all address points through them, and a sidecar in the field +// spells them exactly like this. pub const P0_X: ParamId = ParamId("p0_x"); pub const P0_Y: ParamId = ParamId("p0_y"); pub const P1_X: ParamId = ParamId("p1_x"); @@ -56,9 +255,53 @@ pub const P3_Y: ParamId = ParamId("p3_y"); pub const P4_X: ParamId = ParamId("p4_x"); pub const P4_Y: ParamId = ParamId("p4_y"); -/// The parameters the curve widget owns, in point order. -static CURVE_PARAMS: [ParamId; POINTS * 2] = - [P0_X, P0_Y, P1_X, P1_Y, P2_X, P2_Y, P3_X, P3_Y, P4_X, P4_Y]; +/// The ten point ids of one channel, in point order, x before y. +/// +/// A macro because `concat!` needs literals — the same reason the colour +/// mixer's band parameters are macro-generated — and because writing forty ids +/// out by hand is forty chances to transpose two characters in a way that +/// compiles and silently drives the wrong point. +macro_rules! channel_ids { + ($($prefix:literal),* $(,)?) => { + [$( + ParamId(concat!($prefix, "p0_x")), ParamId(concat!($prefix, "p0_y")), + ParamId(concat!($prefix, "p1_x")), ParamId(concat!($prefix, "p1_y")), + ParamId(concat!($prefix, "p2_x")), ParamId(concat!($prefix, "p2_y")), + ParamId(concat!($prefix, "p3_x")), ParamId(concat!($prefix, "p3_y")), + ParamId(concat!($prefix, "p4_x")), ParamId(concat!($prefix, "p4_y")), + )*] + }; +} + +/// The parameters the curve widget owns: every channel, in point order. +/// +/// The widget claims all forty, so a frontend that draws the curve draws all +/// four of them and no point appears a second time as a stray slider beneath +/// it. The prefixes are the ones [`Channel::prefix`] declares, spelled out +/// again here because `concat!` cannot call a function; `the_ids_match_the_ +/// channel_prefixes` is what stops the two drifting. +static CURVE_PARAMS: [ParamId; CHANNELS * POINTS * 2] = channel_ids!["", "r_", "g_", "b_"]; + +/// The uniform names each channel's fragment reads, x and y interleaved. +/// +/// Parallel to [`CURVE_PARAMS`] and deliberately its own table: uniform names +/// are the shader's business and parameter ids are the sidecar's, and tying +/// the two together would make a rename in one file change the meaning of the +/// other. The master's are unprefixed for the same reason its parameters are — +/// a master-only edit generates the shader it always generated, so nothing +/// that keyed on that source has to notice the channels arriving. +static UNIFORM_NAMES: [[&str; POINTS * 2]; CHANNELS] = [ + ["x0", "y0", "x1", "y1", "x2", "y2", "x3", "y3", "x4", "y4"], + [ + "r_x0", "r_y0", "r_x1", "r_y1", "r_x2", "r_y2", "r_x3", "r_y3", "r_x4", "r_y4", + ], + [ + "g_x0", "g_y0", "g_x1", "g_y1", "g_x2", "g_y2", "g_x3", "g_y3", "g_x4", "g_y4", + ], + [ + "b_x0", "b_y0", "b_x1", "b_y1", "b_x2", "b_y2", "b_x3", "b_y3", "b_x4", "b_y4", + ], +]; /// A coordinate parameter: 0…1 with enough precision to place a point /// exactly, and a default putting the curve on the identity diagonal. @@ -77,35 +320,86 @@ const fn coord(id: &'static str, label: &'static str, default: f32) -> ParamDesc ) } +/// TRACES: FR-DEV-3a +/// Where a coordinate sits in the operation's grid. +/// +/// **This is how the channel dimension reaches the interface without the +/// interface learning what a channel is.** The forty parameters are one +/// control — a point coordinate — applied to four subjects, which is exactly +/// what a [`Facet`] describes and the same shape the colour mixer uses for its +/// twelve hue bands. A panel that groups a widget's parameters by their +/// subject gets a four-way selector over the curves for free, names each entry +/// from the key the channel published, and never contains the word "red". +/// +/// A frontend is free to ignore all of it and render forty sliders; nothing +/// becomes unreachable, it merely reads as forty anonymous coordinates. +const fn facet_of(aspect: &'static str, channel: Channel) -> Facet { + Facet { + // What this parameter adjusts: one coordinate of one point. Four + // parameters share it — the same point on each of the four curves. + aspect: LocalizedKey(aspect), + // What it adjusts it on. + subject: channel.subject(), + subject_hue: channel.hue(), + } +} + +/// One channel's ten descriptors, defaulted onto the identity diagonal. +/// +/// Written out per point rather than looped because a `ParamDescriptor` has to +/// be `const` to live in a `static`, and a const loop cannot build a slice. +macro_rules! channel_params { + ($(($prefix:literal, $channel:expr)),* $(,)?) => { + &[$( + coord(concat!($prefix, "p0_x"), "param.curve.p0_x", 0.0) + .faceted(facet_of("param.curve.p0_x", $channel)), + coord(concat!($prefix, "p0_y"), "param.curve.p0_y", 0.0) + .faceted(facet_of("param.curve.p0_y", $channel)), + coord(concat!($prefix, "p1_x"), "param.curve.p1_x", 0.25) + .faceted(facet_of("param.curve.p1_x", $channel)), + coord(concat!($prefix, "p1_y"), "param.curve.p1_y", 0.25) + .faceted(facet_of("param.curve.p1_y", $channel)), + coord(concat!($prefix, "p2_x"), "param.curve.p2_x", 0.5) + .faceted(facet_of("param.curve.p2_x", $channel)), + coord(concat!($prefix, "p2_y"), "param.curve.p2_y", 0.5) + .faceted(facet_of("param.curve.p2_y", $channel)), + coord(concat!($prefix, "p3_x"), "param.curve.p3_x", 0.75) + .faceted(facet_of("param.curve.p3_x", $channel)), + coord(concat!($prefix, "p3_y"), "param.curve.p3_y", 0.75) + .faceted(facet_of("param.curve.p3_y", $channel)), + coord(concat!($prefix, "p4_x"), "param.curve.p4_x", 1.0) + .faceted(facet_of("param.curve.p4_x", $channel)), + coord(concat!($prefix, "p4_y"), "param.curve.p4_y", 1.0) + .faceted(facet_of("param.curve.p4_y", $channel)), + )*] + }; +} + static DESCRIPTOR: OpDescriptor = OpDescriptor { - // Both, and this is the case the plural exists for: an RGB curve is + // Both, and this is the case the plural exists for: the master curve is // tonal and the per-channel curves are chromatic. Filing it under one // would hide it from half the people looking for it. attributes: &[Attribute::Tone, Attribute::Colour], id: ID, label: LocalizedKey("op.tone_curve"), // Defaults lie on y = x, so a fresh curve is the identity and the - // operation reports itself inactive. - params: &[ - coord("p0_x", "param.curve.p0_x", 0.0), - coord("p0_y", "param.curve.p0_y", 0.0), - coord("p1_x", "param.curve.p1_x", 0.25), - coord("p1_y", "param.curve.p1_y", 0.25), - coord("p2_x", "param.curve.p2_x", 0.5), - coord("p2_y", "param.curve.p2_y", 0.5), - coord("p3_x", "param.curve.p3_x", 0.75), - coord("p3_y", "param.curve.p3_y", 0.75), - coord("p4_x", "param.curve.p4_x", 1.0), - coord("p4_y", "param.curve.p4_y", 1.0), + // operation reports itself inactive — on every channel. + // + // The master's ten come first, and stay first: a frontend addresses a + // point by its offset from the first parameter of the run it is drawing, + // and this is also the order one falling back to sliders reads them in. + params: channel_params![ + ("", Channel::Master), + ("r_", Channel::Red), + ("g_", Channel::Green), + ("b_", Channel::Blue), ], }; -static CURVE_HELPERS: &[Helper] = &[ - helpers::LUMINANCE, - helpers::APPLY_TONE_GAIN, - Helper { - name: "curve_span", - source: "\ +/// One span of a monotone cubic Hermite spline. Shared by all four curves. +const CURVE_SPAN: Helper = Helper { + name: "curve_span", + source: "\ // One span of a monotone cubic Hermite spline. // // Takes the span's endpoints and the secants either side of it, rather than @@ -158,15 +452,20 @@ fn curve_span( return h00 * y0 + h10 * h * m0 + h01 * y1 + h11 * h * m1; }", - }, - Helper { - name: "curve_eval", - source: "\ -// Evaluate the five-point tone curve at `x`. +}; + +/// The five-point evaluation. Shared by all four curves — one function, called +/// with whichever curve's points the caller holds, rather than four copies +/// that could be improved one at a time. +const CURVE_EVAL: Helper = Helper { + name: "curve_eval", + source: "\ +// Evaluate a five-point curve at `x`. // // Spans are unrolled and secants passed explicitly; see `curve_span` for why // there is no array indexing here. Points arrive pre-sorted with a minimum -// separation enforced on the CPU, so no division can be by zero. +// separation enforced on the CPU — per curve, so one channel's points cannot +// be rescued by another's — so no division can be by zero. fn curve_eval( x0: f32, y0: f32, x1: f32, y1: f32, x2: f32, y2: f32, x3: f32, y3: f32, x4: f32, y4: f32, x: f32, @@ -188,19 +487,102 @@ fn curve_eval( if (x < x3) { return curve_span(x2, y2, x3, y3, s1, s3, x); } return curve_span(x3, y3, x4, y4, s2, s3, x); }", - }, +}; + +/// One colour component through its own curve. +const CHANNEL_CURVE: Helper = Helper { + name: "channel_curve", + source: "\ +// One colour component through its own curve, on the display-referred axis. +// +// The same encode-curve-decode as the master's, and for the same reason: the +// widget draws a 0..1 grid, so a point placed at the middle of it has to mean +// the middle of the visible range rather than the middle of an unbounded +// scene-referred one. +// +// What differs is that this is applied to the component *directly* rather than +// as a ratio over luminance. That is the whole point of a per-channel curve — +// it changes the proportions between the components, which is what makes it +// chromatic where the master is tonal. +// +// The clamp is the curve's promise rather than an oversight: its last point +// *is* white, so a component arriving above the axis takes the value the curve +// gives at 1. The master does the same to a luminance above 1, through the +// gain it applies; a channel curve that instead let highlights past unchanged +// would tint them differently from every tone below them, which reads as a +// coloured fringe along a blown edge. +fn channel_curve( + v: f32, + x0: f32, y0: f32, x1: f32, y1: f32, x2: f32, y2: f32, + x3: f32, y3: f32, x4: f32, y4: f32, +) -> f32 { + let encoded = pow(clamp(v, 0.0, 1.0), 1.0 / 2.2); + let curved = curve_eval(x0, y0, x1, y1, x2, y2, x3, y3, x4, y4, encoded); + return pow(clamp(curved, 0.0, 1.0), 2.2); +}", +}; + +// The three helper sets, one per shape of edit. +// +// Chosen rather than assembled because [`Operation::helpers`] hands back a +// `&'static [Helper]` and there is nowhere to build a list at call time. Three +// statics rather than one union so a master-only edit — the common case — +// declares no function it does not call, and a grade with no tonal work does +// not drag in the luminance machinery it has no use for. + +/// The master curve alone. +static MASTER_HELPERS: &[Helper] = &[ + helpers::LUMINANCE, + helpers::APPLY_TONE_GAIN, + CURVE_SPAN, + CURVE_EVAL, ]; -/// A tone curve through [`POINTS`] movable points. -#[derive(Debug, Clone)] -pub struct ToneCurve { +/// The per-channel curves alone. +static CHANNEL_HELPERS: &[Helper] = &[CURVE_SPAN, CURVE_EVAL, CHANNEL_CURVE]; + +/// Both. +static ALL_HELPERS: &[Helper] = &[ + helpers::LUMINANCE, + helpers::APPLY_TONE_GAIN, + CURVE_SPAN, + CURVE_EVAL, + CHANNEL_CURVE, +]; + +/// The master curve's fragment: tone, applied as a ratio so hue survives it. +const MASTER_BODY: &str = "\ +let luma = luminance(c); +if (luma > 0.0001) { + // The curve is authored on a display-referred 0..1 axis, which is where + // the eye reads tone and where the widget's grid lives. Scene-referred + // luminance is unbounded, so it is encoded to that axis, curved, and + // decoded back — otherwise a point placed at the middle of the grid + // would not correspond to the middle of the visible range. + let encoded = pow(clamp(luma, 0.0, 1.0), 1.0 / 2.2); + + let curved = curve_eval(x0, y0, x1, y1, x2, y2, x3, y3, x4, y4, encoded); + + let decoded = pow(clamp(curved, 0.0, 1.0), 2.2); + // Applied as a ratio so hue is preserved, exactly as contrast does. + c = apply_tone_gain(c, decoded / luma); +}"; + +/// A five-point monotone spline. +/// +/// One of these per channel. The point *values* live here and the parameter +/// *names* live in [`CURVE_PARAMS`], which is what lets the master keep the +/// ids it was born with while the code below stops caring which curve it is +/// holding. +#[derive(Debug, Clone, Copy)] +struct Curve { xs: [f32; POINTS], ys: [f32; POINTS], } -impl Default for ToneCurve { - fn default() -> Self { - // The identity diagonal. +impl Curve { + /// The identity diagonal. + const fn identity() -> Self { let mut xs = [0.0f32; POINTS]; let mut ys = [0.0f32; POINTS]; let mut i = 0; @@ -212,32 +594,15 @@ impl Default for ToneCurve { } Self { xs, ys } } -} - -impl ToneCurve { - pub fn new() -> Self { - Self::default() - } - - /// Map a parameter id to `(point index, is_y)`. - fn index_of(id: ParamId) -> Option<(usize, bool)> { - let (point, axis) = id.0.split_once('_')?; - let index: usize = point.strip_prefix('p')?.parse().ok()?; - if index >= POINTS { - return None; - } - match axis { - "x" => Some((index, false)), - "y" => Some((index, true)), - _ => None, - } - } /// The x coordinates, sorted and separated. /// /// The widget cannot reorder points, but a sidecar can carry anything and /// a spline through unordered or coincident x values divides by zero. - /// Enforced here so the shader never has to check. + /// Enforced here so the shader never has to check — and enforced on each + /// curve independently, because a NaN on the blue channel blanks the image + /// exactly as thoroughly as one on the master, and it is the one nobody + /// thinks to try. fn sorted_xs(&self) -> [f32; POINTS] { const MIN_GAP: f32 = 0.001; let mut xs = self.xs; @@ -259,7 +624,7 @@ impl ToneCurve { xs } - /// Whether the curve differs from the identity. + /// Whether this curve differs from the identity. fn differs_from_identity(&self) -> bool { self.xs .iter() @@ -268,6 +633,76 @@ impl ToneCurve { } } +/// TRACES: FR-DEV-3 +/// A master tone curve and one curve per colour channel. +#[derive(Debug, Clone)] +pub struct ToneCurve { + /// Indexed by [`Channel::index`]. + curves: [Curve; CHANNELS], +} + +impl Default for ToneCurve { + fn default() -> Self { + Self { + curves: [Curve::identity(); CHANNELS], + } + } +} + +impl ToneCurve { + pub fn new() -> Self { + Self::default() + } + + /// TRACES: FR-CAT-8 + /// Map a parameter id to `(channel, point index, is_y)`. + /// + /// **An id with no channel prefix is the master curve**, which is what + /// makes a sidecar written before the per-channel curves existed load and + /// mean what it meant: `p2_y` was the master's third point then and parses + /// to the master's third point now. Nothing needs a version check, because + /// nothing was renamed — the new curves took new names instead. + /// + /// Only the three known prefixes are recognised, so an id from a *newer* + /// build naming a curve this one does not have falls out as `None` and is + /// warned about, rather than being read as some other point. The sidecar + /// preserves the line either way (see [`crate::sidecar`]), so the edit + /// survives the round trip through a build that cannot apply it. + fn index_of(id: ParamId) -> Option<(Channel, usize, bool)> { + let (channel, rest) = match id.0.split_once('_') { + Some(("r", rest)) => (Channel::Red, rest), + Some(("g", rest)) => (Channel::Green, rest), + Some(("b", rest)) => (Channel::Blue, rest), + // No recognised prefix: the id names the master's own point, in + // the spelling it has always had. + _ => (Channel::Master, id.0), + }; + + let (point, axis) = rest.split_once('_')?; + let index: usize = point.strip_prefix('p')?.parse().ok()?; + if index >= POINTS { + return None; + } + match axis { + "x" => Some((channel, index, false)), + "y" => Some((channel, index, true)), + _ => None, + } + } + + fn curve(&self, channel: Channel) -> &Curve { + &self.curves[channel.index()] + } + + /// Whether any per-channel curve contributes anything. + fn channels_active(&self) -> bool { + Channel::ALL + .iter() + .filter(|c| c.component().is_some()) + .any(|c| self.curve(*c).differs_from_identity()) + } +} + impl Operation for ToneCurve { fn descriptor(&self) -> &'static OpDescriptor { &DESCRIPTOR @@ -275,22 +710,22 @@ impl Operation for ToneCurve { fn set_param(&mut self, id: ParamId, value: f32) { match Self::index_of(id) { - Some((i, true)) => self.ys[i] = value, - Some((i, false)) => self.xs[i] = value, + Some((c, i, true)) => self.curves[c.index()].ys[i] = value, + Some((c, i, false)) => self.curves[c.index()].xs[i] = value, None => log::warn!("tone_curve: unknown parameter {id}"), } } fn param(&self, id: ParamId) -> f32 { match Self::index_of(id) { - Some((i, true)) => self.ys[i], - Some((i, false)) => self.xs[i], + Some((c, i, true)) => self.curve(c).ys[i], + Some((c, i, false)) => self.curve(c).xs[i], None => 0.0, } } fn is_active(&self) -> bool { - self.differs_from_identity() + self.curves.iter().any(Curve::differs_from_identity) } fn presentation(&self) -> Option { @@ -305,88 +740,96 @@ impl Operation for ToneCurve { two_dimensional: true, precise_pointing: true, }, + // All four curves. A widget claiming only the master's ten would + // leave the other thirty stranded as sliders beneath the plot; + // which of the four it draws at a time is its own affair, and the + // facets are what let it decide without naming a channel. params: &CURVE_PARAMS, }) } fn wgsl_body(&self) -> String { - "\ -let luma = luminance(c); -if (luma > 0.0001) { - // The curve is authored on a display-referred 0..1 axis, which is where - // the eye reads tone and where the widget's grid lives. Scene-referred - // luminance is unbounded, so it is encoded to that axis, curved, and - // decoded back — otherwise a point placed at the middle of the grid - // would not correspond to the middle of the visible range. - let encoded = pow(clamp(luma, 0.0, 1.0), 1.0 / 2.2); + let mut body = String::new(); - let curved = curve_eval(x0, y0, x1, y1, x2, y2, x3, y3, x4, y4, encoded); + // Tone first, then colour on top of it — see the module documentation + // for why this order and not the other one. + if self.curve(Channel::Master).differs_from_identity() { + body.push_str(MASTER_BODY); + body.push('\n'); + } - let decoded = pow(clamp(curved, 0.0, 1.0), 2.2); - // Applied as a ratio so hue is preserved, exactly as contrast does. - c = apply_tone_gain(c, decoded / luma); -} -c = max(c, vec3(0.0));" - .into() + for channel in Channel::ALL { + let Some(component) = channel.component() else { + continue; + }; + // An untouched channel is not a curve evaluated to the identity; + // it is nothing at all in the generated source. + if !self.curve(channel).differs_from_identity() { + continue; + } + // The arguments come out of the same table the uniforms are + // declared from, so a call and its uniform block cannot disagree + // about a name. + let args = UNIFORM_NAMES[channel.index()].join(", "); + let _ = writeln!(body, "c.{component} = channel_curve(c.{component}, {args});"); + } + + // Whichever curves ran, the result has to be a colour: the spline's + // tangents can carry a point at the floor a very small distance below + // zero, and a negative component poisons every operation after this + // one. + body.push_str("c = max(c, vec3(0.0));"); + body } fn uniforms(&self) -> Vec { - let xs = self.sorted_xs(); - vec![ - Uniform { - name: "x0", - value: xs[0], - }, - Uniform { - name: "x1", - value: xs[1], - }, - Uniform { - name: "x2", - value: xs[2], - }, - Uniform { - name: "x3", - value: xs[3], - }, - Uniform { - name: "x4", - value: xs[4], - }, - Uniform { - name: "y0", - value: self.ys[0], - }, - Uniform { - name: "y1", - value: self.ys[1], - }, - Uniform { - name: "y2", - value: self.ys[2], - }, - Uniform { - name: "y3", - value: self.ys[3], - }, - Uniform { - name: "y4", - value: self.ys[4], - }, - ] + let mut out = Vec::new(); + for channel in Channel::ALL { + let curve = self.curve(channel); + // An untouched curve declares nothing, which is what makes three + // unused curves cost nothing rather than thirty uniform slots. + if !curve.differs_from_identity() { + continue; + } + let names = &UNIFORM_NAMES[channel.index()]; + let xs = curve.sorted_xs(); + for i in 0..POINTS { + out.push(Uniform { + name: names[i * 2], + value: xs[i], + }); + out.push(Uniform { + name: names[i * 2 + 1], + value: curve.ys[i], + }); + } + } + out } fn helpers(&self) -> &'static [Helper] { - CURVE_HELPERS + match ( + self.curve(Channel::Master).differs_from_identity(), + self.channels_active(), + ) { + (true, false) => MASTER_HELPERS, + (false, true) => CHANNEL_HELPERS, + // Both — and the fourth case, neither, which the composer never + // asks because an inactive operation is skipped whole. + _ => ALL_HELPERS, + } } } -/// Evaluate the curve on the CPU. +/// Evaluate a curve on the CPU. /// /// The same maths as the shader, used by the widget to draw the line it is /// editing. Duplicating it is deliberate: the alternative is a GPU readback /// per frame to draw a 200-pixel polyline (ARCH §6.1), and the shared tests /// below pin the two implementations to the same values. +/// +/// Takes points rather than a channel because it has no idea which curve it is +/// drawing and does not need one — four curves are four calls. pub fn evaluate(xs: &[f32; POINTS], ys: &[f32; POINTS], x: f32) -> f32 { if x <= xs[0] { return ys[0]; @@ -457,10 +900,13 @@ mod tests { use super::*; fn identity() -> ([f32; POINTS], [f32; POINTS]) { - let c = ToneCurve::new(); + let c = Curve::identity(); (c.xs, c.ys) } + /// The three curves that are not the master. + const COLOURS: [Channel; 3] = [Channel::Red, Channel::Green, Channel::Blue]; + #[test] fn a_fresh_curve_is_the_identity_and_inactive() { // Opening an unedited image must show the image. @@ -490,7 +936,83 @@ mod tests { p.id ); } - assert_eq!(DESCRIPTOR.params.len(), POINTS * 2); + assert_eq!(DESCRIPTOR.params.len(), CHANNELS * POINTS * 2); + } + + #[test] + fn the_ids_match_the_channel_prefixes() { + // `concat!` cannot call `Channel::prefix`, so the prefixes are written + // twice. This is what stops the two spellings drifting apart — which + // would produce a parameter the descriptor declares and `index_of` + // routes somewhere else. + for channel in Channel::ALL { + for (i, id) in channel.params().iter().enumerate() { + assert!( + id.0.starts_with(channel.prefix()), + "{id} is not on {channel:?}" + ); + let point = i / 2; + let is_y = i % 2 == 1; + assert_eq!(ToneCurve::index_of(*id), Some((channel, point, is_y))); + } + } + } + + #[test] + fn the_master_curves_parameters_are_spelled_as_they_always_were() { + // **The sidecar compatibility test.** These ten ids are written into + // every file produced before the per-channel curves existed, and a + // sidecar is the authoritative store of an edit (ARCH §6.12). + // Renaming one — to `m_p2_y`, say, for symmetry with `r_p2_y` — would + // silently drop that point from every edit in the field. + for (i, (x, y)) in [ + (P0_X, P0_Y), + (P1_X, P1_Y), + (P2_X, P2_Y), + (P3_X, P3_Y), + (P4_X, P4_Y), + ] + .into_iter() + .enumerate() + { + assert_eq!(ToneCurve::index_of(x), Some((Channel::Master, i, false))); + assert_eq!(ToneCurve::index_of(y), Some((Channel::Master, i, true))); + assert_eq!(coordinate(Channel::Master, i, Axis::X), x); + assert_eq!(coordinate(Channel::Master, i, Axis::Y), y); + assert!( + DESCRIPTOR.params.iter().any(|p| p.id == x), + "{x} left the descriptor" + ); + } + } + + #[test] + fn a_master_point_from_an_older_sidecar_still_moves_the_master_curve() { + // The same claim from the other end: the *value* arrives where it used + // to, not merely the name. + let mut c = ToneCurve::new(); + c.set_param(ParamId("p2_y"), 0.65); + assert_eq!(c.curve(Channel::Master).ys[2], 0.65); + for channel in COLOURS { + assert!( + !c.curve(channel).differs_from_identity(), + "{channel:?} moved when only the master was set" + ); + } + } + + #[test] + fn each_channel_owns_its_own_points() { + // The failure this guards is one array behind four names: set red, + // read blue, and see red's value. + let mut c = ToneCurve::new(); + for (channel, value) in COLOURS.into_iter().zip([0.6, 0.7, 0.8]) { + c.set_param(coordinate(channel, 2, Axis::Y), value); + } + assert_eq!(c.param(coordinate(Channel::Red, 2, Axis::Y)), 0.6); + assert_eq!(c.param(coordinate(Channel::Green, 2, Axis::Y)), 0.7); + assert_eq!(c.param(coordinate(Channel::Blue, 2, Axis::Y)), 0.8); + assert_eq!(c.param(P2_Y), 0.5, "the master must not have moved"); } #[test] @@ -501,6 +1023,18 @@ mod tests { assert_eq!(c.param(P2_Y), 0.65); } + #[test] + fn moving_a_channel_point_activates_the_operation() { + // An edit that touches only the blue curve is still an edit; an + // `is_active` that looked at the master alone would drop it from the + // shader and show the untouched image. + for channel in COLOURS { + let mut c = ToneCurve::new(); + c.set_param(coordinate(channel, 1, Axis::Y), 0.4); + assert!(c.is_active(), "{channel:?} did not activate the operation"); + } + } + #[test] fn the_curve_passes_through_its_control_points() { // The property that makes the widget honest: the line drawn through @@ -509,14 +1043,15 @@ mod tests { c.set_param(P1_Y, 0.15); c.set_param(P3_Y, 0.85); - let xs = c.sorted_xs(); + let master = c.curve(Channel::Master); + let xs = master.sorted_xs(); for i in 0..POINTS { - let y = evaluate(&xs, &c.ys, xs[i]); + let y = evaluate(&xs, &master.ys, xs[i]); assert!( - (y - c.ys[i]).abs() < 1e-4, + (y - master.ys[i]).abs() < 1e-4, "point {i} at x={} evaluated to {y}, expected {}", xs[i], - c.ys[i] + master.ys[i] ); } } @@ -530,11 +1065,12 @@ mod tests { c.set_param(P1_Y, 0.10); c.set_param(P3_Y, 0.90); - let xs = c.sorted_xs(); + let master = c.curve(Channel::Master); + let xs = master.sorted_xs(); let mut previous = f32::NEG_INFINITY; for i in 0..=200 { let x = i as f32 / 200.0; - let y = evaluate(&xs, &c.ys, x); + let y = evaluate(&xs, &master.ys, x); assert!( y >= previous - 1e-5, "curve decreased at x={x}: {y} after {previous}" @@ -554,16 +1090,37 @@ mod tests { c.set_param(P3_Y, 0.97); c.set_param(P4_Y, 1.0); - let xs = c.sorted_xs(); + let master = c.curve(Channel::Master); + let xs = master.sorted_xs(); let mut previous = f32::NEG_INFINITY; for i in 0..=200 { - let y = evaluate(&xs, &c.ys, i as f32 / 200.0); + let y = evaluate(&xs, &master.ys, i as f32 / 200.0); assert!(y >= previous - 1e-5, "decreased at {i}"); assert!(y.is_finite(), "non-finite at {i}"); previous = y; } } + #[test] + fn a_channel_curve_stays_monotonic() { + // The same guarantee the master carries, and it matters more here: a + // non-monotone blue curve inverts blue locally, which is a hue + // reversal rather than a dark halo — harder to see and much harder to + // attribute to the control that caused it. + let mut c = ToneCurve::new(); + c.set_param(coordinate(Channel::Blue, 1, Axis::Y), 0.05); + c.set_param(coordinate(Channel::Blue, 3, Axis::Y), 0.95); + + let blue = c.curve(Channel::Blue); + let xs = blue.sorted_xs(); + let mut previous = f32::NEG_INFINITY; + for i in 0..=200 { + let y = evaluate(&xs, &blue.ys, i as f32 / 200.0); + assert!(y >= previous - 1e-5, "blue decreased at {i}"); + previous = y; + } + } + #[test] fn a_flat_span_stays_flat() { // Two points at the same height must not bow between them. @@ -572,10 +1129,11 @@ mod tests { c.set_param(P2_Y, 0.5); c.set_param(P3_Y, 0.5); - let xs = c.sorted_xs(); + let master = c.curve(Channel::Master); + let xs = master.sorted_xs(); for i in 0..=20 { let x = 0.25 + (i as f32 / 20.0) * 0.5; - let y = evaluate(&xs, &c.ys, x); + let y = evaluate(&xs, &master.ys, x); assert!((y - 0.5).abs() < 1e-4, "at {x} the flat span gave {y}"); } } @@ -588,35 +1146,41 @@ mod tests { } #[test] - fn coincident_x_values_are_separated() { + fn coincident_x_values_are_separated_on_every_channel() { // A sidecar can carry anything; a spline through two points at the - // same x divides by zero and produces NaN across the image. - let mut c = ToneCurve::new(); - c.set_param(P1_X, 0.5); - c.set_param(P2_X, 0.5); - c.set_param(P3_X, 0.5); + // same x divides by zero and produces NaN across the image. The + // guarantee has to hold per curve, because the sort is per curve. + for channel in Channel::ALL { + let mut c = ToneCurve::new(); + for point in 1..4 { + c.set_param(coordinate(channel, point, Axis::X), 0.5); + } - let xs = c.sorted_xs(); - for i in 1..POINTS { - assert!( - xs[i] > xs[i - 1], - "x values must be strictly increasing, got {xs:?}" - ); - } - // And the result must be usable, not merely non-crashing. - for i in 0..=50 { - assert!(evaluate(&xs, &c.ys, i as f32 / 50.0).is_finite()); + let curve = c.curve(channel); + let xs = curve.sorted_xs(); + for i in 1..POINTS { + assert!( + xs[i] > xs[i - 1], + "{channel:?}: x values must be strictly increasing, got {xs:?}" + ); + } + // And the result must be usable, not merely non-crashing. + for i in 0..=50 { + assert!(evaluate(&xs, &curve.ys, i as f32 / 50.0).is_finite()); + } } } #[test] - fn out_of_order_x_values_are_sorted() { - let mut c = ToneCurve::new(); - c.set_param(P1_X, 0.9); - c.set_param(P3_X, 0.1); - let xs = c.sorted_xs(); - for i in 1..POINTS { - assert!(xs[i] > xs[i - 1], "not sorted: {xs:?}"); + fn out_of_order_x_values_are_sorted_on_every_channel() { + for channel in Channel::ALL { + let mut c = ToneCurve::new(); + c.set_param(coordinate(channel, 1, Axis::X), 0.9); + c.set_param(coordinate(channel, 3, Axis::X), 0.1); + let xs = c.curve(channel).sorted_xs(); + for i in 1..POINTS { + assert!(xs[i] > xs[i - 1], "{channel:?} not sorted: {xs:?}"); + } } } @@ -643,10 +1207,65 @@ mod tests { } } + #[test] + fn the_widgets_parameters_are_grouped_by_the_curve_they_belong_to() { + // What a channel selector is built out of. A panel groups the widget's + // parameters by their facet's subject and gets four curves, in this + // order, without knowing that a colour channel is a thing — so each + // channel's run has to be contiguous, complete, and labelled. + let presentation = ToneCurve::new().presentation().expect("declares a widget"); + for channel in Channel::ALL { + let base = channel.index() * POINTS * 2; + assert_eq!( + &presentation.params[base..base + POINTS * 2], + channel.params(), + "{channel:?}'s points are not contiguous in the widget's list" + ); + for id in channel.params() { + let facet = DESCRIPTOR + .param(*id) + .expect("declared") + .facet + .expect("a curve point says which curve it is on"); + assert_eq!(facet.subject, channel.subject()); + assert_eq!(facet.subject_hue, channel.hue()); + } + } + } + + #[test] + fn the_four_curves_share_one_aspect_per_coordinate() { + // The other half of the grid: the same point on all four curves is one + // control applied to four subjects, which is what makes a panel able to + // draw one plot and change its subject. + for point in 0..POINTS { + for axis in [Axis::X, Axis::Y] { + let aspects: Vec<_> = Channel::ALL + .iter() + .map(|c| { + DESCRIPTOR + .param(coordinate(*c, point, axis)) + .expect("declared") + .facet + .expect("faceted") + .aspect + }) + .collect(); + assert!( + aspects.windows(2).all(|w| w[0] == w[1]), + "point {point} {axis:?} does not share an aspect across the curves" + ); + } + } + } + #[test] fn the_fragment_reads_every_declared_uniform() { let mut c = ToneCurve::new(); c.set_param(P2_Y, 0.7); + for channel in COLOURS { + c.set_param(coordinate(channel, 2, Axis::Y), 0.6); + } let body = c.wgsl_body(); for u in c.uniforms() { assert!( @@ -657,12 +1276,119 @@ mod tests { } } + #[test] + fn an_untouched_channel_costs_nothing() { + // **The property that makes four curves affordable.** A photograph + // edited with the master curve alone must generate what it generated + // when this operation held one curve: the same ten uniforms, the same + // fragment, and not one line about red, green or blue. + let mut c = ToneCurve::new(); + c.set_param(P2_Y, 0.7); + + let body = c.wgsl_body(); + assert!(body.contains("curve_eval("), "the master curve is missing"); + assert!( + !body.contains("channel_curve("), + "an untouched channel reached the shader:\n{body}" + ); + let names: Vec<&str> = c.uniforms().iter().map(|u| u.name).collect(); + assert_eq!(names.len(), POINTS * 2, "only the master declares uniforms"); + assert!( + !names.iter().any(|n| n.contains('_')), + "an untouched channel declared uniforms: {names:?}" + ); + assert_eq!(c.helpers(), MASTER_HELPERS); + } + + #[test] + fn an_untouched_master_costs_nothing() { + // The mirror image, and the case a naive implementation gets wrong: a + // grade with no tonal work should not pay for a luminance evaluation + // that maps every pixel to itself. + let mut c = ToneCurve::new(); + c.set_param(coordinate(Channel::Blue, 0, Axis::Y), 0.08); + + let body = c.wgsl_body(); + assert!( + !body.contains("luminance("), + "the identity master curve reached the shader:\n{body}" + ); + assert!(body.contains("c.b = channel_curve(c.b,")); + assert!(!body.contains("c.r = "), "red was untouched:\n{body}"); + let names: Vec<&str> = c.uniforms().iter().map(|u| u.name).collect(); + assert_eq!(names.len(), POINTS * 2); + assert!(names.iter().all(|n| n.starts_with("b_")), "{names:?}"); + assert_eq!(c.helpers(), CHANNEL_HELPERS); + } + + #[test] + fn a_neutral_curve_contributes_no_uniforms_at_all() { + // Belt and braces around `is_active`: the composer skips an inactive + // operation, but one that declared uniforms while claiming to be + // neutral would push the whole uniform block out of step the day that + // changed. + let c = ToneCurve::new(); + assert!(!c.is_active()); + assert!(c.uniforms().is_empty()); + assert_eq!(c.wgsl_body(), "c = max(c, vec3(0.0));"); + } + + #[test] + fn the_master_curve_runs_before_the_channel_curves() { + // **The composition order, asserted rather than described.** The + // channels grade the tones the master produced; the other order is a + // visibly different image, and it is the kind of change that arrives + // by accident when someone reorders a loop. + let mut c = ToneCurve::new(); + c.set_param(P2_Y, 0.7); + c.set_param(coordinate(Channel::Red, 1, Axis::Y), 0.3); + + let body = c.wgsl_body(); + let master = body.find("apply_tone_gain").expect("the master curve runs"); + let red = body.find("c.r = channel_curve").expect("the red curve runs"); + assert!(master < red, "the master curve must run first:\n{body}"); + } + + #[test] + fn the_channels_run_in_the_order_they_are_listed() { + // Not because the result depends on it — the three act on separate + // components — but because a reader comparing the generated shader + // with this file should not have to wonder whether it does. + let mut c = ToneCurve::new(); + for channel in COLOURS { + c.set_param(coordinate(channel, 2, Axis::Y), 0.6); + } + let body = c.wgsl_body(); + let at = |s: &str| body.find(s).unwrap_or_else(|| panic!("{s} missing")); + assert!(at("c.r = ") < at("c.g = ")); + assert!(at("c.g = ") < at("c.b = ")); + } + #[test] fn unknown_parameters_are_ignored() { let mut c = ToneCurve::new(); c.set_param(ParamId("p9_x"), 0.5); c.set_param(ParamId("nonsense"), 0.5); c.set_param(ParamId("p1_z"), 0.5); + // A curve from a build that has more of them than this one does. + c.set_param(ParamId("k_p1_y"), 0.5); + c.set_param(ParamId("r_p9_y"), 0.5); assert!(!c.is_active()); } + + #[test] + fn every_channel_is_nameable_and_distinct() { + // The selector is built from these, so two channels sharing a key + // would draw two entries with one name and no way to tell which is + // which. + let mut keys: Vec<&str> = Channel::ALL.iter().map(|c| c.subject().0).collect(); + let before = keys.len(); + keys.sort_unstable(); + keys.dedup(); + assert_eq!(before, keys.len(), "two channels share a name"); + assert_eq!(Channel::ALL.len(), CHANNELS); + for c in Channel::ALL { + assert!(!c.subject().0.is_empty()); + } + } } diff --git a/core/dr-pipeline/src/sidecar.rs b/core/dr-pipeline/src/sidecar.rs index 3b28fd8..a7b8d8f 100644 --- a/core/dr-pipeline/src/sidecar.rs +++ b/core/dr-pipeline/src/sidecar.rs @@ -1088,7 +1088,7 @@ impl std::error::Error for ParseError {} mod tests { use super::*; use crate::framing; - use crate::ops::{exposure, saturation, white_balance}; + use crate::ops::{curve, exposure, saturation, white_balance}; fn edited() -> EditGraph { let mut g = EditGraph::default_chain(); @@ -1273,6 +1273,90 @@ mod tests { assert_eq!(once, twice); } + /// TRACES: FR-DEV-3 | FR-CAT-8 + /// A file written before the tone curve had per-channel curves. + /// + /// Spelled out as literal text rather than produced by `to_text`, because + /// the claim is about *those bytes*: a sidecar generated by this build + /// would agree with this build by construction, and would go on agreeing + /// with it through a rename that broke every file on disk. + #[test] + fn a_sidecar_from_before_the_channel_curves_still_names_the_master() { + let text = "drsc 1\n\n[version u1]\nname = Default\nrevision = 4\nmodified = 9\n\ + tone_curve.p1_y = 0.15\ntone_curve.p3_y = 0.85\n"; + let parsed = Sidecar::parse(text).expect("valid"); + let mut g = EditGraph::default_chain(); + parsed.default_version().expect("a version").apply(&mut g); + + // The S-curve the file describes, on the master curve and nowhere + // else. + assert_eq!(g.param(curve::ID, curve::P1_Y), Some(0.15)); + assert_eq!(g.param(curve::ID, curve::P3_Y), Some(0.85)); + for channel in [curve::Channel::Red, curve::Channel::Green, curve::Channel::Blue] { + for point in 0..curve::POINTS { + for axis in [curve::Axis::X, curve::Axis::Y] { + let id = curve::coordinate(channel, point, axis); + let expected = g + .capabilities() + .iter() + .find(|c| c.id == curve::ID) + .and_then(|c| c.params.iter().find(|p| p.id == id)) + .map(|p| p.default); + assert_eq!( + g.param(curve::ID, id), + expected, + "{id} moved, and no line in the file mentions it" + ); + } + } + } + + // And writing it back produces the same two lines: the curves the file + // never mentioned are still at their defaults, so they are still + // absent (`only_non_default_values_are_written`). + let written = Sidecar::parse(&Sidecar::parse(text).expect("valid").to_text()) + .expect("valid") + .to_text(); + assert!(written.contains("tone_curve.p1_y = 0.15"), "{written}"); + assert!(written.contains("tone_curve.p3_y = 0.85"), "{written}"); + assert!( + !written.contains("tone_curve.r_"), + "an untouched channel curve was written out:\n{written}" + ); + } + + /// TRACES: FR-DEV-3 + /// The other direction: the new curves persist like any other parameter. + #[test] + fn a_per_channel_curve_survives_the_round_trip() { + let mut g = EditGraph::default_chain(); + // A faded shadow: blue lifted at the black point, red pulled down. + let blue = curve::coordinate(curve::Channel::Blue, 0, curve::Axis::Y); + let red = curve::coordinate(curve::Channel::Red, 4, curve::Axis::Y); + g.set_param(curve::ID, blue, 0.08); + g.set_param(curve::ID, red, 0.92); + g.set_param(curve::ID, curve::P2_Y, 0.55); + + let mut sidecar = Sidecar::new(); + sidecar.put(version_of(&g)); + let text = sidecar.to_text(); + // Keyed by the channel-prefixed id, which is what makes the master's + // unprefixed ones safe to leave alone. + assert!(text.contains("tone_curve.b_p0_y = 0.08"), "{text}"); + + let parsed = Sidecar::parse(&text).expect("valid"); + let mut restored = EditGraph::default_chain(); + parsed + .default_version() + .expect("a version") + .apply(&mut restored); + + assert_eq!(restored.param(curve::ID, blue), Some(0.08)); + assert_eq!(restored.param(curve::ID, red), Some(0.92)); + assert_eq!(restored.param(curve::ID, curve::P2_Y), Some(0.55)); + assert!(!restored.is_neutral()); + } + #[test] fn an_unknown_operation_survives_a_round_trip() { // The data-loss case that matters: a device running an older build diff --git a/core/dr-pipeline/tests/tone_curve.rs b/core/dr-pipeline/tests/tone_curve.rs new file mode 100644 index 0000000..d54abb4 --- /dev/null +++ b/core/dr-pipeline/tests/tone_curve.rs @@ -0,0 +1,202 @@ +//! TRACES: FR-DEV-3 +//! The tone curve's four curves, seen from outside the crate. +//! +//! The unit tests beside the operation assert what one `ToneCurve` does. These +//! assert the two properties that only show up once it is a node in a chain +//! with a sidecar under it: that a file written before the per-channel curves +//! existed still describes the edit it described, and that the three new +//! curves cost a photograph that does not use them precisely nothing. + +use dr_pipeline::ops::curve::{self, Axis, Channel}; +use dr_pipeline::{EditGraph, Sidecar}; + +/// A version block carrying `params`, in the on-disk spelling. +fn sidecar_with(params: &[(&str, f32)]) -> String { + let mut text = String::from("drsc 1\n\n[version u1]\nname = Default\nrevision = 2\nmodified = 0\n"); + for (key, value) in params { + text.push_str(&format!("{key} = {value}\n")); + } + text +} + +fn apply(text: &str) -> EditGraph { + let sidecar = Sidecar::parse(text).expect("a valid sidecar"); + let mut graph = EditGraph::default_chain(); + sidecar + .default_version() + .expect("a default version") + .apply(&mut graph); + graph +} + +/// TRACES: FR-CAT-8 +/// **The compatibility guarantee, end to end.** +/// +/// An edit made before this build existed has to produce the same image now. +/// Asserted on the generated shader rather than on the parameter values, +/// because that is what the photograph is actually made of: same source, same +/// uniforms, same picture. +#[test] +fn an_edit_written_before_the_channel_curves_renders_as_it_did() { + let from_file = apply(&sidecar_with(&[ + ("tone_curve.p1_y", 0.15), + ("tone_curve.p3_y", 0.85), + ])); + + let mut by_hand = EditGraph::default_chain(); + by_hand.set_param(curve::ID, curve::P1_Y, 0.15); + by_hand.set_param(curve::ID, curve::P3_Y, 0.85); + + let restored = from_file.compose(); + let expected = by_hand.compose(); + assert_eq!(restored.source, expected.source); + assert_eq!(restored.uniforms, expected.uniforms); + assert_eq!(restored.structure_hash, expected.structure_hash); +} + +/// **Neutral means absent, and stays absent with four times as much to be +/// neutral about.** +/// +/// The curve carries forty parameters now. An image edited with an S-curve and +/// nothing else must generate the shader it generated when it carried ten: +/// three untouched curves are not three identity evaluations, they are nothing +/// at all. +#[test] +fn three_untouched_curves_cost_nothing() { + let mut graph = EditGraph::default_chain(); + graph.set_param(curve::ID, curve::P2_Y, 0.62); + let shader = graph.compose(); + + assert!( + shader.source.contains("---- tone_curve ----"), + "the master curve must reach the shader" + ); + assert!( + !shader.source.contains("channel_curve"), + "an untouched channel curve reached the shader:\n{}", + shader.source + ); + for prefix in ["r_", "g_", "b_"] { + assert!( + !shader.source.contains(&format!("tone_curve_{prefix}")), + "an untouched channel curve declared uniforms:\n{}", + shader.source + ); + } +} + +/// And with none of them touched, the operation is not there at all — the +/// property `a_fresh_graph_is_neutral` asserts for the chain, restated for the +/// node that just quadrupled in size. +#[test] +fn a_curve_at_its_defaults_is_absent_from_the_shader() { + let graph = EditGraph::default_chain(); + assert!(graph.is_neutral()); + assert!(!graph.compose().source.contains("tone_curve")); + + // Including when every one of the forty parameters has been explicitly + // written to its own default, which is what a sidecar round trip through + // a build with a different idea of "default" would produce. + let mut written = EditGraph::default_chain(); + for cap in written.capabilities() { + if cap.id != curve::ID { + continue; + } + for p in &cap.params { + written.set_param(curve::ID, p.id, p.default); + } + } + assert!(written.is_neutral()); +} + +/// A grade with no tonal work is a real edit, and it must not drag the +/// luminance path in behind it. +#[test] +fn a_channel_curve_reaches_the_shader_on_its_own() { + let mut graph = EditGraph::default_chain(); + graph.set_param( + curve::ID, + curve::coordinate(Channel::Blue, 0, Axis::Y), + 0.08, + ); + + let shader = graph.compose(); + assert!(shader.source.contains("c.b = channel_curve(c.b,")); + assert!( + !shader.source.contains("apply_tone_gain"), + "the identity master curve reached the shader:\n{}", + shader.source + ); + // Blue's ten points, and nothing else: the other two curves are at the + // identity and contribute no slot to the uniform block. + for prefix in ["r_", "g_"] { + assert!( + !shader.source.contains(&format!("tone_curve_{prefix}")), + "an untouched channel curve declared uniforms:\n{}", + shader.source + ); + } + for point in 0..curve::POINTS { + assert!( + shader.source.contains(&format!("tone_curve_b_x{point}")), + "blue's point {point} is missing from the uniform block" + ); + } +} + +/// Each curve is its own shader, so the pipeline cache cannot hand the red +/// curve's compiled program to an edit that moved the green one. +#[test] +fn every_curve_generates_a_distinct_shader() { + let mut hashes: Vec = Vec::new(); + for channel in Channel::ALL { + let mut graph = EditGraph::default_chain(); + graph.set_param(curve::ID, curve::coordinate(channel, 2, Axis::Y), 0.62); + hashes.push(graph.compose().structure_hash); + } + let before = hashes.len(); + hashes.sort_unstable(); + hashes.dedup(); + assert_eq!(before, hashes.len(), "two curves share a compiled shader"); +} + +/// The forty parameters all persist, and the file names each one once. +#[test] +fn every_point_of_every_curve_round_trips_through_a_sidecar() { + let mut graph = EditGraph::default_chain(); + // A different y per coordinate, so a point wired to the wrong channel + // cannot pass by holding the value it was supposed to hold anyway. + // Thirty-secondths because they survive both the decimal the file is + // written in and the binary32 it is read back into exactly — this test is + // about which parameter a value lands in, and a rounding difference here + // would fail it for an unrelated reason. + let mut step = 1; + for channel in Channel::ALL { + for point in 0..curve::POINTS { + graph.set_param( + curve::ID, + curve::coordinate(channel, point, Axis::Y), + step as f32 / 32.0, + ); + step += 1; + } + } + + let mut sidecar = Sidecar::new(); + sidecar.put(dr_pipeline::sidecar::Version::from_graph( + "u1", "Default", &graph, + )); + let restored = apply(&sidecar.to_text()); + + for channel in Channel::ALL { + for point in 0..curve::POINTS { + let id = curve::coordinate(channel, point, Axis::Y); + assert_eq!( + restored.param(curve::ID, id), + graph.param(curve::ID, id), + "{id} did not survive the sidecar" + ); + } + } +} + diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index 6863a65..8820974 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -273,6 +273,20 @@ pub struct DevelopSession { /// attributes leaves it at — the tabs are the interface's idea, not the /// core's, and nothing breaks without them (ARCH §4.3a). active_tab: Option, + /// TRACES: FR-DEV-3 + /// Which of the curve widget's subjects the panel is plotting. + /// + /// The tone curve is four curves — one over tone and one per colour + /// channel — and one square plot draws one of them at a time. The index + /// is into the subjects the operation's parameters are faceted on, in the + /// order it declares them, so nothing here knows that "red" exists. + /// + /// **Interface state, not part of the edit.** It changes no pixel, so it + /// is not a parameter, it is not in the graph, it is not in the sidecar + /// and it is not on the undo stack — the same standing as which tab is + /// open. One value rather than one per operation, for the same reason + /// `curve_samples` is one polyline: the panel draws one curve. + curve_channel: usize, } impl DevelopSession { @@ -339,6 +353,7 @@ impl DevelopSession { active_mask: None, show_overlay: false, active_tab: None, + curve_channel: 0, } } @@ -349,8 +364,12 @@ impl DevelopSession { pub fn rows(&self) -> Vec { let caps = self.scoped_capabilities(); match self.active_tab { - Some(attribute) => rows_filtered(&caps, |op| op.attributes.contains(&attribute)), - None => rows_from(&caps), + Some(attribute) => rows_filtered( + &caps, + |op| op.attributes.contains(&attribute), + self.curve_channel, + ), + None => rows_filtered(&caps, |_| true, self.curve_channel), } } @@ -384,8 +403,10 @@ impl DevelopSession { .into_iter() .filter(|a| *a != Attribute::Geometry) .filter(|a| { - caps.iter() - .any(|c| c.attributes.contains(a) && !rows_filtered(&caps, |o| o.attributes.contains(a)).is_empty()) + caps.iter().any(|c| { + c.attributes.contains(a) + && !rows_filtered(&caps, |o| o.attributes.contains(a), 0).is_empty() + }) }) .map(|a| (a, crate::labels::resolve(a.label().0))) .collect() @@ -494,8 +515,13 @@ pub(crate) fn supported(widget: WidgetKind) -> bool { /// FR-DEV-3c acceptance test asks for — an operation the frontend has never /// heard of appearing in a generated panel — and it cannot be asserted at all /// if generating a row requires a device. +/// +/// `#[cfg(test)]` since the panel began passing the selected curve down: the +/// session always has one to pass, and a wrapper that quietly picked the first +/// would be a second answer to a question the session already answers. +#[cfg(test)] pub(crate) fn rows_from(caps: &[OpCapability]) -> Vec { - rows_filtered(caps, |_| true) + rows_filtered(caps, |_| true, 0) } /// The panel model for the capabilities `keep` accepts. @@ -508,9 +534,16 @@ pub(crate) fn rows_from(caps: &[OpCapability]) -> Vec { /// /// Getting that backwards is how a slider ends up driving a different /// operation, which is the kind of fault that looks like a rendering bug. +/// +/// `curve_channel` is which subject a multi-subject widget is showing — the +/// tone curve's four curves are one plot with a selector over it. It is passed +/// in rather than read from anywhere because this function is deliberately +/// free-standing: the descriptor-to-panel path has to be exercisable against a +/// hand-built capability list with no session behind it. pub(crate) fn rows_filtered( caps: &[OpCapability], keep: impl Fn(&OpCapability) -> bool, + curve_channel: usize, ) -> Vec { let mut rows = Vec::new(); for (op_index, op) in caps.iter().enumerate() { @@ -558,7 +591,9 @@ pub(crate) fn rows_filtered( // to the core stops this compiling until someone has decided, // here, whether the panel draws it. let row = match widget { - WidgetKind::ToneCurve => curve_row(op_index, group_head, op, presentation), + WidgetKind::ToneCurve => { + curve_row(op_index, group_head, op, presentation, curve_channel) + } // Canvas-hosted kinds returned above; the rest are not // implemented and reached sliders via `choose`. WidgetKind::ColourWheel @@ -671,8 +706,76 @@ pub(crate) fn rows_filtered( rows } +/// One run of a curve widget's parameters: the points of a single curve. +/// +/// A widget may span several curves — the tone curve is one plot over a master +/// curve and three colour channels — and it says so the way the colour mixer +/// says it has twelve bands: by faceting each parameter with the *subject* it +/// acts on. Consecutive parameters sharing a subject are one curve. +struct CurveRun { + /// The subject's localisation key, or `None` where the widget's parameters + /// carry no facet at all and are therefore a single unnamed curve. + subject: Option<&'static str>, + /// Where this run's points begin in the operation's parameter list. What + /// a drag routes back through, so it must be a position in `op.params` + /// and not in the presentation's list. + base: usize, + /// How many coordinates it holds. + len: usize, +} + +/// TRACES: FR-DEV-3a +/// The curves a curve widget spans, in the order the operation declares them. +/// +/// **This is the whole of the panel's knowledge of colour channels: none.** It +/// groups by whatever subject the parameters carry, so an operation offering a +/// master curve and three channels gets a four-way selector, one offering a +/// single unfaceted curve gets no selector at all, and one that grows a fifth +/// curve tomorrow needs no change here. +/// +/// Returns `None` where the parameters do not look like point coordinates — +/// an odd count, a run that is not contiguous in the capability list — in +/// which case the caller falls back to sliders rather than drawing a widget +/// over a layout it has guessed at. +fn curve_runs(op: &OpCapability, presentation: &Presentation) -> Option> { + // Points are x/y pairs, so an odd count means the operation and this code + // disagree about the layout. + if presentation.params.len() < 2 || !presentation.params.len().is_multiple_of(2) { + log::warn!("{}: curve widget needs an even parameter count", op.id); + return None; + } + + let mut runs: Vec = Vec::new(); + for id in presentation.params { + // The widget addresses points by offset from the first of its run, so + // a run has to be contiguous in the capability list. + let at = op.params.iter().position(|p| p.id == *id)?; + let subject = op.params[at].facet.as_ref().map(|f| f.subject.0); + + match runs.last_mut() { + Some(run) if run.subject == subject && run.base + run.len == at => run.len += 1, + _ => runs.push(CurveRun { + subject, + base: at, + len: 1, + }), + } + } + + if runs.iter().any(|r| !r.len.is_multiple_of(2)) { + log::warn!("{}: a curve's points are not contiguous", op.id); + return None; + } + Some(runs) +} + /// One row standing for a whole curve. /// +/// `channel` picks which of the widget's curves is plotted; it is clamped +/// rather than validated, because the selection is interface state that +/// outlives a change of photograph and the new image's operation may have +/// fewer curves than the old one's. +/// /// Returns `None` if the operation's parameters do not look like point /// coordinates, in which case the caller falls back to sliders rather than /// rendering a broken widget. @@ -681,38 +784,21 @@ fn curve_row( group_head: usize, op: &OpCapability, presentation: &Presentation, + channel: usize, ) -> Option { - // Points are x/y pairs, so an odd count means the operation and this - // code disagree about the layout. - if presentation.params.len() < 2 || !presentation.params.len().is_multiple_of(2) { - log::warn!("{}: curve widget needs an even parameter count", op.id); - return None; - } + let runs = curve_runs(op, presentation)?; + let run = runs.get(channel.min(runs.len().saturating_sub(1)))?; - // The widget addresses points by offset from the first, so they must - // be contiguous in the capability list. - let base = op - .params + let points: Vec = op.params[run.base..run.base + run.len] .iter() - .position(|p| p.id == presentation.params[0])?; - for (i, id) in presentation.params.iter().enumerate() { - if op.params.get(base + i).map(|p| p.id) != Some(*id) { - log::warn!("{}: curve parameters are not contiguous", op.id); - return None; - } - } - - let points: Vec = presentation - .params - .iter() - .filter_map(|id| op.params.iter().find(|p| p.id == *id)) .map(|p| p.value) .collect(); Some(ParamRow { op_index: op_index as i32, - // The first point parameter; the widget offsets from here. - param_index: base as i32, + // The first point parameter *of the curve on show*; the widget offsets + // from here, so switching curve is what re-points the drag. + param_index: run.base as i32, op_label: labels::resolve(op.label.0).into(), param_label: String::new().into(), // A widget spanning a whole operation is not a row in anyone's @@ -733,21 +819,97 @@ fn curve_row( precision: 4, unit: String::new().into(), points: slint::ModelRc::new(slint::VecModel::from(points)), - // A curve is not a choice between named alternatives. + // A curve is not a choice between named alternatives. The curves it + // can switch between are named on the panel rather than on the row — + // see `DevelopSession::curve_channels` for why they cannot ride here. choices: no_choices(), }) } impl DevelopSession { - /// The curve's shape, sampled for drawing. + /// TRACES: FR-DEV-3 + /// The names of the curves the widget can switch between. + /// + /// Empty where there is only one, which is also the answer for a frontend + /// with no curve at all: a selector over a single choice is a row of + /// nothing. + /// + /// **Derived from the facets, so nothing here names a colour channel.** + /// The operation says its forty points are one control applied to four + /// subjects and publishes a localisation key for each; this resolves the + /// keys and hands over four words. An operation that grew a fifth curve + /// would appear here on its own. + /// + /// A panel property rather than a field on the curve's `ParamRow`, and the + /// reason is Slint's: a row's models are compared by identity, so a fresh + /// list of names built on every parameter event would make the row look + /// changed every time, and rewriting a row rebuilds the repeater item + /// underneath it — destroying the `TouchArea` holding the drag in + /// progress. The same hazard `rows`'s in-place point update exists to + /// avoid. Nothing in this list is a drag target, so up here it is safe to + /// replace wholesale, exactly as [`Self::curve_samples`] is. + pub fn curve_channels(&self) -> Vec { + for op in &self.scoped_capabilities() { + let Some(presentation) = &op.presentation else { + continue; + }; + if presentation.choose(supported) != Some(WidgetKind::ToneCurve) { + continue; + } + let Some(runs) = curve_runs(op, presentation) else { + continue; + }; + if runs.len() < 2 { + continue; + } + return runs + .iter() + .map(|r| r.subject.map(labels::resolve).unwrap_or_default()) + .collect(); + } + Vec::new() + } + + /// Which curve the widget is plotting, as an index into + /// [`Self::curve_channels`]. + pub fn curve_channel(&self) -> i32 { + self.curve_channel as i32 + } + + /// Plot a different one of the operation's curves. + /// + /// Out-of-range indices are ignored rather than clamped: the only thing + /// that can send one is a stale interface event, and quietly moving the + /// selection somewhere the user did not point is worse than doing nothing. + pub fn set_curve_channel(&mut self, index: i32) { + let Ok(index) = usize::try_from(index) else { + return; + }; + if index < self.curve_channels().len() { + self.curve_channel = index; + } + } + + /// The plotted curve's shape, sampled for drawing. /// /// Evaluated with `dr_pipeline`'s own spline, so the line the user drags /// is the line the shader applies. The alternative — reading the curve /// back off the GPU — is the round-trip ARCH §6.1 forbids, to draw a /// polyline. + /// + /// The line drawn is the *selected* curve's own shape, not the composition + /// of it with the master. Two curves overlaid on one grid is a plot of two + /// things, and the one being dragged has to be the one whose points are + /// under the pointer. pub fn curve_samples(&self) -> Vec { const SAMPLES: usize = 96; + // The selection is an index over the subjects the panel found, which + // for this operation is its channel order. Clamped rather than + // trusted: a selection made on one photograph outlives the change to + // the next. + let channel = curve::Channel::ALL[self.curve_channel.min(curve::CHANNELS - 1)]; + let mut xs = [0.0f32; curve::POINTS]; let mut ys = [0.0f32; curve::POINTS]; let mut found = false; @@ -757,16 +919,18 @@ impl DevelopSession { continue; } found = true; - for (i, p) in cap.params.iter().enumerate() { - let point = i / 2; - if point >= curve::POINTS { - break; - } - if i % 2 == 0 { - xs[point] = p.value; - } else { - ys[point] = p.value; - } + // By id rather than by position, so which curve is plotted is + // decided by naming it and not by arithmetic over the parameter + // list. + let value = |id| { + cap.params + .iter() + .find(|p| p.id == id) + .map_or(0.0, |p| p.value) + }; + for i in 0..curve::POINTS { + xs[i] = value(curve::coordinate(channel, i, curve::Axis::X)); + ys[i] = value(curve::coordinate(channel, i, curve::Axis::Y)); } } if !found { @@ -3419,9 +3583,9 @@ mod tests { #[test] fn the_curve_collapses_to_a_single_row() { - // Ten point parameters must appear as one curve control, not ten - // sliders — otherwise the widget and the sliders both render and the - // panel shows the same values twice. + // Every point parameter — all four curves' worth — must appear as one + // curve control, not as forty sliders. Otherwise the widget and the + // sliders both render and the panel shows the same values twice. let graph = EditGraph::default_chain(); let curve_cap = graph .capabilities() @@ -3429,7 +3593,11 @@ mod tests { .find(|c| c.id == curve::ID) .expect("the chain includes a tone curve"); - assert_eq!(curve_cap.params.len(), curve::POINTS * 2); + assert_eq!( + curve_cap.params.len(), + curve::CHANNELS * curve::POINTS * 2, + "a master curve and one per colour channel" + ); let presentation = curve_cap .presentation .as_ref() @@ -3469,6 +3637,144 @@ mod tests { } } + /// The panel's whole knowledge of colour channels, asserted to be none. + /// + /// It groups the widget's parameters by the subject the *operation* put on + /// them and finds four curves; nothing below says "red", and an operation + /// that grew a fifth curve would arrive here on its own. + #[test] + fn a_curve_widget_offers_one_run_per_subject() { + let graph = EditGraph::default_chain(); + let cap = graph + .capabilities() + .into_iter() + .find(|c| c.id == curve::ID) + .expect("tone curve present"); + let presentation = cap.presentation.as_ref().expect("declares a widget"); + + let runs = curve_runs(&cap, presentation).expect("a curve-shaped operation"); + assert_eq!(runs.len(), curve::CHANNELS); + for (i, run) in runs.iter().enumerate() { + assert_eq!(run.len, curve::POINTS * 2, "run {i} is not five points"); + assert_eq!(run.base, i * curve::POINTS * 2); + assert!(run.subject.is_some(), "run {i} is unnamed"); + } + } + + #[test] + fn switching_curve_repoints_the_row() { + use slint::Model as _; + + // What a drag routes through. The row's `param_index` is the base of + // the curve *on show*, so picking a different one must move it — if it + // did not, dragging a point on the red curve would write to the + // master's. + let graph = EditGraph::default_chain(); + let caps = graph.capabilities(); + let curve_at = caps + .iter() + .position(|c| c.id == curve::ID) + .expect("tone curve present"); + + let mut bases = Vec::new(); + for channel in 0..curve::CHANNELS { + let rows = rows_filtered(&caps, |_| true, channel); + let row = rows + .iter() + .find(|r| r.op_index as usize == curve_at) + .expect("the curve has a row"); + assert_eq!(row.kind, "curve"); + assert_eq!( + row.points.row_count(), + curve::POINTS * 2, + "one curve's points, not all four curves'" + ); + bases.push(row.param_index); + } + + assert_eq!( + bases, + (0..curve::CHANNELS) + .map(|i| (i * curve::POINTS * 2) as i32) + .collect::>() + ); + } + + #[test] + fn a_selection_the_operation_cannot_honour_falls_back_to_its_last_curve() { + // The selection outlives the photograph it was made on, and the next + // image's operation may offer fewer curves. Clamping keeps a plot on + // the grid; the alternative is a curve row that vanishes, which reads + // as the tone curve having disappeared from the panel. + let graph = EditGraph::default_chain(); + let caps = graph.capabilities(); + let rows = rows_filtered(&caps, |_| true, 99); + let row = rows + .iter() + .find(|r| r.kind == "curve") + .expect("the curve still has a row"); + assert_eq!( + row.param_index, + ((curve::CHANNELS - 1) * curve::POINTS * 2) as i32 + ); + } + + #[test] + fn an_operation_whose_points_are_unfaceted_is_one_curve() { + // A curve widget that spans a single unnamed curve — which is what + // this operation was before the channels arrived, and what any other + // node declaring a `tone_curve` widget over ten scalars would be. + // It must draw, and it must offer no choice. + use dr_pipeline::{LocalizedKey, ParamCapability, WidgetDemand}; + use slint::Model as _; + + static IDS: [ParamId; 4] = [ + ParamId("p0_x"), + ParamId("p0_y"), + ParamId("p1_x"), + ParamId("p1_y"), + ]; + let param = |id: ParamId| ParamCapability { + id, + label: LocalizedKey("param.point"), + kind: ParamKind::Scalar { + min: 0.0, + max: 1.0, + scale: dr_pipeline::Scale::Linear, + unit: Unit::None, + precision: 4, + }, + default: 0.0, + value: 0.0, + facet: None, + }; + let plain = OpCapability { + id: OpId("invented_curve"), + label: LocalizedKey("op.invented_curve"), + active: false, + presentation: Some(Presentation { + widgets: &[WidgetKind::ToneCurve], + demand: WidgetDemand { + two_dimensional: true, + precise_pointing: true, + }, + params: &IDS, + }), + params: IDS.iter().map(|id| param(*id)).collect(), + attributes: &[dr_pipeline::Attribute::Tone], + }; + + let presentation = plain.presentation.as_ref().expect("declares a widget"); + let runs = curve_runs(&plain, presentation).expect("curve-shaped"); + assert_eq!(runs.len(), 1, "one unnamed curve"); + assert_eq!(runs[0].subject, None); + + let rows = rows_from(&[plain]); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].kind, "curve"); + assert_eq!(rows[0].points.row_count(), IDS.len()); + } + #[test] fn curve_samples_start_on_the_diagonal() { // A fresh curve is the identity, so the drawn line must be the 45° diff --git a/ui/dr-ui/src/labels.rs b/ui/dr-ui/src/labels.rs index 4d2f242..f40c076 100644 --- a/ui/dr-ui/src/labels.rs +++ b/ui/dr-ui/src/labels.rs @@ -54,6 +54,19 @@ pub fn resolve(key: &str) -> String { "param.channel.sat" => "Saturation".into(), "param.channel.lum" => "Luminance".into(), + // The tone curve's four curves, which its points are *subject* to. + // + // Catalogued rather than derived because the master curve's key would + // otherwise read "Rgb": these are the terms of a four-way choice, and + // one of them miscapitalised is the one the eye goes to. The three + // colours would derive correctly and are written out beside it anyway, + // since a list where one entry is translated and three are guessed is + // the shape a half-finished translation takes. + "channel.rgb" => "RGB".into(), + "channel.red" => "Red".into(), + "channel.green" => "Green".into(), + "channel.blue" => "Blue".into(), + // The hue bands, which a faceted row is *subject* to. // // Catalogued even where `derive` would produce the same word, because diff --git a/ui/dr-ui/src/lib.rs b/ui/dr-ui/src/lib.rs index acc62aa..82cea65 100644 --- a/ui/dr-ui/src/lib.rs +++ b/ui/dr-ui/src/lib.rs @@ -583,8 +583,24 @@ pub(crate) fn sync_rows( // curve must be drawn whatever shape it is in. rows.set_vec(current); curve_moved = true; + + // The curves the widget can switch between, named. They can only + // change with the operation set, which is what this branch means, so + // the walk that derives them is not on the parameter-event path. + let channels: Vec = match session.borrow().as_ref() { + Some(s) => s.curve_channels().into_iter().map(Into::into).collect(), + None => Vec::new(), + }; + window.set_curve_channels(slint::ModelRc::new(slint::VecModel::from(channels))); } + // Which curve is plotted, on every pass. Picking one that happens to be + // shaped like the last — two untouched curves are both the diagonal — + // moves no point, so this cannot ride on the resample below: the chips + // would go on highlighting the curve the user just navigated away from. + let channel = session.borrow().as_ref().map_or(0, |s| s.curve_channel()); + window.set_curve_channel(channel); + if !curve_moved { return; } @@ -1804,6 +1820,24 @@ pub fn run(paths: Vec) -> Result<()> { redraw(&w); }); } + { + // Which of the curve's curves the plot is showing. **No redraw**, and + // that is the whole character of this control: it changes no + // parameter, so the photograph is already correct on screen and + // recomputing it would be a frame spent to produce the same pixels. + // For the same reason it records no history step — there is nothing + // to undo — and the sidecar never hears about it. + let weak = window.as_weak(); + let session = session.clone(); + let rows = rows.clone(); + window.on_curve_channel_picked(move |index| { + let Some(w) = weak.upgrade() else { return }; + if let Some(s) = session.borrow_mut().as_mut() { + s.set_curve_channel(index); + } + sync_rows(&w, &rows, &session); + }); + } // ---- undo and redo (FR-DEV-5) --------------------------------------- // diff --git a/ui/dr-ui/ui/adjust.slint b/ui/dr-ui/ui/adjust.slint index b73adb1..3e0fe78 100644 --- a/ui/dr-ui/ui/adjust.slint +++ b/ui/dr-ui/ui/adjust.slint @@ -365,10 +365,17 @@ component ParamControl inherits Rectangle { in property data; /// Curve rows only; ignored by every other kind. in property <[float]> curve-samples; + /// The curves this widget can plot, named. Empty, or one entry, where + /// there is nothing to choose between — see `curve-channel-picked`. + in property <[string]> curve-channels; + /// Which of `curve-channels` is on the grid. + in property curve-channel; callback param-changed(int, int, float); callback param-reset(int, int); callback curve-reset(int); + /// Plot a different one of the operation's curves. + callback curve-channel-picked(int); callback drag-changed(bool); height: layout.preferred-height; @@ -420,20 +427,49 @@ component ParamControl inherits Rectangle { } } - if root.data.kind == "curve": CurveEditor { - points: root.data.points; - samples: root.curve-samples; - drag-changed(on) => { root.drag-changed(on); } - // A point carries two parameters, so the parameter index is the - // row's base plus the point's offset. This component still knows - // nothing about which operation it belongs to. - point-moved(point, x, y) => { - root.param-changed( - root.data.op-index, root.data.param-index + point * 2, x); - root.param-changed( - root.data.op-index, root.data.param-index + point * 2 + 1, y); + // A curve, and — where the operation offers more than one — the choice + // of which curve is on the grid. + // + // One plot rather than four stacked ones: the curves are read against + // the diagonal and against each other, which needs the grid large, and + // four grids at a quarter of the width would each be too small to + // place a point in. So the selector switches the subject of a single + // plot, and the names in it come from the core — this file does not + // know that a colour channel is what is being chosen between, only + // that the widget said it spans several named things. + if root.data.kind == "curve": VerticalLayout { + spacing: Theme.gap-sm; + + if root.curve-channels.length > 1: Segmented { + // The operation's own name, which nothing else in this row + // draws: a curve row heads no group, so without this the plot + // would sit in the panel unlabelled. + label: root.data.op-label; + options: root.curve-channels; + selected: root.curve-channel; + picked(i) => { root.curve-channel-picked(i); } + } + + CurveEditor { + points: root.data.points; + samples: root.curve-samples; + drag-changed(on) => { root.drag-changed(on); } + // A point carries two parameters, so the parameter index is + // the row's base plus the point's offset. The base is the + // first point of the curve *on show*, so switching curve + // re-points the drag and this component still knows nothing + // about which operation — or which curve — it is drawing. + point-moved(point, x, y) => { + root.param-changed( + root.data.op-index, root.data.param-index + point * 2, x); + root.param-changed( + root.data.op-index, root.data.param-index + point * 2 + 1, y); + } + // Resetting a curve resets the operation, which is all four of + // them — a photographer who double-clicks to start again means + // the control, not the curve that happens to be on show. + reset => { root.curve-reset(root.data.op-index); } } - reset => { root.curve-reset(root.data.op-index); } } } } @@ -825,9 +861,16 @@ export component AdjustPanel inherits Rectangle { /// 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; + /// The curves the tone curve widget can plot, named by the core. Fewer + /// than two of them means there is nothing to choose and no selector. + in property <[string]> curve-channels; + /// Which of them `curve-samples` and the row's points describe. + in property curve-channel; callback param-changed(int, int, float); callback param-reset(int, int); callback curve-reset(int); + /// Plot a different one of the curve's curves. + callback curve-channel-picked(int); /// Return every parameter of one operation to its default — the reset on /// a section's own header, beside the panel-wide one. callback op-reset(int); @@ -986,12 +1029,15 @@ export component AdjustPanel inherits Rectangle { ParamControl { data: row; curve-samples: root.curve-samples; + curve-channels: root.curve-channels; + curve-channel: root.curve-channel; drag-changed(on) => { root.slider-dragging = on; } param-changed(op, param, v) => { root.param-changed(op, param, v); } param-reset(op, param) => { root.param-reset(op, param); } curve-reset(op) => { root.curve-reset(op); } + curve-channel-picked(i) => { root.curve-channel-picked(i); } } } } diff --git a/ui/dr-ui/ui/app.slint b/ui/dr-ui/ui/app.slint index e0b1f78..c055ecd 100644 --- a/ui/dr-ui/ui/app.slint +++ b/ui/dr-ui/ui/app.slint @@ -716,9 +716,14 @@ export component AppWindow inherits Window { // The tone curve's sampled shape, evaluated by the core so the drawn // line and the applied one cannot disagree. in property <[float]> curve-samples; + // The curves that widget can plot, named by the core, and which of them + // `curve-samples` describes. Fewer than two means nothing to choose. + in property <[string]> curve-channels; + in property curve-channel; callback param-changed(int, int, float); callback param-reset(int, int); callback curve-reset(int); + callback curve-channel-picked(int); callback reset-all(); // --- copying settings between photographs (FR-DEV-6) --- @@ -2177,6 +2182,11 @@ in property panel-visible: true; enabled: root.adjust-enabled; scope: root.adjust-scope; curve-samples: root.curve-samples; + curve-channels: root.curve-channels; + curve-channel: root.curve-channel; + curve-channel-picked(i) => { + root.curve-channel-picked(i); + } param-changed(op, param, value) => { root.param-changed(op, param, value); }