From 978132fdcaf60e9f2ce2514d0aaf04e44d4b0925 Mon Sep 17 00:00:00 2001 From: Markus Friedel <20045158+MarkusFriedel@users.noreply.github.com> Date: Mon, 12 Oct 2026 17:37:42 +0200 Subject: [PATCH] LR-EDIT-COLOR-TEMP: the Kelvin Temp slider has Lightroom's scale, 7000 K in the middle (#1295) Co-Authored-By: Claude Opus 5.5 --- crates/develop/src/controls.rs | 79 +++++++++++++++ crates/ui-egui/src/lib.rs | 2 + crates/ui-egui/src/tests_temp_slider.rs | 125 ++++++++++++++++++++++++ crates/ui-egui/src/widgets.rs | 54 +++++++++- docs/parity-checklist.md | 2 +- 5 files changed, 256 insertions(+), 6 deletions(-) create mode 100644 crates/ui-egui/src/tests_temp_slider.rs diff --git a/crates/develop/src/controls.rs b/crates/develop/src/controls.rs index 83b26403d..715e8ac33 100644 --- a/crates/develop/src/controls.rs +++ b/crates/develop/src/controls.rs @@ -99,8 +99,45 @@ impl ControlSpec { let s = format!("{:.*}", self.decimals as usize, v); if v > 0.0 && self.min < 0.0 { format!("+{s}") } else { s } } + /// Where `v` sits on the slider track, from 0 (left end) to 1 (right end). Linear, except for the Kelvin + /// temperature, which follows Lightroom's scale: 2000–7000 K fill the left half evenly (100 K per 1 %), and + /// the right half grows logarithmically up to 50000 K, so everyday temperatures aren't crowded into the + /// left sixth of the track. The relative temperature of rendered files (−100…+100, already in mireds) stays + /// linear. + pub fn position(&self, v: f64) -> f64 { + let v = self.clamp(v); + let p = match self.kelvin_scale() { + Some((centre, right)) if v > centre => 0.5 + 0.5 * (1.0 + (v - centre) / KELVIN_KNEE).ln() / right, + Some((centre, _)) => 0.5 * (v - self.min) / (centre - self.min), + None => (v - self.min) / (self.max - self.min).max(1e-9), + }; + if p.is_finite() { p.clamp(0.0, 1.0) } else { 0.0 } + } + /// The value at track position `p` (0…1): the inverse of [`ControlSpec::position`], not yet on the steps. + pub fn value_at(&self, p: f64) -> f64 { + let p = if p.is_finite() { p.clamp(0.0, 1.0) } else { 0.0 }; + let v = match self.kelvin_scale() { + Some(_) if p >= 1.0 => self.max, + Some((centre, right)) if p > 0.5 => centre + KELVIN_KNEE * (((2.0 * p - 1.0) * right).exp() - 1.0), + Some((centre, _)) => self.min + 2.0 * p * (centre - self.min), + None => self.min + p * (self.max - self.min), + }; + self.clamp(v) + } + /// The Kelvin temperature's track centre and `ln` of its right half's span (see [`ControlSpec::position`]); + /// `None` for every other control, or a range the centre isn't inside. + fn kelvin_scale(&self) -> Option<(f64, f64)> { + let centre = KELVIN_CENTRE; + (self.id == "wb.temp" && self.min < centre && centre < self.max).then(|| (centre, (1.0 + (self.max - centre) / KELVIN_KNEE).ln())) + } } +/// The Kelvin temperature at the middle of its track, as in Lightroom. +const KELVIN_CENTRE: f64 = 7000.0; +/// Where the right half of the Kelvin track turns logarithmic: 1000 K gives it close to the left half's slope +/// just past the centre (76 K against 100 K per 1 % of the track), so the thumb moves evenly across it. +const KELVIN_KNEE: f64 = 1000.0; + macro_rules! controls { ($( $id:literal => $($field:ident).+ , $label:literal, $section:ident, $min:expr, $max:expr, $def:expr, $step:expr, $dec:expr, $track:expr; )*) => { /// All numeric develop controls, in panel order. @@ -508,4 +545,46 @@ mod tests { assert_eq!(e.format(-1.0), "-1.00"); assert_eq!(find("wb.temp").unwrap().format(5500.0), "5500"); } + + /// The Kelvin temperature's track follows Lightroom's (positions measured in Lightroom Classic against + /// the linear Tint slider; the model is within 2 % of them); every other control stays linear. + #[test] + fn the_kelvin_track_is_lightrooms_and_others_stay_linear() { + let temp = find("wb.temp").unwrap(); + for (k, lightroom, model) in [ + (2000.0, 0.0, 0.0), + (3650.0, 0.167, 0.165), + (5400.0, 0.333, 0.34), + (7000.0, 0.5, 0.5), + (15000.0, 0.79, 0.79), + (20000.0, 0.853, 0.849), + (30000.0, 0.90, 0.92), + (50000.0, 1.0, 1.0), + ] { + let p = temp.position(k); + assert!((p - model).abs() < 0.001, "{k} K at {p}, the model says {model}"); + assert!((p - lightroom).abs() <= 0.021, "{k} K at {p}, Lightroom at {lightroom}"); + } + // every 50 K step survives value → position → value, and the track never runs backwards + let mut last = -1.0; + for k in (2000..=50000).step_by(50).map(f64::from) { + let p = temp.position(k); + assert!(p > last, "{k} K"); + last = p; + let back = (temp.value_at(p) / temp.step).round() * temp.step; + assert_eq!(back, k, "{k} K → {p} → {back}"); + } + assert_eq!((temp.value_at(-1.0), temp.value_at(2.0), temp.value_at(f64::NAN)), (2000.0, 50000.0, 2000.0)); + assert_eq!((temp.position(1.0), temp.position(1e9), temp.position(f64::NAN)), (0.0, 1.0, temp.position(temp.default))); + let exposure = find("light.exposure").unwrap(); + for (v, p) in [(-5.0, 0.0), (-2.5, 0.25), (0.0, 0.5), (1.0, 0.6), (5.0, 1.0)] { + assert!((exposure.position(v) - p).abs() < 1e-12 && (exposure.value_at(p) - v).abs() < 1e-12, "{v}"); + } + // the relative temperature of rendered files (its own spec, centred on 0) isn't a Kelvin scale + let relative = ControlSpec { id: "wb.tempRel", min: -100.0, max: 100.0, default: 0.0, step: 1.0, ..*temp }; + assert_eq!((relative.position(-50.0), relative.value_at(0.75)), (0.25, 50.0)); + // a Kelvin range that doesn't contain the centre falls back to linear rather than dividing by zero + let narrow = ControlSpec { min: 7000.0, max: 9000.0, ..*temp }; + assert_eq!((narrow.position(8000.0), narrow.value_at(0.5)), (0.5, 8000.0)); + } } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 9a387c77c..87572e1be 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -112,6 +112,8 @@ mod tests_switch_library; #[cfg(test)] mod tests_sync; #[cfg(test)] +mod tests_temp_slider; +#[cfg(test)] mod tests_titlebar; #[cfg(test)] mod tests_unsaved; diff --git a/crates/ui-egui/src/tests_temp_slider.rs b/crates/ui-egui/src/tests_temp_slider.rs new file mode 100644 index 000000000..c8e4e0d4c --- /dev/null +++ b/crates/ui-egui/src/tests_temp_slider.rs @@ -0,0 +1,125 @@ +//! Headless tests of the Temp slider's scale: the Kelvin temperature of a raw follows Lightroom's track +//! (2000–7000 K on the left half, logarithmic up to 50000 K on the right); the relative temperature of a +//! rendered photo and every other slider stay linear. + +use std::time::Duration; + +use serde_json::json; + +use crate::headless::Headless; +use crate::{LightcraftApp, Services}; + +const T: Duration = Duration::from_secs(20); +const SETTLE: Duration = Duration::from_secs(120); + +/// The demo library in Detail with the Edit panel's Light and Color sections open, on a raw photo (Kelvin +/// white balance) when `raw`, else on a demo JPEG (relative white balance). +fn edit_panel(raw: bool) -> Headless { + use lightcraft_catalog::{MediaKind, Op, Photo, PhotoId, Source}; + let app = LightcraftApp::new(lightcraft_engine::Session::with_demo(), Services { png: None, ..Default::default() }); + let mut h = Headless::new(app, [1400.0, 900.0], 1.0); + let id = if raw { + // a DNG raw photo (its file need not exist: the sliders only read its settings) + let id = PhotoId(h.app.session.catalog.photos().map(|p| p.id.0).max().unwrap_or(0) + 1); + let path = std::env::temp_dir().join("lc-temp-slider").join("IMG_1.dng"); + let mut photo = + Photo::new(id, Source::File { path: path.to_string_lossy().into_owned() }, "IMG_1.dng", "DNG", 6000, 4000, "2026-10-01T00:00:00"); + photo.kind = MediaKind::Raw; + h.app.session.commit("setup", Op::AddPhoto { photo: Box::new(photo) }).unwrap(); + id.0 + } else { + h.app.session.visible_cloned()[0].0 + }; + assert_eq!(h.request("engine.execute", json!({"command": "library.select", "params": {"ids": [id]}}), T)["ok"], true); + assert_eq!(h.request("ui.set", json!({"view": "detail", "right": "edit"}), T)["ok"], true); + h.app.ui.open_sections = vec!["light".to_string(), "color".to_string()]; + h.settle(SETTLE); + h +} + +fn widget(h: &Headless, id: &str) -> egui::Rect { + h.app.widgets.iter().find(|(w, _)| w == id).map(|(_, r)| *r).unwrap_or_else(|| panic!("no widget {id}")) +} + +/// Where the slider's thumb (the hollow ring on the track's centre line) is drawn, from 0 to 1 along the track. +fn thumb(h: &Headless, control: &str) -> f32 { + let track = widget(h, &format!("slider:{control}")); + let rings: Vec = h + .view + .painted_shapes() + .iter() + .filter_map(|s| match s { + egui::epaint::Shape::Circle(c) if c.radius == 7.0 && (c.center.y - track.center().y).abs() < 0.5 => Some(c.center.x), + _ => None, + }) + .filter(|x| (track.left() - 0.5..=track.right() + 0.5).contains(x)) + .collect(); + assert!(!rings.is_empty() && rings.iter().all(|x| (x - rings[0]).abs() < 0.01), "one thumb on {control}: {rings:?}"); + (rings[0] - track.left()) / track.width() +} + +fn develop(h: &Headless) -> lightcraft_develop::DevelopSettings { + let id = h.app.session.selection.active.unwrap(); + (*h.app.session.develop_of(id).unwrap_or_default()).clone() +} + +fn set(h: &mut Headless, control: &str, value: f64) { + let r = h.request("engine.execute", json!({"command": "develop.set", "params": {"control": control, "value": value}}), T); + assert_eq!(r["ok"], true, "{r}"); + h.settle(SETTLE); +} + +/// The thumb sits where Lightroom puts it (within 1 % of the model fitted to Lightroom's positions), clicking or +/// dragging to the middle of the track gives 7000 K, and the far end is still 50000 K. +#[test] +fn the_kelvin_temp_slider_has_lightrooms_scale() { + let mut h = edit_panel(true); + for (kelvin, model) in [(3650.0, 0.165), (5400.0, 0.34), (7000.0, 0.5), (15000.0, 0.79), (20000.0, 0.849)] { + set(&mut h, "wb.temp", kelvin); + assert_eq!(develop(&h).wb.temp, kelvin); + let at = thumb(&h, "wb.temp"); + assert!((at - model).abs() < 0.01, "{kelvin} K drawn at {at}, the model says {model}"); + } + set(&mut h, "wb.temp", 4850.0); + assert_eq!(h.request("ui.clickWidget", json!({"id": "slider:wb.temp", "fx": 0.5}), T)["ok"], true); + h.settle(SETTLE); + assert_eq!(develop(&h).wb.temp, 7000.0, "a click in the middle of the track"); + // drag from As Shot on a Z 6II NEF (about 4850 K, 28 % along the track) to the middle + set(&mut h, "wb.temp", 4850.0); + let track = widget(&h, "slider:wb.temp"); + let from = track.left() + thumb(&h, "wb.temp") * track.width(); + let r = h.request("ui.drag", json!({"x": from, "y": track.center().y, "toX": track.center().x, "toY": track.center().y, "steps": 12}), T); + assert_eq!(r["ok"], true, "{r}"); + h.settle(SETTLE); + let k = develop(&h).wb.temp; + assert!((k - 7000.0).abs() <= 50.0, "dragged to the middle: {k} K"); + assert_eq!(h.request("ui.clickWidget", json!({"id": "slider:wb.temp", "fx": 1.0}), T)["ok"], true); + h.settle(SETTLE); + assert_eq!(develop(&h).wb.temp, 50000.0); +} + +/// Exposure (like every slider but the Kelvin temperature) keeps its linear track. +#[test] +fn other_sliders_stay_linear() { + let mut h = edit_panel(true); + set(&mut h, "light.exposure", 1.0); + assert!((thumb(&h, "light.exposure") - 0.6).abs() < 0.005, "+1.00 of −5…+5 at 60 %"); + assert_eq!(h.request("ui.clickWidget", json!({"id": "slider:light.exposure", "fx": 0.75}), T)["ok"], true); + h.settle(SETTLE); + assert!((develop(&h).light.exposure - 2.5).abs() < 0.011, "{}", develop(&h).light.exposure); +} + +/// The relative temperature of a rendered photo (−100…+100, 0 = as shot, in the middle) is unchanged. +#[test] +fn the_relative_temp_slider_stays_linear() { + let mut h = edit_panel(false); + assert!(h.app.widgets.iter().all(|(w, _)| w != "slider:wb.temp"), "a JPEG has the relative scale"); + assert!((thumb(&h, "wb.tempRel") - 0.5).abs() < 0.005, "as shot in the middle"); + assert_eq!(h.request("ui.clickWidget", json!({"id": "slider:wb.tempRel", "fx": 0.75}), T)["ok"], true); + h.settle(SETTLE); + assert!((thumb(&h, "wb.tempRel") - 0.75).abs() < 0.005, "the thumb where it was clicked"); + // +50 on the relative scale: 40 mired warmer than 6500 K + let k = develop(&h).wb.temp; + let rel = (1e6 / 6500.0 - 1e6 / k) / 0.8; + assert!((rel - 50.0).abs() < 0.6, "{k} K is {rel} on the relative scale"); +} diff --git a/crates/ui-egui/src/widgets.rs b/crates/ui-egui/src/widgets.rs index 0872b2ec4..0260a6e2e 100644 --- a/crates/ui-egui/src/widgets.rs +++ b/crates/ui-egui/src/widgets.rs @@ -216,9 +216,9 @@ pub fn slider(ui: &mut Ui, spec: &ControlSpec, value: f64, enabled: bool, label_ ui.memory_mut(|m| m.surrender_focus(crate::text_field::id(&format!("sliderField:{}", spec.id)))); } let mut out = SliderOut::default(); - let span = (spec.max - spec.min).max(1e-9); - let to_x = |v: f64| track_rect.left() + ((v - spec.min) / span).clamp(0.0, 1.0) as f32 * track_rect.width(); - let from_x = |x: f32| spec.min + ((x - track_rect.left()) / track_rect.width()).clamp(0.0, 1.0) as f64 * span; + // value ↔ track position: linear, except for the Kelvin temperature (`ControlSpec::position`) + let to_x = |v: f64| track_rect.left() + spec.position(v) as f32 * track_rect.width(); + let from_x = |x: f32| spec.value_at(f64::from((x - track_rect.left()) / track_rect.width())); let mut v = value; if let Some((mut text, frames, pressed)) = typing.take() { // Keep the owned opening release focused without restarting its draft or selection. @@ -267,13 +267,19 @@ pub fn slider(ui: &mut Ui, spec: &ControlSpec, value: f64, enabled: bool, label_ && let Some(p) = resp.interact_pointer_pos() { let fine = ui.input(|i| i.modifiers.shift); + let step = spec.step.max(1e-9); let nv = if fine { + // a tenth of the pointer's movement along the track; the unrounded position is kept, so a + // slow drag adds up instead of rounding back to the same step every frame + let fine_id = id.with("fine-pos"); let dx = ui.input(|i| i.pointer.delta().x); - value + dx as f64 * span / track_rect.width() as f64 * 0.1 + let kept = ui.data(|m| m.get_temp::(fine_id)).filter(|&p| ((spec.value_at(p) / step).round() * step - value).abs() < step / 2.0); + let pos = (kept.unwrap_or_else(|| spec.position(value)) + f64::from(dx / track_rect.width()) * 0.1).clamp(0.0, 1.0); + ui.data_mut(|m| m.insert_temp(fine_id, pos)); + spec.value_at(pos) } else { from_x(p.x) }; - let step = spec.step.max(1e-9); let nv = ((nv / step).round() * step).clamp(spec.min, spec.max); if (nv - value).abs() > 1e-12 { out.value = Some(nv); @@ -634,6 +640,44 @@ mod tests { assert_eq!(remainder, 0.0); } + /// Shift-drag moves a tenth of the pointer's way along the track, also on the Kelvin track (where a pixel of + /// fine drag near the centre is about 4 K): a pointer creeping a pixel a frame still adds up to steps. + #[test] + fn a_slow_shift_drag_adds_up() { + let ctx = egui::Context::default(); + let temp = lightcraft_develop::controls::find("wb.temp").unwrap(); + let mut value = 7000.0; + let frame = |time: f64, events: Vec, value: f64| { + let input = egui::RawInput { + screen_rect: Some(Rect::from_min_size(pos2(0.0, 0.0), vec2(320.0, 120.0))), + time: Some(time), + events, + ..Default::default() + }; + let mut out = SliderOut::default(); + ctx.run_ui(input, |ui| out = slider(ui, temp, value, true, None)).textures_delta.clear(); + let track = take_registry(&ctx).into_iter().find(|(id, _)| id == "slider:wb.temp").unwrap().1; + (out, track) + }; + // Shift is held from the start (the modifiers stay until they change) + let (_, track) = frame(0.0, vec![egui::Event::ModifiersChanged(egui::Modifiers::SHIFT)], value); + let mut at = pos2(track.left() + temp.position(value) as f32 * track.width(), track.center().y); + let press = + |pos, pressed| egui::Event::PointerButton { pos, button: egui::PointerButton::Primary, pressed, modifiers: egui::Modifiers::SHIFT }; + frame(0.1, vec![egui::Event::PointerMoved(at), press(at, true)], value); + let moves = 60; + for i in 0..moves { + at.x += 1.0; + let (out, _) = frame(0.2 + f64::from(i) * 0.02, vec![egui::Event::PointerMoved(at)], value); + value = out.value.unwrap_or(value); + } + frame(2.0, vec![press(at, false)], value); + // the drag starts once the pointer has moved a few pixels; every pixel after that counts + let lo = temp.value_at(temp.position(7000.0) + f64::from(moves - 10) / f64::from(track.width()) * 0.1); + let hi = temp.value_at(temp.position(7000.0) + f64::from(moves) / f64::from(track.width()) * 0.1); + assert!(value >= lo - 50.0 && value <= hi + 50.0 && value > 7000.0, "{value} K, expected {lo:.0}–{hi:.0} K"); + } + /// Typed values (issue #322): signs, a decimal comma, units; on the control's steps, in range. #[test] fn typed_values_are_stepped_and_clamped() { diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 77ae26e4c..e5921ad9d 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -190,7 +190,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`) | 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); raws without matrices anywhere and rendered files use the relative −100..+100 scale (6500 K / 0 = as shot). 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 | +| 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`) | 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); raws without matrices anywhere and rendered files use the relative −100..+100 scale (6500 K / 0 = as shot). 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 | | 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` | |