From 7421837c8a8976382e3907c0cd511a4503a9d7dc Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sat, 22 Aug 2026 10:10:00 +0200 Subject: [PATCH] Let an operation say what it is about, so the panel can group without naming MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tool tabs need a taxonomy, and the taxonomy was the problem: a table in `ui/` mapping operation to tab breaks FR-DEV-3a, and a `group:` field risks what `ui-refinement.md` condemned `starts-group` for — the core deciding where the panel draws things. `Attribute` threads the needle. It says what an operation *is* — tone, colour, detail, optics, geometry, effect — which is the same category as `ParamKind` and squarely on the core's side of ARCH §4.3a's line. What is drawn, where it sits and whether it is visible stay the frontend's. There is no attribute for "the third tab", the enum's order is declaration order rather than screen order, and a frontend may render these as tabs, as headings, or ignore them. The payoff is that a tab strip can be *derived*: the groups are the attributes present in the capability list, so the interface names no operation and needs no table to keep in step. An operation joins the right group by declaring what it is, which is the one thing its author is well placed to say. Plural, because the tone curve is genuinely both — an RGB curve is tonal and the per-channel curves are chromatic, and filing it under one would hide it from half the people looking for it. Required and non-empty, enforced in `build.rs`, and the failure was checked by removing the line rather than assumed. An operation with no attribute is invisible to a panel that groups by them; a build that stops costs ten seconds, a control nobody can find costs more. The vocabulary is closed for the same reason: a typo would otherwise invent a category holding exactly one operation, which looks like a deliberate one until somebody counts. Six tests over the real chain, including the hand-written operations that `build.rs` never sees and so cannot check. --- core/dr-pipeline/build.rs | 74 +++++++++++- core/dr-pipeline/ops/README.md | 27 +++++ core/dr-pipeline/ops/blacks_whites.yaml | 1 + core/dr-pipeline/ops/brilliance.yaml | 1 + core/dr-pipeline/ops/colour_mixer.yaml | 1 + core/dr-pipeline/ops/contrast.yaml | 1 + core/dr-pipeline/ops/exposure.yaml | 1 + core/dr-pipeline/ops/highlights_shadows.yaml | 1 + core/dr-pipeline/ops/saturation.yaml | 1 + core/dr-pipeline/ops/tone_curve.yaml | 5 + core/dr-pipeline/ops/vibrance.yaml | 1 + core/dr-pipeline/ops/white_balance.yaml | 1 + core/dr-pipeline/src/descriptor.rs | 105 ++++++++++++++++ core/dr-pipeline/src/framing.rs | 9 +- core/dr-pipeline/src/graph.rs | 14 +++ core/dr-pipeline/src/lens.rs | 4 +- core/dr-pipeline/src/lib.rs | 2 +- core/dr-pipeline/src/mask.rs | 1 + core/dr-pipeline/src/operation.rs | 4 +- core/dr-pipeline/src/ops/aberration.rs | 3 +- core/dr-pipeline/src/ops/colour_mixer.rs | 3 +- core/dr-pipeline/src/ops/curve.rs | 10 +- core/dr-pipeline/src/ops/distortion.rs | 3 +- core/dr-pipeline/src/ops/vignetting.rs | 6 +- core/dr-pipeline/tests/attributes.rs | 120 +++++++++++++++++++ ui/dr-ui/src/develop.rs | 3 + 26 files changed, 385 insertions(+), 17 deletions(-) create mode 100644 core/dr-pipeline/tests/attributes.rs diff --git a/core/dr-pipeline/build.rs b/core/dr-pipeline/build.rs index 90a1fdc..5abf89e 100644 --- a/core/dr-pipeline/build.rs +++ b/core/dr-pipeline/build.rs @@ -131,12 +131,65 @@ struct TestDef { expect_helper_wgsl: Vec<(String, Vec)>, } +/// The attributes an operation declares, validated against the vocabulary. +/// +/// **Required, and non-empty.** An operation with no attribute is invisible to +/// a frontend that filters by them, and a control that silently does not exist +/// is a far worse failure than a build that stops — especially when the cause +/// is one missing line in a YAML file nobody had reason to open. Failing here +/// costs whoever adds an operation ten seconds; failing at runtime costs a +/// photographer a control they cannot find and cannot know is missing. +/// +/// The vocabulary is closed on purpose. A typo would otherwise invent a +/// category containing exactly one operation, which is indistinguishable from +/// a deliberate new one until somebody notices the tab with a single control +/// in it. +fn read_attributes(root: &Mapping) -> Result, String> { + const KNOWN: [&str; 6] = ["tone", "colour", "detail", "optics", "geometry", "effect"]; + + let value = root.get("attributes").ok_or_else(|| { + format!( + "missing `attributes:`; every operation must say what it is about, \ + one or more of {KNOWN:?}. It is what lets the panel group \ + operations without naming any of them (ARCH §4.3a)." + ) + })?; + + let list = value + .as_sequence() + .ok_or("`attributes:` must be a list, even with one entry")?; + + let mut out = Vec::new(); + for entry in list { + let name = as_str(entry, "attributes")?; + if !KNOWN.contains(&name) { + return Err(format!( + "unknown attribute {name:?}; expected one of {KNOWN:?}" + )); + } + if out.contains(&name.to_string()) { + return Err(format!("attribute {name:?} is listed twice")); + } + out.push(name.to_string()); + } + + if out.is_empty() { + return Err("`attributes:` is empty; an operation with no attribute \ + would not appear in a panel that groups by them" + .into()); + } + Ok(out) +} + /// A node: either declared in full, or a pointer to a hand-written type. enum Node { Declared { id: String, label: String, order: i64, + /// What the operation is about (ARCH §4.3a). Never empty — see + /// `read_attributes`. + attributes: Vec, doc: Option, placement: Option, params: Vec, @@ -398,6 +451,7 @@ fn read_node(path: &Path, shared: &BTreeSet<&str>) -> Result { } let label = as_str(root.get("label").ok_or("missing `label:`")?, "label")?.to_string(); + let attributes = read_attributes(root)?; let params = read_params(root)?; let param_names: BTreeSet<&str> = params.iter().map(|p| p.id.as_str()).collect(); @@ -436,6 +490,7 @@ fn read_node(path: &Path, shared: &BTreeSet<&str>) -> Result { label, order, doc: opt_prose(root, "doc", "doc")?, + attributes, placement, params, uniforms, @@ -1404,6 +1459,7 @@ fn emit_node(out: &mut String, node: &Node) -> Result<(), String> { let Node::Declared { id, label, + attributes, doc, params, uniforms, @@ -1434,7 +1490,8 @@ fn emit_node(out: &mut String, node: &Node) -> Result<(), String> { out.push_str( " #[allow(unused_imports)]\n\ \x20 use crate::descriptor::{\n\ - \x20 LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Presentation,\n\ + \x20 Attribute, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId,\n\ + \x20 Presentation,\n\ \x20 Scale, Unit, WidgetDemand, WidgetKind,\n\ \x20 };\n\ \x20 #[allow(unused_imports)]\n\ @@ -1467,7 +1524,20 @@ fn emit_node(out: &mut String, node: &Node) -> Result<(), String> { } let _ = writeln!(out, " {},", p.ctor); } - out.push_str(" ],\n };\n"); + out.push_str(" ],\n"); + + // What the operation is about. The panel groups by these and names no + // operation, which is what keeps FR-DEV-3a true as the set grows. + let attrs: Vec = attributes + .iter() + .map(|a| { + let mut c = a.chars(); + let head = c.next().expect("attribute names are non-empty").to_uppercase(); + format!("Attribute::{head}{}", c.as_str()) + }) + .collect(); + let _ = writeln!(out, " attributes: &[{}],", attrs.join(", ")); + out.push_str(" };\n"); // Helpers: node-local definitions first, then the assembled list. for h in local_helpers { diff --git a/core/dr-pipeline/ops/README.md b/core/dr-pipeline/ops/README.md index f3669c6..29344fa 100644 --- a/core/dr-pipeline/ops/README.md +++ b/core/dr-pipeline/ops/README.md @@ -9,6 +9,29 @@ indistinguishable downstream from a hand-written operation: the same `&'static OpDescriptor`, the same fused-shader composition, the same sidecar round-trip. +## `attributes:` — what the operation is about + +Required, one or more of `tone`, `colour`, `detail`, `optics`, `geometry`, +`effect`. The build fails without it, and that is deliberate: an operation with +no attribute is invisible to an interface that groups by them, and a control +that silently does not exist is a worse failure than a build that stops. + +**Say what the operation is, not where you want it drawn.** The core declares +capabilities; the frontend composes (ARCH §4.3a). `tone` means "this is about +lightness", not "put this in the second tab" — the interface is free to render +attributes as tabs, as headings, or not at all, and that choice may differ +between a tablet and a desktop without this file changing. + +**Plural is normal.** The tone curve is `[tone, colour]` because an RGB curve +is tonal and the per-channel curves are chromatic; it appears wherever the +frontend decides that means. Reach for a second attribute when an operation +genuinely answers two questions, not to make it easier to find. + +**The vocabulary is closed.** A typo is a build error rather than a new +category containing exactly one operation — which looks identical to a +deliberate new category until somebody notices the group with one control in +it. + ## What you get for free A node dropped in here arrives with: @@ -16,6 +39,9 @@ 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:`. +- **A place in the panel**, from `attributes:`. The interface groups by these + and names no operation, so a node arrives in the right group by saying what + it is rather than by anyone editing a list in `ui/`. - **Persistence.** Parameters are ordinary scalars, so the sidecar writes them with everything else. - **Its tests**, compiled from `tests:` and run with `cargo test`. @@ -28,6 +54,7 @@ A node dropped in here arrives with: id: exposure # must match the filename label: op.exposure # a localisation key, never a display string order: 20 # where it sits in the chain +attributes: [tone] # what it is about; one or more, and required doc: | # becomes the generated module's documentation Exposure — a linear gain, expressed in stops. diff --git a/core/dr-pipeline/ops/blacks_whites.yaml b/core/dr-pipeline/ops/blacks_whites.yaml index c0d7978..257999f 100644 --- a/core/dr-pipeline/ops/blacks_whites.yaml +++ b/core/dr-pipeline/ops/blacks_whites.yaml @@ -1,6 +1,7 @@ id: blacks_whites label: op.blacks_whites order: 50 +attributes: [tone] doc: | Blacks and whites — the endpoints, where the image clips. diff --git a/core/dr-pipeline/ops/brilliance.yaml b/core/dr-pipeline/ops/brilliance.yaml index d5477be..1594028 100644 --- a/core/dr-pipeline/ops/brilliance.yaml +++ b/core/dr-pipeline/ops/brilliance.yaml @@ -1,6 +1,7 @@ id: brilliance label: op.brilliance order: 70 +attributes: [tone] doc: | Brilliance — shadows up and highlights down at once. diff --git a/core/dr-pipeline/ops/colour_mixer.yaml b/core/dr-pipeline/ops/colour_mixer.yaml index d70d313..c8b7555 100644 --- a/core/dr-pipeline/ops/colour_mixer.yaml +++ b/core/dr-pipeline/ops/colour_mixer.yaml @@ -1,5 +1,6 @@ id: colour_mixer order: 100 +attributes: [colour] rust: ColourMixer why_rust: | diff --git a/core/dr-pipeline/ops/contrast.yaml b/core/dr-pipeline/ops/contrast.yaml index 2d4a610..db6da2e 100644 --- a/core/dr-pipeline/ops/contrast.yaml +++ b/core/dr-pipeline/ops/contrast.yaml @@ -1,6 +1,7 @@ id: contrast label: op.contrast order: 30 +attributes: [tone] doc: | Contrast — an S-curve about a fixed mid-point. diff --git a/core/dr-pipeline/ops/exposure.yaml b/core/dr-pipeline/ops/exposure.yaml index 0ef0849..2463185 100644 --- a/core/dr-pipeline/ops/exposure.yaml +++ b/core/dr-pipeline/ops/exposure.yaml @@ -2,6 +2,7 @@ id: exposure label: op.exposure order: 20 +attributes: [tone] doc: | Exposure — a linear gain, expressed in stops. diff --git a/core/dr-pipeline/ops/highlights_shadows.yaml b/core/dr-pipeline/ops/highlights_shadows.yaml index c5a69f4..fed1943 100644 --- a/core/dr-pipeline/ops/highlights_shadows.yaml +++ b/core/dr-pipeline/ops/highlights_shadows.yaml @@ -1,6 +1,7 @@ id: highlights_shadows label: op.highlights_shadows order: 40 +attributes: [tone] doc: | Highlights and shadows — broad, overlapping recovery at both ends. diff --git a/core/dr-pipeline/ops/saturation.yaml b/core/dr-pipeline/ops/saturation.yaml index 340e9ad..f5cfd13 100644 --- a/core/dr-pipeline/ops/saturation.yaml +++ b/core/dr-pipeline/ops/saturation.yaml @@ -1,6 +1,7 @@ id: saturation label: op.saturation order: 90 +attributes: [colour] doc: | Saturation — every colour's distance from grey, scaled equally. diff --git a/core/dr-pipeline/ops/tone_curve.yaml b/core/dr-pipeline/ops/tone_curve.yaml index b773749..91f9e3b 100644 --- a/core/dr-pipeline/ops/tone_curve.yaml +++ b/core/dr-pipeline/ops/tone_curve.yaml @@ -8,6 +8,11 @@ # to learn. id: tone_curve order: 60 + +# Both, and this is the case the plural exists for: the RGB 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: [tone, colour] rust: ToneCurve why_rust: | diff --git a/core/dr-pipeline/ops/vibrance.yaml b/core/dr-pipeline/ops/vibrance.yaml index f253a85..dd51f23 100644 --- a/core/dr-pipeline/ops/vibrance.yaml +++ b/core/dr-pipeline/ops/vibrance.yaml @@ -1,6 +1,7 @@ id: vibrance label: op.vibrance order: 80 +attributes: [colour] doc: | Vibrance — saturation weighted toward the muted colours. diff --git a/core/dr-pipeline/ops/white_balance.yaml b/core/dr-pipeline/ops/white_balance.yaml index 086d10f..4096965 100644 --- a/core/dr-pipeline/ops/white_balance.yaml +++ b/core/dr-pipeline/ops/white_balance.yaml @@ -1,6 +1,7 @@ id: white_balance label: op.white_balance order: 10 +attributes: [colour] doc: | White balance — temperature and tint, relative to as-shot. diff --git a/core/dr-pipeline/src/descriptor.rs b/core/dr-pipeline/src/descriptor.rs index c8ffe62..6e18352 100644 --- a/core/dr-pipeline/src/descriptor.rs +++ b/core/dr-pipeline/src/descriptor.rs @@ -452,18 +452,123 @@ impl ParamDescriptor { } } +/// What an operation *is about*. +/// +/// A statement of the operation's nature, in the same category as +/// [`ParamKind`]: the core saying what a thing is, not where it is drawn. A +/// frontend may render these as tabs, as section headings, as a filter, or +/// ignore them entirely — that choice is composition and belongs to whoever +/// knows the window (ARCH §4.3a). +/// +/// # Why the core may say this at all +/// +/// The line §4.3a draws is between *what a thing is* and *what is drawn, +/// where it sits, how wide it is, and whether it is visible*. "White balance +/// is a colour operation" is the first kind. It is also knowledge the core is +/// uniquely placed to hold: the person adding an operation knows what it does, +/// and a frontend that had to work it out would be doing so by matching on the +/// operation's name — which is the one thing `ui/` may never do (FR-DEV-3a). +/// +/// What this deliberately is **not** is a tab name. There is no `Attribute` +/// for "the third tab", the order below is declaration order rather than +/// screen order, and an operation carrying two attributes appears wherever the +/// frontend decides that means — twice, once, or nowhere. +/// +/// # Plural on purpose +/// +/// An operation may carry several. The tone curve is genuinely both tonal and +/// chromatic — it has an RGB curve and per-channel curves — and forcing it to +/// pick one would file it away from half the people looking for it. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub enum Attribute { + /// Lightness and its distribution: exposure, contrast, the recovery + /// controls, the tone curve. + Tone, + /// Hue and saturation: white balance, the colour mixer, vibrance. + Colour, + /// Acutance and noise — what the image is made of at the pixel level. + /// Sharpening, noise reduction, texture, clarity. + Detail, + /// Corrections for the lens that took the photograph: distortion, + /// chromatic aberration, vignetting. + Optics, + /// The shape of the frame: crop, straighten, rotation, flips. + Geometry, + /// Applied rather than corrected — a look, not a fix. + Effect, +} + +impl Attribute { + /// Every attribute, in declaration order. + /// + /// Declaration order is roughly the order a photographer works in, which + /// makes it a reasonable *default* for a frontend that wants one. It is + /// not a screen order: nothing here obliges a frontend to show them all, + /// show them in this sequence, or show them at all. + pub const ALL: [Attribute; 6] = [ + Attribute::Tone, + Attribute::Colour, + Attribute::Detail, + Attribute::Optics, + Attribute::Geometry, + Attribute::Effect, + ]; + + /// The localisation key naming this concept. + /// + /// Naming the concept, the way an operation's own `label` names the + /// operation. What a frontend *does* with the name — a tab, a heading, + /// nothing — is still its own affair. + pub fn label(self) -> LocalizedKey { + LocalizedKey(match self { + Self::Tone => "attr.tone", + Self::Colour => "attr.colour", + Self::Detail => "attr.detail", + Self::Optics => "attr.optics", + Self::Geometry => "attr.geometry", + Self::Effect => "attr.effect", + }) + } + + /// Parse the name used in `ops/.yaml`. + pub fn from_name(name: &str) -> Option { + Some(match name { + "tone" => Self::Tone, + "colour" => Self::Colour, + "detail" => Self::Detail, + "optics" => Self::Optics, + "geometry" => Self::Geometry, + "effect" => Self::Effect, + _ => return None, + }) + } +} + /// The static description of an operation. #[derive(Debug, Clone, PartialEq)] pub struct OpDescriptor { pub id: OpId, pub label: LocalizedKey, pub params: &'static [ParamDescriptor], + /// What this operation is about (ARCH §4.3a). + /// + /// **Never empty**, and `build.rs` refuses to generate an operation that + /// declares none. An operation with no attribute would be invisible to a + /// frontend that filters by them, and a control that silently does not + /// exist is a worse failure than a build that stops — particularly when + /// the cause would be a missing line in a YAML file nobody looked at. + pub attributes: &'static [Attribute], } impl OpDescriptor { pub fn param(&self, id: ParamId) -> Option<&ParamDescriptor> { self.params.iter().find(|p| p.id == id) } + + /// Whether this operation is about `attribute`. + pub fn has(&self, attribute: Attribute) -> bool { + self.attributes.contains(&attribute) + } } #[cfg(test)] diff --git a/core/dr-pipeline/src/framing.rs b/core/dr-pipeline/src/framing.rs index 11016ab..2bb5294 100644 --- a/core/dr-pipeline/src/framing.rs +++ b/core/dr-pipeline/src/framing.rs @@ -41,10 +41,8 @@ use std::f32::consts::PI; use std::fmt::Write as _; -use crate::descriptor::{ - LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Presentation, Scale, Unit, - WidgetDemand, WidgetKind, -}; +use crate::descriptor::{Attribute, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Presentation, Scale, Unit, + WidgetDemand, WidgetKind,}; use crate::operation::Affects; pub const ID: OpId = OpId("framing"); @@ -74,6 +72,9 @@ static FRAMING_PARAMS: [ParamId; 8] = [ ]; static DESCRIPTOR: OpDescriptor = OpDescriptor { + // The shape of the frame, and the only operation that changes the + // output's dimensions. + attributes: &[Attribute::Geometry], id: ID, label: LocalizedKey("op.framing"), params: &[ diff --git a/core/dr-pipeline/src/graph.rs b/core/dr-pipeline/src/graph.rs index 09779e5..2b463cf 100644 --- a/core/dr-pipeline/src/graph.rs +++ b/core/dr-pipeline/src/graph.rs @@ -8,6 +8,7 @@ //! so reordering the pipeline needs no code change. use crate::descriptor::{ + Attribute, Facet, LocalizedKey, OpDescriptor, OpId, ParamId, ParamKind, Presentation, }; use crate::framing::{CropRect, Framing}; @@ -37,6 +38,17 @@ pub struct OpCapability { /// the named widget may ignore this and render sliders — the parameters /// are ordinary scalars either way, so nothing becomes unreachable. pub presentation: Option, + /// TRACES: FR-DEV-3a | FR-DEV-3c + /// What this operation is about. + /// + /// **The whole point is that a panel can group by these without knowing + /// what any operation is.** A tab strip built from the attributes present + /// in this list names no operation and needs no table mapping one to the + /// other, so a new operation joins the right group by declaring what it + /// is — which is the only thing its author is well placed to say. + /// + /// Never empty; `build.rs` refuses an operation that declares none. + pub attributes: &'static [Attribute], } /// TRACES: FR-DEV-3a | FR-DEV-3b @@ -182,6 +194,7 @@ impl EditGraph { }) .collect(), presentation: op.presentation(), + attributes: desc.attributes, } }); @@ -210,6 +223,7 @@ impl EditGraph { // sliders for framing *without naming framing* — see // `Framing::presentation`. presentation: self.framing.presentation(), + attributes: desc.attributes, }; ops.chain(std::iter::once(framing)).collect() diff --git a/core/dr-pipeline/src/lens.rs b/core/dr-pipeline/src/lens.rs index 29fb1a1..f0855f3 100644 --- a/core/dr-pipeline/src/lens.rs +++ b/core/dr-pipeline/src/lens.rs @@ -54,7 +54,7 @@ use std::fmt::Write as _; -use crate::descriptor::{OpDescriptor, ParamId}; +use crate::descriptor::{Attribute, OpDescriptor, ParamId}; use crate::operation::{Helper, Uniform}; /// A coordinate-domain operation, applied before the source is sampled. @@ -211,11 +211,13 @@ mod tests { id: OpId("warp_a"), label: LocalizedKey("a"), params: &[ParamDescriptor::amount("amount", "a.amount")], + attributes: &[Attribute::Tone], }; static DESC_B: OpDescriptor = OpDescriptor { id: OpId("warp_b"), label: LocalizedKey("b"), params: &[ParamDescriptor::amount("amount", "b.amount")], + attributes: &[Attribute::Tone], }; struct Fake { diff --git a/core/dr-pipeline/src/lib.rs b/core/dr-pipeline/src/lib.rs index c522aa4..4619418 100644 --- a/core/dr-pipeline/src/lib.rs +++ b/core/dr-pipeline/src/lib.rs @@ -43,7 +43,7 @@ pub mod preset; pub mod sidecar; pub use descriptor::{ - Facet, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, ParamKind, Presentation, + Attribute, Facet, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, ParamKind, Presentation, Scale, Unit, WidgetDemand, WidgetKind, }; pub use framing::{CropRect, Framing}; diff --git a/core/dr-pipeline/src/mask.rs b/core/dr-pipeline/src/mask.rs index 17c7b50..3e44ac3 100644 --- a/core/dr-pipeline/src/mask.rs +++ b/core/dr-pipeline/src/mask.rs @@ -521,6 +521,7 @@ impl MaskLayer { }) .collect(), presentation: op.presentation(), + attributes: desc.attributes, } }) .collect() diff --git a/core/dr-pipeline/src/operation.rs b/core/dr-pipeline/src/operation.rs index 165de80..66337fc 100644 --- a/core/dr-pipeline/src/operation.rs +++ b/core/dr-pipeline/src/operation.rs @@ -24,7 +24,7 @@ use std::fmt::Write as _; use dr_types::{ColourSpace, Transfer}; -use crate::descriptor::{OpDescriptor, ParamId, Presentation}; +use crate::descriptor::{Attribute, OpDescriptor, ParamId, Presentation}; use crate::framing::{Framing, FRAMING_UNIFORM_FIELDS}; use crate::mask::MaskStack; @@ -720,11 +720,13 @@ mod tests { id: OpId("op_a"), label: LocalizedKey("a"), params: &[ParamDescriptor::amount("amount", "a.amount")], + attributes: &[Attribute::Tone], }; static DESC_B: OpDescriptor = OpDescriptor { id: OpId("op_b"), label: LocalizedKey("b"), params: &[ParamDescriptor::amount("amount", "b.amount")], + attributes: &[Attribute::Tone], }; struct Fake { diff --git a/core/dr-pipeline/src/ops/aberration.rs b/core/dr-pipeline/src/ops/aberration.rs index 316274f..bc0195e 100644 --- a/core/dr-pipeline/src/ops/aberration.rs +++ b/core/dr-pipeline/src/ops/aberration.rs @@ -32,7 +32,7 @@ //! least to perceived sharpness. Scaling all three about a virtual reference //! would soften the image even when the correction is right. -use crate::descriptor::{LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Scale, Unit}; +use crate::descriptor::{Attribute, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Scale, Unit}; use crate::lens::Warp; use crate::operation::{Helper, Uniform}; @@ -48,6 +48,7 @@ pub const BLUE: ParamId = ParamId("blue"); const MAX_SCALE: f32 = 0.005; static DESCRIPTOR: OpDescriptor = OpDescriptor { + attributes: &[Attribute::Optics], id: ID, label: LocalizedKey("op.aberration"), params: &[ diff --git a/core/dr-pipeline/src/ops/colour_mixer.rs b/core/dr-pipeline/src/ops/colour_mixer.rs index db94339..f965e02 100644 --- a/core/dr-pipeline/src/ops/colour_mixer.rs +++ b/core/dr-pipeline/src/ops/colour_mixer.rs @@ -30,7 +30,7 @@ //! adjusted, each one's share depends on what the other is set to, so turning //! up one colour's saturation quietly weakened its neighbour's hue shift. -use crate::descriptor::{Facet, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId}; +use crate::descriptor::{Attribute, Facet, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId}; use crate::operation::{Helper, Operation, Uniform}; use crate::ops::helpers; @@ -174,6 +174,7 @@ macro_rules! band_params { // cannot be read out of `BANDS` here. `facets_match_their_bands` below is // what keeps them from drifting. static DESCRIPTOR: OpDescriptor = OpDescriptor { + attributes: &[Attribute::Colour], id: ID, label: LocalizedKey("op.colour_mixer"), params: band_params![ diff --git a/core/dr-pipeline/src/ops/curve.rs b/core/dr-pipeline/src/ops/curve.rs index ca0294e..6bc0bd3 100644 --- a/core/dr-pipeline/src/ops/curve.rs +++ b/core/dr-pipeline/src/ops/curve.rs @@ -31,10 +31,8 @@ //! filter constrains the tangents so the interpolant is monotone wherever the //! data is, which is exactly the guarantee a tone curve needs. -use crate::descriptor::{ - LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Presentation, Scale, Unit, - WidgetDemand, WidgetKind, -}; +use crate::descriptor::{Attribute, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Presentation, Scale, Unit, + WidgetDemand, WidgetKind,}; use crate::operation::{Helper, Operation, Uniform}; use crate::ops::helpers; @@ -80,6 +78,10 @@ const fn coord(id: &'static str, label: &'static str, default: f32) -> ParamDesc } static DESCRIPTOR: OpDescriptor = OpDescriptor { + // Both, and this is the case the plural exists for: an RGB 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 diff --git a/core/dr-pipeline/src/ops/distortion.rs b/core/dr-pipeline/src/ops/distortion.rs index e20707f..345087b 100644 --- a/core/dr-pipeline/src/ops/distortion.rs +++ b/core/dr-pipeline/src/ops/distortion.rs @@ -26,7 +26,7 @@ //! profile is a "make the horizon straight" task, which one term does well. //! The full triple is reachable by loading a profile. -use crate::descriptor::{LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Scale, Unit}; +use crate::descriptor::{Attribute, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Scale, Unit}; use crate::lens::Warp; use crate::operation::{Helper, Uniform}; @@ -34,6 +34,7 @@ pub const ID: OpId = OpId("distortion"); pub const AMOUNT: ParamId = ParamId("amount"); static DESCRIPTOR: OpDescriptor = OpDescriptor { + attributes: &[Attribute::Optics], id: ID, label: LocalizedKey("op.distortion"), // ±100 maps to a ±0.25 cubic coefficient. That covers an uncorrected diff --git a/core/dr-pipeline/src/ops/vignetting.rs b/core/dr-pipeline/src/ops/vignetting.rs index b1547c2..3bf7724 100644 --- a/core/dr-pipeline/src/ops/vignetting.rs +++ b/core/dr-pipeline/src/ops/vignetting.rs @@ -33,7 +33,7 @@ //! stages: a corner recovered by two stops has to be recovered while the //! highlight headroom to hold it still exists (ARCH §5.2). -use crate::descriptor::{LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId}; +use crate::descriptor::{Attribute, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId}; use crate::operation::{Helper, Operation, Uniform}; pub const ID: OpId = OpId("vignetting"); @@ -46,6 +46,10 @@ pub const AMOUNT: ParamId = ParamId("amount"); const MAX_K1: f32 = -0.5; static DESCRIPTOR: OpDescriptor = OpDescriptor { + // Optics rather than effect: this carries lens-profile coefficients + // and corrects what the lens did. A *creative* vignette is a different + // operation that does not exist yet, and would be `Effect`. + attributes: &[Attribute::Optics], id: ID, label: LocalizedKey("op.vignetting"), // Bidirectional deliberately. Negative values *add* falloff, which is a diff --git a/core/dr-pipeline/tests/attributes.rs b/core/dr-pipeline/tests/attributes.rs new file mode 100644 index 0000000..bf62a23 --- /dev/null +++ b/core/dr-pipeline/tests/attributes.rs @@ -0,0 +1,120 @@ +//! What operations say they are about (ARCH §4.3a). +//! +//! The point of an attribute is that a panel can group operations without +//! knowing what any of them is. These assert the properties that makes +//! possible, over the real chain rather than over fixtures — including the +//! hand-written operations, which `build.rs` never sees and so cannot check. + +use dr_pipeline::{Attribute, EditGraph}; + +/// The invariant the whole scheme rests on. +/// +/// An operation with no attribute is invisible to a panel that groups by them. +/// `build.rs` refuses to *generate* one; this covers the ones it does not +/// generate. +#[test] +fn every_operation_says_what_it_is_about() { + let graph = EditGraph::default_chain(); + for cap in graph.capabilities() { + assert!( + !cap.attributes.is_empty(), + "{} declares no attribute, so no panel grouping by attribute \ + would ever show it", + cap.id.0 + ); + } +} + +/// A panel can build its groups from the chain alone, with no table mapping +/// operations to groups — which is what lets `ui/` name no operation +/// (FR-DEV-3a). +#[test] +fn the_groups_are_derivable_from_the_chain() { + let graph = EditGraph::default_chain(); + let caps = graph.capabilities(); + + let mut present: Vec = caps + .iter() + .flat_map(|c| c.attributes.iter().copied()) + .collect(); + present.sort(); + present.dedup(); + + assert!(present.contains(&Attribute::Tone), "the chain has tonal work"); + assert!(present.contains(&Attribute::Colour)); + assert!( + present.contains(&Attribute::Geometry), + "framing is in the capability list and is geometry" + ); + + // And nothing derived is empty: a group with no operations in it would be + // a tab that opens onto nothing. + for a in &present { + assert!( + caps.iter().any(|c| c.attributes.contains(a)), + "{a:?} appeared with nothing in it" + ); + } +} + +/// The case the plural exists for. +#[test] +fn an_operation_may_be_about_two_things() { + let graph = EditGraph::default_chain(); + let curve = graph + .capabilities() + .into_iter() + .find(|c| c.id.0 == "tone_curve") + .expect("the chain has a tone curve"); + + assert!(curve.attributes.contains(&Attribute::Tone)); + assert!( + curve.attributes.contains(&Attribute::Colour), + "the per-channel curves are chromatic; filing it under tone alone \ + would hide it from half the people looking for it" + ); +} + +/// Attributes describe the operation, never the screen — so a frontend that +/// groups by them can still reach every parameter, and one that ignores them +/// loses nothing. +#[test] +fn grouping_reaches_every_parameter() { + let graph = EditGraph::default_chain(); + let caps = graph.capabilities(); + + let ungrouped: usize = caps.iter().map(|c| c.params.len()).sum(); + let reachable: usize = caps + .iter() + .filter(|c| Attribute::ALL.iter().any(|a| c.attributes.contains(a))) + .map(|c| c.params.len()) + .sum(); + + assert_eq!( + reachable, ungrouped, + "a panel showing every attribute must show every parameter" + ); +} + +/// The vocabulary is closed, so a typo cannot invent a category holding one +/// operation — which is indistinguishable from a deliberate new one until +/// somebody notices the tab with a single control in it. +#[test] +fn the_vocabulary_is_closed() { + assert_eq!(Attribute::from_name("tone"), Some(Attribute::Tone)); + assert_eq!(Attribute::from_name("colour"), Some(Attribute::Colour)); + assert_eq!(Attribute::from_name("Tone"), None, "names are lower case"); + assert_eq!(Attribute::from_name("color"), None, "and are spelled once"); + assert_eq!(Attribute::from_name("tonal"), None); +} + +/// Every attribute names a concept the localiser can resolve, the way an +/// operation's own label does. +#[test] +fn every_attribute_is_nameable() { + for a in Attribute::ALL { + let key = a.label().0; + assert!(key.starts_with("attr."), "{a:?} has key {key:?}"); + assert!(key.len() > "attr.".len()); + } +} diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index 02ad81f..f55b958 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -2384,6 +2384,7 @@ mod tests { params: &[ParamId("a"), ParamId("b")], }), params: vec![param("a"), param("b")], + attributes: &[dr_pipeline::Attribute::Tone], }; assert!(rows_from(&[on_canvas]).is_empty()); @@ -2517,6 +2518,7 @@ mod tests { facet: None, }, ], + attributes: &[dr_pipeline::Attribute::Tone], }; let rows = rows_from(&[invented]); @@ -2591,6 +2593,7 @@ mod tests { facet: None, }, ], + attributes: &[dr_pipeline::Attribute::Tone], }; assert!(!supported(WidgetKind::ColourWheel), "precondition");