diff --git a/crates/engine/src/cmd/develop.rs b/crates/engine/src/cmd/develop.rs index fbc3d4041..de21a59dc 100644 --- a/crates/engine/src/cmd/develop.rs +++ b/crates/engine/src/cmd/develop.rs @@ -51,6 +51,80 @@ 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 +/// 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. 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 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); + } + 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 +330,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(()) }) }), @@ -1016,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 e526d4c47..1ce7a6918 100644 --- a/crates/engine/src/tests/color.rs +++ b/crates/engine/src/tests/color.rs @@ -220,3 +220,247 @@ 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); + + // 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 { + 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"); +} + +/// 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/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 36294c787..0d03146fe 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -117,9 +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`. +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 2c155e72c..593b455b1 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; 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 ea3f3fb0c..69d8072c0 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -192,6 +192,8 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts). | 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-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`, 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 a8dcea327..75d8f8fbc 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -28,6 +28,24 @@ ## 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 `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-25 + ### JPEG ICC chunks are validated before colour conversion - JPEG ICC profiles are assembled only when every chunk is present exactly once, the sequence numbers and declared totals agree, and every APP2 segment is complete. Reordered valid chunks still work. An invalid