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

Put the coordinate-domain lens corrections into the graph

Browse Source 
`lens.rs` has held a `Warp` trait, a composer and two implementations —
distortion and lateral chromatic aberration — since they were written, and
`compose_warps` was called by nothing outside its own tests. The corrections
existed, were correct, and never touched a photograph.

`EditGraph` now holds them, and `compose_full` emits them between the framing
prologue and the fetch. Distortion first, then CA: each warp receives the
position the previous one produced, and lateral CA is a magnification about
the optical axis of the *undistorted* frame, so measured on a barrel-distorted
one it would be fitted to a radius no profile describes.

They reach the panel the way framing already does — through `capabilities`.
That was the one open question and existing practice answered it: framing is
also not an `Operation`, also has parameters a photographer sets, and also
arrives through that list. Because `Preset::capture` walks the same list, the
sidecar, the clipboard and the undo stack carry a warp's parameters with
nothing registered anywhere, and no file under `ui/` names one (FR-DEV-3a).

`state()` destructures `EditGraph` field by field precisely so that a new
field cannot be forgotten, and it was not.

Chromatic aberration is the only thing that samples per channel, and
`splits_channels` is what keeps everything else from paying for it. Red and
blue are fetched from positions green is not — green is the reference and
never moves, so a wrong correction still leaves one channel sharp rather than
softening all three. With no CA in the chain the single-fetch path is emitted
instead.

The interpolating sampler is now chosen by framing *or* an active warp. Asking
framing alone would have nearest-neighboured a distortion correction on an
unstraightened frame, and that aliasing reads as a bad profile rather than as
a missing filter.

