Move the film to Effect in the descriptor that is actually read
An earlier commit claimed to move `film_sim` from `[tone, colour]` to `[effect]` and did not. It edited `ops/film_sim.yaml`, where `attributes:` is read, validated against the vocabulary, and then dropped: a `rust:` node publishes its own descriptor, and the type still said tone and colour. The stock went on appearing in the Light group beside exposure and again in Colour beside white balance, exactly as before, and every test passed. Nothing caught it because nothing could. The declaration parsed, the parity tests compare ids rather than attributes, and an operation filed under the wrong groups renders perfectly. It surfaced only on screen, as a missing Effects tab — which is indistinguishable from a category that genuinely has nothing in it, and is precisely how `Optics` looked for as long as it was empty. So three changes rather than one: `FilmSim`'s descriptor declares `Attribute::Effect`, which is the move the earlier commit described. `attributes:` joins the keys a `rust:` node may not carry, beside `params`, `uniforms`, `wgsl`, `helpers`, `define` and `label`. The rule was already written — "its descriptor comes from the type" — and attributes were the one field that slipped past it. A key that is silently ignored is worse than one that is rejected, because it reads as though it worked; the eight hand-written declarations lose a line that never did anything. And a test asserts that every attribute the chain carries reaches the tab strip. That is the property that was actually broken, and its failure mode is invisible from every direction: the controls exist, they are in the shader, and there is no way to filter to them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -9,7 +9,6 @@
|
|||||||
id: capture_sharpen
|
id: capture_sharpen
|
||||||
order: 120
|
order: 120
|
||||||
|
|
||||||
attributes: [detail]
|
|
||||||
rust: CaptureSharpen
|
rust: CaptureSharpen
|
||||||
|
|
||||||
why_rust: |
|
why_rust: |
|
||||||
|
|||||||
@@ -6,7 +6,6 @@
|
|||||||
id: clarity
|
id: clarity
|
||||||
order: 130
|
order: 130
|
||||||
|
|
||||||
attributes: [detail]
|
|
||||||
rust: Clarity
|
rust: Clarity
|
||||||
|
|
||||||
why_rust: |
|
why_rust: |
|
||||||
|
|||||||
@@ -1,6 +1,5 @@
|
|||||||
id: colour_mixer
|
id: colour_mixer
|
||||||
order: 100
|
order: 100
|
||||||
attributes: [colour]
|
|
||||||
rust: ColourMixer
|
rust: ColourMixer
|
||||||
|
|
||||||
why_rust: |
|
why_rust: |
|
||||||
|
|||||||
@@ -1,12 +1,9 @@
|
|||||||
id: film_sim
|
id: film_sim
|
||||||
order: 25
|
order: 25
|
||||||
# Effect, not tone-and-colour. It declared both, which put "Kodachrome" in
|
# What this node is *about* is not written here, and cannot be: a `rust:` node
|
||||||
# the Light group beside exposure and again in Colour beside white balance —
|
# publishes its own descriptor, so `attributes:` in this file would be read,
|
||||||
# two places, neither of which is where anyone looks for it, and both of which
|
# validated and then ignored. See `Attribute::Effect` on `FilmSim`'s descriptor
|
||||||
# it crowded. A stock is `Effect`'s own definition: applied rather than
|
# in `../src/ops/film_sim.rs`.
|
||||||
# corrected, a look and not a fix. That it moves tone and colour is true of
|
|
||||||
# every look and is not what the attribute is for.
|
|
||||||
attributes: [effect]
|
|
||||||
rust: FilmSim
|
rust: FilmSim
|
||||||
|
|
||||||
why_rust: |
|
why_rust: |
|
||||||
|
|||||||
@@ -8,7 +8,6 @@
|
|||||||
id: noise_reduction
|
id: noise_reduction
|
||||||
order: 110
|
order: 110
|
||||||
|
|
||||||
attributes: [detail]
|
|
||||||
rust: NoiseReduction
|
rust: NoiseReduction
|
||||||
|
|
||||||
why_rust: |
|
why_rust: |
|
||||||
|
|||||||
@@ -2,7 +2,6 @@
|
|||||||
id: texture
|
id: texture
|
||||||
order: 140
|
order: 140
|
||||||
|
|
||||||
attributes: [detail]
|
|
||||||
rust: Texture
|
rust: Texture
|
||||||
|
|
||||||
why_rust: |
|
why_rust: |
|
||||||
|
|||||||
@@ -12,7 +12,6 @@ order: 60
|
|||||||
# Both, and this is the case the plural exists for: the RGB curve is
|
# 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
|
# tonal and the per-channel curves are chromatic. Filing it under one
|
||||||
# would hide it from half the people looking for it.
|
# would hide it from half the people looking for it.
|
||||||
attributes: [tone, colour]
|
|
||||||
rust: ToneCurve
|
rust: ToneCurve
|
||||||
|
|
||||||
why_rust: |
|
why_rust: |
|
||||||
|
|||||||
@@ -8,7 +8,6 @@
|
|||||||
id: vignetting
|
id: vignetting
|
||||||
order: 5
|
order: 5
|
||||||
|
|
||||||
attributes: [optics]
|
|
||||||
rust: Vignetting
|
rust: Vignetting
|
||||||
|
|
||||||
why_rust: |
|
why_rust: |
|
||||||
|
|||||||
@@ -494,11 +494,28 @@ pub fn read_node(text: &str, ctx: &str, shared: &BTreeSet<&str>) -> Result<Node,
|
|||||||
if let Some(ty) = root.get("rust") {
|
if let Some(ty) = root.get("rust") {
|
||||||
let ty = as_str(ty, "rust")?.to_string();
|
let ty = as_str(ty, "rust")?.to_string();
|
||||||
check_type_name(&ty)?;
|
check_type_name(&ty)?;
|
||||||
for key in ["params", "uniforms", "wgsl", "helpers", "define", "label"] {
|
// `attributes` is in this list, and was not until a declaration that
|
||||||
|
// said `[effect]` sat above a type that said `[tone, colour]` for a
|
||||||
|
// whole commit without anything noticing. The line parsed, validated
|
||||||
|
// against the vocabulary, and was then dropped on the floor — so the
|
||||||
|
// file read as though it had moved the operation and the panel went on
|
||||||
|
// filing it under two groups it did not belong to. A key that is
|
||||||
|
// *ignored* is worse than one that is rejected, because it looks like
|
||||||
|
// it worked.
|
||||||
|
for key in [
|
||||||
|
"params",
|
||||||
|
"uniforms",
|
||||||
|
"wgsl",
|
||||||
|
"helpers",
|
||||||
|
"define",
|
||||||
|
"label",
|
||||||
|
"attributes",
|
||||||
|
] {
|
||||||
if root.contains_key(key) {
|
if root.contains_key(key) {
|
||||||
return Err(format!(
|
return Err(format!(
|
||||||
"`{key}` is meaningless on a `rust:` node — {ty} publishes \
|
"`{key}` is meaningless on a `rust:` node — {ty} publishes \
|
||||||
its own descriptor. Remove one or the other."
|
its own descriptor, and this file would be ignored. Say it \
|
||||||
|
in the type instead."
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -78,7 +78,17 @@ static DESCRIPTOR: LazyLock<Arc<OpDescriptor>> = LazyLock::new(|| {
|
|||||||
Arc::new(OpDescriptor {
|
Arc::new(OpDescriptor {
|
||||||
// Tone and colour both, and not `Effect`: a stock is not something applied
|
// Tone and colour both, and not `Effect`: a stock is not something applied
|
||||||
// on top of a photograph, it is what the photograph was made on.
|
// on top of a photograph, it is what the photograph was made on.
|
||||||
attributes: vec![Attribute::Tone, Attribute::Colour],
|
// Effect, not tone-and-colour. **This is the descriptor a `rust:` node
|
||||||
|
// is actually read from** — the `attributes:` line in `ops/*.yaml`
|
||||||
|
// describes a *declared* node and is inert here, which is how an
|
||||||
|
// earlier attempt to make this move changed nothing at all.
|
||||||
|
//
|
||||||
|
// A stock is `Effect`'s own definition: applied rather than corrected,
|
||||||
|
// a look and not a fix. Declaring tone and colour put "Kodachrome" in
|
||||||
|
// the Light group beside exposure and again in Colour beside white
|
||||||
|
// balance — two places, neither of which is where anyone looks for it.
|
||||||
|
// That it moves tone and colour is true of every look.
|
||||||
|
attributes: vec![Attribute::Effect],
|
||||||
id: ID,
|
id: ID,
|
||||||
label: LocalizedKey("op.film_sim"),
|
label: LocalizedKey("op.film_sim"),
|
||||||
params: vec![
|
params: vec![
|
||||||
|
|||||||
+15
-15
File diff suppressed because one or more lines are too long
@@ -4922,6 +4922,44 @@ mod tests {
|
|||||||
.clone()
|
.clone()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Every attribute the chain actually carries must reach the tab strip.
|
||||||
|
///
|
||||||
|
/// The strip is generated, so an attribute with operations behind it and
|
||||||
|
/// no tab in front of it is unreachable — the controls exist, are in the
|
||||||
|
/// shader, and cannot be filtered to. That is exactly how `Optics` sat
|
||||||
|
/// invisible for as long as it had no operations, and the failure looks
|
||||||
|
/// identical from the outside whether the cause is an empty category or a
|
||||||
|
/// broken filter.
|
||||||
|
#[test]
|
||||||
|
fn every_attribute_with_operations_gets_a_tab() {
|
||||||
|
let Some(ctx) = headless() else { return };
|
||||||
|
let rgba: Vec<u8> = (0..8 * 8).flat_map(|_| [128u8, 128, 128, 255]).collect();
|
||||||
|
let session = DevelopSession::open_rgb(&ctx, &rgba, 8, 8, dr_types::Orientation::NORMAL)
|
||||||
|
.expect("session");
|
||||||
|
|
||||||
|
let tabs: Vec<dr_pipeline::Attribute> =
|
||||||
|
session.tabs().into_iter().map(|(a, _)| a).collect();
|
||||||
|
|
||||||
|
for attribute in dr_pipeline::Attribute::ALL {
|
||||||
|
// Compose is deliberately absent: its one stage prefers an
|
||||||
|
// on-canvas widget, so a Compose tab would open onto nothing while
|
||||||
|
// `ComposePanel` holds the real controls.
|
||||||
|
if attribute == dr_pipeline::Attribute::Compose {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
let carried = dr_pipeline::EditGraph::default_chain()
|
||||||
|
.capabilities()
|
||||||
|
.iter()
|
||||||
|
.any(|c| c.attributes.contains(&attribute) && !c.params.is_empty());
|
||||||
|
assert_eq!(
|
||||||
|
tabs.contains(&attribute),
|
||||||
|
carried,
|
||||||
|
"{attribute:?}: carried by the chain = {carried}, has a tab = {}",
|
||||||
|
tabs.contains(&attribute)
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// A lookup needs all three of lens, focal length and aperture.
|
/// A lookup needs all three of lens, focal length and aperture.
|
||||||
///
|
///
|
||||||
/// Not pedantry about missing fields: distortion is interpolated across a
|
/// Not pedantry about missing fields: distortion is interpolated across a
|
||||||
|
|||||||
Reference in New Issue
Block a user