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");