diff --git a/core/dr-pipeline/src/ops/colour_mixer.rs b/core/dr-pipeline/src/ops/colour_mixer.rs index 254e581..3113f42 100644 --- a/core/dr-pipeline/src/ops/colour_mixer.rs +++ b/core/dr-pipeline/src/ops/colour_mixer.rs @@ -14,12 +14,21 @@ //! that shows. Overlapping weights mean adjacent bands blend, and a colour //! halfway between two centres receives half of each. //! -//! # Why the weights are normalised +//! # Why the weights sum to one //! -//! With overlap, a pixel's weights sum to more than one, so applying each -//! band's gain independently would compound them. The shader normalises, so -//! setting every band's saturation to +100 gives the same result as setting -//! the global saturation to +100 rather than something far stronger. +//! The falloff window is exactly the band spacing, so at any hue the twelve +//! weights sum to one no matter where that hue falls — a partition of unity. +//! That is what lets each band's gain be applied and added with no further +//! scaling: setting every band's saturation to +100 gives the same result as +//! setting the global saturation to +100 rather than something far stronger, +//! and a band pushed on its own reaches its full documented travel. +//! +//! Dividing by the weight of the *adjusted* bands instead — which is what +//! this used to do — breaks both halves of that. A single adjusted band +//! divides by its own weight and cancels it, so the falloff disappears and +//! the band acts at full strength right up to a hard edge; and with two bands +//! 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::operation::{Helper, Operation, Uniform}; @@ -202,8 +211,7 @@ fn rgb_to_hcl(c: vec3) -> vec3 { var hue = 0.0; if (chroma > 0.00001) { if (hi == c.r) { - // fract handles the wrap from -60 to 300 without a branch. - hue = 60.0 * fract(((c.g - c.b) / chroma) / 6.0 + 1.0) * 6.0 / 6.0; + // The only branch that can come out negative, hence the wrap. hue = 60.0 * (((c.g - c.b) / chroma) % 6.0); if (hue < 0.0) { hue = hue + 360.0; } } else if (hi == c.g) { @@ -220,17 +228,19 @@ fn rgb_to_hcl(c: vec3) -> vec3 { source: "\ // How strongly a hue belongs to a band centred at `centre`. // -// Cosine falloff over +/-60 degrees, so a band reaches zero exactly at its -// neighbours' centres and adjacent weights sum to one across the gap. A -// narrower window would leave hues between bands unreachable; a wider one -// would make every adjustment affect the whole wheel. +// Cosine falloff over +/-30 degrees — the band spacing — so a band reaches +// zero exactly at its neighbours' centres and every hue's twelve weights sum +// to one. A narrower window would leave hues between bands weakly covered; a +// wider one makes the weights sum to more than one, and then no adjustment +// can be applied without scaling it by something that depends on which +// *other* bands are set. fn band_weight(hue: f32, centre: f32) -> f32 { // Shortest angular distance, accounting for the wrap at 360. var d = abs(hue - centre); if (d > 180.0) { d = 360.0 - d; } - if (d >= 60.0) { return 0.0; } - // cos ramp: 1 at the centre, 0 at 60 degrees. - return 0.5 + 0.5 * cos(d * 3.14159265 / 60.0); + if (d >= 30.0) { return 0.0; } + // cos ramp: 1 at the centre, 0 at 30 degrees. + return 0.5 + 0.5 * cos(d * 3.14159265 / 30.0); }", }, Helper { @@ -357,14 +367,12 @@ if (chroma > 0.0001) { lines.push_str( " - // Normalise by the total weight, so overlapping bands blend rather than - // compound. Without this, a hue sitting between two adjusted bands would - // receive roughly twice the intended adjustment. + // The deltas are used as accumulated: the twelve band weights sum to one + // at every hue, so a weighted sum over the adjusted bands is already on + // the right scale, and each band contributes independently of the others. + // `w_total` only says whether any adjusted band reaches this pixel — + // dividing by it would cancel the falloff and couple the bands together. if (w_total > 0.0001) { - d_hue = d_hue / w_total; - d_sat = d_sat / w_total; - d_lum = d_lum / w_total; - // Hue: up to 30 degrees at full travel. Enough to move foliage from // yellow-green to green, not enough to turn it blue by accident. let new_hue = hue + d_hue * 30.0; @@ -658,14 +666,86 @@ mod tests { } #[test] - fn overlapping_weights_are_normalised() { - // Without normalising, a hue between two adjusted bands gets roughly - // double the intended adjustment. + fn the_deltas_are_not_scaled_by_the_adjusted_bands_weight() { + // The bug this closes: dividing each delta by the summed weight of + // the bands that happen to be adjusted made the channels exclusive. + // One band divided by its own weight, cancelling the falloff; two + // bands split a fixed budget, so raising one colour's saturation cut + // its neighbour's hue shift. `band_weights_sum_to_one` is why no + // scaling is needed at all. let mut m = ColourMixer::new(); - m.set_param(ParamId("red_sat"), 50.0); + m.set_param(ParamId("red_hue"), 50.0); m.set_param(ParamId("orange_sat"), 50.0); let body = m.wgsl_body(); - assert!(body.contains("d_sat / w_total")); + for delta in ["d_hue", "d_sat", "d_lum"] { + assert!( + !body.contains(&format!("{delta} / w_total")), + "{delta} is scaled by the adjusted bands' weight" + ); + } + // Still guarded, so a pixel no adjusted band reaches is left alone. + assert!(body.contains("w_total > 0.0001")); + } + + #[test] + fn band_weights_sum_to_one_at_every_hue() { + // The property the shader relies on to add band deltas unscaled, and + // the one that ties the falloff window to the band spacing: widen or + // narrow `band_weight`'s window without moving the centres and this + // fails, which is the point — the sum would no longer be one and + // every adjustment would come out over- or under-strength. + // + // A mirror of the WGSL helper. It is nine lines, and the alternative + // is asserting nothing about the arithmetic that matters most here. + fn band_weight(hue: f32, centre: f32) -> f32 { + let mut d = (hue - centre).abs(); + if d > 180.0 { + d = 360.0 - d; + } + if d >= 30.0 { + return 0.0; + } + 0.5 + 0.5 * (d * std::f32::consts::PI / 30.0).cos() + } + + for step in 0..3600 { + let hue = step as f32 / 10.0; + let sum: f32 = BANDS.iter().map(|b| band_weight(hue, b.hue)).sum(); + assert!( + (sum - 1.0).abs() < 1e-5, + "weights at {hue} degrees sum to {sum}" + ); + } + } + + #[test] + fn the_falloff_window_is_the_band_spacing() { + // The Rust mirror above only proves the sum for the window it copies; + // this is what keeps the copy honest about the shader's own numbers. + let src = MIXER_HELPERS + .iter() + .find(|h| h.name == "band_weight") + .expect("the helper exists") + .source; + assert!(src.contains("d >= 30.0"), "the window is not +/-30 degrees"); + assert!( + src.contains("/ 30.0"), + "the cos ramp is not over 30 degrees" + ); + } + + #[test] + fn a_bands_channels_are_independent_of_its_neighbours() { + // Two adjacent bands, one adjusted for hue and one for saturation. + // Each must emit its own weighted term and nothing that mixes them. + let mut m = ColourMixer::new(); + m.set_param(ParamId("red_hue"), 50.0); + m.set_param(ParamId("orange_sat"), 50.0); + let body = m.wgsl_body(); + assert!(body.contains("d_hue = d_hue + w * red_hue")); + assert!(body.contains("d_sat = d_sat + w * orange_sat")); + assert!(!body.contains("red_sat"), "red's saturation is untouched"); + assert!(!body.contains("orange_hue"), "orange's hue is untouched"); } #[test]