The warps go in the geometry invalidation key rather than the colour one: they
decide which source pixel a colour is read from, so a tile cached across a
distortion change would keep drawing the previous correction. The pipeline
cache needs nothing new — `hash_source` already covers the generated body, and
uniform values never enter it, so arming a warp recompiles and dragging it
does not. Both are asserted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-09-05 15:03:16 +02:00
co-authored by Claude Opus 5
parent e17b909d41
commit a1165ef182
10 changed files with 477 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/dr-pipeline/src/detail.rs
+1
View File
@@ -989,6 +989,7 @@ mod tests {
dr_types::ColourSpace::Srgb,
&crate::mask::MaskStack::new(),
&crate::spot::SpotSet::new(),
&[],
)
}
core/dr-pipeline/src/framing.rs
+1 -1
View File
@@ -2319,7 +2319,7 @@ mod tests {
// And the sampler's last step, which lives in `operation.rs` and is
// the half of the map this file does not emit.
assert!(
crate::operation::sample_source(false).contains("p / aspect + vec2<f32>(0.5)"),
crate::operation::sample_source(false, false).contains("p / aspect + vec2<f32>(0.5)"),
"the sampler's return to texture coordinates moved"
);
}
core/dr-pipeline/src/graph.rs
+210 -5
View File
@@ -120,6 +120,20 @@ pub struct EditGraph {
/// *where* an adjustment applies, and a spot says where a piece of the
/// photograph comes from.
spots: SpotSet,
/// The lens corrections that rewrite coordinates: distortion and lateral
/// chromatic aberration (`docs/architecture.md` §5.2).
///
/// Apart from `ops` for the fourth time, and this one is not about shape
/// but about direction. Every [`Operation`] is a function from colour to
/// colour, and these run *before* a colour exists: they decide which
/// source pixel is read, and CA decides it three times over. See
/// [`crate::lens`] for why that cannot be expressed as an operation.
///
/// Beside [`Self::framing`] in every way that matters — the two compose
/// into one coordinate map, and neither can be applied without the other's
/// result — but held separately because framing also changes the output's
/// dimensions, which a warp never does.
warps: Vec<Box<dyn crate::lens::Warp>>,
}
/// TRACES: FR-DEV-3f
@@ -164,6 +178,16 @@ impl EditGraph {
masks: Arc::new(MaskStack::new()),
film: None,
spots: SpotSet::new(),
// Distortion first, then CA, and the order is the correction's
// rather than a preference. Each warp receives the position the
// previous one produced, and lateral CA is a magnification about
// the optical axis of the *undistorted* frame — measured on a
// barrel-distorted one it would be fitted to a radius the lens
// profile does not describe.
warps: vec![
Box::new(crate::ops::Distortion::new()),
Box::new(crate::ops::Aberration::new()),
],
}
}
@@ -248,6 +272,18 @@ impl EditGraph {
self.ops.iter().map(|o| o.descriptor()).collect()
}
/// Descriptors for the coordinate-domain lens corrections, in order.
///
/// The counterpart to [`Self::descriptors`] and split from it for the same
/// reason framing is absent there: a warp emits its own block in the
/// generated shader rather than a colour fragment, so the codegen tests
/// that count `---- ` markers have to know which kind they are counting.
/// A UI wanting everything still reads [`Self::capabilities`], where all
/// three kinds arrive together and indistinguishably (FR-DEV-3a).
pub fn warp_descriptors(&self) -> Vec<Arc<OpDescriptor>> {
self.warps.iter().map(|w| w.descriptor()).collect()
}
/// TRACES: FR-DEV-3a | FR-DEV-3c
/// Everything a UI needs to build its controls.
///
@@ -286,6 +322,39 @@ impl EditGraph {
}
});
// The lens corrections first, matching where they sit in the shader:
// they rewrite the coordinate before any colour is fetched, so nothing
// below them can be judged until they are right. It also puts the
// three optical corrections together in the panel — these two and the
// vignetting node, which is an ordinary operation and arrives above
// through `ops`.
let warps = self.warps.iter().map(|w| {
let desc = w.descriptor();
OpCapability {
id: desc.id,
label: desc.label,
active: w.is_active(),
params: desc
.params
.iter()
.map(|p| ParamCapability {
id: p.id,
label: p.label,
kind: p.kind.clone(),
default: p.default,
value: w.param(p.id),
facet: p.facet,
})
.collect(),
// No preferred widget. A distortion amount and the two CA
// scales are ordinary scalars, and plain sliders — the
// fallback every frontend implements — are the right control
// for them.
presentation: None,
attributes: desc.attributes.clone(),
}
});
// Framing last, matching where it sits in the pipeline: the crop is
// decided after the image looks right, not before.
let desc = self.framing.descriptor();
@@ -314,7 +383,7 @@ impl EditGraph {
attributes: desc.attributes.clone(),
};
ops.chain(std::iter::once(framing)).collect()
warps.chain(ops).chain(std::iter::once(framing)).collect()
}
/// Set a parameter, clamping to the descriptor's declared range.
@@ -362,6 +431,11 @@ impl EditGraph {
// nothing to register (FR-DEV-3c).
ops: _,
framing: _,
// Reached through `capabilities` with the other two. A warp's
// parameters are ordinary scalars once they are in that list, so
// the sidecar, the clipboard and the undo stack carry them with
// nothing registered anywhere (FR-DEV-3c).
warps: _,
masks,
film,
spots,
@@ -440,6 +514,20 @@ impl EditGraph {
return;
}
// The warps, before the operations. Their ids cannot collide with an
// operation's — `ops/` and the warp list are disjoint by construction,
// and `declared_parity` asserts the chain is exactly what `ops/`
// declares — so the order is for readability rather than precedence.
if let Some(warp) = self.warps.iter_mut().find(|w| w.descriptor().id == op) {
let descriptor = warp.descriptor();
let Some(desc) = descriptor.param(param) else {
log::warn!("unknown parameter {param} on {op}; ignoring");
return;
};
warp.set_param(param, desc.clamp(value));
return;
}
let Some(operation) = self.ops.iter_mut().find(|o| o.descriptor().id == op) else {
// A sidecar naming an operation this build does not have. The
// rest of the edit must still apply.
@@ -465,6 +553,9 @@ impl EditGraph {
.param(param)
.map(|_| self.framing.param(param));
}
if let Some(warp) = self.warps.iter().find(|w| w.descriptor().id == op) {
return warp.descriptor().param(param).map(|_| warp.param(param));
}
self.ops
.iter()
.find(|o| o.descriptor().id == op)
@@ -478,6 +569,11 @@ impl EditGraph {
op.set_param(p.id, p.default);
}
}
for warp in &mut self.warps {
for p in &warp.descriptor().params {
warp.set_param(p.id, p.default);
}
}
self.framing.reset();
// Masks go too, and this is why `apply` can be a replacement rather
// than an overlay: a sidecar with no mask blocks means an edit with no
@@ -542,7 +638,14 @@ impl EditGraph {
/// graph renders to the screen and to a file in the same breath, and the
/// two want different answers.
pub fn compose_for(&self, output: dr_types::ColourSpace) -> ComposedShader {
compose_full(&self.ops, &self.framing, output, &self.masks, &self.spots)
compose_full(
&self.ops,
&self.framing,
output,
&self.masks,
&self.spots,
&self.warps,
)
}
/// TRACES: FR-DSP-1
@@ -637,6 +740,22 @@ impl EditGraph {
u64::from(crate::operation::canonical_bits(self.framing.param(p.id))),
);
}
// The warps belong to the geometry key, not the colour one: they decide
// which source pixel a colour is read from, so a cached *result* of
// this stage is wrong the moment one moves. Hashed by id as well as by
// value, so two warps swapping their amounts is not the same edit.
for warp in &self.warps {
let descriptor = warp.descriptor();
geometry = hash_bytes(geometry, descriptor.id.0.as_bytes());
for p in &descriptor.params {
geometry = hash_bytes(geometry, p.id.0.as_bytes());
geometry = mix(
geometry,
u64::from(crate::operation::canonical_bits(warp.param(p.id))),
);
}
}
// The view rect is not a parameter and not in the structure key — it
// is not an edit (see `Framing::view`). It is still an input to every
// rendered pixel, so a cache that ignored it would show the wrong part
@@ -823,20 +942,106 @@ mod tests {
assert_ne!(first.uniforms, second.uniforms);
}
/// The whole point of putting the warps in `capabilities`: everything that
/// walks that list carries them, with nothing registered anywhere.
#[test]
fn a_warp_is_carried_by_the_machinery_it_never_told_about_itself() {
use crate::ops::{aberration, distortion};
let mut g = EditGraph::default_chain();
g.set_param(distortion::ID, distortion::AMOUNT, 40.0);
g.set_param(aberration::ID, aberration::RED, 25.0);
assert_eq!(g.param(distortion::ID, distortion::AMOUNT), Some(40.0));
assert_eq!(g.param(aberration::ID, aberration::RED), Some(25.0));
// Through the same capture/apply the sidecar, the clipboard and the
// undo stack all use.
let state = g.state();
let mut restored = EditGraph::default_chain();
// No film in this graph, so nothing is owed; the result is asserted
// rather than dropped because ignoring it elsewhere would leave a
// photograph rendering without its stock.
assert_eq!(restored.set_state(&state), FilmRebake::NotNeeded);
assert_eq!(
restored.param(distortion::ID, distortion::AMOUNT),
Some(40.0),
"a distortion correction did not survive a state round trip, so \
reopening the photograph would silently drop it"
);
assert_eq!(restored.param(aberration::ID, aberration::RED), Some(25.0));
}
/// A warp is an edit, so `reset` has to reach it. It did not until the
/// loop was added: a reset that left the lens corrections standing would
/// mean "back to the file as it is" quietly did not mean that.
#[test]
fn resetting_the_graph_neutralises_the_warps() {
use crate::ops::distortion;
let mut g = EditGraph::default_chain();
g.set_param(distortion::ID, distortion::AMOUNT, 40.0);
g.reset();
assert_eq!(g.param(distortion::ID, distortion::AMOUNT), Some(0.0));
}
/// The warps belong to the geometry key, not the colour one.
///
/// A tile cache keyed on geometry holds the *result* of the coordinate
/// stage. Moving a distortion slider changes which source pixel every
/// output pixel reads, so a cache that did not notice would keep drawing
/// the previous correction — visibly, and only where it had already
/// cached.
#[test]
fn a_warp_moves_the_geometry_key_and_leaves_the_others_alone() {
use crate::operation::Affects;
use crate::ops::distortion;
let mut g = EditGraph::default_chain();
let before = g.invalidation();
g.set_param(distortion::ID, distortion::AMOUNT, 40.0);
let after = g.invalidation();
assert_ne!(
before.of(Affects::Geometry),
after.of(Affects::Geometry),
"a distortion change must invalidate the geometry stage"
);
assert_eq!(
before.of(Affects::Colour),
after.of(Affects::Colour),
"and must not invalidate the colour stage, which it does not touch"
);
}
#[test]
fn capabilities_describe_every_operation_and_parameter() {
// The UI builds its whole panel from this. Anything missing here is
// something the UI would have to hardcode.
let g = EditGraph::default_chain();
let caps = g.capabilities();
// Every operation, plus framing — which is not an operation and so
// is absent from `descriptors`, but must still reach the panel.
assert_eq!(caps.len(), g.descriptors().len() + 1);
// Every operation, plus everything that is *not* an operation and so
// is absent from `descriptors`: the framing, and the coordinate-domain
// lens corrections. All of them have parameters a photographer sets,
// so all of them have to reach the panel — and the panel is forbidden
// from naming any of them (FR-DEV-3a), which leaves this list as the
// only way they can arrive.
assert_eq!(caps.len(), g.descriptors().len() + g.warps.len() + 1);
assert!(
caps.iter().any(|c| c.id == crate::framing::ID),
"framing must appear in the capability list, or the UI cannot \
build a crop control without naming it"
);
for warp in &g.warps {
let id = warp.descriptor().id;
assert!(
caps.iter().any(|c| c.id == id),
"{id} must appear in the capability list, or its correction is \
in the shader with no control anywhere that can reach it"
);
}
for cap in &caps {
assert!(!cap.params.is_empty(), "{} exposes no parameters", cap.id);
core/dr-pipeline/src/operation.rs
+224 -8
View File
@@ -510,6 +510,11 @@ pub fn compose_with_framing(
output,
&MaskStack::new(),
&crate::spot::SpotSet::new(),
// No lens corrections. This entry point exists for callers that have
// an operation chain and nothing else — the codegen tests, and the
// export path before it grew a graph — and a warp is not something a
// caller can hold without one.
&[],
)
}
@@ -531,7 +536,13 @@ pub fn compose_full(
output: ColourSpace,
masks: &MaskStack,
spots: &crate::spot::SpotSet,
warps: &[Box<dyn crate::lens::Warp>],
) -> ComposedShader {
// The lens corrections, composed into one coordinate transform. Beside
// `framing` because they are the other half of the same stage: framing
// says which part of the source this output pixel comes from, and a warp
// says where the lens put it once it got there.
let warp = crate::lens::compose_warps(warps);
// Active *point* operations. A neighbourhood operation is filtered out
// here rather than asked for a fragment it cannot write: it reads pixels
// it is not writing, so it belongs to the detail stage that runs after
@@ -636,6 +647,19 @@ pub fn compose_full(
);
uniform_values.extend_from_slice(&framing.uniforms());
// The warps, immediately after framing and before any operation, matching
// where they sit in the shader. Slot order is emission order and nothing
// addresses a slot by number, so this only has to be consistent with
// itself — but keeping it in pipeline order is what makes the generated
// struct readable next to the generated body.
uniform_fields.push_str(&warp.uniform_fields);
uniform_values.extend_from_slice(&warp.uniforms);
for h in &warp.helpers {
if !helpers.iter().any(|existing| existing.name == h.name) {
helpers.push(*h);
}
}
for op in &active {
let id = op.descriptor().id.0;
let prefix = sanitise(id);
@@ -702,17 +726,35 @@ pub fn compose_full(
// The coordinate stage: output pixel -> source position -> colour. Emitted
// ahead of the operation fragments, which receive the sampled `c`.
let prologue = format!(
"{}\n{}",
framing.wgsl_prologue(),
sample_source(framing.needs_interpolation())
);
let sampler_helper = if framing.needs_interpolation() {
BILINEAR_HELPER
// A warp puts an output pixel between source pixels exactly as a free
// angle does, so either one forces the interpolating sampler. Asking
// framing alone — which is what this did before the warps existed — would
// have nearest-neighboured a distortion correction on an unstraightened
// frame, and the aliasing would have looked like a bad profile.
let interpolate = framing.needs_interpolation() || warp.is_active();
// Declared ahead of the warp block, which assigns to them. They enter
// equal to `p` so that a chain mixing a splitting warp with a
// non-splitting one still carries every earlier correction into the red
// and blue paths — a distortion correction must move all three channels,
// and only the CA that follows it may move them apart.
let channel_positions = if warp.splits_channels {
"\n // Per-channel source positions, for the lateral CA correction.\n\
\x20 var p_r = p;\n\
\x20 var p_b = p;\n"
} else {
""
};
let prologue = format!(
"{}{}{}\n{}",
framing.wgsl_prologue(),
channel_positions,
warp.body,
sample_source(interpolate, warp.splits_channels)
);
let sampler_helper = if interpolate { BILINEAR_HELPER } else { "" };
// The tail, and it is the whole of the difference between the two output
// modes. Everything above — the prologue, the fragments, the mask layers,
// the camera matrix — is emitted identically either way, so an operation
@@ -1049,7 +1091,51 @@ fn encode_output(c: vec3<f32>) -> vec3<f32> {{
/// Split out because it is the join between the coordinate stage and the
/// colour stage, and because the choice it makes — an exact integer load, or
/// a filtered sample — is the one thing the free-angle case changes.
pub(crate) fn sample_source(interpolate: bool) -> &'static str {
pub(crate) fn sample_source(interpolate: bool, splits_channels: bool) -> &'static str {
if splits_channels {
// Lateral chromatic aberration is a per-channel radial magnification,
// so red and blue are fetched from positions green is not — which is
// the whole reason a warp is not an `Operation`. By the time a colour
// reaches an operation the three channels have been sampled together
// and the divergence is gone.
//
// Green never moves. It is the reference the other two are scaled
// about, so a correction that is wrong still leaves one channel sharp
// rather than softening all three.
return " // Back to texture coordinates, once per channel.
let uv_src = p / aspect + vec2<f32>(0.5);
let uv_r = p_r / aspect + vec2<f32>(0.5);
let uv_b = p_b / aspect + vec2<f32>(0.5);
// Tested on green alone, not on all three.
//
// The three positions differ by a fraction of a pixel at any correction a
// real lens needs, so testing each would only let the outermost row of the
// frame disagree with itself about whether it exists — which draws a
// coloured fringe along the edge, the exact artefact this is here to
// remove. `sample_bilinear` clamps its own texel indices, so red and blue
// land on the edge pixel rather than out of bounds.
if (any(uv_src < vec2<f32>(0.0)) || any(uv_src >= vec2<f32>(1.0))) {
textureStore(output, vec2<i32>(gid.xy), vec4<f32>(0.0, 0.0, 0.0, 1.0));
return;
}
// TRACES: FR-DEV-3f
// Where this pixel sits on the *source*, in source pixels. Green's
// position, since that is the one that did not move.
let source_px = uv_src * vec2<f32>(src_dims);
let radius = length(p) / (0.5 * length(aspect));
// Three fetches, one channel kept from each. Two thirds of the work is
// discarded, which is why `Warp::splits_channels` exists: with no CA in
// the chain the single-sample path below is emitted instead.
var c = vec3<f32>(
sample_bilinear(uv_r, src_dims).r,
sample_bilinear(uv_src, src_dims).g,
sample_bilinear(uv_b, src_dims).b,
);
";
}
if interpolate {
" // Back to texture coordinates.
let uv_src = p / aspect + vec2<f32>(0.5);
@@ -1427,6 +1513,136 @@ mod tests {
}
}
use crate::lens::Warp as _;
/// A neutral warp list must leave the shader exactly as it was.
///
/// The property the whole `is_active` filter exists for: an unedited
/// photograph keeps the integer `textureLoad` path, and pays nothing —
/// not a bilinear fetch, not a uniform slot, not a recompile — for
/// corrections nobody has asked for.
#[test]
fn warps_at_neutral_change_nothing_at_all() {
let ops = crate::ops::chain();
let framing = Framing::new();
let without = compose_full(
&ops,
&framing,
ColourSpace::Srgb,
&MaskStack::new(),
&crate::spot::SpotSet::new(),
&[],
);
let with_neutral = compose_full(
&ops,
&framing,
ColourSpace::Srgb,
&MaskStack::new(),
&crate::spot::SpotSet::new(),
&[
Box::new(crate::ops::Distortion::new()) as Box<dyn crate::lens::Warp>,
Box::new(crate::ops::Aberration::new()),
],
);
assert_eq!(without.source, with_neutral.source);
assert_eq!(without.structure_hash, with_neutral.structure_hash);
assert_eq!(without.uniforms, with_neutral.uniforms);
assert!(
!without.source.contains("sample_bilinear"),
"an unwarped, unstraightened frame must keep the integer load path"
);
}
/// Distortion alone samples once; chromatic aberration samples three times.
///
/// `splits_channels` is the whole reason for this test. Lateral CA fetches
/// red and blue from positions green is not, and paying that everywhere
/// would triple the texture bandwidth of the common case — a distortion
/// correction with no CA, which is most lens profiles.
#[test]
fn only_chromatic_aberration_splits_the_channels() {
let compose_with = |warps: Vec<Box<dyn crate::lens::Warp>>| {
compose_full(
&crate::ops::chain(),
&Framing::new(),
ColourSpace::Srgb,
&MaskStack::new(),
&crate::spot::SpotSet::new(),
&warps,
)
.source
};
let mut distortion = crate::ops::Distortion::new();
distortion.set_param(crate::ops::distortion::AMOUNT, 40.0);
let only_distortion = compose_with(vec![Box::new(distortion)]);
assert!(
only_distortion.contains("---- warp: distortion ----"),
"an active distortion must reach the shader"
);
assert!(
only_distortion.contains("sample_bilinear"),
"a warp puts output pixels between source pixels, so it forces \
the interpolating sampler even on an unstraightened frame"
);
assert!(
!only_distortion.contains("var p_r"),
"distortion moves all three channels together and must not pay \
for the per-channel path"
);
let mut ca = crate::ops::Aberration::new();
ca.set_param(crate::ops::aberration::RED, 25.0);
let with_ca = compose_with(vec![Box::new(ca)]);
assert!(with_ca.contains("var p_r"), "CA needs per-channel positions");
assert_eq!(
with_ca.matches("sample_bilinear(").count(),
// Three fetches in the body, plus the helper's own definition.
4,
"CA must fetch each channel from its own position"
);
}
/// An active warp is a different shader and must not reuse the cached one.
///
/// Covered by `hash_source` rather than by anything warp-specific — the
/// body is written into the source — but asserted because the alternative
/// failure is silent: the correction would simply never appear, exactly as
/// a zoom did before `Framing::structure_key` gained its last bit.
#[test]
fn arming_a_warp_recompiles_but_moving_its_slider_does_not() {
let compose_at = |amount: f32| {
let mut d = crate::ops::Distortion::new();
d.set_param(crate::ops::distortion::AMOUNT, amount);
compose_full(
&crate::ops::chain(),
&Framing::new(),
ColourSpace::Srgb,
&MaskStack::new(),
&crate::spot::SpotSet::new(),
&[Box::new(d) as Box<dyn crate::lens::Warp>],
)
};
let neutral = compose_at(0.0);
let armed = compose_at(40.0);
let further = compose_at(70.0);
assert_ne!(
neutral.structure_hash, armed.structure_hash,
"arming a warp adds a block to the shader and must recompile"
);
assert_eq!(
armed.structure_hash, further.structure_hash,
"its magnitude is a uniform, so a drag must not recompile"
);
assert_ne!(armed.uniforms, further.uniforms);
}
#[test]
fn structure_hash_ignores_values_but_tracks_the_op_set() {
// The property the shader cache depends on: moving a slider must not