diff --git a/core/dr-pipeline/src/operation.rs b/core/dr-pipeline/src/operation.rs index 2684689..15dc2f4 100644 --- a/core/dr-pipeline/src/operation.rs +++ b/core/dr-pipeline/src/operation.rs @@ -397,16 +397,32 @@ fn main(@builtin(global_invocation_id) gid: vec3) {{ active.len() ); - // Framing enters the hash by structure only — which branches its prologue - // generated, never how far a slider moved. Dragging the crop handles must - // reuse the compiled pipeline and re-upload uniforms. + // Taken over the generated source, because the source *is* the structure: + // it is what gets compiled, and two compositions that produce different + // WGSL are two pipelines however alike their op-sets look. // - // The output space enters it too, and must: it changes the source, so two - // spaces sharing a hash would have the second silently rendered with the - // first's shader — a Display P3 export that came out sRGB and said - // otherwise. + // Hashing the list of active operation ids instead — which is what this + // did — assumed every operation emits the same code whatever its + // parameters say. The colour mixer does not: it emits a block and a + // uniform only for the bands that are set, so a red adjustment and a blue + // one are the same op-set and different shaders. They shared a cache + // entry, so the second was rendered with the first's compiled pipeline + // while its uniforms were uploaded in an order that pipeline never agreed + // to — whichever band was adjusted first kept acting, and every other + // band appeared dead. + // + // Values still do not enter it, since no operation writes a parameter + // value into its source; they arrive as uniforms, and dragging a slider + // regenerates identical text. One that did inline a value would have to + // recompile to be correct, and hashing the source says so rather than + // silently reusing the wrong pipeline. + // + // Framing and the output space are mixed in as well, though both already + // shape the source: the prologue's branches and the encode function are + // written into it. Belt and braces on the two inputs whose contribution to + // the source is indirect. let structure_hash = mix( - mix(hash_structure(&active), framing.structure_key()), + mix(hash_source(&source), framing.structure_key()), output as u64, ); @@ -575,22 +591,22 @@ fn mix(mut h: u64, value: u64) -> u64 { h } -/// Hash the op-set and order — the structure, not the values. +/// Hash the generated WGSL — the structure of the shader, not the values. /// -/// Two edits with the same operations at different slider positions produce -/// the same hash and reuse one compiled pipeline (ARCH §6.13: the hash is -/// over integer state only, so it is exactly deterministic). -fn hash_structure(active: &[&dyn Operation]) -> u64 { +/// Whole-source rather than a summary of what went into it: a summary has to +/// be kept in step with every operation's code generation by hand, and the +/// one that was here fell out of step with the colour mixer, which emits +/// different code for different bands. +/// +/// Still integer state hashed on the CPU, as ARCH §6.13 requires of a cache +/// key: the text is generated from parameters that are neutral or not, never +/// from a rendered float. +fn hash_source(source: &str) -> u64 { // FNV-1a: no dependency, stable across runs and platforms, which the // shader cache key requires. let mut h: u64 = 0xcbf2_9ce4_8422_2325; - for op in active { - for byte in op.descriptor().id.0.as_bytes() { - h ^= u64::from(*byte); - h = h.wrapping_mul(0x100_0000_01b3); - } - // A separator, so ["ab", "c"] and ["a", "bc"] differ. - h ^= 0xff; + for byte in source.as_bytes() { + h ^= u64::from(*byte); h = h.wrapping_mul(0x100_0000_01b3); } h diff --git a/core/dr-pipeline/src/ops/colour_mixer.rs b/core/dr-pipeline/src/ops/colour_mixer.rs index 3113f42..db94339 100644 --- a/core/dr-pipeline/src/ops/colour_mixer.rs +++ b/core/dr-pipeline/src/ops/colour_mixer.rs @@ -687,6 +687,64 @@ mod tests { assert!(body.contains("w_total > 0.0001")); } + #[test] + fn two_bands_are_two_shaders() { + // The bug this closes: the shader cache was keyed on the set of + // active operations, and this operation is active whichever band is + // set. A red adjustment and a blue one therefore shared a compiled + // pipeline — the first one to compile — and the second was rendered + // with the first's code while its uniform was uploaded into the + // first's slot. Every band but the one compiled first appeared to do + // nothing, and moving its slider moved the other band's colour. + // + // Asserted here rather than only in `operation.rs` because this is + // the operation that generates per-value code, and so the one whose + // shaders must not collide. + let compose_with = |id, v| { + let mut m = ColourMixer::new(); + m.set_param(ParamId(id), v); + let ops: Vec> = vec![Box::new(m)]; + crate::operation::compose(&ops) + }; + + let red = compose_with("red_sat", 60.0); + let blue = compose_with("blue_sat", 60.0); + assert_ne!( + red.source, blue.source, + "two bands emit different code, which is the premise" + ); + assert_ne!( + red.structure_hash, blue.structure_hash, + "two bands must not share a compiled pipeline" + ); + + // The same band at a different setting is the same shader, which is + // what keeps a slider drag from recompiling once per frame. + let red_harder = compose_with("red_sat", 90.0); + assert_eq!(red.structure_hash, red_harder.structure_hash); + assert_ne!(red.uniforms, red_harder.uniforms); + } + + #[test] + fn every_band_and_channel_is_its_own_shader() { + // All thirty-six, because the collision above was not special to red + // and blue: any two settings that generate different code and hash + // alike put one of them on the other's pipeline. + let mut seen = std::collections::HashMap::new(); + for band in BANDS.iter() { + for ch in Channel::ALL { + let id = format!("{}_{}", band.key, ch.suffix()); + let mut m = ColourMixer::new(); + m.set_param(ParamId(Box::leak(id.clone().into_boxed_str())), 50.0); + let ops: Vec> = vec![Box::new(m)]; + let hash = crate::operation::compose(&ops).structure_hash; + if let Some(other) = seen.insert(hash, id.clone()) { + panic!("{id} and {other} share a pipeline"); + } + } + } + } + #[test] fn band_weights_sum_to_one_at_every_hue() { // The property the shader relies on to add band deltas unscaled, and