From 049034e2477addf9df4573e499469cf295d35252 Mon Sep 17 00:00:00 2001 From: hnoerenberg <55043719+hnoerenberg@users.noreply.github.com> Date: Sun, 25 Oct 2026 16:05:43 +0100 Subject: [PATCH 1/2] LR-EDIT-COLOR-TEMP: a one-field white balance partial keeps the other field's look (#2910) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `develop.merge`, `lightcraft-cli render --settings` and MCP `set_develop` merged a white balance partial that names only `temp` or only `tint` straight into the photo's settings, so the field the partial leaves out — stored on the photo's scale — is re-read on the partial's scale. On a raw edited in Kelvin that moves the green/magenta balance of photos the partial never mentioned: `wb {temp: 5800}` on a photo at 5000 K / +12 tint lands on the relative scale, a shift of ~2.5 tint units. Resolve a one-field white balance partial against the photo: bring its stored white balance onto the partial's scale with the photo's own as-shot white, merge, then put the result back on the scale that photo is edited on. The inverse of `legacy_to_kelvin` is the composition of the two Bradford adaptations run backwards: `lightcraft_pipeline::local::kelvin_to_legacy`. It maps the source's own white to the relative scale's 6500 / 0 exactly, and answers None for a white the relative scale cannot hold. Partials naming both fields or neither, presets and the crs / Luminar importers are untouched: they always write `temp`, `tint` and `scale` together. Tests: the round trip over the slider's reach, in both directions, and hostile inputs (`crates/pipeline/src/local.rs`); the merge cases both ways plus the unchanged ones, and a restated value that must not move the white at all (`crates/engine/src/tests_color.rs`). Both fail before the change. Co-Authored-By: Claude --- crates/engine/src/cmd/develop.rs | 47 ++++++++++++- crates/engine/src/tests_color.rs | 112 +++++++++++++++++++++++++++++++ crates/pipeline/src/local.rs | 98 +++++++++++++++++++++++++++ docs/control-protocol.md | 6 +- docs/mcp.md | 2 +- docs/parity-checklist.md | 2 +- docs/whats-new.md | 12 ++++ 7 files changed, 275 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/cmd/develop.rs b/crates/engine/src/cmd/develop.rs index fbc3d4041..78957f5b7 100644 --- a/crates/engine/src/cmd/develop.rs +++ b/crates/engine/src/cmd/develop.rs @@ -51,6 +51,49 @@ fn refit_crop_angle(touched: bool, before: CropGeometry, d: &mut DevelopSettings const CROP_ANGLE: &str = "crop.angle"; +/// Merge a partial settings JSON into a photo's settings (issue #2910). `DevelopSettings::merged` keeps a white +/// balance that names no scale on the relative scale; on a photo edited in Kelvin the field the partial does not +/// name (a partial with only `temp`, or only `tint`) would then be read on the other scale, giving a white neither +/// the photo nor the partial asked for. A one-field white balance partial is therefore resolved against the photo +/// first: the photo's white balance is brought onto the partial's scale with the photo's as-shot white +/// (`kelvin_to_legacy` / `legacy_to_kelvin` via `normalize_wb`), the partial merges unchanged, and the result lands +/// back on the photo's editing scale (`normalize_wb`), where the kept field carries the look the photo had. +/// Partials naming both fields, or neither, mean the same as before; so do presets and the crs / Luminar importers, +/// which always write `temp`, `tint` and `scale` together through their own paths. +pub(crate) fn merge_settings_for_photo( + d: &DevelopSettings, + partial: &Value, + info: &lightcraft_pipeline::SourceInfo, +) -> std::result::Result { + let mut d = (*d).clone(); + let wb = partial.get("wb").and_then(Value::as_object); + let one_field = wb.is_some_and(|wb| wb.contains_key("temp") != wb.contains_key("tint")); + // only a photo whose source is edited in Kelvin has two scales to trip over + let kelvin_source = one_field && info.raw && !info.relative_wb; + if kelvin_source { + // the scale the partial's numbers are written on: an unmentioned scale means the relative scale, as `merged` reads it + let partial_scale = match wb.and_then(|wb| wb.get("scale")).and_then(Value::as_str) { + Some("kelvin") => lightcraft_develop::WbScale::Kelvin, + _ => lightcraft_develop::WbScale::Legacy, + }; + // put the photo onto the Kelvin scale (converting a relative look once), then onto the partial's + lightcraft_pipeline::local::normalize_wb(&mut d, info); + if d.wb.scale != partial_scale && partial_scale == lightcraft_develop::WbScale::Legacy { + let as_shot = (info.as_shot_temp, info.as_shot_tint); + let (kt, ktint) = lightcraft_pipeline::local::effective_wb(info, &d); + if let Some((lt, ltint)) = lightcraft_pipeline::local::kelvin_to_legacy(kt, ktint, as_shot) { + (d.wb.mode, d.wb.temp, d.wb.tint, d.wb.scale) = (WbMode::Custom, lt, ltint, lightcraft_develop::WbScale::Legacy); + } + } + } + let mut out = d.merged(partial)?; + if kelvin_source { + // and the result back onto the scale this photo is edited on + lightcraft_pipeline::local::normalize_wb(&mut out, info); + } + Ok(out) +} + /// `v` as a list of numbers; `None` unless it is an array of nothing but numbers. fn numbers(v: Option<&Value>) -> Option> { v?.as_array()?.iter().map(Value::as_f64).collect() @@ -256,8 +299,10 @@ pub fn specs() -> Vec { cmd!("develop.merge", "Apply Settings JSON", [], None, "{settings: partial DevelopSettings JSON, label?}", has_active, |s, p| { let partial = p.get("settings").cloned().ok_or_else(|| bad("develop.merge", "missing settings"))?; let label = str_param(p, "label").unwrap_or("Edit").to_string(); + let id = active(s, "develop.merge")?; + let info = s.source_info(id); edit(s, "develop.merge", &label, |d| { - *d = d.merged(&partial).map_err(|e| bad("develop.merge", e.to_string()))?; + *d = merge_settings_for_photo(d, &partial, &info).map_err(|e| bad("develop.merge", e.to_string()))?; Ok(()) }) }), diff --git a/crates/engine/src/tests_color.rs b/crates/engine/src/tests_color.rs index e526d4c47..a9761b2e9 100644 --- a/crates/engine/src/tests_color.rs +++ b/crates/engine/src/tests_color.rs @@ -220,3 +220,115 @@ fn white_balance_commands_write_kelvin_on_raws_whose_camera_has_matrices() { fn lightcraft_engine_effective(info: &lightcraft_pipeline::SourceInfo, d: &lightcraft_develop::DevelopSettings) -> (f64, f64) { lightcraft_pipeline::local::effective_wb(info, d) } + +/// Issue #2910: a `develop.merge` white-balance partial that names only `temp` or only `tint` is resolved against +/// the photo, so the field the partial does not name keeps the look it had instead of being re-read on the +/// partial's scale. The partial's own number keeps the meaning an unmentioned scale always had (relative), and a +/// partial that says `scale: "kelvin"` is read in Kelvin. +#[test] +fn one_field_wb_partial_merge_keeps_the_other_field_on_its_scale() { + use lightcraft_catalog::{MediaKind, Op, Photo, Source}; + use lightcraft_develop::{DevelopSettings, WbMode, WbScale}; + let mut s = Session::new(); + let add = |s: &mut Session, name: &str, camera: &str, as_shot: (f64, f64)| { + let id = s.catalog.alloc_photo_id(); + let mut p = Photo::new(id, Source::File { path: format!("/photos/{name}") }, name, "ARW", 64, 64, "2026-01-01T00:00:00"); + p.kind = MediaKind::Raw; + p.meta.camera = camera.into(); + p.as_shot_wb = Some(as_shot); + p.develop = std::sync::Arc::new(p.camera_defaults()); + s.catalog.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + id + }; + let as_shot = (4986.0, -2.0); + let kelvin = add(&mut s, "a.arw", "SONY ILCE-7M3", as_shot); + let relative = add(&mut s, "b.arw", "SONY ILCE-200", (6500.0, 0.0)); + let dev = |s: &Session, id| (*s.develop_of(id).unwrap()).clone(); + let l2k = |t, ti| lightcraft_pipeline::local::legacy_to_kelvin(t, ti, as_shot); + let k2l = |t, ti| lightcraft_pipeline::local::kelvin_to_legacy(t, ti, as_shot).unwrap(); + // two whites this close in (u, v) read as the same colour change; the bug's re-read of one kept field moves + // the white by ~8e-4 here, so a same-white check with this tolerance catches it + let close = |a: (f64, f64), b: (f64, f64)| { + let (au, av) = lightcraft_color::cct::xy_to_uv(lightcraft_color::cct::temp_tint_to_xy(a.0, a.1)); + let (bu, bv) = lightcraft_color::cct::xy_to_uv(lightcraft_color::cct::temp_tint_to_xy(b.0, b.1)); + (au - bu).hypot(av - bv) < 2e-4 + }; + // a stored white balance equal to an expected conversion, forgiving the last bits of the two code paths and + // the round-trip of the scale conversions; the bug moved the tint by 2+ units, far outside this + let nearly = |wb: &lightcraft_develop::WhiteBalance, want: &(f64, f64), why: &str| { + assert!((wb.temp - want.0).abs() < 1.0 && (wb.tint - want.1).abs() < 0.5, "{why}: {wb:?} vs {want:?}"); + }; + s.execute("library.select", &json!({"ids": [kelvin.0], "active": kelvin.0})).unwrap(); + + // a temp-only partial on a photo edited in Kelvin: the tint keeps its Kelvin look (the bug re-read it as a + // relative shift and moved the green/magenta balance although the partial never touched tint) + let edited = DevelopSettings { + wb: lightcraft_develop::WhiteBalance { mode: WbMode::Custom, temp: 5000.0, tint: 12.0, scale: WbScale::Kelvin }, + ..DevelopSettings::for_raw(as_shot.0, as_shot.1) + }; + s.set_develop(kelvin, edited.clone(), "kelvin edit").unwrap(); + s.execute("develop.merge", &json!({"settings": {"wb": {"temp": 5800.0}}})).unwrap(); + let d = dev(&s, kelvin); + let keep = k2l(5000.0, 12.0); + assert_eq!(d.wb.scale, WbScale::Kelvin, "the photo goes back on the scale it is edited on: {:?}", d.wb); + nearly(&d.wb, &l2k(5800.0, keep.1), "the partial's number keeps the meaning an unmarked scale always had"); + // restating the photo's own temperature (the value it carries on the scale an unmarked partial means) leaves + // its white exactly where it was: on main the kept tint was re-read as a relative shift, drifting the white + s.set_develop(kelvin, edited.clone(), "kelvin edit").unwrap(); + let look = lightcraft_engine_effective(&s.source_info(kelvin), &edited); + s.execute("develop.merge", &json!({"settings": {"wb": {"temp": keep.0}}})).unwrap(); + let again = lightcraft_engine_effective(&s.source_info(kelvin), &dev(&s, kelvin)); + assert!(close(look, again), "a restated one-field partial leaves the white alone: {look:?} -> {again:?}"); + // and the reverse: a tint-only partial keeps the temperature's look + s.execute("develop.merge", &json!({"settings": {"wb": {"tint": -20.0}}})).unwrap(); + let d = dev(&s, kelvin); + nearly(&d.wb, &l2k(keep.0, -20.0), "a tint-only partial keeps the temperature's look"); + // a partial that names its scale is read on it: the same temperature asked for in Kelvin, kept tint unharmed + s.set_develop(kelvin, edited.clone(), "kelvin edit").unwrap(); + s.execute("develop.merge", &json!({"settings": {"wb": {"temp": 5800.0, "scale": "kelvin"}}})).unwrap(); + let d = dev(&s, kelvin); + assert_eq!((d.wb.mode, d.wb.temp, d.wb.tint, d.wb.scale), (WbMode::Custom, 5800.0, 12.0, WbScale::Kelvin), "{:?}", d.wb); + + // from As Shot as well: the as-shot tint keeps its meaning (the bug read it as a relative shift from as shot) + s.execute("develop.merge", &json!({"settings": {"wb": {"mode": "asShot"}}})).unwrap(); + s.execute("develop.merge", &json!({"settings": {"wb": {"temp": 5800.0}}})).unwrap(); + let d = dev(&s, kelvin); + nearly(&d.wb, &l2k(5800.0, 0.0), "the as-shot tint keeps its meaning"); + assert_eq!(d.wb.scale, WbScale::Kelvin); + + // the other direction: an old relative Custom value (no scale) plus a Kelvin temp-only partial — the photo's + // white balance is taken onto the Kelvin scale first, so the kept tint is the Kelvin value of the photo's look + let old = DevelopSettings { + wb: lightcraft_develop::WhiteBalance { mode: WbMode::Custom, temp: 5800.0, tint: 20.0, scale: WbScale::Legacy }, + ..DevelopSettings::for_raw(6500.0, 0.0) + }; + s.set_develop(kelvin, old.clone(), "old relative").unwrap(); + let keep = l2k(5800.0, 20.0); + assert!(!close((5800.0, 20.0), (keep.0, keep.1)), "the two readings really differ on this camera, so the next check can catch the bug"); + s.execute("develop.merge", &json!({"settings": {"wb": {"temp": 5800.0, "scale": "kelvin"}}})).unwrap(); + let d = dev(&s, kelvin); + assert_eq!((d.wb.mode, d.wb.scale), (WbMode::Custom, WbScale::Kelvin), "{:?}", d.wb); + nearly(&d.wb, &(5800.0, keep.1), "the kept tint is the old relative tint carried onto the Kelvin scale"); + + // a partial naming both fields, or neither, behaves exactly as before + s.execute("develop.merge", &json!({"settings": {"wb": {"mode": "custom", "temp": 6500.0, "tint": 0.0}}})).unwrap(); + let d = dev(&s, kelvin); + assert_eq!(d.wb.scale, WbScale::Legacy, "a partial without a scale keeps the relative meaning"); + assert_eq!(lightcraft_engine_effective(&s.source_info(kelvin), &d), (as_shot.0, as_shot.1), "6500 / 0 without a scale is as shot"); + // the whole white balance arrives on the photo's scale when the partial says both fields and the scale + s.execute("develop.merge", &json!({"settings": {"wb": {"mode": "custom", "temp": 5200.0, "tint": 7.0, "scale": "kelvin"}}})).unwrap(); + let d = dev(&s, kelvin); + assert_eq!((d.wb.temp, d.wb.tint, d.wb.scale), (5200.0, 7.0, WbScale::Kelvin), "{:?}", d.wb); + // a one-field partial on a photo edited on the relative scale (no Kelvin scale there): unchanged behaviour, stays relative + s.execute("library.select", &json!({"ids": [relative.0], "active": relative.0})).unwrap(); + let before = dev(&s, relative); + s.execute("develop.merge", &json!({"settings": {"wb": {"temp": 5800.0}}})).unwrap(); + let d = dev(&s, relative); + assert_eq!((d.wb.temp, d.wb.tint, d.wb.scale), (5800.0, before.wb.tint, WbScale::Legacy), "{:?}", d.wb); + // a non-white-balance partial merges through the same path unchanged + let wb_before = dev(&s, relative).wb; + s.execute("develop.merge", &json!({"settings": {"light": {"contrast": 20}}})).unwrap(); + let d = dev(&s, relative); + assert_eq!(d.light.contrast, 20.0); + assert_eq!(d.wb, wb_before, "the white balance is untouched"); +} diff --git a/crates/pipeline/src/local.rs b/crates/pipeline/src/local.rs index c91872d38..a9dd9cbe6 100644 --- a/crates/pipeline/src/local.rs +++ b/crates/pipeline/src/local.rs @@ -140,6 +140,39 @@ pub fn legacy_to_kelvin(temp: f64, tint: f64, as_shot: (f64, f64)) -> (f64, f64) } } +/// The relative-scale `(temp, tint)` that [`legacy_to_kelvin`] would read as the Kelvin `(temp, tint)` on a source +/// whose real as-shot white is `as_shot`: the inverse of [`legacy_white`]'s composition. It runs the Bradford step +/// backwards — the old target's cone response times the old reference's over the as-shot white's — and reads the +/// result back through [`xy_to_temp_tint`]. `(6500, 0)` is the relative scale's as-shot reference, so the Kelvin +/// white of the source itself maps exactly to it. `None` when the arithmetic has nowhere to go (a non-finite input, +/// a white outside the chromaticity plane, or an as-shot cone response of zero, where the forward step discarded it). +pub fn kelvin_to_legacy(temp: f64, tint: f64, as_shot: (f64, f64)) -> Option<(f64, f64)> { + use lightcraft_color::{BRADFORD, Xy}; + if !(temp.is_finite() && tint.is_finite() && as_shot.0.is_finite() && as_shot.1.is_finite()) { + return None; + } + if (temp - as_shot.0).abs() < 1e-9 && (tint - as_shot.1).abs() < 1e-9 { + // the source's own white: the relative scale's as-shot reference, exactly + return Some((6500.0, 0.0)); + } + let cones = |t: f64, tint: f64| BRADFORD.apply(temp_tint_to_xy(t, tint).to_xyz()); + let (target, reference, shot) = (cones(temp, tint), cones(6500.0, 0.0), cones(as_shot.0, as_shot.1)); + let inverse = BRADFORD.inverse()?; + let mut old = [0.0; 3]; + for i in 0..3 { + if shot[i].abs() < 1e-12 { + return None; + } + old[i] = target[i] * reference[i] / shot[i]; + } + let xy = Xy::from_xyz(inverse.apply(old)); + if !(xy.x.is_finite() && xy.y.is_finite() && xy.y > 0.0) { + return None; + } + let (t, tint) = xy_to_temp_tint(xy); + (t.is_finite() && tint.is_finite()).then_some((t, tint)) +} + /// Bring the white balance of `d` onto the scale `info` is edited on, before a new value is written into it: on a /// raw with a Kelvin scale the values are marked Kelvin, and ones still on the relative scale become the Custom /// Kelvin / tint that renders the same ([`legacy_to_kelvin`]; a preset mode's look is kept as that custom @@ -703,6 +736,7 @@ pub fn airlight_of(mut v: Vec) -> f32 { #[cfg(test)] mod tests { use super::*; + use lightcraft_color::cct::xy_to_uv; #[test] fn guided_preserves_step_edges_and_flattens_texture() { @@ -796,6 +830,70 @@ mod tests { scene_linear_pre(&mut img, &SourceInfo::default(), &s); assert!((img.get(1, 1)[2] - 0.6).abs() < 1e-5); } + + /// [`kelvin_to_legacy`] inverts [`legacy_to_kelvin`] over the whole relative slider range and back: the + /// round trip returns the start, in both directions, to the precision of the Kelvin / tint parametrisation. + #[test] + fn kelvin_to_legacy_inverts_legacy_to_kelvin() { + for as_shot in [(4986.0, -2.0), (6500.0, 0.0), (3200.0, 14.0), (9500.0, -12.0)] { + // the relative slider's whole reach: temp 0..100 (2000–13550 K around 6500), tint −150..150 + for pct in [0.0, 12.5, 25.0, 50.0, 75.0, 100.0] { + for tint in [-150.0, -40.0, 0.0, 33.0, 150.0] { + let (lt, ltint) = controls_relative(pct, tint); + let (kt, ktint) = legacy_to_kelvin(lt, ltint, as_shot); + // past the Kelvin parametrisation's own reach (its search runs 1000–50000 K) the forward + // conversion has already clamped, so the pair is not expected to come back + if !(1050.0..=49_000.0).contains(&kt) { + continue; + } + let back = kelvin_to_legacy(kt, ktint, as_shot).unwrap(); + let (x1, y1) = xy_to_uv(temp_tint_to_xy(lt, ltint)); + let (x2, y2) = xy_to_uv(temp_tint_to_xy(back.0, back.1)); + // the pair may differ by more than the slider's own step at the extreme corners (the Kelvin + // parametrisation is read back through a search), but never by much, and the white it renders + // is the same + assert!( + (back.0 - lt).abs() <= lt.abs() * 0.01 && (back.1 - ltint).abs() < 0.5, + "as shot {as_shot:?}: relative ({lt}, {ltint}) -> kelvin ({kt}, {ktint}) -> {back:?}" + ); + assert!((x1 - x2).hypot(y1 - y2) < 1e-3, "as shot {as_shot:?}: relative ({lt}, {ltint}) -> kelvin ({kt}, {ktint}) -> {back:?}"); + } + } + // the other direction: every white the relative scale can hold maps back to a pair that renders the same + for kt in [as_shot.0, 2500.0, 4200.0, 6500.0, 12000.0] { + for ktint in [-60.0, 0.0, 45.0] { + let Some((lt, ltint)) = kelvin_to_legacy(kt, ktint, as_shot) else { continue }; + // outside the slider's reach the relative scale has no value for this white: the forward + // conversion would clamp, so the round trip is not expected to close there + if !(2000.0..=13550.0).contains(<) || ltint.abs() > 150.0 { + continue; + } + let (kt2, ktint2) = legacy_to_kelvin(lt, ltint, as_shot); + let (x1, y1) = xy_to_uv(temp_tint_to_xy(kt, ktint)); + let (x2, y2) = xy_to_uv(temp_tint_to_xy(kt2, ktint2)); + assert!( + (x1 - x2).hypot(y1 - y2) < 1e-3, + "as shot {as_shot:?}: kelvin ({kt}, {ktint}) -> relative ({lt}, {ltint}) -> ({kt2}, {ktint2})" + ); + } + } + // the source's own white is the relative scale's as-shot reference, exactly + assert_eq!(kelvin_to_legacy(as_shot.0, as_shot.1, as_shot), Some((6500.0, 0.0))); + } + // the exact inverse of the special case inside legacy_to_kelvin + assert_eq!(kelvin_to_legacy(4986.0, -2.0, (4986.0, -2.0)), Some((6500.0, 0.0))); + assert_eq!(legacy_to_kelvin(6500.0, 0.0, (4986.0, -2.0)), (4986.0, -2.0)); + // hostile numbers answer None, never a panic or a plausible lie + assert_eq!(kelvin_to_legacy(f64::NAN, 0.0, (5000.0, 0.0)), None); + assert_eq!(kelvin_to_legacy(5000.0, f64::INFINITY, (5000.0, 0.0)), None); + assert_eq!(kelvin_to_legacy(5000.0, 0.0, (f64::NAN, 0.0)), None); + } + + /// The relative scale's `(temp, tint)` for a slider temperature at `pct` % (2000–7000 K fill the left half) and tint. + fn controls_relative(pct: f64, tint: f64) -> (f64, f64) { + let t = if pct <= 50.0 { 2000.0 + pct * 100.0 } else { 7000.0 + (pct - 50.0) * 131.0 }; + (t, tint) + } } #[cfg(test)] diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 3b4c7ed95..ddfe95914 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -119,7 +119,11 @@ reads the file's as-shot white; rendered files and raws without matrices use a r whose 6500 K / 0 is the as-shot look (the Edit panel shows it as −100..+100). The stored `wb` of such a Kelvin raw says `"scale": "kelvin"`; a partial settings JSON given to `develop.merge` or a preset whose `wb` sets `temp` / `tint` **without** `scale` keeps the relative meaning it always had (6500 = as shot) on every raw, so agents that mean -Kelvin on a Kelvin raw pass `"scale": "kelvin"` or use `develop.set` / `develop.wb`. +Kelvin on a Kelvin raw pass `"scale": "kelvin"` or use `develop.set` / `develop.wb`. A partial that names only one +of `temp` / `tint` is resolved against the photo (issue #2910): the photo's white balance is first brought onto the +partial's scale with the photo's own as-shot white, so the field the partial does not name keeps the look it had +instead of being re-read on the partial's scale, and the stored result is back on the scale the photo is edited on +(`"scale": "kelvin"` on a Kelvin raw). Partials naming both fields mean the same as before. Image navigation is also available directly as the UI command `view.navigate`, with `{zoom?: "fit" | "fill" | {"percent": number}, pan?: [x, y]}`. Percentage zoom accepts fractional diff --git a/docs/mcp.md b/docs/mcp.md index 4800c37d8..24eff0154 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -175,7 +175,7 @@ per-request `_meta` receive `resultType: complete` and list/read cache hints; re | `select_photos {ids, active?, mode?}` | Set the selection / active photo. Every id (and `active`) must be in the library: an unknown id is a tool error (`no such photo 9999`) and the selection and active photo are left as they were | | `list_controls {section?}` | Every develop slider: id (`light.exposure`…), range, default, current value | | `get_develop {id?}` | Full develop-settings JSON, with `process`: the rendering process the photo is on ([process-versions.md](process-versions.md)) | -| `set_develop {id?, values?, settings?, label?}` | `values`: `{controlId: number}`; `settings`: partial develop JSON deep-merged. Undoable. A `crop.angle` value turns the crop as `crop.straighten` does: kept if it still fits, else shrunk to the largest crop of its aspect inside the photo (`settings` is merged as given, unfitted). White balance: `values` with `wb.temp` / `wb.tint` is Kelvin on raws with colour matrices (DNG, and cameras LightCraft has matrices for) and relative (6500 = as shot) elsewhere; a `settings.wb` with `temp` / `tint` and no `"scale": "kelvin"` is read on the relative scale on every raw (see [control-protocol.md](control-protocol.md)) | +| `set_develop {id?, values?, settings?, label?}` | `values`: `{controlId: number}`; `settings`: partial develop JSON deep-merged. Undoable. A `crop.angle` value turns the crop as `crop.straighten` does: kept if it still fits, else shrunk to the largest crop of its aspect inside the photo (`settings` is merged as given, unfitted). White balance: `values` with `wb.temp` / `wb.tint` is Kelvin on raws with colour matrices (DNG, and cameras LightCraft has matrices for) and relative (6500 = as shot) elsewhere; a `settings.wb` with `temp` / `tint` and no `"scale": "kelvin"` is read on the relative scale on every raw, and one that names only `temp` or only `tint` resolves the field it leaves out against the photo, so that field keeps its look (#2910) (see [control-protocol.md](control-protocol.md)) | | `apply_preset {preset, amount?, ids?}` | Apply a preset (ids from `cmd_presets_list`) | | `crop {id?, rect?, angle?, reset?}` | Normalized crop rect `[x0,y0,x1,y1]` and straighten angle; at least one of `rect`, `angle`, `reset: true`. The crop always ends inside the photo: an `angle` alone keeps the crop if it still fits, else shrinks it to the largest of its aspect that does | | `render_photo {id?, size?, format?, path?}` | Render with current settings → **image content** (PNG, or JPEG with `format: "jpeg"`), long edge `size` (default 1024) | diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 080680b9f..5b0143005 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -191,7 +191,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts). | LR-EDIT-LIGHT-CURVE-TAT | Drag-on-image curve adjust | P1 | ✅ | `cmd:develop.targeted` (`target: curve`) | | | LR-EDIT-COLOR-WB-PRESET | White-balance presets | P0 | ✅ | `cmd:develop.wb` | | | LR-EDIT-COLOR-WB-PICKER | White-balance eyedropper | P0 | ✅ | `cmd:tool.wbPicker`, `cmd:develop.wbPick`, `crates/ui-egui/src/tests_wb_selector.rs` | the Edit panel's selector button does what W does (from Compare, Survey or the grid it opens the photo in Detail); no magnified loupe while picking | -| LR-EDIT-COLOR-TEMP | Temperature | P0 | ✅ | `ctl:wb.temp`, `crates/pipeline/src/local.rs` (`wb_matrix_for`, `legacy_white`), `crates/raw/src/color.rs` (`rebalance`), `crates/raw/src/spectral.rs` (`find_camera`), `crates/catalog/src/model.rs` (`relative_wb_camera`), `crates/develop/src/controls.rs` (`ControlSpec::position`), `crates/raw/src/kelvin_presets.rs` | Kelvin (2000–50000 K, As Shot read from the file) on raws with colour matrices: in the file (DNG: re-developed for the chosen white through the camera's own matrices, camera-space white balance as Lightroom; the eyedropper / Auto invert the same model) or measured for the camera model (`lightcraft_raw::spectral`, 52 bodies, issue #730: the matrices turn the as-shot multipliers into Kelvin and the slider back, the look stays the file's fit, and the picture is adapted from the as-shot white, Bradford), or read from the camera's own white-balance presets (Sony ARW `SR2SubIFD`, Sony ILCA-99M2, ILCA-77M2, SLT-A33, DSC-RX10, ILCE-3500, ILCE-7S: Sony's Kelvin dial reads back within 0.3 % on 5 settings of 4 bodies; Canon CR2 `ColorData` presets, PowerShot G1 X Mark III: the camera's own As Shot temperature reads back within 5 % on its 9 white-balance settings; or built-in presets from the camera's own stated temperatures, `crates/raw/src/kelvin_builtin.rs`: Nikon Z5II, whose Natural Auto shot reads back at 5036 K against the 5030 K the camera wrote). The Kelvin slider's track is Lightroom's: 7000 K in the middle, 2000–7000 K evenly on the left half, logarithmic up to 50000 K on the right (within 2 % of positions measured in Lightroom Classic), for drawing, clicks and drags; wheel / arrow-key nudges and Quick Develop still step a fixed 50 / 100 / 500 K; the relative slider stays linear. Settings written on the relative scale before a camera's matrices reached it keep their look (`WbScale`). Not verified against Lightroom's Kelvin: on the ILCE-7M3 the matrices read 7–10 % below Sony's dial (issue #730), and whether the matrices or the dial are off is open; raws without matrices or presets anywhere and rendered files use the relative −100..+100 scale (6500 K / 0 = as shot). | +| LR-EDIT-COLOR-TEMP | Temperature | P0 | ✅ | `ctl:wb.temp`, `crates/pipeline/src/local.rs` (`wb_matrix_for`, `legacy_white`), `crates/raw/src/color.rs` (`rebalance`), `crates/raw/src/spectral.rs` (`find_camera`), `crates/catalog/src/model.rs` (`relative_wb_camera`), `crates/develop/src/controls.rs` (`ControlSpec::position`), `crates/raw/src/kelvin_presets.rs` | Kelvin (2000–50000 K, As Shot read from the file) on raws with colour matrices: in the file (DNG: re-developed for the chosen white through the camera's own matrices, camera-space white balance as Lightroom; the eyedropper / Auto invert the same model) or measured for the camera model (`lightcraft_raw::spectral`, 52 bodies, issue #730: the matrices turn the as-shot multipliers into Kelvin and the slider back, the look stays the file's fit, and the picture is adapted from the as-shot white, Bradford), or read from the camera's own white-balance presets (Sony ARW `SR2SubIFD`, Sony ILCA-99M2, ILCA-77M2, SLT-A33, DSC-RX10, ILCE-3500, ILCE-7S: Sony's Kelvin dial reads back within 0.3 % on 5 settings of 4 bodies; Canon CR2 `ColorData` presets, PowerShot G1 X Mark III: the camera's own As Shot temperature reads back within 5 % on its 9 white-balance settings; or built-in presets from the camera's own stated temperatures, `crates/raw/src/kelvin_builtin.rs`: Nikon Z5II, whose Natural Auto shot reads back at 5036 K against the 5030 K the camera wrote). The Kelvin slider's track is Lightroom's: 7000 K in the middle, 2000–7000 K evenly on the left half, logarithmic up to 50000 K on the right (within 2 % of positions measured in Lightroom Classic), for drawing, clicks and drags; wheel / arrow-key nudges and Quick Develop still step a fixed 50 / 100 / 500 K; the relative slider stays linear. Settings written on the relative scale before a camera's matrices reached it keep their look (`WbScale`). A partial white balance naming only `temp` or only `tint` (`develop.merge`, `render --settings`, MCP `set_develop`, a hand-written preset) is resolved against the photo, so the field it leaves out keeps the look it had: `merge_settings_for_photo` in `crates/engine/src/cmd/develop.rs` and `kelvin_to_legacy` in `crates/pipeline/src/local.rs`, issue #2910, tests in `crates/engine/src/tests_color.rs`. Not verified against Lightroom's Kelvin: on the ILCE-7M3 the matrices read 7–10 % below Sony's dial (issue #730), and whether the matrices or the dial are off is open; raws without matrices or presets anywhere and rendered files use the relative −100..+100 scale (6500 K / 0 = as shot). | | LR-EDIT-COLOR-TINT | Tint | P0 | ✅ | `ctl:wb.tint`, `docs/tint-direction.md` | DNG / Lightroom sign: negative (left) adds green, positive (right) magenta; XMP `crs:Tint` values render as in Lightroom (issue #188); CPU/GPU regression tests | | LR-EDIT-COLOR-VIBRANCE | Vibrance | P0 | ✅ | `ctl:color.vibrance` | | | LR-EDIT-COLOR-SATURATION | Saturation | P0 | ✅ | `ctl:color.saturation` | | diff --git a/docs/whats-new.md b/docs/whats-new.md index c6c259200..68117bc72 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -1,5 +1,17 @@ # What's new in LightCraft +## 2026-10-25 + +### A one-field white balance edit no longer shifts the other one +- Settings that name only a white balance temperature, or only a tint — `develop.merge`, + `lightcraft-cli render --settings`, an MCP `develop.merge` call, or a hand-written preset — used to change the + colour balance of photos they never mentioned. On a raw edited in Kelvin the field the partial kept was re-read + on the other scale: `wb {temp: 5800}` on a photo at 5000 K / +12 tint came back at 5800 K with the +12 read as a + relative shift, moving the green–magenta balance by a visible amount. The kept field now carries the look the + photo had, and the result is stored on the scale that photo is edited on. Presets, XMP import and settings that + name both fields (or neither) behave exactly as before, and a partial that says `"scale": "kelvin"` is still + read in Kelvin. + ## 2026-10-24 ### Relative-date filters and smart albums follow the clock From fa7e5b9cb4fc69f04f7739032a10b928d5c7fc9a Mon Sep 17 00:00:00 2001 From: hnoerenberg <55043719+hnoerenberg@users.noreply.github.com> Date: Sun, 25 Oct 2026 20:38:44 +0100 Subject: [PATCH 2/2] LR-EDIT-COLOR-TEMP: a one-field white balance partial lands where a mode answers its own white (#2910) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewing #2952 turned up two more shapes of the same bug, both on the path this branch is about. `effective_wb` ignores the temp / tint stored on an `As Shot` white balance — it answers the file's as-shot white — and on a raw edited in Kelvin it also ignores a named preset mode's stored numbers, through `WbMode::preset`. So a one-field partial that names its scale, `wb {temp: 5800, scale: "kelvin"}`, merged into such a photo, was stored and then read by nothing: the panel shows 5800 K while the render stays at as shot, and the stored tint no longer matches what renders. An unmarked partial already materialized a Custom value through the relative-scale conversion; the named-scale ones never did. The photo's preparation now writes out the white the photo really renders as Custom values before the partial lands, for `As Shot` and for preset modes on a Kelvin source. `Auto` keeps its mode — its gray is resolved and stored when the mode is set, not at render time — and a partial that names a `mode` itself stays authoritative. Sources without the Kelvin scale (a raw whose camera has no matrices, a rendered file) had the same swallow, and the number reaches their render now. The issue asks for one photo-aware merge in every caller that writes settings onto a photo, and names preset apply. `preset.apply` ran `Preset::apply` → `apply_partial` → `DevelopSettings::merged` and never saw the photo's scale, so a preset authored by hand naming one white balance number still had the original bug; measured on a photo at 5000 K / +12, it landed the photo on the relative scale and moved the tint it never mentioned. The preparation and the normalize-back now wrap `Preset::apply` per photo, with the amount blending and the mask merging untouched, which also puts an amount below 100 % on a single scale. Presets the app writes (`preset.create` → `extract_groups`) and the crs / XMP importers name mode, temp, tint and scale together and so land in the branch that behaves exactly as before. Tests: the `As Shot` and preset-mode cases in the #2910 merge test, and the new `preset.apply` test, fail before this commit (the first stores the number in a mode that renders the photo's old white; the second demotes the photo to `Legacy`); the same test then pins that `preset.apply` and `develop.merge` agree on a one-field white balance, which is the point of having one merge. Docs: the rule for an agent is in `docs/control-protocol.md` and `docs/mcp.md`, recorded on the `LR-EDIT-COLOR-TEMP` row of `docs/parity-checklist.md`, and in `docs/whats-new.md`. Co-Authored-By: Claude --- crates/engine/src/cmd/develop.rs | 96 ++++++++++++++++------ crates/engine/src/tests_color.rs | 132 +++++++++++++++++++++++++++++++ docs/control-protocol.md | 20 +++-- docs/mcp.md | 2 +- docs/parity-checklist.md | 2 +- docs/whats-new.md | 20 +++-- 6 files changed, 230 insertions(+), 42 deletions(-) diff --git a/crates/engine/src/cmd/develop.rs b/crates/engine/src/cmd/develop.rs index 78957f5b7..de21a59dc 100644 --- a/crates/engine/src/cmd/develop.rs +++ b/crates/engine/src/cmd/develop.rs @@ -51,6 +51,54 @@ fn refit_crop_angle(touched: bool, before: CropGeometry, d: &mut DevelopSettings const CROP_ANGLE: &str = "crop.angle"; +/// Whether a white balance mode renders its own white instead of the numbers stored on the photo: `As Shot` +/// renders the source's as-shot white, and on a raw edited in Kelvin a named preset renders its own Kelvin / tint +/// ([`WbMode::preset`]). Either way a number written into `temp` / `tint` is stored but read by nothing, so the +/// panel shows the number while the render uses the mode's white. `Custom` reads its stored numbers, and so does +/// `Auto` on such a raw: its white is resolved and stored when the mode is set (`develop.wb`), not at render time. +fn wb_mode_ignores_its_numbers(mode: WbMode, info: &lightcraft_pipeline::SourceInfo) -> bool { + mode == WbMode::AsShot || (info.raw && !info.relative_wb && mode.preset().is_some()) +} + +/// Bring a photo's settings onto the scale and mode a white balance partial names, so a one-field partial means +/// what it says (issue #2910, and its review). `None` when the partial needs no such preparation: it names both +/// numbers, neither, or a mode of its own. [`merge_settings_for_photo`] and `preset.apply` both start here, and +/// both finish with `normalize_wb` so the photo ends on the scale it is edited on. +pub(crate) fn prepare_photo_for_wb_partial(d: &DevelopSettings, partial: &Value, info: &lightcraft_pipeline::SourceInfo) -> Option { + let mut d = (*d).clone(); + let wb = partial.get("wb").and_then(Value::as_object); + // the partial names one of the two numbers, and not both or neither + let one_field = wb.is_some_and(|wb| wb.contains_key("temp") != wb.contains_key("tint")); + if !one_field { + return None; + } + // a photo whose source is edited in Kelvin is the one with two scales to trip over + let kelvin_source = info.raw && !info.relative_wb; + // the scale the partial's numbers are written on: an unmentioned scale means the relative scale, as `merged` reads it + let partial_scale = match wb.as_ref().and_then(|wb| wb.get("scale")).and_then(Value::as_str) { + Some("kelvin") => lightcraft_develop::WbScale::Kelvin, + _ => lightcraft_develop::WbScale::Legacy, + }; + // put the photo onto the scale it is edited on: its values marked Kelvin, or a look still on the relative + // scale converted to the Kelvin / tint that renders the same + lightcraft_pipeline::local::normalize_wb(&mut d, info); + // a mode that answers its own white would swallow the partial's number: write the white the photo really + // renders as Custom values, so the number means what it says and the other field carries that white's look + if !wb.is_some_and(|wb| wb.contains_key("mode")) && wb_mode_ignores_its_numbers(d.wb.mode, info) { + let (t, tint) = lightcraft_pipeline::local::effective_wb(info, &d); + (d.wb.mode, d.wb.temp, d.wb.tint) = (WbMode::Custom, t, tint); + } + if kelvin_source && partial_scale == lightcraft_develop::WbScale::Legacy { + // and onto the partial's scale, so the field the partial leaves out is the photo's own look read there + let as_shot = (info.as_shot_temp, info.as_shot_tint); + let (kt, ktint) = lightcraft_pipeline::local::effective_wb(info, &d); + if let Some((lt, ltint)) = lightcraft_pipeline::local::kelvin_to_legacy(kt, ktint, as_shot) { + (d.wb.mode, d.wb.temp, d.wb.tint, d.wb.scale) = (WbMode::Custom, lt, ltint, lightcraft_develop::WbScale::Legacy); + } + } + Some(d) +} + /// Merge a partial settings JSON into a photo's settings (issue #2910). `DevelopSettings::merged` keeps a white /// balance that names no scale on the relative scale; on a photo edited in Kelvin the field the partial does not /// name (a partial with only `temp`, or only `tint`) would then be read on the other scale, giving a white neither @@ -58,36 +106,19 @@ const CROP_ANGLE: &str = "crop.angle"; /// first: the photo's white balance is brought onto the partial's scale with the photo's as-shot white /// (`kelvin_to_legacy` / `legacy_to_kelvin` via `normalize_wb`), the partial merges unchanged, and the result lands /// back on the photo's editing scale (`normalize_wb`), where the kept field carries the look the photo had. -/// Partials naming both fields, or neither, mean the same as before; so do presets and the crs / Luminar importers, -/// which always write `temp`, `tint` and `scale` together through their own paths. +/// Partials naming both fields, or neither, mean the same as before. A photo whose white balance mode renders its +/// own white — `As Shot`, or a named preset on a raw edited in Kelvin — has that white written out as Custom values +/// first ([`wb_mode_ignores_its_numbers`]), because a mode that answers its own white would otherwise store the +/// partial's number without any render reading it; a partial that names a `mode` itself stays authoritative. pub(crate) fn merge_settings_for_photo( d: &DevelopSettings, partial: &Value, info: &lightcraft_pipeline::SourceInfo, ) -> std::result::Result { - let mut d = (*d).clone(); - let wb = partial.get("wb").and_then(Value::as_object); - let one_field = wb.is_some_and(|wb| wb.contains_key("temp") != wb.contains_key("tint")); - // only a photo whose source is edited in Kelvin has two scales to trip over - let kelvin_source = one_field && info.raw && !info.relative_wb; - if kelvin_source { - // the scale the partial's numbers are written on: an unmentioned scale means the relative scale, as `merged` reads it - let partial_scale = match wb.and_then(|wb| wb.get("scale")).and_then(Value::as_str) { - Some("kelvin") => lightcraft_develop::WbScale::Kelvin, - _ => lightcraft_develop::WbScale::Legacy, - }; - // put the photo onto the Kelvin scale (converting a relative look once), then onto the partial's - lightcraft_pipeline::local::normalize_wb(&mut d, info); - if d.wb.scale != partial_scale && partial_scale == lightcraft_develop::WbScale::Legacy { - let as_shot = (info.as_shot_temp, info.as_shot_tint); - let (kt, ktint) = lightcraft_pipeline::local::effective_wb(info, &d); - if let Some((lt, ltint)) = lightcraft_pipeline::local::kelvin_to_legacy(kt, ktint, as_shot) { - (d.wb.mode, d.wb.temp, d.wb.tint, d.wb.scale) = (WbMode::Custom, lt, ltint, lightcraft_develop::WbScale::Legacy); - } - } - } - let mut out = d.merged(partial)?; - if kelvin_source { + let prepared = prepare_photo_for_wb_partial(d, partial, info); + let on_kelvin_scale = prepared.is_some() && info.raw && !info.relative_wb; + let mut out = prepared.as_ref().unwrap_or(d).merged(partial)?; + if on_kelvin_scale { // and the result back onto the scale this photo is edited on lightcraft_pipeline::local::normalize_wb(&mut out, info); } @@ -1061,7 +1092,20 @@ pub fn specs() -> Vec { let ops = ids .iter() .filter_map(|id| s.develop_of(*id).map(|d| (*id, d))) - .filter_map(|(id, d)| s.develop_op(id, preset.apply(&d, amount), &format!("Preset: {}", preset.name))) + .filter_map(|(id, d)| { + // a hand-written preset may name one white balance number only, and then it means the same as + // a partial: bring the photo onto that scale first, with its own as-shot white, and put the + // result back on the scale the photo is edited on (issue #2910) — the merge `develop.merge` uses + let info = s.source_info(id); + let prepared = prepare_photo_for_wb_partial(&d, &preset.settings, &info); + let on_kelvin_scale = prepared.is_some() && info.raw && !info.relative_wb; + let base: &DevelopSettings = prepared.as_ref().unwrap_or(&d); + let mut next = preset.apply(base, amount); + if on_kelvin_scale { + lightcraft_pipeline::local::normalize_wb(&mut next, &info); + } + s.develop_op(id, next, &format!("Preset: {}", preset.name)) + }) .collect::>(); s.commit(&format!("Preset: {}", preset.name), Op::Batch { ops })?; ok() diff --git a/crates/engine/src/tests_color.rs b/crates/engine/src/tests_color.rs index a9761b2e9..1ce7a6918 100644 --- a/crates/engine/src/tests_color.rs +++ b/crates/engine/src/tests_color.rs @@ -296,6 +296,64 @@ fn one_field_wb_partial_merge_keeps_the_other_field_on_its_scale() { nearly(&d.wb, &l2k(5800.0, 0.0), "the as-shot tint keeps its meaning"); assert_eq!(d.wb.scale, WbScale::Kelvin); + // A mode that answers its own white (As Shot, or a named preset on a raw edited in Kelvin) ignores the numbers + // stored on the photo, so a one-field partial naming its scale landed on a white balance that stored the number + // where no render would read it: the panel showed 5800 while the photo stayed at as shot. The white the photo + // really renders is written out as Custom values first, so the named number means what it says and the field the + // partial leaves out carries that same white (review of #2952). + let as_shot_photo = DevelopSettings { + wb: lightcraft_develop::WhiteBalance { mode: WbMode::AsShot, temp: 4986.0, tint: -2.0, scale: WbScale::Kelvin }, + ..DevelopSettings::for_raw(as_shot.0, as_shot.1) + }; + s.set_develop(kelvin, as_shot_photo.clone(), "as shot").unwrap(); + assert_eq!(lightcraft_engine_effective(&s.source_info(kelvin), &as_shot_photo), as_shot, "as shot renders the file's own white"); + s.execute("develop.merge", &json!({"settings": {"wb": {"temp": 5800.0, "scale": "kelvin"}}})).unwrap(); + let d = dev(&s, kelvin); + assert_eq!((d.wb.mode, d.wb.temp, d.wb.tint, d.wb.scale), (WbMode::Custom, 5800.0, -2.0, WbScale::Kelvin), "{:?}", d.wb); + assert_eq!(lightcraft_engine_effective(&s.source_info(kelvin), &d), (5800.0, -2.0), "the temperature the partial asked for renders"); + // the tint side of the same hole + s.set_develop(kelvin, as_shot_photo.clone(), "as shot").unwrap(); + s.execute("develop.merge", &json!({"settings": {"wb": {"tint": 20.0, "scale": "kelvin"}}})).unwrap(); + let d = dev(&s, kelvin); + assert_eq!((d.wb.mode, d.wb.temp, d.wb.tint, d.wb.scale), (WbMode::Custom, as_shot.0, 20.0, WbScale::Kelvin), "{:?}", d.wb); + assert_eq!(lightcraft_engine_effective(&s.source_info(kelvin), &d), (as_shot.0, 20.0)); + // a named preset mode answers its own Kelvin white on such a raw, so its numbers are swallowed the same way + let preset_photo = DevelopSettings { + wb: lightcraft_develop::WhiteBalance { mode: WbMode::Cloudy, temp: 4986.0, tint: -2.0, scale: WbScale::Kelvin }, + ..DevelopSettings::for_raw(as_shot.0, as_shot.1) + }; + s.set_develop(kelvin, preset_photo.clone(), "cloudy").unwrap(); + let cloudy = lightcraft_engine_effective(&s.source_info(kelvin), &preset_photo); + assert_ne!(cloudy, (4986.0, -2.0), "the preset mode renders its own white, not the stored numbers"); + s.execute("develop.merge", &json!({"settings": {"wb": {"temp": 5800.0, "scale": "kelvin"}}})).unwrap(); + let d = dev(&s, kelvin); + assert_eq!((d.wb.mode, d.wb.temp, d.wb.tint, d.wb.scale), (WbMode::Custom, 5800.0, cloudy.1, WbScale::Kelvin), "{:?}", d.wb); + assert_eq!(lightcraft_engine_effective(&s.source_info(kelvin), &d), (5800.0, cloudy.1), "the kept tint is that white's own tint"); + // `Auto` resolves its gray when the mode is set and reads its stored numbers back: its mode stays, nothing frozen + let auto_photo = DevelopSettings { + wb: lightcraft_develop::WhiteBalance { mode: WbMode::Auto, temp: 5100.0, tint: 3.0, scale: WbScale::Kelvin }, + ..DevelopSettings::for_raw(as_shot.0, as_shot.1) + }; + s.set_develop(kelvin, auto_photo, "auto").unwrap(); + s.execute("develop.merge", &json!({"settings": {"wb": {"temp": 5800.0, "scale": "kelvin"}}})).unwrap(); + let d = dev(&s, kelvin); + assert_eq!((d.wb.mode, d.wb.temp, d.wb.tint), (WbMode::Auto, 5800.0, 3.0), "{:?}", d.wb); + // a partial that names a mode stays authoritative: naming `asShot` puts the photo back on the file's white + s.set_develop(kelvin, as_shot_photo.clone(), "as shot").unwrap(); + s.execute("develop.merge", &json!({"settings": {"wb": {"mode": "asShot", "temp": 5800.0, "scale": "kelvin"}}})).unwrap(); + let d = dev(&s, kelvin); + assert_eq!((d.wb.mode, lightcraft_engine_effective(&s.source_info(kelvin), &d)), (WbMode::AsShot, as_shot), "{:?}", d.wb); + // the same swallow on a source without the Kelvin scale (a raw with no matrices): the number was stored and read + // by nothing there too, and now reaches the render + s.execute("library.select", &json!({"ids": [relative.0], "active": relative.0})).unwrap(); + let before = dev(&s, relative); + assert_eq!(lightcraft_engine_effective(&s.source_info(relative), &before), (6500.0, 0.0), "as shot on this body is 6500 / 0"); + s.execute("develop.merge", &json!({"settings": {"wb": {"temp": 5800.0}}})).unwrap(); + let d = dev(&s, relative); + assert_eq!((d.wb.mode, d.wb.temp, d.wb.tint, d.wb.scale), (WbMode::Custom, 5800.0, 0.0, WbScale::Legacy), "{:?}", d.wb); + assert_eq!(lightcraft_engine_effective(&s.source_info(relative), &d), (5800.0, 0.0), "the number the partial named renders"); + s.execute("library.select", &json!({"ids": [kelvin.0], "active": kelvin.0})).unwrap(); + // the other direction: an old relative Custom value (no scale) plus a Kelvin temp-only partial — the photo's // white balance is taken onto the Kelvin scale first, so the kept tint is the Kelvin value of the photo's look let old = DevelopSettings { @@ -332,3 +390,77 @@ fn one_field_wb_partial_merge_keeps_the_other_field_on_its_scale() { assert_eq!(d.light.contrast, 20.0); assert_eq!(d.wb, wb_before, "the white balance is untouched"); } + +/// The issue asks for one photo-aware merge in every caller that writes settings onto a photo, and that includes +/// `preset.apply`: a preset a person or an agent authors by hand can name a single white balance number, and on a +/// photo edited in Kelvin that demoted the field it never mentioned to the relative scale, exactly as +/// `develop.merge` did. Presets the app makes and the crs / Luminar importers name `mode`, `temp`, `tint` and +/// `scale` together, and those keep their old path through `Preset::apply` → `apply_partial` unchanged. +#[test] +fn one_field_wb_preset_apply_keeps_the_other_field_on_its_scale() { + use lightcraft_catalog::{MediaKind, Op, Photo, Source}; + use lightcraft_develop::{DevelopSettings, WbMode, WbScale}; + let mut s = Session::new(); + let add = |s: &mut Session, name: &str, camera: &str, as_shot: (f64, f64)| { + let id = s.catalog.alloc_photo_id(); + let mut p = Photo::new(id, Source::File { path: format!("/photos/{name}") }, name, "ARW", 64, 64, "2026-01-01T00:00:00"); + p.kind = MediaKind::Raw; + p.meta.camera = camera.into(); + p.as_shot_wb = Some(as_shot); + p.develop = std::sync::Arc::new(p.camera_defaults()); + s.catalog.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + id + }; + let as_shot = (4986.0, -2.0); + let kelvin = add(&mut s, "preset-wb.arw", "SONY ILCE-7M3", as_shot); + let l2k = |t, ti| lightcraft_pipeline::local::legacy_to_kelvin(t, ti, as_shot); + let k2l = |t, ti| lightcraft_pipeline::local::kelvin_to_legacy(t, ti, as_shot).unwrap(); + let nearly = |wb: &lightcraft_develop::WhiteBalance, want: &(f64, f64), why: &str| { + assert!((wb.temp - want.0).abs() < 1.0 && (wb.tint - want.1).abs() < 0.5, "{why}: {wb:?} vs {want:?}"); + }; + s.execute("library.select", &json!({"ids": [kelvin.0], "active": kelvin.0})).unwrap(); + let edited = DevelopSettings { + wb: lightcraft_develop::WhiteBalance { mode: WbMode::Custom, temp: 5000.0, tint: 12.0, scale: WbScale::Kelvin }, + ..DevelopSettings::for_raw(as_shot.0, as_shot.1) + }; + s.set_develop(kelvin, edited.clone(), "kelvin edit").unwrap(); + + // a hand-written preset naming one number: the tint it never mentions keeps its Kelvin look, instead of the + // old path that stored 5800 and re-read the photo's +12 tint as a relative shift + s.presets.push(lightcraft_develop::Preset { + id: "user.temp-only".into(), + name: "Temp only".into(), + group: "User".into(), + settings: json!({"wb": {"temp": 5800.0}}), + favorite: false, + builtin: false, + }); + s.execute("preset.apply", &json!({"id": "user.temp-only"})).unwrap(); + let d = (*s.develop_of(kelvin).unwrap()).clone(); + let keep = k2l(5000.0, 12.0); + assert_eq!(d.wb.mode, WbMode::Custom, "{:?}", d.wb); + assert_eq!(d.wb.scale, WbScale::Kelvin, "the preset left the photo on the scale it is edited on: {:?}", d.wb); + nearly(&d.wb, &l2k(5800.0, keep.1), "the preset's number keeps the meaning an unmarked scale always had"); + // an amount below 100 % blends between the photo's own white and the preset's, on one scale, and never + // mentions the other field at all + s.set_develop(kelvin, edited.clone(), "kelvin edit").unwrap(); + s.execute("preset.apply", &json!({"id": "user.temp-only", "amount": 50})).unwrap(); + let half = (*s.develop_of(kelvin).unwrap()).clone(); + let full = l2k(5800.0, keep.1); + assert_eq!(half.wb.scale, WbScale::Kelvin, "{:?}", half.wb); + assert!(half.wb.temp < 5000.0 && half.wb.temp > full.0, "between the photo's own white and the preset's: {:?}", half.wb); + nearly(&half.wb, &(half.wb.temp, 12.0), "the tint the preset never named stays exactly the photo's"); + // the same number through `develop.merge` gives the same white: that is the point of one shared merge + s.set_develop(kelvin, edited.clone(), "kelvin edit").unwrap(); + s.execute("develop.merge", &json!({"settings": {"wb": {"temp": 5800.0}}})).unwrap(); + let merged = (*s.develop_of(kelvin).unwrap()).clone(); + nearly(&merged.wb, &(d.wb.temp, d.wb.tint), "preset apply and develop.merge agree on a one-field white balance"); + // a preset made from the photo names the whole white balance: its own path, unchanged + s.execute("preset.create", &json!({"name": "My kelvin look", "groups": ["whiteBalance"]})).unwrap(); + let made = s.presets.iter().find(|p| p.name == "My kelvin look").cloned().unwrap(); + let look = (*s.develop_of(kelvin).unwrap()).clone(); + s.execute("develop.merge", &json!({"settings": {"wb": {"mode": "asShot"}}})).unwrap(); + s.execute("preset.apply", &json!({"id": made.id})).unwrap(); + let back = (*s.develop_of(kelvin).unwrap()).clone(); + assert_eq!(back.wb, look.wb, "a preset made from the photo puts its whole white balance back: {:?} vs {:?}", back.wb, look.wb); +} diff --git a/docs/control-protocol.md b/docs/control-protocol.md index ddfe95914..ef2b56b2f 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -117,13 +117,19 @@ raws with colour matrices — in the file (DNG) or measured for the camera model #730) — and on raws whose white-balance presets give the scale (`lightcraft_raw::kelvin_presets`), where As Shot reads the file's as-shot white; rendered files and raws without matrices use a relative scale whose 6500 K / 0 is the as-shot look (the Edit panel shows it as −100..+100). The stored `wb` of such a Kelvin raw -says `"scale": "kelvin"`; a partial settings JSON given to `develop.merge` or a preset whose `wb` sets `temp` / `tint` -**without** `scale` keeps the relative meaning it always had (6500 = as shot) on every raw, so agents that mean -Kelvin on a Kelvin raw pass `"scale": "kelvin"` or use `develop.set` / `develop.wb`. A partial that names only one -of `temp` / `tint` is resolved against the photo (issue #2910): the photo's white balance is first brought onto the -partial's scale with the photo's own as-shot white, so the field the partial does not name keeps the look it had -instead of being re-read on the partial's scale, and the stored result is back on the scale the photo is edited on -(`"scale": "kelvin"` on a Kelvin raw). Partials naming both fields mean the same as before. +says `"scale": "kelvin"`; a partial settings JSON given to `develop.merge`, to a preset's `wb`, or to +`lightcraft-cli render --settings` and which sets `temp` / `tint` **without** `scale` keeps the relative meaning it +always had (6500 = as shot) on every raw, so agents that mean Kelvin on a Kelvin raw pass `"scale": "kelvin"` or use +`develop.set` / `develop.wb`. A partial that names only one of `temp` / `tint` is resolved against the photo +(issue #2910): the photo's white balance is first brought onto the partial's scale with the photo's own as-shot +white, so the field the partial does not name keeps the look it had instead of being re-read on the partial's scale, +and the stored result is back on the scale the photo is edited on (`"scale": "kelvin"` on a Kelvin raw). The same +holds for `preset.apply` when the preset names only one of the two numbers; a preset written by the app, or imported +from crs / XMP, names `mode`, `temp`, `tint` and `scale` together and behaves exactly as before. A photo whose white +balance mode answers its own white — `As Shot`, or a named preset on a raw edited in Kelvin — has that white written +out as `Custom` values first, because a number merged into such a mode would otherwise be stored where no render +reads it (the panel would show 5800 while the photo stayed at as shot); a partial that names a `mode` itself stays +authoritative. Partials naming both fields mean the same as before. Image navigation is also available directly as the UI command `view.navigate`, with `{zoom?: "fit" | "fill" | {"percent": number}, pan?: [x, y]}`. Percentage zoom accepts fractional diff --git a/docs/mcp.md b/docs/mcp.md index 24eff0154..505a5b9ec 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -175,7 +175,7 @@ per-request `_meta` receive `resultType: complete` and list/read cache hints; re | `select_photos {ids, active?, mode?}` | Set the selection / active photo. Every id (and `active`) must be in the library: an unknown id is a tool error (`no such photo 9999`) and the selection and active photo are left as they were | | `list_controls {section?}` | Every develop slider: id (`light.exposure`…), range, default, current value | | `get_develop {id?}` | Full develop-settings JSON, with `process`: the rendering process the photo is on ([process-versions.md](process-versions.md)) | -| `set_develop {id?, values?, settings?, label?}` | `values`: `{controlId: number}`; `settings`: partial develop JSON deep-merged. Undoable. A `crop.angle` value turns the crop as `crop.straighten` does: kept if it still fits, else shrunk to the largest crop of its aspect inside the photo (`settings` is merged as given, unfitted). White balance: `values` with `wb.temp` / `wb.tint` is Kelvin on raws with colour matrices (DNG, and cameras LightCraft has matrices for) and relative (6500 = as shot) elsewhere; a `settings.wb` with `temp` / `tint` and no `"scale": "kelvin"` is read on the relative scale on every raw, and one that names only `temp` or only `tint` resolves the field it leaves out against the photo, so that field keeps its look (#2910) (see [control-protocol.md](control-protocol.md)) | +| `set_develop {id?, values?, settings?, label?}` | `values`: `{controlId: number}`; `settings`: partial develop JSON deep-merged. Undoable. A `crop.angle` value turns the crop as `crop.straighten` does: kept if it still fits, else shrunk to the largest crop of its aspect inside the photo (`settings` is merged as given, unfitted). White balance: `values` with `wb.temp` / `wb.tint` is Kelvin on raws with colour matrices (DNG, and cameras LightCraft has matrices for) and relative (6500 = as shot) elsewhere; a `settings.wb` with `temp` / `tint` and no `"scale": "kelvin"` is read on the relative scale on every raw, and one that names only `temp` or only `tint` resolves the field it leaves out against the photo, so that field keeps its look; on a photo still on As Shot (or on a named white balance preset, on a raw with matrices) the white it really renders is written out as Custom values first, so the number named takes effect instead of being stored unread (#2910, and its review); `apply_preset` resolves a preset that names one number the same way (see [control-protocol.md](control-protocol.md)) | | `apply_preset {preset, amount?, ids?}` | Apply a preset (ids from `cmd_presets_list`) | | `crop {id?, rect?, angle?, reset?}` | Normalized crop rect `[x0,y0,x1,y1]` and straighten angle; at least one of `rect`, `angle`, `reset: true`. The crop always ends inside the photo: an `angle` alone keeps the crop if it still fits, else shrinks it to the largest of its aspect that does | | `render_photo {id?, size?, format?, path?}` | Render with current settings → **image content** (PNG, or JPEG with `format: "jpeg"`), long edge `size` (default 1024) | diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 5b0143005..02c5caebd 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -191,7 +191,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts). | LR-EDIT-LIGHT-CURVE-TAT | Drag-on-image curve adjust | P1 | ✅ | `cmd:develop.targeted` (`target: curve`) | | | LR-EDIT-COLOR-WB-PRESET | White-balance presets | P0 | ✅ | `cmd:develop.wb` | | | LR-EDIT-COLOR-WB-PICKER | White-balance eyedropper | P0 | ✅ | `cmd:tool.wbPicker`, `cmd:develop.wbPick`, `crates/ui-egui/src/tests_wb_selector.rs` | the Edit panel's selector button does what W does (from Compare, Survey or the grid it opens the photo in Detail); no magnified loupe while picking | -| LR-EDIT-COLOR-TEMP | Temperature | P0 | ✅ | `ctl:wb.temp`, `crates/pipeline/src/local.rs` (`wb_matrix_for`, `legacy_white`), `crates/raw/src/color.rs` (`rebalance`), `crates/raw/src/spectral.rs` (`find_camera`), `crates/catalog/src/model.rs` (`relative_wb_camera`), `crates/develop/src/controls.rs` (`ControlSpec::position`), `crates/raw/src/kelvin_presets.rs` | Kelvin (2000–50000 K, As Shot read from the file) on raws with colour matrices: in the file (DNG: re-developed for the chosen white through the camera's own matrices, camera-space white balance as Lightroom; the eyedropper / Auto invert the same model) or measured for the camera model (`lightcraft_raw::spectral`, 52 bodies, issue #730: the matrices turn the as-shot multipliers into Kelvin and the slider back, the look stays the file's fit, and the picture is adapted from the as-shot white, Bradford), or read from the camera's own white-balance presets (Sony ARW `SR2SubIFD`, Sony ILCA-99M2, ILCA-77M2, SLT-A33, DSC-RX10, ILCE-3500, ILCE-7S: Sony's Kelvin dial reads back within 0.3 % on 5 settings of 4 bodies; Canon CR2 `ColorData` presets, PowerShot G1 X Mark III: the camera's own As Shot temperature reads back within 5 % on its 9 white-balance settings; or built-in presets from the camera's own stated temperatures, `crates/raw/src/kelvin_builtin.rs`: Nikon Z5II, whose Natural Auto shot reads back at 5036 K against the 5030 K the camera wrote). The Kelvin slider's track is Lightroom's: 7000 K in the middle, 2000–7000 K evenly on the left half, logarithmic up to 50000 K on the right (within 2 % of positions measured in Lightroom Classic), for drawing, clicks and drags; wheel / arrow-key nudges and Quick Develop still step a fixed 50 / 100 / 500 K; the relative slider stays linear. Settings written on the relative scale before a camera's matrices reached it keep their look (`WbScale`). A partial white balance naming only `temp` or only `tint` (`develop.merge`, `render --settings`, MCP `set_develop`, a hand-written preset) is resolved against the photo, so the field it leaves out keeps the look it had: `merge_settings_for_photo` in `crates/engine/src/cmd/develop.rs` and `kelvin_to_legacy` in `crates/pipeline/src/local.rs`, issue #2910, tests in `crates/engine/src/tests_color.rs`. Not verified against Lightroom's Kelvin: on the ILCE-7M3 the matrices read 7–10 % below Sony's dial (issue #730), and whether the matrices or the dial are off is open; raws without matrices or presets anywhere and rendered files use the relative −100..+100 scale (6500 K / 0 = as shot). | +| LR-EDIT-COLOR-TEMP | Temperature | P0 | ✅ | `ctl:wb.temp`, `crates/pipeline/src/local.rs` (`wb_matrix_for`, `legacy_white`), `crates/raw/src/color.rs` (`rebalance`), `crates/raw/src/spectral.rs` (`find_camera`), `crates/catalog/src/model.rs` (`relative_wb_camera`), `crates/develop/src/controls.rs` (`ControlSpec::position`), `crates/raw/src/kelvin_presets.rs` | Kelvin (2000–50000 K, As Shot read from the file) on raws with colour matrices: in the file (DNG: re-developed for the chosen white through the camera's own matrices, camera-space white balance as Lightroom; the eyedropper / Auto invert the same model) or measured for the camera model (`lightcraft_raw::spectral`, 52 bodies, issue #730: the matrices turn the as-shot multipliers into Kelvin and the slider back, the look stays the file's fit, and the picture is adapted from the as-shot white, Bradford), or read from the camera's own white-balance presets (Sony ARW `SR2SubIFD`, Sony ILCA-99M2, ILCA-77M2, SLT-A33, DSC-RX10, ILCE-3500, ILCE-7S: Sony's Kelvin dial reads back within 0.3 % on 5 settings of 4 bodies; Canon CR2 `ColorData` presets, PowerShot G1 X Mark III: the camera's own As Shot temperature reads back within 5 % on its 9 white-balance settings; or built-in presets from the camera's own stated temperatures, `crates/raw/src/kelvin_builtin.rs`: Nikon Z5II, whose Natural Auto shot reads back at 5036 K against the 5030 K the camera wrote). The Kelvin slider's track is Lightroom's: 7000 K in the middle, 2000–7000 K evenly on the left half, logarithmic up to 50000 K on the right (within 2 % of positions measured in Lightroom Classic), for drawing, clicks and drags; wheel / arrow-key nudges and Quick Develop still step a fixed 50 / 100 / 500 K; the relative slider stays linear. Settings written on the relative scale before a camera's matrices reached it keep their look (`WbScale`). A partial white balance naming only `temp` or only `tint` (`develop.merge`, `render --settings`, MCP `set_develop`, or `preset.apply` of a hand-written preset naming one of them) is resolved against the photo, so the field it leaves out keeps the look it had, and a photo whose mode answers its own white (`As Shot`, or a named preset on a raw with a Kelvin scale) has the white it really renders written out as Custom values the partial can move: `prepare_photo_for_wb_partial` and `merge_settings_for_photo` in `crates/engine/src/cmd/develop.rs` and `kelvin_to_legacy` in `crates/pipeline/src/local.rs`, issue #2910, tests in `crates/engine/src/tests_color.rs`. Not verified against Lightroom's Kelvin: on the ILCE-7M3 the matrices read 7–10 % below Sony's dial (issue #730), and whether the matrices or the dial are off is open; raws without matrices or presets anywhere and rendered files use the relative −100..+100 scale (6500 K / 0 = as shot). | | LR-EDIT-COLOR-TINT | Tint | P0 | ✅ | `ctl:wb.tint`, `docs/tint-direction.md` | DNG / Lightroom sign: negative (left) adds green, positive (right) magenta; XMP `crs:Tint` values render as in Lightroom (issue #188); CPU/GPU regression tests | | LR-EDIT-COLOR-VIBRANCE | Vibrance | P0 | ✅ | `ctl:color.vibrance` | | | LR-EDIT-COLOR-SATURATION | Saturation | P0 | ✅ | `ctl:color.saturation` | | diff --git a/docs/whats-new.md b/docs/whats-new.md index 68117bc72..db921cc55 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -4,13 +4,19 @@ ### A one-field white balance edit no longer shifts the other one - Settings that name only a white balance temperature, or only a tint — `develop.merge`, - `lightcraft-cli render --settings`, an MCP `develop.merge` call, or a hand-written preset — used to change the - colour balance of photos they never mentioned. On a raw edited in Kelvin the field the partial kept was re-read - on the other scale: `wb {temp: 5800}` on a photo at 5000 K / +12 tint came back at 5800 K with the +12 read as a - relative shift, moving the green–magenta balance by a visible amount. The kept field now carries the look the - photo had, and the result is stored on the scale that photo is edited on. Presets, XMP import and settings that - name both fields (or neither) behave exactly as before, and a partial that says `"scale": "kelvin"` is still - read in Kelvin. + `lightcraft-cli render --settings `, an MCP `set_develop {settings: …}` call, or a hand-written + preset naming one of the two — used to change the colour balance of photos they never mentioned. On a raw edited in + Kelvin the field the partial left out was re-read on the other scale: `wb {temp: 5800}` on a photo at 5000 K / + +12 tint came back at 5800 K with the +12 read as a relative shift, moving the green–magenta balance by a visible + amount. The kept field now carries the look the photo had, and the result is stored on the scale that photo is + edited on. +- The same partial on a photo still on As Shot — or on a named white balance preset, on a raw with a Kelvin scale — + stored the number without any render ever reading it: those modes answer their own white, so the panel showed + 5800 K over a picture still at the file's white. That white is now written out as Custom values before the partial + lands; a partial that names a mode itself still gets that mode. +- Presets made from a photo and the crs / XMP and Luminar importers name mode, temperature, tint and scale together, + and partials naming both fields (or neither) behave exactly as before; a partial that says `"scale": "kelvin"` is + still read in Kelvin. ## 2026-10-24