dtourolle/DarkRoom
1
0
Fork 0
Code Issues 39 Pull Requests Actions Packages Projects Releases 20 Wiki Activity

Stop the mixer's bands from dividing one budget between them

Browse Source 
Each band's delta was scaled by the summed weight of the bands that happened
to be adjusted:

    d_hue = d_hue / w_total;
    d_sat = d_sat / w_total;

The divisor is the wrong quantity, and it fails in two directions.

A band adjusted on its own divides by its own weight and cancels it — w*v/w is
v — so the falloff does nothing. Green saturation at +100 hit a pixel at 179°
exactly as hard as one at 120° and not at all at 181°: full strength across the
whole window, then a cliff. That seam is the thing the overlap exists to
prevent, and it was there the moment a single band was touched.

Worse, the divisor counts a band's weight even for a channel that band says
nothing about, so the controls compete. `red_hue` at +100 shifts a pure red
pixel +30°. Set `orange_sat` as well and the same pixel shifts +20°, while
orange's saturation bleeds into pure red at a third strength. Hue and
saturation on *neighbouring* colours were trading against each other — the two
sliders behaving as though only one of them could be spent.

The real fault is upstream of the division: the falloff window was ±60° while
the bands sit 30° apart, so the twelve weights sum to 2.0 rather than 1.0 and
*something* had to correct for it. Narrowing the window to the band spacing
makes them a partition of unity, and then nothing has to. The division is gone;
`w_total` stays, demoted to what it should always have been — the test for
whether any adjusted band reaches this pixel at all.

Everything the module header claimed is now true rather than aspirational: a
band reaches zero at its neighbours' centres, a hue halfway between two gets
half of each, twelve bands at +100 equals global +100, and a band pushed alone
reaches the full 30° of travel its own comment documents instead of whatever
fraction the other sliders left it.

`overlapping_weights_are_normalised` asserted the broken arithmetic verbatim,
so it is replaced rather than repaired. In its place: no delta may be divided
by w_total, the guard must survive, two adjacent bands must emit independent
terms, and — the property the rest now rests on — the twelve weights must sum
to one, swept at 0.1° around the wheel. That last test carries a Rust mirror of
`band_weight`, so a third test pins the mirror to the shader's own constants;
a copy nothing checks is how the window and the spacing drifted apart in the
first place.

Also gone: the red branch of `rgb_to_hcl` computed its hue twice and threw the
first away.

This changes how existing edits render. Mixer adjustments are more selective,
and where they were quietly cancelling each other they no longer are, so a
saved sidecar will not come back looking the same.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-08-16 23:36:52 +02:00
co-authored by Claude Opus 5
parent 69b12e327f
commit 5e4b8de18d
1 changed files with 106 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/dr-pipeline/src/ops/colour_mixer.rs
+106 -26
View File
@@ -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<f32>) -> vec3<f32> {
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<f32>) -> vec3<f32> {
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]