Point at something grey and let the pipeline work out the rest
FR-DEV-3 has asked for "white balance (temperature/tint, and picker)" since it was written, and only the first half existed. `WidgetKind::WhitePoint` was in the vocabulary and `develop::supported` answered false for it, so the node degraded to two sliders — correct behaviour that had quietly become the only behaviour. Sampling a neutral is the first move of the global tonal pass and every colour judgement afterwards is measured against where the grey was put, so guessing at two sliders until a wall stops looking green is the wrong way round. The awkward part is that a picker genuinely needs to know how far a hundred units of temperature move red against blue, and that number is declared in the node's own file. So the inversion lives in `dr_pipeline::neutral` rather than in the interface: the canvas hands over a colour, the core finds the operation that asked to be driven by a pixel and bisects its declared response until the sample comes back grey. Nothing in `ui/` names white balance, and nothing holds a second copy of a response that would be wrong the first time somebody adjusted the range. A bisection rather than a closed-form inverse because only monotonicity is part of the bargain — the expression is free to become a table tomorrow. The result is rounded to the precision the control is drawn at, which is not cosmetic: unrounded, sampling something already neutral lands a ten-thousandth off zero, and the photograph comes back modified with an undo step for a correction of nothing. On the panel side this needed one distinction the generated path was missing. `is_on_canvas` was being read as "and so the panel draws nothing for it", which is right for a crop — four edge fractions are not controls anyone drags in a list — and wrong for an eyedropper, which *writes* temperature and tint and leaves them exactly the controls a photographer reaches for next. So a sampling widget keeps its sliders and puts the affordance that arms the canvas in the group's heading, built like the reset beside it. One click, one sample, one history step: `Edit::Action` never coalesces, and there is no hover preview to fill the stack with temperatures nobody chose. Declaring the presentation also groups temperature and tint under one undo step, where they were two. That follows from what `Presentation` means and reads correctly — white balance is one decision — but it is a change, and worth saying so.
This commit is contained in:
+246
-7
@@ -1185,12 +1185,46 @@ pub(crate) fn supported(widget: WidgetKind) -> bool {
|
||||
// the panel contributes `ComposePanel`, the affordance that turns it
|
||||
// on.
|
||||
WidgetKind::CropOverlay => true,
|
||||
// TRACES: FR-DEV-3
|
||||
// Hosted on the canvas too — a click on the photograph — with the
|
||||
// affordance that arms it in the group's own heading. See
|
||||
// `samples_the_canvas` for why this one leaves its sliders standing
|
||||
// where the crop takes them away.
|
||||
WidgetKind::WhitePoint => true,
|
||||
// Not implemented. Listed rather than caught by a wildcard so the next
|
||||
// kind added to the core surfaces here as a compile error.
|
||||
WidgetKind::ColourWheel
|
||||
| WidgetKind::GradientHandle
|
||||
| WidgetKind::BrushMask
|
||||
| WidgetKind::WhitePoint => false,
|
||||
WidgetKind::ColourWheel | WidgetKind::GradientHandle | WidgetKind::BrushMask => false,
|
||||
}
|
||||
}
|
||||
|
||||
/// TRACES: FR-DEV-3a | FR-UI-7
|
||||
/// Whether an on-canvas widget *reads* the photograph rather than replacing
|
||||
/// its parameters with handles.
|
||||
///
|
||||
/// **This is the distinction that stopped the picker eating its own sliders.**
|
||||
/// [`WidgetKind::is_on_canvas`] says where a widget is manipulated, and the
|
||||
/// panel had been treating that as also meaning "and so the panel draws
|
||||
/// nothing for it". For a crop that is right: four edge fractions and an angle
|
||||
/// are not controls anybody drags in a list, and the whole reason the crop is
|
||||
/// on the photograph is that they are unusable anywhere else.
|
||||
///
|
||||
/// An eyedropper is the other thing. It *writes* temperature and tint — they
|
||||
/// remain exactly the controls a photographer reaches for afterwards, because
|
||||
/// a sampled neutral is a starting point and warming a portrait past it is the
|
||||
/// next move, not a mistake. Taking the sliders away to make room for the
|
||||
/// picker would be trading a control for a control.
|
||||
///
|
||||
/// So the panel draws the group as usual and puts the affordance that arms the
|
||||
/// canvas in its heading. FR-DEV-3's "temperature/tint, **and** picker" is one
|
||||
/// word doing a lot of work, and this is the word.
|
||||
fn samples_the_canvas(widget: WidgetKind) -> bool {
|
||||
match widget {
|
||||
WidgetKind::WhitePoint => true,
|
||||
// Dragged rather than sampled: the parameters *are* the handles.
|
||||
WidgetKind::CropOverlay | WidgetKind::GradientHandle | WidgetKind::BrushMask => false,
|
||||
// Not on the canvas at all, so nothing asks. Listed rather than
|
||||
// wildcarded for the reason `supported` lists its own.
|
||||
WidgetKind::ToneCurve | WidgetKind::ColourWheel => false,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1240,6 +1274,11 @@ pub(crate) fn rows_filtered(
|
||||
// Where this operation's rows begin. The panel groups by walking
|
||||
// back to it, so it has to be taken before any row is pushed.
|
||||
let group_head = rows.len();
|
||||
// TRACES: FR-DEV-3
|
||||
// Whether this group's heading carries the affordance that arms an
|
||||
// on-canvas sampler. Set below, from what the operation asked for and
|
||||
// nothing else — the panel never learns which operation it is.
|
||||
let mut group_samples = false;
|
||||
// An operation may ask for one widget spanning several
|
||||
// parameters. Honouring it is optional — dropping this block
|
||||
// renders the same parameters as ordinary sliders, and the edit
|
||||
@@ -1270,7 +1309,15 @@ pub(crate) fn rows_filtered(
|
||||
// build (ARCH §4.3a draws the line at the *generated* panel
|
||||
// naming stages, not at the interface having hand-made
|
||||
// widgets).
|
||||
if widget.is_on_canvas() {
|
||||
//
|
||||
// TRACES: FR-DEV-3
|
||||
// **Unless the canvas is *reading* rather than driving.** An
|
||||
// eyedropper writes temperature and tint and leaves them as
|
||||
// the controls they were, so its group is drawn in full and
|
||||
// only the affordance moves to the heading. See
|
||||
// `samples_the_canvas` for the whole of that argument.
|
||||
group_samples = samples_the_canvas(widget);
|
||||
if widget.is_on_canvas() && !group_samples {
|
||||
continue;
|
||||
}
|
||||
|
||||
@@ -1281,8 +1328,10 @@ pub(crate) fn rows_filtered(
|
||||
WidgetKind::ToneCurve => {
|
||||
curve_row(op_index, group_head, op, presentation, curve_channel)
|
||||
}
|
||||
// Canvas-hosted kinds returned above; the rest are not
|
||||
// implemented and reached sliders via `choose`.
|
||||
// Canvas-hosted kinds returned above, except a
|
||||
// sampler, which falls through to its own sliders; the
|
||||
// rest are not implemented and reached sliders via
|
||||
// `choose`.
|
||||
WidgetKind::ColourWheel
|
||||
| WidgetKind::CropOverlay
|
||||
| WidgetKind::GradientHandle
|
||||
@@ -1371,6 +1420,7 @@ pub(crate) fn rows_filtered(
|
||||
group_head: group_head as i32,
|
||||
group_len,
|
||||
group_modified,
|
||||
group_samples,
|
||||
kind: kind.into(),
|
||||
value: p.value,
|
||||
default_value: p.default,
|
||||
@@ -1498,6 +1548,8 @@ fn curve_row(
|
||||
// the group it heads is itself and nothing else.
|
||||
group_len: 1,
|
||||
group_modified: op.params.iter().any(|p| p.value != p.default),
|
||||
// A curve is drawn, not sampled. Its own affordance is the plot.
|
||||
group_samples: false,
|
||||
kind: "curve".into(),
|
||||
value: 0.0,
|
||||
default_value: 0.0,
|
||||
@@ -3671,6 +3723,118 @@ impl DevelopSession {
|
||||
self.set_film(None);
|
||||
}
|
||||
|
||||
/// TRACES: FR-DEV-3 | FR-DEV-5
|
||||
/// Set the white balance from a point on the photograph.
|
||||
///
|
||||
/// `x` and `y` are fractions of the *visible* image — the coordinates a
|
||||
/// click on the canvas arrives in — so a photographer inspecting a
|
||||
/// highlight at 4× samples the pixel they are actually looking at.
|
||||
///
|
||||
/// **Nothing here knows it is white balance.** The colour goes to
|
||||
/// [`dr_pipeline::neutral`], which finds the operation that asked to be
|
||||
/// driven by a pixel and inverts its declared response; this side supplies
|
||||
/// the pixel and the undo step and nothing else. That is FR-DEV-3a's line
|
||||
/// in its awkward case: a picker genuinely needs to know how far a hundred
|
||||
/// units of temperature move red against blue, and that number is declared
|
||||
/// in the node's own file, so the interface must not be the thing that
|
||||
/// holds a second copy of it.
|
||||
///
|
||||
/// **One step per sample**, and no step at all for a sample that could not
|
||||
/// be used — a point in the deep shadows has no balance in it to correct.
|
||||
/// `Edit::Action` never coalesces, so two clicks are two decisions
|
||||
/// however quickly they follow each other, which is what a photographer
|
||||
/// trying a wall and then a cloud expects to be able to undo one at a
|
||||
/// time.
|
||||
///
|
||||
/// Returns whether the photograph moved.
|
||||
pub fn sample_neutral(&mut self, x: f32, y: f32) -> bool {
|
||||
let Some(sample) = self.sample_as_shot(x, y) else {
|
||||
return false;
|
||||
};
|
||||
if !dr_pipeline::neutral::neutralise(&mut self.graph, sample) {
|
||||
return false;
|
||||
}
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::SAMPLED_NEUTRAL));
|
||||
true
|
||||
}
|
||||
|
||||
/// The colour at a point with every adjustment taken off, in linear RGB.
|
||||
///
|
||||
/// **Measured before the chain rather than off the screen**, and that is
|
||||
/// the difference between a picker that converges and one that chases
|
||||
/// itself. The frame on the canvas has already been through the white
|
||||
/// balance being solved for, the tone curve, the contrast and whatever
|
||||
/// else is on; neutralising *that* pixel would be correcting a correction,
|
||||
/// and the second sample of the same wall would land somewhere else. With
|
||||
/// the adjustments stripped the value read is the colour as the file has
|
||||
/// it, which is the domain `neutralise` is defined over.
|
||||
///
|
||||
/// The framing stays on, exactly as it does for [`Self::render_original`]
|
||||
/// and for the same reason: `x` and `y` are fractions of what is on
|
||||
/// screen, and a probe rendered without the crop and the zoom would be
|
||||
/// answering about a different part of the photograph.
|
||||
///
|
||||
/// **Rendered small on purpose.** A 192px probe of the visible region
|
||||
/// averages a small neighbourhood into each of its pixels, which is what
|
||||
/// every eyedropper does deliberately: a single photosite off a noisy
|
||||
/// shadow is a worse answer than the patch around it, and the photographer
|
||||
/// is pointing at a grey card rather than at a pixel. It is also two
|
||||
/// dispatches' worth of work on a click.
|
||||
///
|
||||
/// This overwrites the frame the adjust pass is holding, so the caller
|
||||
/// must redraw — which the callback that samples does anyway, since the
|
||||
/// picture has just changed.
|
||||
fn sample_as_shot(&mut self, x: f32, y: f32) -> Option<[f32; 3]> {
|
||||
/// Long edge of the probe render. See the note above on why it is
|
||||
/// small rather than large.
|
||||
const PROBE_EDGE: u32 = 192;
|
||||
|
||||
// sRGB regardless of the display: this is a measurement, not something
|
||||
// anybody looks at, and decoding it needs a transfer function known
|
||||
// here. Composing for a wide-gamut panel would put the reading in a
|
||||
// space the arithmetic below does not undo.
|
||||
let space = dr_types::ColourSpace::Srgb;
|
||||
|
||||
let saved = self.graph.state();
|
||||
self.strip_adjustments();
|
||||
|
||||
let (sw, sh) = self.demosaiced.size();
|
||||
let (fw, fh) = self.graph.output_size(sw, sh);
|
||||
let (w, h) = fit(fw, fh, PROBE_EDGE, PROBE_EDGE);
|
||||
let shader = self.graph.compose_for(space);
|
||||
let probe = self
|
||||
.render_with_masks(&shader, w, h, space)
|
||||
.and_then(|()| self.adjust.export_pixels().map_err(|e| e.to_string()));
|
||||
|
||||
// Restored whatever happened, for the reason every other suspension
|
||||
// here restores: leaving the graph stripped after a failed probe would
|
||||
// discard the edit silently.
|
||||
let debt = self.graph.set_state(&saved);
|
||||
self.pay_film_debt(&debt);
|
||||
|
||||
let (rgba, pw, ph) = probe
|
||||
.inspect_err(|e| log::warn!("could not read a neutral off the frame: {e}"))
|
||||
.ok()?;
|
||||
|
||||
let (pw, ph) = (pw as usize, ph as usize);
|
||||
let px = ((x.clamp(0.0, 1.0) * pw as f32) as usize).min(pw.saturating_sub(1));
|
||||
let py = ((y.clamp(0.0, 1.0) * ph as f32) as usize).min(ph.saturating_sub(1));
|
||||
let at = (py * pw + px) * 4;
|
||||
let pixel = rgba.get(at..at + 3)?;
|
||||
|
||||
// The probe was encoded for the screen; the solve is multiplicative
|
||||
// and only means anything in linear light (ARCH §5.2). Undone with
|
||||
// the space's own transfer function rather than a second copy of the
|
||||
// curve written out here.
|
||||
let transfer = space.transfer();
|
||||
Some([
|
||||
transfer.decode(f32::from(pixel[0]) / 255.0),
|
||||
transfer.decode(f32::from(pixel[1]) / 255.0),
|
||||
transfer.decode(f32::from(pixel[2]) / 255.0),
|
||||
])
|
||||
}
|
||||
|
||||
/// TRACES: FR-PLAT-AND-5 | NFR-RES-1
|
||||
/// Give back the GPU memory this session is holding only to be fast.
|
||||
///
|
||||
@@ -5597,6 +5761,37 @@ mod tests {
|
||||
assert!(!session.framing_edits_image());
|
||||
}
|
||||
|
||||
/// TRACES: FR-DEV-3 | FR-DEV-5
|
||||
/// Sampling something that is already neutral corrects nothing, and says
|
||||
/// so by leaving the stack alone.
|
||||
///
|
||||
/// The failure this guards is a picker that lands a ten-thousandth off
|
||||
/// zero: the photograph would come back marked modified, an undo step
|
||||
/// would appear for a correction of nothing, and the sidecar would gain a
|
||||
/// temperature the photographer never chose. The solve rounds to the
|
||||
/// precision the control is drawn at, which is what makes "no correction"
|
||||
/// representable at all.
|
||||
#[test]
|
||||
fn sampling_a_grey_that_is_already_grey_leaves_the_photograph_alone() {
|
||||
let Some(ctx) = headless() else { return };
|
||||
let (mut session, _) = grey_session(&ctx);
|
||||
let steps = session.history_rows().len();
|
||||
|
||||
assert!(
|
||||
session.sample_neutral(0.5, 0.5),
|
||||
"a flat grey frame is a usable sample"
|
||||
);
|
||||
assert!(
|
||||
session.is_neutral(),
|
||||
"there was nothing to correct, so nothing was corrected"
|
||||
);
|
||||
assert_eq!(
|
||||
session.history_rows().len(),
|
||||
steps,
|
||||
"and a correction of nothing is not a step"
|
||||
);
|
||||
}
|
||||
|
||||
/// TRACES: FR-DEV-7 | FR-DEV-5
|
||||
/// A held comparison hands the edit straight back.
|
||||
///
|
||||
@@ -6449,6 +6644,50 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// TRACES: FR-DEV-3 | FR-DEV-3a
|
||||
/// A canvas *sampler* adds an affordance; it does not take the sliders.
|
||||
///
|
||||
/// The distinction the panel had been missing. `is_on_canvas` was read as
|
||||
/// "and so the panel draws nothing", which is right for a crop and wrong
|
||||
/// for an eyedropper — FR-DEV-3 asks for "temperature/tint, **and**
|
||||
/// picker", and a picker that ate the two sliders would have traded one
|
||||
/// control for another. Asserted against the chain rather than a literal,
|
||||
/// so it keeps testing the property when the declaration moves.
|
||||
#[test]
|
||||
fn a_sampled_operation_keeps_the_sliders_it_writes() {
|
||||
let graph = EditGraph::default_chain();
|
||||
let caps = graph.capabilities();
|
||||
|
||||
let (index, cap) = caps
|
||||
.iter()
|
||||
.enumerate()
|
||||
.find(|(_, c)| {
|
||||
c.presentation
|
||||
.as_ref()
|
||||
.is_some_and(|p| p.widgets.contains(&WidgetKind::WhitePoint))
|
||||
})
|
||||
.expect("some operation asks to be driven by a pixel");
|
||||
|
||||
let rows = rows_from(&caps);
|
||||
let mine: Vec<_> = rows.iter().filter(|r| r.op_index == index as i32).collect();
|
||||
|
||||
assert_eq!(
|
||||
mine.len(),
|
||||
cap.params.len(),
|
||||
"every parameter of a sampled operation still has its own control"
|
||||
);
|
||||
assert!(
|
||||
mine.iter().all(|r| r.group_samples),
|
||||
"and the group's heading carries the picker"
|
||||
);
|
||||
assert!(
|
||||
rows.iter()
|
||||
.filter(|r| r.op_index != index as i32)
|
||||
.all(|r| !r.group_samples),
|
||||
"no other group claims one"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn framing_is_not_generated_as_sliders() {
|
||||
// `ComposePanel` presents crop, rotation, flips and straightening as
|
||||
|
||||
@@ -29,6 +29,14 @@ pub mod step {
|
||||
pub const PASTE: LocalizedKey = LocalizedKey("history.paste");
|
||||
pub const FILM: LocalizedKey = LocalizedKey("history.film");
|
||||
|
||||
/// TRACES: FR-DEV-3
|
||||
/// A neutral was picked off the photograph, setting the white balance.
|
||||
///
|
||||
/// Named for what the photographer did rather than for the parameters it
|
||||
/// moved: two of them changed, and "Temperature" beside "Tint" in the list
|
||||
/// would describe the mechanism rather than the decision.
|
||||
pub const SAMPLED_NEUTRAL: LocalizedKey = LocalizedKey("history.sampled_neutral");
|
||||
|
||||
pub const RESET_ALL: LocalizedKey = LocalizedKey("history.reset_all");
|
||||
pub const RESET_OP: LocalizedKey = LocalizedKey("history.reset_op");
|
||||
pub const RESET_PARAM: LocalizedKey = LocalizedKey("history.reset_param");
|
||||
@@ -77,6 +85,7 @@ pub mod step {
|
||||
dr_pipeline::history::UNNAMED,
|
||||
PASTE,
|
||||
FILM,
|
||||
SAMPLED_NEUTRAL,
|
||||
RESET_ALL,
|
||||
RESET_OP,
|
||||
RESET_PARAM,
|
||||
@@ -253,6 +262,7 @@ fn catalogued(key: &str) -> Option<&'static str> {
|
||||
"history.edit" => "Edit",
|
||||
"history.paste" => "Paste Settings",
|
||||
"history.film" => "Film Stock",
|
||||
"history.sampled_neutral" => "Sample Neutral",
|
||||
|
||||
"history.reset_all" => "Reset Everything",
|
||||
"history.reset_op" => "Reset Group",
|
||||
|
||||
@@ -378,6 +378,11 @@ fn reset_view_state(window: &AppWindow) {
|
||||
// back anyway, but a press that opens the next photograph — the roll is a
|
||||
// tap away — must not carry a held original onto it.
|
||||
window.set_showing_original(false);
|
||||
// TRACES: FR-DEV-3
|
||||
// And an armed sampler goes down with it: a picker left waiting would make
|
||||
// the first click on the *next* photograph sample it, which is a click
|
||||
// nobody meant to spend.
|
||||
window.set_sampling_op(-1);
|
||||
// TRACES: FR-DSP-7
|
||||
// Emptied rather than left standing: the previous photograph's histogram
|
||||
// beside the next one's filename is a confident, precise lie, and the gap
|
||||
@@ -2833,6 +2838,39 @@ pub fn run(paths: Vec<PathBuf>) -> Result<()> {
|
||||
render_now(&w, false);
|
||||
});
|
||||
}
|
||||
{
|
||||
// TRACES: FR-DEV-3
|
||||
// A neutral picked off the photograph.
|
||||
//
|
||||
// The operation is not named here and is not passed in: the click
|
||||
// carries a point, the session turns it into a colour, and
|
||||
// `dr_pipeline::neutral` finds the operation that asked to be driven
|
||||
// by one. This callback's whole contribution is that a click on the
|
||||
// canvas is a click on the canvas.
|
||||
//
|
||||
// The rows are re-synced because the sample moved parameters the panel
|
||||
// is showing — two of them, from one press, which is exactly the case
|
||||
// a paste and an undo already go through this path for.
|
||||
let weak = window.as_weak();
|
||||
let session = session.clone();
|
||||
let redraw = redraw.clone();
|
||||
let rows = rows.clone();
|
||||
window.on_neutral_picked(move |x, y| {
|
||||
let Some(w) = weak.upgrade() else { return };
|
||||
let sampled = session
|
||||
.borrow_mut()
|
||||
.as_mut()
|
||||
.is_some_and(|s| s.sample_neutral(x, y));
|
||||
if sampled {
|
||||
sync_rows(&w, &rows, &session);
|
||||
}
|
||||
// Redrawn either way. A sample that found nothing usable still
|
||||
// overwrote the frame the adjust pass was holding, and leaving the
|
||||
// canvas showing a probe of the unedited image would look like the
|
||||
// edit had been thrown away.
|
||||
redraw(&w);
|
||||
});
|
||||
}
|
||||
|
||||
// TRACES: FR-DEV-5 | FR-DEV-7
|
||||
// Clicking a row. Arriving six steps away costs what arriving from one
|
||||
|
||||
Reference in New Issue
Block a user