diff --git a/core/dr-gpu/examples/develop.rs b/core/dr-gpu/examples/develop.rs index da1f865..9fa6084 100644 --- a/core/dr-gpu/examples/develop.rs +++ b/core/dr-gpu/examples/develop.rs @@ -12,7 +12,10 @@ //! it. This is a diagnostic, not the export path (FR-EXP-*). use dr_gpu::{AdjustPass, Demosaicer, GpuContext}; -use dr_pipeline::ops::{colour, colour_mixer, contrast, curve, exposure, tone, white_balance}; +use dr_pipeline::ops::{ + blacks_whites, brilliance, colour_mixer, contrast, curve, exposure, highlights_shadows, + saturation, vibrance, white_balance, +}; use dr_pipeline::{EditGraph, ParamId}; fn main() { @@ -52,17 +55,25 @@ fn main() { match preset.as_str() { "punchy" => { graph.set_param(exposure::ID, exposure::EXPOSURE, 0.3); - graph.set_param(tone::HIGHLIGHTS_SHADOWS_ID, tone::HIGHLIGHTS, -40.0); - graph.set_param(tone::HIGHLIGHTS_SHADOWS_ID, tone::SHADOWS, 30.0); - graph.set_param(tone::BLACKS_WHITES_ID, tone::BLACKS, -20.0); - graph.set_param(tone::BLACKS_WHITES_ID, tone::WHITES, 25.0); - graph.set_param(colour::VIBRANCE_ID, colour::VIBRANCE, 35.0); + graph.set_param( + highlights_shadows::ID, + highlights_shadows::HIGHLIGHTS, + -40.0, + ); + graph.set_param(highlights_shadows::ID, highlights_shadows::SHADOWS, 30.0); + graph.set_param(blacks_whites::ID, blacks_whites::BLACKS, -20.0); + graph.set_param(blacks_whites::ID, blacks_whites::WHITES, 25.0); + graph.set_param(vibrance::ID, vibrance::VIBRANCE, 35.0); } "recover" => { graph.set_param(exposure::ID, exposure::EXPOSURE, -0.5); - graph.set_param(tone::HIGHLIGHTS_SHADOWS_ID, tone::HIGHLIGHTS, -80.0); - graph.set_param(tone::HIGHLIGHTS_SHADOWS_ID, tone::SHADOWS, 60.0); - graph.set_param(colour::BRILLIANCE_ID, colour::BRILLIANCE, 40.0); + graph.set_param( + highlights_shadows::ID, + highlights_shadows::HIGHLIGHTS, + -80.0, + ); + graph.set_param(highlights_shadows::ID, highlights_shadows::SHADOWS, 60.0); + graph.set_param(brilliance::ID, brilliance::BRILLIANCE, 40.0); graph.set_param(white_balance::ID, white_balance::TEMPERATURE, 15.0); } // Contrast alone, so its effect can be judged without anything else diff --git a/core/dr-gpu/src/adjust.rs b/core/dr-gpu/src/adjust.rs index 0956c5f..1715b01 100644 --- a/core/dr-gpu/src/adjust.rs +++ b/core/dr-gpu/src/adjust.rs @@ -415,7 +415,7 @@ fn numbered(src: &str) -> String { mod tests { use super::*; use dr_decode::{CfaPattern, CropRect, RawImage}; - use dr_pipeline::ops::{colour, exposure}; + use dr_pipeline::ops::{exposure, saturation}; use dr_pipeline::EditGraph; use crate::Demosaicer; @@ -1028,7 +1028,7 @@ mod tests { .expect("demosaic"); let mut g = EditGraph::default_chain(); - g.set_param(colour::SATURATION_ID, colour::SATURATION, -100.0); + g.set_param(saturation::ID, saturation::SATURATION, -100.0); let shader = g.compose(); let px = { let t = pass.render(&img, &shader, 16, 16).expect("render"); @@ -1075,12 +1075,12 @@ mod tests { pass.render(&img, &g.compose(), 16, 16).expect("render"); assert_eq!(pass.cached_pipelines(), 1); - g.set_param(colour::SATURATION_ID, colour::SATURATION, 40.0); + g.set_param(saturation::ID, saturation::SATURATION, 40.0); pass.render(&img, &g.compose(), 16, 16).expect("render"); assert_eq!(pass.cached_pipelines(), 2); // Returning to the earlier state must reuse, not compile a third. - g.set_param(colour::SATURATION_ID, colour::SATURATION, 0.0); + g.set_param(saturation::ID, saturation::SATURATION, 0.0); pass.render(&img, &g.compose(), 16, 16).expect("render"); assert_eq!(pass.cached_pipelines(), 2); } diff --git a/core/dr-pipeline/build.rs b/core/dr-pipeline/build.rs index 97c3510..3718f5f 100644 --- a/core/dr-pipeline/build.rs +++ b/core/dr-pipeline/build.rs @@ -92,7 +92,6 @@ struct HelperDef { /// One parameter of a declared node. struct ParamDef { id: String, - label: String, doc: Option, /// The `ParamDescriptor` constructor call, already rendered. ctor: String, @@ -226,7 +225,7 @@ fn node_files(dir: &Path) -> Result, String> { let mut files: Vec = entries .into_iter() .map(|e| e.path()) - .filter(|p| p.extension().is_some_and(|e| e == "yaml")) + .filter(|p| p.extension().and_then(|e| e.to_str()) == Some("yaml")) .filter(|p| !file_stem(p).starts_with(NON_NODE_PREFIX)) .collect(); files.sort(); @@ -349,11 +348,7 @@ fn read_node(path: &Path, shared: &BTreeSet<&str>) -> Result { let doc = read_yaml(path)?; let root = as_mapping(&doc, "the document")?; - let id = as_str( - root.get("id").ok_or("missing `id:`")?, - "id", - )? - .to_string(); + let id = as_str(root.get("id").ok_or("missing `id:`")?, "id")?.to_string(); check_ident(&id, "id")?; let order = root @@ -369,7 +364,7 @@ fn read_node(path: &Path, shared: &BTreeSet<&str>) -> Result { // mean two sources for one node's parameters. if let Some(ty) = root.get("rust") { let ty = as_str(ty, "rust")?.to_string(); - check_ident(&ty, "rust")?; + check_type_name(&ty)?; for key in ["params", "uniforms", "wgsl", "helpers", "define", "label"] { if root.contains_key(key) { return Err(format!( @@ -465,7 +460,6 @@ fn read_params(root: &Mapping) -> Result, String> { let (ctor, default, min, max) = param_ctor(&id, &label, kind, spec, &ctx)?; out.push(ParamDef { id, - label, doc: opt_prose(spec, "doc", &ctx)?, ctor, default, @@ -558,7 +552,11 @@ fn param_ctor( )) } }; - let scale = match spec.get("scale").and_then(Value::as_str).unwrap_or("linear") { + let scale = match spec + .get("scale") + .and_then(Value::as_str) + .unwrap_or("linear") + { "linear" => "Scale::Linear", "perceptual" => "Scale::Perceptual", other => { @@ -594,14 +592,13 @@ fn param_ctor( )) } }) - .map(|(ctor, default, min, max): (String, f64, f64, f64)| { - // Caught here rather than at runtime: a default outside its own range - // makes the control open somewhere it cannot be dragged back to. - (ctor, default, min, max) - }) - .and_then(|(ctor, default, min, max)| { + .and_then(|(ctor, default, min, max): (String, f64, f64, f64)| { + // Caught at build time rather than surfacing as a control that opens + // where it cannot be dragged back to. if min >= max { - return Err(format!("`{ctx}` has an empty range: min {min} >= max {max}")); + return Err(format!( + "`{ctx}` has an empty range: min {min} >= max {max}" + )); } if default < min || default > max { return Err(format!( @@ -738,8 +735,11 @@ fn read_tests( let mut seen = BTreeSet::new(); for item in list { let m = as_mapping(item, "a test")?; - let name = as_str(m.get("name").ok_or("a test has no `name:`")?, "tests[].name")? - .to_string(); + let name = as_str( + m.get("name").ok_or("a test has no `name:`")?, + "tests[].name", + )? + .to_string(); check_ident(&name, &format!("tests.{name}.name"))?; if !seen.insert(name.clone()) { return Err(format!("two tests are both called `{name}`")); @@ -751,7 +751,9 @@ fn read_tests( for (k, val) in as_mapping(v, &format!("{ctx}.set"))? { let key = as_str(k, &format!("{ctx}.set key"))?.to_string(); let Some(param) = params.iter().find(|p| p.id == key) else { - return Err(format!("`{ctx}.set` names `{key}`, which is not a parameter")); + return Err(format!( + "`{ctx}.set` names `{key}`, which is not a parameter" + )); }; let value = val .as_f64() @@ -834,7 +836,9 @@ fn read_tests( .as_sequence() .ok_or_else(|| format!("`{ctx}.expect_helper_wgsl.{key}` must be a list"))? { - needles.push(as_str(item, &format!("{ctx}.expect_helper_wgsl.{key}[]"))?.to_string()); + needles.push( + as_str(item, &format!("{ctx}.expect_helper_wgsl.{key}[]"))?.to_string(), + ); } expect_helper_wgsl.push((key, needles)); } @@ -906,7 +910,41 @@ fn compile_expr(src: &str, params: &BTreeSet<&str>) -> Result { parser.tokens[parser.at] )); } - render(&expr, params) + render(&expr, params).map(|s| unwrap_parens(&s)) +} + +/// Drop one redundant pair of enclosing parentheses. +/// +/// [`render`] parenthesises every binary expression, which is what keeps +/// precedence correct under composition. At the outermost position — a +/// function argument, or the whole expression — that pair is redundant, and +/// `rustc` warns about it. Generated code is the one place a warning cannot be +/// fixed where it appears, so it is fixed here. +fn unwrap_parens(s: &str) -> String { + let inner = match s.strip_prefix('(').and_then(|s| s.strip_suffix(')')) { + Some(inner) => inner, + None => return s.to_string(), + }; + // Only when the two are actually a pair: `(a) * (b)` also starts with `(` + // and ends with `)`, and stripping those would change what it means. + let mut depth = 0i32; + for c in inner.chars() { + match c { + '(' => depth += 1, + ')' => { + depth -= 1; + if depth < 0 { + return s.to_string(); + } + } + _ => {} + } + } + if depth == 0 { + inner.to_string() + } else { + s.to_string() + } } #[derive(Debug, Clone, PartialEq)] @@ -934,7 +972,9 @@ fn tokenise(src: &str) -> Result, String> { let c = bytes[i]; if c.is_whitespace() { i += 1; - } else if c.is_ascii_digit() || (c == '.' && bytes.get(i + 1).is_some_and(char::is_ascii_digit)) { + } else if c.is_ascii_digit() + || (c == '.' && bytes.get(i + 1).is_some_and(char::is_ascii_digit)) + { let start = i; while i < bytes.len() && (bytes[i].is_ascii_digit() || bytes[i] == '.') { i += 1; @@ -1077,12 +1117,16 @@ fn render(expr: &Expr, params: &BTreeSet<&str>) -> Result { } format!("self.{name}") } - Expr::Neg(inner) => format!("-({})", render(inner, params)?), + // One pair of parentheses, never two: the inner expression brings its + // own, and `-((a * b))` is a clippy warning in code nobody can edit. + // The pair that remains is load-bearing — `-(a + b)` and `-a + b` are + // different numbers. + Expr::Neg(inner) => format!("-({})", unwrap_parens(&render(inner, params)?)), Expr::Bin(op, l, r) => format!("({} {op} {})", render(l, params)?, render(r, params)?), Expr::Call(name, args) => { let rendered: Vec = args .iter() - .map(|a| render(a, params)) + .map(|a| render(a, params).map(|s| unwrap_parens(&s))) .collect::>()?; let arity = |n: usize| -> Result<(), String> { if rendered.len() != n { @@ -1225,10 +1269,15 @@ fn emit_node(out: &mut String, node: &Node) -> Result<(), String> { } let _ = writeln!(out, " //! Generated from `ops/{id}.yaml`.\n"); + // `Scale`, `Unit` and `Helper` are used only by some nodes — a node with + // no `kind: scalar` parameter and no helpers needs neither — so the group + // is allowed to go unused rather than being assembled per node. out.push_str( - " use crate::descriptor::{LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId};\n\ + " #[allow(unused_imports)]\n\ + \x20 use crate::descriptor::{\n\ + \x20 LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Scale, Unit,\n\ + \x20 };\n\ \x20 #[allow(unused_imports)]\n\ - \x20 use crate::descriptor::{Scale, Unit};\n\ \x20 use crate::operation::{Helper, Operation, Uniform};\n\n", ); @@ -1285,7 +1334,10 @@ fn emit_node(out: &mut String, node: &Node) -> Result<(), String> { } // State. - let _ = writeln!(out, "\n #[derive(Debug, Default, Clone)]\n pub struct {ty} {{"); + let _ = writeln!( + out, + "\n #[derive(Debug, Default, Clone)]\n pub struct {ty} {{" + ); for p in params { let _ = writeln!(out, " {}: f32,", p.id); } @@ -1301,7 +1353,9 @@ fn emit_node(out: &mut String, node: &Node) -> Result<(), String> { " fn descriptor(&self) -> &'static OpDescriptor {\n &DESCRIPTOR\n }\n\n", ); - out.push_str(" fn set_param(&mut self, id: ParamId, value: f32) {\n match id {\n"); + out.push_str( + " fn set_param(&mut self, id: ParamId, value: f32) {\n match id {\n", + ); for p in params { let _ = writeln!( out, @@ -1377,7 +1431,17 @@ fn emit_tests(out: &mut String, ty: &str, params: &[ParamDef], tests: &[TestDef] if tests.is_empty() { return; } - out.push_str("\n #[cfg(test)]\n mod tests {\n use super::*;\n\n"); + // `approx_constant`: an expected uniform value is a physical quantity, and + // one occasionally coincides with a maths constant — half a stop of white + // balance is exp2(0.5), which is √2. Writing `SQRT_2` there would name the + // arithmetic instead of the photography, and the declaration says `half a + // stop` in prose beside it. + out.push_str( + "\n #[cfg(test)]\n\ + \x20 #[allow(clippy::approx_constant)]\n\ + \x20 mod tests {\n\ + \x20 use super::*;\n\n", + ); out.push_str( " /// One uniform's value, by the name the node declared it under.\n\ \x20 fn uniform(op: &impl Operation, name: &str) -> f32 {\n\ @@ -1396,12 +1460,10 @@ fn emit_tests(out: &mut String, ty: &str, params: &[ParamDef], tests: &[TestDef] if let Some(why) = &t.why { out.push_str(&comment(why, "//", 12)); } - let _ = writeln!(out, " let mut op = {ty}::new();"); - // `mut` is unused when a test asserts about the neutral state, and a - // warning in generated code is noise nobody can fix at the source. - if t.set.is_empty() { - out.push_str(" let op = &mut op;\n let op = &*op;\n"); - } + // `mut` only where a value is actually set: an unused-mut warning in + // generated code is noise nobody can fix at the source. + let binding = if t.set.is_empty() { "op" } else { "mut op" }; + let _ = writeln!(out, " let {binding} = {ty}::new();"); for (param, value) in &t.set { let id = params .iter() @@ -1436,10 +1498,17 @@ fn emit_tests(out: &mut String, ty: &str, params: &[ParamDef], tests: &[TestDef] ); } if let Some(active) = t.expect_active { + let (negation, expected) = if active { + ("", "active") + } else { + ("!", "inactive") + }; let _ = writeln!( out, - " assert_{}!(op.is_active());", - if active { "" } else { "!true == false; assert" } + " assert!(\n\ + \x20 {negation}op.is_active(),\n\ + \x20 \"the operation should be {expected} at these settings\"\n\ + \x20 );" ); } for needle in &t.expect_wgsl { @@ -1489,7 +1558,12 @@ fn emit_chain(out: &mut String, nodes: &[Node]) { \x20 vec![\n", ); for node in nodes { - let _ = writeln!(out, " // ---- {} (order {})", node.id(), node.order()); + let _ = writeln!( + out, + " // ---- {} (order {})", + node.id(), + node.order() + ); if let Some(placement) = node.placement() { out.push_str(&comment(placement, "//", 8)); } @@ -1666,3 +1740,18 @@ fn check_ident(name: &str, ctx: &str) -> Result<(), String> { } Ok(()) } + +/// `rust:` names a type in `crate::ops`, so it is PascalCase rather than an +/// id. Checked only for shape — whether the type exists, and whether it +/// implements `Operation`, is for the compiler to say. +fn check_type_name(name: &str) -> Result<(), String> { + let ok = name.chars().next().is_some_and(|c| c.is_ascii_uppercase()) + && name.chars().all(|c| c.is_ascii_alphanumeric()); + if !ok { + return Err(format!( + "`rust: {name}` must be the PascalCase name of a type in \ + `crate::ops`, such as `ToneCurve`" + )); + } + Ok(()) +} diff --git a/core/dr-pipeline/ops/README.md b/core/dr-pipeline/ops/README.md new file mode 100644 index 0000000..454dcca --- /dev/null +++ b/core/dr-pipeline/ops/README.md @@ -0,0 +1,141 @@ +# Develop nodes + +One file per operation. Adding a node to the pipeline is adding a file to this +directory — there is no list to extend, no shader to edit, and no UI change. + +`../build.rs` compiles each declaration into Rust implementing +[`Operation`](../src/operation.rs), generated into `OUT_DIR`. The result is +indistinguishable downstream from a hand-written operation: the same +`&'static OpDescriptor`, the same fused-shader composition, the same sidecar +round-trip. + +## What you get for free + +A node dropped in here arrives with: + +- **Controls.** The develop panel builds them from the parameters' declared + kinds (FR-DEV-3c). It never learns the node's name. +- **A place in the chain**, from `order:`. +- **Persistence.** Parameters are ordinary scalars, so the sidecar writes them + with everything else. +- **Its tests**, compiled from `tests:` and run with `cargo test`. +- **Neutral-means-absent.** At its defaults the node contributes no code, no + uniform, and no branch to the generated shader. + +## The shape of a file + +```yaml +id: exposure # must match the filename +label: op.exposure # a localisation key, never a display string +order: 20 # where it sits in the chain + +doc: | # becomes the generated module's documentation + Exposure — a linear gain, expressed in stops. +placement: | # why it sits where it does; appears above the + A correction to capture. # entry in the generated chain + +params: + exposure: + label: param.exposure + kind: stops + min: -5 + max: 5 + doc: | # optional prose, kept with the descriptor + ±5 stops. + +uniforms: + gain: exp2(exposure) # shorthand, or a mapping with `value:` and `doc:` + +helpers: [luminance] # names from _helpers.yaml +define: # helpers this node alone needs + my_curve: | + fn my_curve(x: f32) -> f32 { return x; } + +wgsl: | # `c` is linear RGB in and out + c = c * gain; + +tests: + - name: one_stop_is_a_doubling + why: The definition of a stop. + set: { exposure: 1 } + expect: { gain: 2.0 } +``` + +### Parameter kinds + +| `kind` | Shape | Extra keys | +|---|---|---| +| `amount` | −100…100, neutral at 0 — the familiar photographic control | — | +| `stops` | exposure-like, in stops | `min`, `max` | +| `fraction` | 0…1 | `default` | +| `switch` | a toggle, neutral off | — | +| `scalar` | anything else | `min`, `max`, `default`, `unit`, `scale`, `precision` | + +Reach for `amount` first. Writing its range out by hand in each node is how one +of them comes to disagree with the rest. + +### Uniform expressions + +Arithmetic (`+ - * /`), parentheses, numbers, this node's parameters, and: +`exp2` `log2` `exp` `sqrt` `abs` `floor` `ceil` `round` `pow` `min` `max` +`clamp` `mix`. + +Compiled to Rust, so an unknown name or a wrong arity is a build error naming +the file and the key. Deliberately small: a node is a description, and letting +it name arbitrary Rust would make this a second, worse place to write code. + +### Activity + +By default a node is active exactly when some parameter has moved off its +default, which is the honest rule and the right one for nearly everything. A +node whose neutral is something else declares `active:` as an expression — +non-zero means active. + +### Tests + +| Key | Asserts | +|---|---| +| `set` | parameter values to apply first; build fails if out of range | +| `expect` | uniform values, within 1e-5 | +| `expect_range` | a uniform lies in `[low, high]` | +| `expect_active` | whether the node reaches the shader | +| `expect_wgsl` | substrings the fragment must contain | +| `expect_helper_wgsl` | substrings a named helper must contain | + +`why:` becomes a comment in the generated test. Use it — a test named for a +property should say what breaks without it. + +## Hand-written nodes + +Some operations are not four facts, and forcing them into this schema would +produce a worse language aimed at one caller. Those stay in +[`../src/ops/`](../src/ops/) and declare only their position here: + +```yaml +id: tone_curve +order: 60 +rust: ToneCurve +why_rust: | + Its neutral is a relationship between five points, not a set of values. +``` + +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 +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. + +## Errors + +The build script reports failures by naming the key you got wrong, and exits +rather than panicking, so the message is the message and not a backtrace. It +refuses: a duplicate `order:`, a filename disagreeing with its `id:`, a default +outside its own range, a test value the graph would clamp before the node saw +it, a helper that does not define the function it names, an expression naming +something that is not a parameter, and a declared node whose name collides with +a file in `../src/ops/`. diff --git a/core/dr-pipeline/src/graph.rs b/core/dr-pipeline/src/graph.rs index f122a7e..9d0abea 100644 --- a/core/dr-pipeline/src/graph.rs +++ b/core/dr-pipeline/src/graph.rs @@ -79,30 +79,20 @@ pub struct EditGraph { impl EditGraph { /// The default develop chain, in pipeline order (ARCH §5.2). /// - /// Order is not arbitrary. White balance and exposure come first because - /// they are corrections to how the scene was captured, and the tonal - /// operations that follow should act on a correctly exposed image. - /// Colour comes last, so vibrance responds to the tones the user has - /// actually settled on rather than the ones they started with. + /// The order is not written here. Each node declares its own place with + /// an `order:` in `ops/.yaml`, and [`ops::chain`] is generated from + /// those — so adding an operation, or moving one, is an edit to a + /// declaration rather than to this file. + /// + /// The order itself is still not arbitrary. White balance and exposure + /// come first because they are corrections to how the scene was captured, + /// and the tonal operations that follow should act on a correctly exposed + /// image. Colour comes last, so vibrance responds to the tones the user + /// has actually settled on rather than the ones they started with. Each + /// node records that reasoning for itself, under `placement:`. pub fn default_chain() -> Self { Self { - ops: vec![ - Box::new(ops::WhiteBalance::new()), - Box::new(ops::Exposure::new()), - Box::new(ops::Contrast::new()), - Box::new(ops::HighlightsShadows::new()), - Box::new(ops::BlacksWhites::new()), - // After the region controls, so the curve is the final word - // on tone: a photographer reaches for it to fix what the - // fixed-weight controls could not place exactly. - Box::new(ops::ToneCurve::new()), - Box::new(ops::Brilliance::new()), - Box::new(ops::Vibrance::new()), - Box::new(ops::Saturation::new()), - // The mixer comes last: it is the finishing control, and it - // should act on the tones the user has already settled. - Box::new(ops::ColourMixer::new()), - ], + ops: ops::chain(), framing: Framing::new(), } } @@ -569,8 +559,8 @@ mod tests { let before = g.compose().structure_hash; g.set_param( - crate::ops::colour::SATURATION_ID, - crate::ops::colour::SATURATION, + crate::ops::saturation::ID, + crate::ops::saturation::SATURATION, 30.0, ); assert_ne!(before, g.compose().structure_hash); diff --git a/core/dr-pipeline/src/ops/colour.rs b/core/dr-pipeline/src/ops/colour.rs deleted file mode 100644 index bac85f2..0000000 --- a/core/dr-pipeline/src/ops/colour.rs +++ /dev/null @@ -1,397 +0,0 @@ -//! Colour operations: vibrance, saturation, and brilliance. -//! -//! # Vibrance versus saturation -//! -//! Saturation scales every colour's distance from grey equally. Vibrance -//! scales it *more for muted colours than for already-saturated ones*, and -//! protects skin tones. The difference matters: pushing saturation on a -//! portrait turns faces orange long before the background improves, which is -//! precisely the problem vibrance was invented to solve. -//! -//! # Brilliance -//! -//! Apple's control, and a genuinely different idea from either: it lifts -//! shadows and pulls highlights *simultaneously*, applying the opposite -//! correction at each end of the range while leaving mid-tones alone. The -//! result reads as "more light in the scene" rather than "less contrast", -//! because local relationships survive where a plain contrast reduction -//! flattens them. -//! -//! It overlaps with highlights/shadows deliberately — one control doing both -//! in a fixed relationship is easier to reach for than two controls needing -//! to be balanced against each other. - -use crate::descriptor::{LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId}; -use crate::operation::{Helper, Operation, Uniform}; -use crate::ops::tone::TONE_HELPERS; - -// --------------------------------------------------------------------------- -// Saturation -// --------------------------------------------------------------------------- - -pub const SATURATION_ID: OpId = OpId("saturation"); -pub const SATURATION: ParamId = ParamId("saturation"); - -static SAT_DESCRIPTOR: OpDescriptor = OpDescriptor { - id: SATURATION_ID, - label: LocalizedKey("op.saturation"), - params: &[ParamDescriptor::amount("saturation", "param.saturation")], -}; - -#[derive(Debug, Default, Clone)] -pub struct Saturation { - amount: f32, -} - -impl Saturation { - pub fn new() -> Self { - Self::default() - } -} - -impl Operation for Saturation { - fn descriptor(&self) -> &'static OpDescriptor { - &SAT_DESCRIPTOR - } - - fn set_param(&mut self, id: ParamId, value: f32) { - match id { - SATURATION => self.amount = value, - _ => log::warn!("saturation: unknown parameter {id}"), - } - } - - fn param(&self, id: ParamId) -> f32 { - match id { - SATURATION => self.amount, - _ => 0.0, - } - } - - fn is_active(&self) -> bool { - self.amount != 0.0 - } - - fn wgsl_body(&self) -> String { - "\ -// Interpolate away from the luminance-preserving grey. A factor of 0 is -// monochrome, 1 is unchanged, above 1 is more saturated. -let luma = luminance(c); -c = mix(vec3(luma), c, factor); -c = max(c, vec3(0.0));" - .into() - } - - fn uniforms(&self) -> Vec { - // -100 reaches exactly monochrome; +100 doubles the distance from - // grey. The floor at zero matters: a negative factor would push a - // colour past grey into its complement, inverting hues. - vec![Uniform { - name: "factor", - value: (1.0 + self.amount / 100.0).max(0.0), - }] - } - - fn helpers(&self) -> &'static [Helper] { - TONE_HELPERS - } -} - -// --------------------------------------------------------------------------- -// Vibrance -// --------------------------------------------------------------------------- - -pub const VIBRANCE_ID: OpId = OpId("vibrance"); -pub const VIBRANCE: ParamId = ParamId("vibrance"); - -static VIB_DESCRIPTOR: OpDescriptor = OpDescriptor { - id: VIBRANCE_ID, - label: LocalizedKey("op.vibrance"), - params: &[ParamDescriptor::amount("vibrance", "param.vibrance")], -}; - -#[derive(Debug, Default, Clone)] -pub struct Vibrance { - amount: f32, -} - -impl Vibrance { - pub fn new() -> Self { - Self::default() - } -} - -impl Operation for Vibrance { - fn descriptor(&self) -> &'static OpDescriptor { - &VIB_DESCRIPTOR - } - - fn set_param(&mut self, id: ParamId, value: f32) { - match id { - VIBRANCE => self.amount = value, - _ => log::warn!("vibrance: unknown parameter {id}"), - } - } - - fn param(&self, id: ParamId) -> f32 { - match id { - VIBRANCE => self.amount, - _ => 0.0, - } - } - - fn is_active(&self) -> bool { - self.amount != 0.0 - } - - fn wgsl_body(&self) -> String { - "\ -let luma = luminance(c); -let sat = colour_saturation(c); - -// The vibrance curve: full effect on grey, tapering to nothing on colours -// that are already saturated. Squaring the falloff keeps the mid-range -// responsive while still protecting the extremes. -let falloff = (1.0 - sat) * (1.0 - sat); - -// Skin protection. Skin sits in a narrow band of hue where red leads green -// leads blue; pushing it is what makes vibrance look wrong on portraits. -// Detected by channel ordering rather than a hue angle, which costs a -// conversion and buys nothing here. -let is_skin = f32(c.r > c.g && c.g > c.b); -let skin_guard = 1.0 - is_skin * 0.5; - -let strength = amount * falloff * skin_guard; -c = mix(vec3(luma), c, 1.0 + strength); -c = max(c, vec3(0.0));" - .into() - } - - fn uniforms(&self) -> Vec { - vec![Uniform { - name: "amount", - value: self.amount / 100.0, - }] - } - - fn helpers(&self) -> &'static [Helper] { - // Needs both luminance (from tone) and the saturation measure. - // Duplicates across the two lists are deduplicated by the composer. - COLOUR_AND_TONE - } -} - -/// The helper set vibrance needs: luminance plus the saturation measure. -static COLOUR_AND_TONE: &[Helper] = crate::ops::helpers::COLOUR; - -// --------------------------------------------------------------------------- -// Brilliance -// --------------------------------------------------------------------------- - -pub const BRILLIANCE_ID: OpId = OpId("brilliance"); -pub const BRILLIANCE: ParamId = ParamId("brilliance"); - -static BRIL_DESCRIPTOR: OpDescriptor = OpDescriptor { - id: BRILLIANCE_ID, - label: LocalizedKey("op.brilliance"), - params: &[ParamDescriptor::amount("brilliance", "param.brilliance")], -}; - -#[derive(Debug, Default, Clone)] -pub struct Brilliance { - amount: f32, -} - -impl Brilliance { - pub fn new() -> Self { - Self::default() - } -} - -impl Operation for Brilliance { - fn descriptor(&self) -> &'static OpDescriptor { - &BRIL_DESCRIPTOR - } - - fn set_param(&mut self, id: ParamId, value: f32) { - match id { - BRILLIANCE => self.amount = value, - _ => log::warn!("brilliance: unknown parameter {id}"), - } - } - - fn param(&self, id: ParamId) -> f32 { - match id { - BRILLIANCE => self.amount, - _ => 0.0, - } - } - - fn is_active(&self) -> bool { - self.amount != 0.0 - } - - fn wgsl_body(&self) -> String { - "\ -let luma = luminance(c); -let pos = tone_position(luma); - -// Opposite corrections at the two ends: shadows up, highlights down, both -// tapering to nothing at the mid-point. This is what separates brilliance -// from a contrast control — mid-tones keep their local relationships, so -// the image gains apparent light rather than losing structure. -let lift = (1.0 - smoothstep(0.0, 0.5, pos)) * amount; -let pull = smoothstep(0.5, 1.0, pos) * amount; - -// A mild saturation compensation. Flattening the tonal range washes colour -// out; without this, brilliance looks faded at useful settings. -let gain = exp2(lift - pull); -c = c * gain; - -let luma_after = luminance(c); -c = mix(vec3(luma_after), c, 1.0 + max(amount, 0.0) * 0.15); -c = max(c, vec3(0.0));" - .into() - } - - fn uniforms(&self) -> Vec { - vec![Uniform { - name: "amount", - // Half a stop at each end at full travel — the two ends move - // apart by a stop in total, which is a strong but not - // destructive flattening. - value: self.amount / 100.0 * 0.5, - }] - } - - fn helpers(&self) -> &'static [Helper] { - TONE_HELPERS - } -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::operation::compose; - - #[test] - fn all_three_start_neutral() { - assert!(!Saturation::new().is_active()); - assert!(!Vibrance::new().is_active()); - assert!(!Brilliance::new().is_active()); - } - - #[test] - fn full_negative_saturation_reaches_monochrome() { - // The property that makes -100 meaningful: it must land exactly on - // grey, not merely near it. - let mut s = Saturation::new(); - s.set_param(SATURATION, -100.0); - assert_eq!(s.uniforms()[0].value, 0.0); - } - - #[test] - fn positive_saturation_increases_the_factor() { - let mut s = Saturation::new(); - s.set_param(SATURATION, 100.0); - assert!((s.uniforms()[0].value - 2.0).abs() < 1e-6); - } - - #[test] - fn the_saturation_factor_never_goes_negative() { - // A negative factor would invert hues — a colour past monochrome - // becomes its complement, which is never wanted here. - let mut s = Saturation::new(); - s.set_param(SATURATION, -200.0); - assert!(s.uniforms()[0].value >= 0.0); - } - - #[test] - fn vibrance_protects_skin_in_its_fragment() { - // The distinguishing behaviour; if the guard is dropped, portraits - // go orange and the control is indistinguishable from saturation. - let mut v = Vibrance::new(); - v.set_param(VIBRANCE, 50.0); - let body = v.wgsl_body(); - assert!(body.contains("skin_guard"), "skin protection must survive"); - assert!( - body.contains("falloff"), - "the roll-off is what makes it vibrance" - ); - } - - #[test] - fn vibrance_and_saturation_are_distinct_operations() { - // They must not share an id, or the composer would emit one and the - // UI would show one control for two behaviours. - assert_ne!(VIBRANCE_ID, SATURATION_ID); - } - - #[test] - fn brilliance_moves_the_ends_in_opposite_directions() { - let mut b = Brilliance::new(); - b.set_param(BRILLIANCE, 100.0); - let body = b.wgsl_body(); - assert!(body.contains("lift"), "shadows must rise"); - assert!(body.contains("pull"), "highlights must fall"); - assert!( - body.contains("lift - pull"), - "the two must oppose, or this is just an exposure control" - ); - } - - #[test] - fn brilliance_travel_is_bounded() { - let mut b = Brilliance::new(); - b.set_param(BRILLIANCE, 100.0); - let v = b.uniforms()[0].value; - assert!((0.0..=0.6).contains(&v), "amount {v} is too aggressive"); - } - - #[test] - fn helpers_are_shared_across_every_colour_operation() { - // Vibrance declares its own helper list; if its luminance source - // drifted from tone's, the composer would emit whichever came first - // and the two operations would disagree about luminance. - let ops: Vec> = vec![ - Box::new({ - let mut o = Vibrance::new(); - o.set_param(VIBRANCE, 40.0); - o - }), - Box::new({ - let mut o = Saturation::new(); - o.set_param(SATURATION, 20.0); - o - }), - Box::new({ - let mut o = Brilliance::new(); - o.set_param(BRILLIANCE, 30.0); - o - }), - ]; - let shader = compose(&ops); - assert_eq!( - shader.source.matches("fn luminance(").count(), - 1, - "luminance must be declared exactly once" - ); - assert_eq!(shader.source.matches("fn colour_saturation(").count(), 1); - } - - #[test] - fn tone_and_colour_agree_on_luminance() { - // Both sets reference the shared definition. If someone reintroduces - // a local copy, the composer would emit whichever operation came - // first and the two would compute luminance differently. - let from_tone = TONE_HELPERS - .iter() - .find(|h| h.name == "luminance") - .expect("tone declares luminance"); - let from_colour = COLOUR_AND_TONE - .iter() - .find(|h| h.name == "luminance") - .expect("colour declares luminance"); - assert_eq!(from_tone.source, from_colour.source); - } -} diff --git a/core/dr-pipeline/src/ops/contrast.rs b/core/dr-pipeline/src/ops/contrast.rs deleted file mode 100644 index b97e649..0000000 --- a/core/dr-pipeline/src/ops/contrast.rs +++ /dev/null @@ -1,213 +0,0 @@ -//! Contrast — an S-curve about a fixed mid-point. -//! -//! Pushes tones away from middle grey (positive) or toward it (negative), -//! pivoting where the eye reads "neither light nor dark". In linear light -//! that point is 0.18, not 0.5: a scene-referred value of 0.5 is roughly a -//! stop and a half above middle grey, and pivoting there would darken almost -//! every photograph. -//! -//! The curve is applied in a perceptual domain rather than directly to linear -//! values. Applied linearly, an S-curve crushes shadows far harder than it -//! lifts highlights, because linear light devotes most of its range to the -//! brightest stop. - -use crate::descriptor::{LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId}; -use crate::operation::{Helper, Operation, Uniform}; -use crate::ops::helpers; - -pub const ID: OpId = OpId("contrast"); -pub const CONTRAST: ParamId = ParamId("contrast"); - -static DESCRIPTOR: OpDescriptor = OpDescriptor { - id: ID, - label: LocalizedKey("op.contrast"), - params: &[ParamDescriptor::amount("contrast", "param.contrast")], -}; - -/// The helpers this operation needs, including its own S-curve. -static CONTRAST_HELPERS: &[Helper] = &[ - helpers::LUMINANCE, - helpers::APPLY_TONE_GAIN, - Helper { - name: "contrast_curve", - source: "\ -// A symmetric S-curve on a 0..1 perceptual position. -// -// `amount` above zero steepens, below zero flattens. The smoothstep form is -// used for the steepening direction because it has zero gradient at both -// ends, so the curve cannot invert however hard it is pushed — the failure -// that makes naive gain-about-a-pivot unusable past moderate settings. -fn contrast_curve(x: f32, amount: f32) -> f32 { - let clamped = clamp(x, 0.0, 1.0); - if (amount >= 0.0) { - // Blend toward a smoothstep, which is the S. - let s = clamped * clamped * (3.0 - 2.0 * clamped); - return mix(clamped, s, amount); - } - // Flattening: pull toward the mid-point. At amount = -1 every tone - // collapses to 0.5, which is the meaningful limit of 'no contrast'. - return mix(clamped, 0.5, -amount); -}", - }, -]; - -#[derive(Debug, Default, Clone)] -pub struct Contrast { - amount: f32, -} - -impl Contrast { - pub fn new() -> Self { - Self::default() - } -} - -impl Operation for Contrast { - fn descriptor(&self) -> &'static OpDescriptor { - &DESCRIPTOR - } - - fn set_param(&mut self, id: ParamId, value: f32) { - match id { - CONTRAST => self.amount = value, - _ => log::warn!("contrast: unknown parameter {id}"), - } - } - - fn param(&self, id: ParamId) -> f32 { - match id { - CONTRAST => self.amount, - _ => 0.0, - } - } - - fn is_active(&self) -> bool { - self.amount != 0.0 - } - - fn wgsl_body(&self) -> String { - "\ -let luma = luminance(c); -if (luma > 0.0001) { - // Work on luminance and rescale the colour by the ratio, rather than - // curving each channel independently. Per-channel contrast shifts hue - // wherever the channels differ — the classic symptom being skies going - // cyan as contrast rises. - // - // MIDDLE_GREY is 0.18: the linear value the eye reads as mid-tone. The - // curve operates on luma/(2*0.18) so that middle grey lands at the - // curve's own 0.5 pivot. - let pos = clamp(luma / 0.36, 0.0, 1.0); - let curved = contrast_curve(pos, amount); - // Not `target`: that is a WGSL reserved keyword, and using it produces a - // parse error in generated code rather than anywhere a reader would look. - let curved_luma = curved * 0.36; - c = apply_tone_gain(c, curved_luma / luma); -} -c = max(c, vec3(0.0));" - .into() - } - - fn uniforms(&self) -> Vec { - vec![Uniform { - name: "amount", - value: self.amount / 100.0, - }] - } - - fn helpers(&self) -> &'static [Helper] { - CONTRAST_HELPERS - } -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::operation::compose; - - #[test] - fn neutral_does_nothing() { - let c = Contrast::new(); - assert!(!c.is_active()); - assert_eq!(c.uniforms()[0].value, 0.0); - } - - #[test] - fn the_amount_is_normalised_to_unit_range() { - // The shader's curve expects -1..1; the descriptor speaks -100..100. - let mut c = Contrast::new(); - c.set_param(CONTRAST, 100.0); - assert!((c.uniforms()[0].value - 1.0).abs() < 1e-6); - c.set_param(CONTRAST, -100.0); - assert!((c.uniforms()[0].value + 1.0).abs() < 1e-6); - } - - #[test] - fn contrast_works_on_luminance_not_per_channel() { - // Curving each channel separately shifts hue; the ratio form is what - // keeps a blue sky blue as contrast rises. - let mut c = Contrast::new(); - c.set_param(CONTRAST, 50.0); - let body = c.wgsl_body(); - assert!(body.contains("luminance(c)")); - assert!( - body.contains("apply_tone_gain"), - "the colour must be scaled by a ratio, not curved per channel" - ); - } - - #[test] - fn the_pivot_is_middle_grey_not_half() { - // Pivoting at 0.5 in linear light would darken nearly every image: - // scene-referred 0.5 is well above what the eye calls mid-tone. - let c = Contrast::new(); - assert!( - c.wgsl_body().contains("0.36"), - "the curve must pivot about middle grey (0.18, doubled to place \ - it at the curve's own midpoint)" - ); - } - - #[test] - fn the_curve_cannot_invert() { - // A gain-about-a-pivot form produces a non-monotonic curve past - // moderate settings, which inverts tones. smoothstep cannot. - let helper = CONTRAST_HELPERS - .iter() - .find(|h| h.name == "contrast_curve") - .expect("declares its curve"); - assert!(helper.source.contains("3.0 - 2.0 * clamped")); - } - - #[test] - fn it_composes_with_the_other_tonal_operations() { - // Contrast, highlights/shadows and brilliance all want `luminance`; - // the composer must emit it once. - let ops: Vec> = vec![ - Box::new({ - let mut o = Contrast::new(); - o.set_param(CONTRAST, 40.0); - o - }), - Box::new({ - let mut o = crate::ops::HighlightsShadows::new(); - o.set_param(crate::ops::tone::HIGHLIGHTS, -30.0); - o - }), - ]; - let shader = compose(&ops); - assert_eq!(shader.source.matches("fn luminance(").count(), 1); - assert_eq!(shader.source.matches("fn apply_tone_gain(").count(), 1); - assert_eq!(shader.source.matches("fn contrast_curve(").count(), 1); - } - - #[test] - fn a_division_by_luminance_is_guarded() { - // A black pixel has zero luminance; dividing by it would produce NaN - // and propagate through everything downstream. - assert!( - Contrast::new().wgsl_body().contains("luma > 0.0001"), - "the ratio must be guarded against black pixels" - ); - } -} diff --git a/core/dr-pipeline/src/ops/exposure.rs b/core/dr-pipeline/src/ops/exposure.rs deleted file mode 100644 index 223bb9c..0000000 --- a/core/dr-pipeline/src/ops/exposure.rs +++ /dev/null @@ -1,116 +0,0 @@ -//! Exposure — a linear gain, expressed in stops. -//! -//! The simplest operation in the pipeline and the one that most justifies -//! working in linear light: a stop is a doubling, so exposure is a single -//! multiply. Applied to gamma-encoded data it would be neither a doubling nor -//! reversible, which is why this stage sits where it does (ARCH §5.2). - -use crate::descriptor::{LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId}; -use crate::operation::{Operation, Uniform}; - -pub const ID: OpId = OpId("exposure"); -pub const EXPOSURE: ParamId = ParamId("exposure"); - -static DESCRIPTOR: OpDescriptor = OpDescriptor { - id: ID, - label: LocalizedKey("op.exposure"), - // ±5 stops. Wider than most edits need, but recovering a badly - // underexposed frame is a real use and raw data often supports it. - params: &[ParamDescriptor::stops( - "exposure", - "param.exposure", - -5.0, - 5.0, - )], -}; - -#[derive(Debug, Default, Clone)] -pub struct Exposure { - stops: f32, -} - -impl Exposure { - pub fn new() -> Self { - Self::default() - } - - /// The linear gain for the current setting. - fn gain(&self) -> f32 { - f32::exp2(self.stops) - } -} - -impl Operation for Exposure { - fn descriptor(&self) -> &'static OpDescriptor { - &DESCRIPTOR - } - - fn set_param(&mut self, id: ParamId, value: f32) { - match id { - EXPOSURE => self.stops = value, - _ => log::warn!("exposure: unknown parameter {id}"), - } - } - - fn param(&self, id: ParamId) -> f32 { - match id { - EXPOSURE => self.stops, - _ => 0.0, - } - } - - fn is_active(&self) -> bool { - self.stops != 0.0 - } - - fn wgsl_body(&self) -> String { - "c = c * gain;".into() - } - - fn uniforms(&self) -> Vec { - vec![Uniform { - name: "gain", - value: self.gain(), - }] - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn neutral_does_nothing() { - let e = Exposure::new(); - assert!(!e.is_active()); - assert_eq!(e.gain(), 1.0); - } - - #[test] - fn one_stop_is_a_doubling() { - // The definition of a stop. If this is wrong, every exposure - // adjustment is subtly off and no test of "looks right" would catch - // it. - let mut e = Exposure::new(); - e.set_param(EXPOSURE, 1.0); - assert!((e.gain() - 2.0).abs() < 1e-6); - - e.set_param(EXPOSURE, -1.0); - assert!((e.gain() - 0.5).abs() < 1e-6); - } - - #[test] - fn stops_compose_additively() { - // +2 stops must equal +1 applied twice. - let mut e = Exposure::new(); - e.set_param(EXPOSURE, 2.0); - assert!((e.gain() - 4.0).abs() < 1e-5); - } - - #[test] - fn the_range_covers_a_badly_exposed_frame() { - let d = &DESCRIPTOR.params[0]; - assert_eq!(d.clamp(-9.0), -5.0); - assert_eq!(d.clamp(9.0), 5.0); - } -} diff --git a/core/dr-pipeline/src/ops/helpers.rs b/core/dr-pipeline/src/ops/helpers.rs deleted file mode 100644 index c960c74..0000000 --- a/core/dr-pipeline/src/ops/helpers.rs +++ /dev/null @@ -1,117 +0,0 @@ -//! WGSL helper functions shared between operations. -//! -//! **Single source of truth.** Several operations need the same helpers, and -//! each declares a `&'static [Helper]` naming the ones it uses. The composer -//! deduplicates by *name*, so if two lists carried different source for the -//! same name it would silently emit whichever came first — and two operations -//! would compute, say, luminance differently depending on graph order. That -//! is a genuinely hard bug to see, so the sources are defined exactly once -//! here and referenced everywhere else. - -use crate::operation::Helper; - -/// Rec. 709 luminance. -pub const LUMINANCE: Helper = Helper { - name: "luminance", - source: "\ -// Rec. 709 luminance, the weighting that matches sRGB primaries. -// -// Applied to camera-space values it is an approximation — the true weights -// depend on the camera matrix — but using it here keeps the tonal operations -// working on sensor-native data, where highlight headroom still exists. -fn luminance(c: vec3) -> f32 { - return dot(c, vec3(0.2126, 0.7152, 0.0722)); -}", -}; - -/// Linear luminance mapped to a perceptual 0..1 position. -pub const TONE_POSITION: Helper = Helper { - name: "tone_position", - source: "\ -// Map linear luminance onto a perceptual 0..1 position. -// -// Tonal controls must feel evenly spaced to the eye, and linear light is -// not: middle grey sits at 0.18, so a linear weight would call almost -// everything a shadow. The cube root approximates lightness cheaply and -// behaves well near zero, where a log would diverge. -fn tone_position(luma: f32) -> f32 { - return clamp(pow(max(luma, 0.0), 1.0 / 3.0), 0.0, 1.0); -}", -}; - -/// Hue-preserving gain. -pub const APPLY_TONE_GAIN: Helper = Helper { - name: "apply_tone_gain", - source: "\ -// Scale a colour by a gain while preserving its hue. -// -// Multiplying the three channels equally keeps chromaticity fixed, so -// lifting shadows does not desaturate them the way an additive lift would. -fn apply_tone_gain(c: vec3, gain: f32) -> vec3 { - return c * gain; -}", -}; - -/// Distance from grey, as HSV chroma. -pub const COLOUR_SATURATION: Helper = Helper { - name: "colour_saturation", - source: "\ -// How far a colour sits from grey, in 0..1. -// -// The max-minus-min definition (HSV chroma) rather than a standard -// deviation: it matches what the eye reads as 'colourfulness' and it is what -// makes vibrance's roll-off land where users expect. -fn colour_saturation(c: vec3) -> f32 { - let hi = max(c.r, max(c.g, c.b)); - let lo = min(c.r, min(c.g, c.b)); - if (hi <= 0.0) { - return 0.0; - } - return (hi - lo) / hi; -}", -}; - -/// The set the tonal operations need. -pub static TONE: &[Helper] = &[LUMINANCE, TONE_POSITION, APPLY_TONE_GAIN]; - -/// The set the colour operations need. -pub static COLOUR: &[Helper] = &[LUMINANCE, TONE_POSITION, COLOUR_SATURATION]; - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn every_helper_defines_the_function_it_names() { - // A mismatch between the dedup key and the function actually emitted - // would produce either a duplicate definition or a missing one. - for h in TONE.iter().chain(COLOUR.iter()) { - assert!( - h.source.contains(&format!("fn {}(", h.name)), - "helper {} does not define fn {}", - h.name, - h.name - ); - } - } - - #[test] - fn helpers_shared_between_sets_are_the_same_value() { - // The drift this module exists to prevent: same name, different - // source, and the composer silently picks one. - let tone_luma = TONE.iter().find(|h| h.name == "luminance").unwrap(); - let colour_luma = COLOUR.iter().find(|h| h.name == "luminance").unwrap(); - assert_eq!(tone_luma.source, colour_luma.source); - } - - #[test] - fn no_set_lists_the_same_helper_twice() { - for set in [TONE, COLOUR] { - let mut names: Vec<&str> = set.iter().map(|h| h.name).collect(); - let before = names.len(); - names.sort_unstable(); - names.dedup(); - assert_eq!(before, names.len(), "a helper set lists a duplicate"); - } - } -} diff --git a/core/dr-pipeline/src/ops/mod.rs b/core/dr-pipeline/src/ops/mod.rs index cac5c54..34362cb 100644 --- a/core/dr-pipeline/src/ops/mod.rs +++ b/core/dr-pipeline/src/ops/mod.rs @@ -1,35 +1,173 @@ //! The develop operations. //! -//! Each operation is a self-contained file. Adding one means writing that file -//! and adding it to [`crate::graph::EditGraph::default_chain`] — no central -//! shader to edit, no UI change (FR-DEV-3c). +//! # Adding one //! -//! Most implement [`crate::operation::Operation`], a function from colour to -//! colour. The optical corrections ([`distortion`]) implement -//! [`crate::lens::Warp`] instead, because they rewrite *coordinates* before -//! the source is sampled rather than transforming a colour after it. Both -//! publish the same [`crate::descriptor::OpDescriptor`], so the UI builds -//! controls for them identically and never learns the difference. +//! Write `ops/.yaml` and rebuild. That is the whole procedure: the node +//! appears in the chain at its declared `order`, the develop panel grows the +//! controls its parameters describe (FR-DEV-3c), the sidecar persists them +//! because they are ordinary parameters, and its declared tests run with +//! everything else. +//! +//! There is no list to extend here, no shader to edit, and no UI change. +//! `build.rs` compiles each declaration into a module implementing +//! [`crate::operation::Operation`], and [`chain`] is generated from the +//! `order:` each node carries. +//! +//! # The two kinds of node +//! +//! **Declared** nodes are the majority: parameters, uniform expressions over +//! those parameters, and a WGSL fragment. Nothing about them is Rust. +//! +//! **Hand-written** nodes are the exceptions, and they are exceptions for a +//! reason rather than for want of migrating. The tone curve interpolates +//! between five points and its neutral is a *relationship* between them; the +//! colour mixer generates thirty-six faceted parameters from twelve computed +//! hue bands; [`vignetting`] carries lens-profile coefficients that are not +//! parameters at all. A schema stretched to cover those would be a worse +//! language than Rust, aimed at one caller each. +//! +//! Both publish the same [`crate::descriptor::OpDescriptor`], so nothing +//! downstream can tell them apart. A hand-written node still declares its +//! place in the chain in `ops/.yaml` with `rust:`, so the directory +//! remains the one place the pipeline's order is written down. +//! +//! # The optical corrections +//! +//! [`distortion`] and [`aberration`] implement [`crate::lens::Warp`] rather +//! than `Operation`, because they rewrite *coordinates* before the source is +//! sampled rather than transforming a colour after it. They are not part of +//! the develop chain and do not appear in `ops/`. +// Hand-written nodes. Each is listed in `ops/` with `rust:`, which is what +// places it in the chain; these are the implementations that entry points at. pub mod aberration; -pub mod colour; pub mod colour_mixer; -pub mod contrast; pub mod curve; pub mod distortion; -pub mod exposure; -pub mod helpers; -pub mod tone; pub mod vignetting; -pub mod white_balance; pub use aberration::Aberration; -pub use colour::{Brilliance, Saturation, Vibrance}; pub use colour_mixer::ColourMixer; -pub use contrast::Contrast; pub use curve::ToneCurve; pub use distortion::Distortion; -pub use exposure::Exposure; -pub use tone::{BlacksWhites, HighlightsShadows}; pub use vignetting::Vignetting; + +// The declared nodes, plus `helpers` and `chain`. Generated into OUT_DIR by +// `build.rs` from `ops/*.yaml` — see that file for why it does not land here +// beside the sources it looks exactly like. +include!(concat!(env!("OUT_DIR"), "/nodes.rs")); + +// Re-exported so a caller writes `ops::Exposure` as it did when these were +// hand-written files, and so the chain reads the same either way. +pub use blacks_whites::BlacksWhites; +pub use brilliance::Brilliance; +pub use contrast::Contrast; +pub use exposure::Exposure; +pub use highlights_shadows::HighlightsShadows; +pub use saturation::Saturation; +pub use vibrance::Vibrance; pub use white_balance::WhiteBalance; + +#[cfg(test)] +mod tests { + use super::*; + use std::collections::BTreeSet; + + #[test] + fn the_chain_is_what_the_declarations_say_it_is() { + // The only check available on a `rust:` node: `build.rs` cannot read + // the Rust type's descriptor, so it emits the declared id and this + // asserts the type agrees. A `rust:` entry whose id drifts from its + // implementation would otherwise reorder the pipeline silently. + let built: Vec<&str> = chain().iter().map(|o| o.descriptor().id.0).collect(); + assert_eq!(built, DECLARED_IDS); + } + + #[test] + fn every_helper_defines_the_function_it_names() { + // A mismatch between the dedup key and the function actually emitted + // would produce either a duplicate definition or a missing one. + // `build.rs` rejects this at the declaration; this asserts the + // generated registry kept the property. + for h in helpers::ALL { + assert!( + h.source.contains(&format!("fn {}(", h.name)), + "helper {} does not define fn {}", + h.name, + h.name + ); + } + } + + #[test] + fn no_two_helpers_share_a_name() { + // The drift the single-source-of-truth rule exists to prevent: same + // name, different source, and the composer silently picks one. + let mut names = BTreeSet::new(); + for h in helpers::ALL { + assert!( + names.insert(h.name), + "two helpers are both called {}", + h.name + ); + } + } + + #[test] + fn a_node_only_requests_helpers_that_exist() { + // Follows from the build-time check, but asserted end to end: a + // fragment calling a function no helper defines compiles here and + // fails in the shader, which is the expensive place to find it. + let known: BTreeSet<&str> = helpers::ALL.iter().map(|h| h.name).collect(); + for op in chain() { + for h in op.helpers() { + assert!( + known.contains(h.name) || h.source.contains(&format!("fn {}(", h.name)), + "{} requests helper {}, which is neither shared nor \ + defined by the node", + op.descriptor().id, + h.name + ); + } + } + } + + #[test] + fn every_node_starts_neutral() { + // An unedited image must be the image. A node whose defaults are not + // its neutral would apply itself to every photograph on open. + for op in chain() { + assert!( + !op.is_active(), + "{} is active at its defaults", + op.descriptor().id + ); + } + } + + #[test] + fn every_declared_parameter_round_trips() { + // The generated `set_param`/`param` pair is mechanical, which is + // exactly why it is worth checking: a wrong field in one arm reads + // perfectly and silently breaks the sidecar. + for mut op in chain() { + let descriptor = op.descriptor(); + for p in descriptor.params { + let crate::descriptor::ParamKind::Scalar { min, max, .. } = p.kind else { + continue; + }; + // A value inside the range and away from the default, so a + // stuck field cannot pass by returning the default. + let target = (p.default + (max - p.default) * 0.5).clamp(min, max); + op.set_param(p.id, target); + assert_eq!( + op.param(p.id), + target, + "{}.{} did not round-trip", + descriptor.id, + p.id + ); + } + } + } +} diff --git a/core/dr-pipeline/src/ops/tone.rs b/core/dr-pipeline/src/ops/tone.rs deleted file mode 100644 index 1e9ff21..0000000 --- a/core/dr-pipeline/src/ops/tone.rs +++ /dev/null @@ -1,312 +0,0 @@ -//! Tonal range operations: highlights/shadows and blacks/whites. -//! -//! Both work by building a smooth weight over the luminance range and -//! applying a gain where that weight is high. The distinction between the two -//! pairs is *where* they act and *how sharply*: -//! -//! - **Highlights and shadows** are broad and overlapping, recovering detail -//! across the upper and lower thirds. They are the controls used to tame a -//! contrasty scene. -//! - **Blacks and whites** act at the very ends, setting where the image -//! clips. They are the controls used to place the endpoints. -//! -//! Weights are built from smoothstep rather than a hard threshold: a sharp -//! boundary produces visible banding on a gradient — a sky is the worst case, -//! and it is also the most common subject for these controls. - -use crate::descriptor::{LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId}; -use crate::operation::{Helper, Operation, Uniform}; - -/// The helpers both tonal operations need. -/// -/// Sources live in [`crate::ops::helpers`] — see that module for why they are -/// defined exactly once. -pub use crate::ops::helpers::TONE as TONE_HELPERS; - -// --------------------------------------------------------------------------- -// Highlights and shadows -// --------------------------------------------------------------------------- - -pub const HIGHLIGHTS_SHADOWS_ID: OpId = OpId("highlights_shadows"); -pub const HIGHLIGHTS: ParamId = ParamId("highlights"); -pub const SHADOWS: ParamId = ParamId("shadows"); - -static HS_DESCRIPTOR: OpDescriptor = OpDescriptor { - id: HIGHLIGHTS_SHADOWS_ID, - label: LocalizedKey("op.highlights_shadows"), - params: &[ - // Negative recovers highlights, the overwhelmingly common direction, - // matching the convention every other developer uses. - ParamDescriptor::amount("highlights", "param.highlights"), - ParamDescriptor::amount("shadows", "param.shadows"), - ], -}; - -#[derive(Debug, Default, Clone)] -pub struct HighlightsShadows { - highlights: f32, - shadows: f32, -} - -impl HighlightsShadows { - pub fn new() -> Self { - Self::default() - } -} - -impl Operation for HighlightsShadows { - fn descriptor(&self) -> &'static OpDescriptor { - &HS_DESCRIPTOR - } - - fn set_param(&mut self, id: ParamId, value: f32) { - match id { - HIGHLIGHTS => self.highlights = value, - SHADOWS => self.shadows = value, - _ => log::warn!("highlights_shadows: unknown parameter {id}"), - } - } - - fn param(&self, id: ParamId) -> f32 { - match id { - HIGHLIGHTS => self.highlights, - SHADOWS => self.shadows, - _ => 0.0, - } - } - - fn is_active(&self) -> bool { - self.highlights != 0.0 || self.shadows != 0.0 - } - - fn wgsl_body(&self) -> String { - "\ -let luma = luminance(c); -let pos = tone_position(luma); - -// Broad, overlapping weights. Highlights ramp in over the upper half, -// shadows out over the lower half, so a mid-tone is barely touched by -// either and the two controls blend rather than fighting at the join. -let hi_w = smoothstep(0.5, 1.0, pos); -let lo_w = 1.0 - smoothstep(0.0, 0.5, pos); - -// Each control contributes up to a stop of gain at full deflection. -// exp2 keeps the effect symmetric: -100 and +100 are inverse. -let hi_gain = exp2(hi_amount * hi_w); -let lo_gain = exp2(lo_amount * lo_w); - -c = apply_tone_gain(c, hi_gain * lo_gain);" - .into() - } - - fn uniforms(&self) -> Vec { - vec![ - Uniform { - name: "hi_amount", - // A full stop at the extreme; enough to recover a bright sky - // without inverting the tonal relationship. - value: self.highlights / 100.0, - }, - Uniform { - name: "lo_amount", - value: self.shadows / 100.0, - }, - ] - } - - fn helpers(&self) -> &'static [Helper] { - TONE_HELPERS - } -} - -// --------------------------------------------------------------------------- -// Blacks and whites -// --------------------------------------------------------------------------- - -pub const BLACKS_WHITES_ID: OpId = OpId("blacks_whites"); -pub const BLACKS: ParamId = ParamId("blacks"); -pub const WHITES: ParamId = ParamId("whites"); - -static BW_DESCRIPTOR: OpDescriptor = OpDescriptor { - id: BLACKS_WHITES_ID, - label: LocalizedKey("op.blacks_whites"), - params: &[ - ParamDescriptor::amount("blacks", "param.blacks"), - ParamDescriptor::amount("whites", "param.whites"), - ], -}; - -#[derive(Debug, Default, Clone)] -pub struct BlacksWhites { - blacks: f32, - whites: f32, -} - -impl BlacksWhites { - pub fn new() -> Self { - Self::default() - } -} - -impl Operation for BlacksWhites { - fn descriptor(&self) -> &'static OpDescriptor { - &BW_DESCRIPTOR - } - - fn set_param(&mut self, id: ParamId, value: f32) { - match id { - BLACKS => self.blacks = value, - WHITES => self.whites = value, - _ => log::warn!("blacks_whites: unknown parameter {id}"), - } - } - - fn param(&self, id: ParamId) -> f32 { - match id { - BLACKS => self.blacks, - WHITES => self.whites, - _ => 0.0, - } - } - - fn is_active(&self) -> bool { - self.blacks != 0.0 || self.whites != 0.0 - } - - fn wgsl_body(&self) -> String { - "\ -let luma = luminance(c); -let pos = tone_position(luma); - -// Narrow weights concentrated at each end — this is what separates these -// controls from highlights/shadows, which are broad. Whites act only in the -// top quarter, blacks only in the bottom quarter. -let white_w = smoothstep(0.75, 1.0, pos); -let black_w = 1.0 - smoothstep(0.0, 0.25, pos); - -// Whites scale the top end multiplicatively, moving the clipping point. -let white_gain = exp2(white_amount * white_w); -c = apply_tone_gain(c, white_gain); - -// Blacks shift the floor. This one is deliberately *additive*: the point of -// a blacks control is to set where the image reaches zero, and a multiply -// can never bring a non-zero value to zero nor lift a true black off it. -c = c + vec3(black_amount * black_w); - -// The subtractive direction can push below zero, which is not light. -c = max(c, vec3(0.0));" - .into() - } - - fn uniforms(&self) -> Vec { - vec![ - Uniform { - name: "white_amount", - value: self.whites / 100.0, - }, - Uniform { - name: "black_amount", - // A small linear offset. Scene-referred black sits near zero, - // so the useful range here is far smaller than a stop — 0.02 - // is already a visible lift on a dark frame. - value: self.blacks / 100.0 * 0.02, - }, - ] - } - - fn helpers(&self) -> &'static [Helper] { - TONE_HELPERS - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn both_operations_start_neutral() { - assert!(!HighlightsShadows::new().is_active()); - assert!(!BlacksWhites::new().is_active()); - } - - #[test] - fn one_parameter_is_enough_to_activate() { - let mut hs = HighlightsShadows::new(); - hs.set_param(HIGHLIGHTS, -50.0); - assert!(hs.is_active()); - - let mut bw = BlacksWhites::new(); - bw.set_param(WHITES, 20.0); - assert!(bw.is_active()); - } - - #[test] - fn highlight_recovery_is_the_negative_direction() { - // The convention users expect: dragging left recovers. - let mut hs = HighlightsShadows::new(); - hs.set_param(HIGHLIGHTS, -100.0); - let u = hs.uniforms(); - assert!( - u[0].value < 0.0, - "negative highlights must produce a gain below 1" - ); - assert!((u[0].value + 1.0).abs() < 1e-6, "full travel is one stop"); - } - - #[test] - fn tonal_amounts_are_symmetric() { - let mut up = HighlightsShadows::new(); - up.set_param(SHADOWS, 100.0); - let mut down = HighlightsShadows::new(); - down.set_param(SHADOWS, -100.0); - // exp2 of equal and opposite exponents multiplies to 1. - assert!((up.uniforms()[1].value + down.uniforms()[1].value).abs() < 1e-6); - } - - #[test] - fn the_blacks_offset_stays_small() { - // Scene-referred black is near zero; a full-stop control here would - // be unusable, moving the image to grey at a fraction of its travel. - let mut bw = BlacksWhites::new(); - bw.set_param(BLACKS, 100.0); - let offset = bw - .uniforms() - .iter() - .find(|u| u.name == "black_amount") - .expect("black_amount") - .value; - assert!( - (0.0..=0.05).contains(&offset), - "offset {offset} is too large for scene-referred data" - ); - } - - #[test] - fn the_two_operations_share_helpers_without_duplicating_them() { - // Both request TONE_HELPERS; the composer must emit each once. - let ops: Vec> = vec![ - Box::new({ - let mut o = HighlightsShadows::new(); - o.set_param(HIGHLIGHTS, -30.0); - o - }), - Box::new({ - let mut o = BlacksWhites::new(); - o.set_param(BLACKS, 30.0); - o - }), - ]; - let shader = crate::operation::compose(&ops); - assert_eq!(shader.source.matches("fn luminance(").count(), 1); - assert_eq!(shader.source.matches("fn tone_position(").count(), 1); - } - - #[test] - fn unknown_parameters_are_ignored_rather_than_panicking() { - // A sidecar written by a newer version may name a parameter this - // build does not have; the image must still open. - let mut hs = HighlightsShadows::new(); - hs.set_param(ParamId("from_the_future"), 50.0); - assert!(!hs.is_active()); - } -} diff --git a/core/dr-pipeline/src/ops/white_balance.rs b/core/dr-pipeline/src/ops/white_balance.rs deleted file mode 100644 index d4d25d0..0000000 --- a/core/dr-pipeline/src/ops/white_balance.rs +++ /dev/null @@ -1,192 +0,0 @@ -//! White balance — temperature and tint, relative to as-shot. -//! -//! Expressed as an offset from what the camera chose rather than an absolute -//! kelvin value. Neutral means "as shot", so the control starts where the -//! image already is and a reset returns there. An absolute scale would make -//! the neutral position depend on the file, which is exactly the confusion -//! Lightroom's temperature slider creates on non-raw files. -//! -//! The as-shot multipliers themselves are applied here too, folded into the -//! same multiply — they come from the uniform block rather than the fragment, -//! because every image has them even when this operation is neutral. - -use crate::descriptor::{LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Scale, Unit}; -use crate::operation::{Operation, Uniform}; - -pub const ID: OpId = OpId("white_balance"); -pub const TEMPERATURE: ParamId = ParamId("temperature"); -pub const TINT: ParamId = ParamId("tint"); - -static DESCRIPTOR: OpDescriptor = OpDescriptor { - id: ID, - label: LocalizedKey("op.white_balance"), - params: &[ - // Warmer is positive, matching every other raw developer: dragging - // right makes the image warmer, even though that means *lowering* - // the colour temperature being corrected for. - ParamDescriptor::scalar( - "temperature", - "param.temperature", - -100.0, - 100.0, - 0.0, - Unit::None, - Scale::Linear, - 0, - ), - ParamDescriptor::amount("tint", "param.tint"), - ], -}; - -#[derive(Debug, Default, Clone)] -pub struct WhiteBalance { - temperature: f32, - tint: f32, -} - -impl WhiteBalance { - pub fn new() -> Self { - Self::default() - } - - /// Per-channel multipliers for the current settings. - /// - /// Temperature trades red against blue; tint trades green against - /// magenta. Both are scaled so the full range is a strong but not - /// destructive correction, and green is held near unity so the control - /// does not double as an exposure slider. - fn multipliers(&self) -> [f32; 3] { - // ±0.5 in log2 at the extremes — half a stop of channel shift, which - // covers ordinary illuminant error without letting the slider blow a - // channel on its own. - let t = self.temperature / 100.0 * 0.5; - let g = self.tint / 100.0 * 0.5; - - [ - f32::exp2(t), - f32::exp2(-g), - // Blue moves opposite red, so a neutral grey stays grey as the - // control moves. - f32::exp2(-t), - ] - } -} - -impl Operation for WhiteBalance { - fn descriptor(&self) -> &'static OpDescriptor { - &DESCRIPTOR - } - - fn set_param(&mut self, id: ParamId, value: f32) { - match id { - TEMPERATURE => self.temperature = value, - TINT => self.tint = value, - _ => log::warn!("white_balance: unknown parameter {id}"), - } - } - - fn param(&self, id: ParamId) -> f32 { - match id { - TEMPERATURE => self.temperature, - TINT => self.tint, - _ => 0.0, - } - } - - fn is_active(&self) -> bool { - self.temperature != 0.0 || self.tint != 0.0 - } - - fn wgsl_body(&self) -> String { - // as_shot_wb comes from the base uniform block: it applies to every - // image regardless of whether this operation is active, so the adjust - // pass folds it in separately. Here we apply only the user's offset. - "c = c * vec3(mul_r, mul_g, mul_b);".into() - } - - fn uniforms(&self) -> Vec { - let m = self.multipliers(); - vec![ - Uniform { - name: "mul_r", - value: m[0], - }, - Uniform { - name: "mul_g", - value: m[1], - }, - Uniform { - name: "mul_b", - value: m[2], - }, - ] - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn neutral_is_as_shot() { - let wb = WhiteBalance::new(); - assert!(!wb.is_active(), "a fresh control must not alter the image"); - assert_eq!(wb.multipliers(), [1.0, 1.0, 1.0]); - } - - #[test] - fn warming_raises_red_and_lowers_blue() { - let mut wb = WhiteBalance::new(); - wb.set_param(TEMPERATURE, 100.0); - let m = wb.multipliers(); - assert!(m[0] > 1.0, "red should rise, got {}", m[0]); - assert!(m[2] < 1.0, "blue should fall, got {}", m[2]); - } - - #[test] - fn cooling_is_the_inverse_of_warming() { - let mut warm = WhiteBalance::new(); - warm.set_param(TEMPERATURE, 60.0); - let mut cool = WhiteBalance::new(); - cool.set_param(TEMPERATURE, -60.0); - - let (w, c) = (warm.multipliers(), cool.multipliers()); - // Warming by n then cooling by n must return to neutral. - assert!((w[0] * c[0] - 1.0).abs() < 1e-5); - assert!((w[2] * c[2] - 1.0).abs() < 1e-5); - } - - #[test] - fn temperature_leaves_green_alone() { - // Otherwise the control doubles as an exposure slider, because green - // carries most of the luminance. - let mut wb = WhiteBalance::new(); - wb.set_param(TEMPERATURE, 100.0); - assert_eq!(wb.multipliers()[1], 1.0); - } - - #[test] - fn tint_moves_green_against_magenta() { - let mut wb = WhiteBalance::new(); - wb.set_param(TINT, 100.0); - let m = wb.multipliers(); - assert!(m[1] < 1.0, "positive tint reduces green (toward magenta)"); - assert_eq!(m[0], 1.0, "tint must not touch red"); - assert_eq!(m[2], 1.0, "tint must not touch blue"); - } - - #[test] - fn the_extremes_stay_within_half_a_stop() { - // A white balance control that can blow a channel by itself is a - // trap; correction belongs in a range where highlights survive. - let mut wb = WhiteBalance::new(); - wb.set_param(TEMPERATURE, 100.0); - wb.set_param(TINT, 100.0); - for m in wb.multipliers() { - assert!( - (0.70..=1.42).contains(&m), - "multiplier {m} exceeds half a stop" - ); - } - } -} diff --git a/core/dr-pipeline/src/sidecar.rs b/core/dr-pipeline/src/sidecar.rs index 3f3e4ee..e1a60bc 100644 --- a/core/dr-pipeline/src/sidecar.rs +++ b/core/dr-pipeline/src/sidecar.rs @@ -563,7 +563,7 @@ impl std::error::Error for ParseError {} mod tests { use super::*; use crate::framing; - use crate::ops::{colour, exposure, white_balance}; + use crate::ops::{exposure, saturation, white_balance}; fn edited() -> EditGraph { let mut g = EditGraph::default_chain(); @@ -586,7 +586,7 @@ mod tests { assert!(v .params .contains_key(&("exposure".into(), "exposure".into()))); - assert!(!v.params.keys().any(|(op, _)| op == colour::SATURATION_ID.0)); + assert!(!v.params.keys().any(|(op, _)| op == saturation::ID.0)); } #[test] @@ -821,7 +821,7 @@ mod tests { let mut a = Version::from_graph("u1", "Colour", &edited()); a.is_default = true; let mut mono = EditGraph::default_chain(); - mono.set_param(colour::SATURATION_ID, colour::SATURATION, -100.0); + mono.set_param(saturation::ID, saturation::SATURATION, -100.0); sidecar.put(a); sidecar.put(Version::from_graph("u2", "Mono", &mono)); @@ -832,7 +832,7 @@ mod tests { let mut g = EditGraph::default_chain(); parsed.versions["u2"].apply(&mut g); assert_eq!( - g.param(colour::SATURATION_ID, colour::SATURATION), + g.param(saturation::ID, saturation::SATURATION), Some(-100.0) ); assert_eq!(g.param(exposure::ID, exposure::EXPOSURE), Some(0.0)); diff --git a/docs/traceability.md b/docs/traceability.md index 61729f7..1171d33 100644 --- a/docs/traceability.md +++ b/docs/traceability.md @@ -9,8 +9,8 @@ Denominators are parsed from [`requirements.md`](requirements.md) at run time, n | Metric | Value | |---|---| -| Source files scanned | 96 | -| TRACES tags found | 144 | +| Source files scanned | 102 | +| TRACES tags found | 145 | | Requirements defined | 151 | | Requirements covered | 73 | | **Coverage** | **48.3%** (73/151) | @@ -33,12 +33,12 @@ _None._ | ID | Tagged in | |---|---| -| FR-CAT-1 | [`core/dr-catalog/src/scan.rs:1`](../core/dr-catalog/src/scan.rs#L1), [`core/dr-sync/src/scan.rs:93`](../core/dr-sync/src/scan.rs#L93), [`core/dr-types/src/lib.rs:185`](../core/dr-types/src/lib.rs#L185), [`tools/traceability/src/lib.rs:473`](../tools/traceability/src/lib.rs#L473), [`tools/traceability/src/lib.rs:505`](../tools/traceability/src/lib.rs#L505), [`ui/dr-ui/src/activity.rs:1`](../ui/dr-ui/src/activity.rs#L1), [`ui/dr-ui/src/library.rs:1`](../ui/dr-ui/src/library.rs#L1) | +| FR-CAT-1 | [`core/dr-catalog/src/scan.rs:1`](../core/dr-catalog/src/scan.rs#L1), [`core/dr-sync/src/scan.rs:93`](../core/dr-sync/src/scan.rs#L93), [`core/dr-types/src/lib.rs:185`](../core/dr-types/src/lib.rs#L185), [`tools/traceability/src/lib.rs:479`](../tools/traceability/src/lib.rs#L479), [`tools/traceability/src/lib.rs:511`](../tools/traceability/src/lib.rs#L511), [`ui/dr-ui/src/activity.rs:1`](../ui/dr-ui/src/activity.rs#L1), [`ui/dr-ui/src/library.rs:1`](../ui/dr-ui/src/library.rs#L1) | | FR-CAT-11 | [`ui/dr-ui/src/library.rs:150`](../ui/dr-ui/src/library.rs#L150) | | FR-CAT-12 | [`core/dr-pipeline/src/sidecar.rs:108`](../core/dr-pipeline/src/sidecar.rs#L108) | | FR-CAT-15 | [`core/dr-catalog/src/schema.rs:258`](../core/dr-catalog/src/schema.rs#L258), [`core/dr-catalog/src/trash.rs:1`](../core/dr-catalog/src/trash.rs#L1), [`core/dr-sync-nextcloud/src/lib.rs:366`](../core/dr-sync-nextcloud/src/lib.rs#L366), [`core/dr-sync/src/lib.rs:122`](../core/dr-sync/src/lib.rs#L122), [`core/dr-sync/src/scan.rs:426`](../core/dr-sync/src/scan.rs#L426), [`core/dr-sync/src/scan.rs:57`](../core/dr-sync/src/scan.rs#L57), [`core/dr-thumbs/src/lib.rs:341`](../core/dr-thumbs/src/lib.rs#L341), [`ui/dr-ui/src/collections_ui.rs:1299`](../ui/dr-ui/src/collections_ui.rs#L1299), [`ui/dr-ui/src/collections_ui.rs:817`](../ui/dr-ui/src/collections_ui.rs#L817), [`ui/dr-ui/src/library.rs:150`](../ui/dr-ui/src/library.rs#L150), [`ui/dr-ui/src/library.rs:167`](../ui/dr-ui/src/library.rs#L167), [`ui/dr-ui/src/library.rs:2020`](../ui/dr-ui/src/library.rs#L2020), [`ui/dr-ui/src/library.rs:2052`](../ui/dr-ui/src/library.rs#L2052), [`ui/dr-ui/src/library_ui.rs:112`](../ui/dr-ui/src/library_ui.rs#L112), [`ui/dr-ui/src/library_ui.rs:444`](../ui/dr-ui/src/library_ui.rs#L444), [`ui/dr-ui/src/trash.rs:1`](../ui/dr-ui/src/trash.rs#L1), [`ui/dr-ui/ui/collections.slint:458`](../ui/dr-ui/ui/collections.slint#L458) | | FR-CAT-1a | [`core/dr-types/src/lib.rs:47`](../core/dr-types/src/lib.rs#L47) | -| FR-CAT-2 | [`core/dr-catalog/src/lib.rs:1`](../core/dr-catalog/src/lib.rs#L1), [`core/dr-catalog/src/schema.rs:1`](../core/dr-catalog/src/schema.rs#L1), [`tools/traceability/src/lib.rs:473`](../tools/traceability/src/lib.rs#L473) | +| FR-CAT-2 | [`core/dr-catalog/src/lib.rs:1`](../core/dr-catalog/src/lib.rs#L1), [`core/dr-catalog/src/schema.rs:1`](../core/dr-catalog/src/schema.rs#L1), [`tools/traceability/src/lib.rs:479`](../tools/traceability/src/lib.rs#L479) | | FR-CAT-3 | [`core/dr-catalog/src/jobs.rs:1`](../core/dr-catalog/src/jobs.rs#L1), [`core/dr-sync/src/scan.rs:69`](../core/dr-sync/src/scan.rs#L69), [`core/dr-thumbs/src/codec.rs:1`](../core/dr-thumbs/src/codec.rs#L1), [`core/dr-thumbs/src/lib.rs:1`](../core/dr-thumbs/src/lib.rs#L1), [`ui/dr-ui/src/derived_sync.rs:1`](../ui/dr-ui/src/derived_sync.rs#L1) | | FR-CAT-4 | [`core/dr-catalog/src/lib.rs:1`](../core/dr-catalog/src/lib.rs#L1), [`core/dr-catalog/src/query.rs:1`](../core/dr-catalog/src/query.rs#L1), [`ui/dr-ui/src/library.rs:1`](../ui/dr-ui/src/library.rs#L1), [`ui/dr-ui/src/library_ui.rs:1`](../ui/dr-ui/src/library_ui.rs#L1) | | FR-CAT-5 | [`core/dr-catalog/src/rating.rs:1`](../core/dr-catalog/src/rating.rs#L1), [`core/dr-decode/src/lib.rs:240`](../core/dr-decode/src/lib.rs#L240), [`core/dr-decode/src/lib.rs:307`](../core/dr-decode/src/lib.rs#L307), [`core/dr-pipeline/src/sidecar.rs:125`](../core/dr-pipeline/src/sidecar.rs#L125) | @@ -50,9 +50,9 @@ _None._ | FR-CULL-2 | [`core/dr-decode/src/locate.rs:1`](../core/dr-decode/src/locate.rs#L1), [`core/dr-decode/src/preview.rs:161`](../core/dr-decode/src/preview.rs#L161) | | FR-CULL-4 | [`core/dr-catalog/src/rating.rs:1`](../core/dr-catalog/src/rating.rs#L1), [`core/dr-pipeline/src/sidecar.rs:125`](../core/dr-pipeline/src/sidecar.rs#L125), [`ui/dr-ui/src/library.rs:177`](../ui/dr-ui/src/library.rs#L177), [`ui/dr-ui/src/library.rs:289`](../ui/dr-ui/src/library.rs#L289) | | FR-DEV-3 | [`core/dr-pipeline/src/framing.rs:174`](../core/dr-pipeline/src/framing.rs#L174) | -| FR-DEV-3a | [`core/dr-pipeline/build.rs:1482`](../core/dr-pipeline/build.rs#L1482), [`core/dr-pipeline/src/descriptor.rs:116`](../core/dr-pipeline/src/descriptor.rs#L116), [`core/dr-pipeline/src/descriptor.rs:91`](../core/dr-pipeline/src/descriptor.rs#L91), [`core/dr-pipeline/src/graph.rs:143`](../core/dr-pipeline/src/graph.rs#L143), [`core/dr-pipeline/src/graph.rs:17`](../core/dr-pipeline/src/graph.rs#L17), [`core/dr-pipeline/src/graph.rs:41`](../core/dr-pipeline/src/graph.rs#L41), [`core/dr-pipeline/src/operation.rs:104`](../core/dr-pipeline/src/operation.rs#L104) | +| FR-DEV-3a | [`core/dr-pipeline/build.rs:1551`](../core/dr-pipeline/build.rs#L1551), [`core/dr-pipeline/ops/exposure.yaml:1`](../core/dr-pipeline/ops/exposure.yaml#L1), [`core/dr-pipeline/src/descriptor.rs:116`](../core/dr-pipeline/src/descriptor.rs#L116), [`core/dr-pipeline/src/descriptor.rs:91`](../core/dr-pipeline/src/descriptor.rs#L91), [`core/dr-pipeline/src/graph.rs:133`](../core/dr-pipeline/src/graph.rs#L133), [`core/dr-pipeline/src/graph.rs:17`](../core/dr-pipeline/src/graph.rs#L17), [`core/dr-pipeline/src/graph.rs:41`](../core/dr-pipeline/src/graph.rs#L41), [`core/dr-pipeline/src/operation.rs:104`](../core/dr-pipeline/src/operation.rs#L104) | | FR-DEV-3b | [`core/dr-pipeline/src/descriptor.rs:91`](../core/dr-pipeline/src/descriptor.rs#L91), [`core/dr-pipeline/src/graph.rs:41`](../core/dr-pipeline/src/graph.rs#L41), [`core/dr-pipeline/src/operation.rs:104`](../core/dr-pipeline/src/operation.rs#L104) | -| FR-DEV-3c | [`core/dr-pipeline/build.rs:1482`](../core/dr-pipeline/build.rs#L1482), [`core/dr-pipeline/src/graph.rs:143`](../core/dr-pipeline/src/graph.rs#L143) | +| FR-DEV-3c | [`core/dr-pipeline/build.rs:1551`](../core/dr-pipeline/build.rs#L1551), [`core/dr-pipeline/ops/exposure.yaml:1`](../core/dr-pipeline/ops/exposure.yaml#L1), [`core/dr-pipeline/src/graph.rs:133`](../core/dr-pipeline/src/graph.rs#L133) | | FR-DEV-3d | [`core/dr-pipeline/src/framing.rs:174`](../core/dr-pipeline/src/framing.rs#L174) | | FR-DEV-3e | [`core/dr-decode/src/lib.rs:492`](../core/dr-decode/src/lib.rs#L492), [`core/dr-decode/src/lib.rs:612`](../core/dr-decode/src/lib.rs#L612) | | FR-DEV-3h | [`core/dr-decode/src/lib.rs:307`](../core/dr-decode/src/lib.rs#L307), [`core/dr-decode/src/preview.rs:29`](../core/dr-decode/src/preview.rs#L29), [`core/dr-pipeline/src/framing.rs:188`](../core/dr-pipeline/src/framing.rs#L188), [`core/dr-types/src/lib.rs:272`](../core/dr-types/src/lib.rs#L272) | @@ -93,7 +93,7 @@ _None._ | NFR-ARCH-2 | [`core/dr-catalog/src/jobs.rs:1`](../core/dr-catalog/src/jobs.rs#L1) | | NFR-ARCH-4 | [`core/dr-catalog/src/error.rs:1`](../core/dr-catalog/src/error.rs#L1), [`core/dr-thumbs/src/error.rs:1`](../core/dr-thumbs/src/error.rs#L1) | | NFR-OPS-1 | [`tools/traceability/src/lib.rs:266`](../tools/traceability/src/lib.rs#L266) | -| NFR-P1 | [`core/dr-catalog/src/lib.rs:1`](../core/dr-catalog/src/lib.rs#L1), [`core/dr-catalog/src/scan.rs:1`](../core/dr-catalog/src/scan.rs#L1), [`tools/traceability/src/lib.rs:473`](../tools/traceability/src/lib.rs#L473) | +| NFR-P1 | [`core/dr-catalog/src/lib.rs:1`](../core/dr-catalog/src/lib.rs#L1), [`core/dr-catalog/src/scan.rs:1`](../core/dr-catalog/src/scan.rs#L1), [`tools/traceability/src/lib.rs:479`](../tools/traceability/src/lib.rs#L479) | | NFR-P13 | [`core/dr-decode/src/preview.rs:134`](../core/dr-decode/src/preview.rs#L134) | | NFR-P9 | [`ui/dr-ui/src/collections_ui.rs:1`](../ui/dr-ui/src/collections_ui.rs#L1), [`ui/dr-ui/src/library.rs:1`](../ui/dr-ui/src/library.rs#L1), [`ui/dr-ui/src/library_ui.rs:1`](../ui/dr-ui/src/library_ui.rs#L1), [`ui/dr-ui/src/trash.rs:1`](../ui/dr-ui/src/trash.rs#L1) | | NFR-R1 | [`core/dr-catalog/src/sync.rs:1`](../core/dr-catalog/src/sync.rs#L1) | @@ -104,7 +104,7 @@ _None._ | NFR-RES-1 | [`ui/dr-ui/src/lib.rs:45`](../ui/dr-ui/src/lib.rs#L45) | | NFR-RES-4 | [`core/dr-catalog/src/cache.rs:1`](../core/dr-catalog/src/cache.rs#L1), [`core/dr-catalog/src/schema.rs:230`](../core/dr-catalog/src/schema.rs#L230), [`core/dr-thumbs/src/codec.rs:1`](../core/dr-thumbs/src/codec.rs#L1), [`core/dr-thumbs/src/lib.rs:1`](../core/dr-thumbs/src/lib.rs#L1), [`core/dr-thumbs/src/lib.rs:341`](../core/dr-thumbs/src/lib.rs#L341) | | NFR-SEC-1 | [`core/dr-decode/src/error.rs:1`](../core/dr-decode/src/error.rs#L1) | -| R1 | [`tools/traceability/src/lib.rs:489`](../tools/traceability/src/lib.rs#L489), [`tools/traceability/src/lib.rs:493`](../tools/traceability/src/lib.rs#L493) | +| R1 | [`tools/traceability/src/lib.rs:495`](../tools/traceability/src/lib.rs#L495), [`tools/traceability/src/lib.rs:499`](../tools/traceability/src/lib.rs#L499) | | R4 | [`core/dr-gpu/src/lib.rs:123`](../core/dr-gpu/src/lib.rs#L123) | ## Not yet tagged diff --git a/tools/traceability/src/lib.rs b/tools/traceability/src/lib.rs index e14fc2e..6481544 100644 --- a/tools/traceability/src/lib.rs +++ b/tools/traceability/src/lib.rs @@ -301,7 +301,13 @@ pub fn compute_coverage(traced: &BTreeSet, defined: &DefinedRequirements } /// Source file extensions scanned for tags. -pub const SOURCE_SUFFIXES: &[&str] = &[".rs", ".slint", ".wgsl"]; +/// +/// `.yaml` is here because a develop operation is now declared rather than +/// written: `core/dr-pipeline/ops/.yaml` is the whole node, and the Rust +/// implementing it is generated into `OUT_DIR`, which is not scanned and could +/// not be linked to from the report if it were. Without this a node would have +/// nowhere to record the requirement it satisfies. +pub const SOURCE_SUFFIXES: &[&str] = &[".rs", ".slint", ".wgsl", ".yaml"]; /// Directories never scanned. const EXCLUDED: &[&str] = &["target", "target-android", ".git", "node_modules", "temp"];