From 86f4bde867d1ffa44519b23d39733eac015e6e52 Mon Sep 17 00:00:00 2001 From: Matthew James Sivak Date: Mon, 12 Oct 2026 13:53:24 +0200 Subject: [PATCH] LR-VIEW-CLIPPING: Alt-drag previews keep the histogram on profiled displays; NR previews (#937) Folds the parts of #995 that #999 lacked into #999: - engine/media.rs: with a display profile, a diagnostic view keeps the photo's histogram (taken before the overlay is drawn) and stays out of the view cache; the clipping previews test sRGB values and are converted for the display afterwards. Before, holding Alt on a P3 display swapped the histogram for the clipping view's (mean luma 0.59 -> 0.999). - Noise Reduction Luminance / Detail / Contrast show greyscale while Alt-dragged (Overlay::for_control). - Tests: diagnostic_views_keep_the_photos_histogram (P3 display; fails without the media.rs change), an end-to-end headless drag that presses and releases Alt mid-drag and checks the loupe pixels (tests_alt_preview.rs), and every Alt-mapped id is a real control. - Docs: NR row, What's New and ui-parity note; parity summary refreshed. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/display.rs | 19 +++++++ crates/engine/src/media.rs | 15 +++++- crates/pipeline/src/visualize.rs | 31 ++++++++++- crates/ui-egui/src/lib.rs | 2 + crates/ui-egui/src/tests_alt_preview.rs | 71 +++++++++++++++++++++++++ docs/parity-checklist.md | 6 +-- docs/ui-parity.md | 7 +-- docs/whats-new.md | 3 +- 8 files changed, 145 insertions(+), 9 deletions(-) create mode 100644 crates/ui-egui/src/tests_alt_preview.rs diff --git a/crates/engine/src/display.rs b/crates/engine/src/display.rs index f47e099b..34840e46 100644 --- a/crates/engine/src/display.rs +++ b/crates/engine/src/display.rs @@ -152,6 +152,25 @@ mod tests { } } + /// Alt-drag previews on a profiled display (issue #937): the histogram stays the photo's (not + /// the preview's), and the clipping view is tested and drawn in sRGB, so it stays black and white. + #[test] + fn diagnostic_views_keep_the_photos_histogram() { + use lightcraft_pipeline::Overlay; + let mut session = crate::Session::with_demo(); + let id = session.visible_cloned()[0]; + let d = std::sync::Arc::new(p3()); + let mut run = |o| session.loupe_job(id, 320, 240, true).unwrap().with_display(Some(d.clone())).with_overlay(o).run().rendered.unwrap(); + let plain = run(Overlay::None); + let clip = run(Overlay::ClipShadows); + let (a, b) = (plain.histogram.mean_luma(), clip.histogram.mean_luma()); + assert!((a - b).abs() < 0.02, "the photo's histogram: {a} vs {b}"); + let px = &clip.image.data; + // (the profile may round white to 254) + let binary = px.iter().filter(|p| p[..3].iter().all(|v| *v <= 2 || *v >= 253)).count(); + assert!(binary as f64 > 0.99 * px.len() as f64, "{binary} of {}", px.len()); + } + #[test] fn load_reports_and_rejects() { let dir = std::env::temp_dir().join(format!("lightcraft-display-{}", std::process::id())); diff --git a/crates/engine/src/media.rs b/crates/engine/src/media.rs index 6f6f0359..cd26f222 100644 --- a/crates/engine/src/media.rs +++ b/crates/engine/src/media.rs @@ -838,11 +838,24 @@ impl RenderJob { // Thumbnails (many small jobs side by side) stay on the CPU; views and exports use // the GPU when there is one. let gpu = self.cache.is_none(); - let (mut rendered, gpu_fallback) = develop_reporting(src, &info, &self.settings, &self.request, self.stages.as_deref(), gpu); + // a clipping preview tests sRGB values, like the histogram: rendered in sRGB, then + // converted for the display below + let clip_view = self.request.overlay.is_clipping(); + let request = if clip_view { RenderRequest { display: None, ..self.request } } else { self.request }; + let (mut rendered, gpu_fallback) = develop_reporting(src, &info, &self.settings, &request, self.stages.as_deref(), gpu); // for a display profile: the image goes to the display's values, the histogram // and the cached view preview stay sRGB let mut srgb = None; let display = match (&self.display, self.request.display) { + // a diagnostic view: the histogram stays the photo's (taken before the overlay + // was drawn), and nothing goes to the view cache + (Some(d), Some(_)) if self.request.overlay != lightcraft_pipeline::Overlay::None => { + let converted = if clip_view { d.profile.from_srgb(&mut rendered.image) } else { d.profile.correct(&mut rendered.image) }; + if let Err(e) = converted { + log::warn!("display profile: {e}"); + } + Some(d.id()) + } (Some(d), Some(_)) => { let keep = self.view_cache.is_some() && self.request.quality == Quality::Full; let (s, h) = crate::display::finish_render(&mut rendered.image, d, keep); diff --git a/crates/pipeline/src/visualize.rs b/crates/pipeline/src/visualize.rs index baa992fd..50b95933 100644 --- a/crates/pipeline/src/visualize.rs +++ b/crates/pipeline/src/visualize.rs @@ -185,13 +185,19 @@ impl Overlay { match id { "light.exposure" | "light.highlights" | "light.whites" => Some(Overlay::ClipHighlights), "light.shadows" | "light.blacks" => Some(Overlay::ClipShadows), - "detail.sharpenAmount" => Some(Overlay::Grayscale), + "detail.sharpenAmount" | "detail.nrLuminance" | "detail.nrDetail" | "detail.nrContrast" => Some(Overlay::Grayscale), "detail.sharpenRadius" | "detail.sharpenDetail" => Some(Overlay::SharpenDetail), "detail.sharpenMasking" => Some(Overlay::SharpenMask), _ => None, } } + /// The clipping previews, which test the sRGB values the histogram counts (a render for a + /// display profile draws them in sRGB, then converts). + pub fn is_clipping(self) -> bool { + matches!(self, Overlay::ClipShadows | Overlay::ClipHighlights) + } + /// Whether the overlay draws from the render's detail planes ([`sharpen_plane`]). pub fn reads_detail(self) -> bool { matches!(self, Overlay::SharpenDetail | Overlay::SharpenMask) @@ -601,7 +607,30 @@ mod tests { assert_eq!(Overlay::for_control("detail.sharpenAmount"), Some(Overlay::Grayscale)); assert_eq!(Overlay::for_control("detail.sharpenDetail"), Some(Overlay::SharpenDetail)); assert_eq!(Overlay::for_control("detail.sharpenMasking"), Some(Overlay::SharpenMask)); + for c in ["detail.nrLuminance", "detail.nrDetail", "detail.nrContrast"] { + assert_eq!(Overlay::for_control(c), Some(Overlay::Grayscale), "{c}"); + } + assert_eq!(Overlay::for_control("detail.nrColor"), None); assert_eq!(Overlay::for_control("light.contrast"), None); + // every mapped id is a real control (a rename would silently drop its preview) + for c in [ + "light.exposure", + "light.highlights", + "light.whites", + "light.shadows", + "light.blacks", + "detail.sharpenAmount", + "detail.sharpenRadius", + "detail.sharpenDetail", + "detail.sharpenMasking", + "detail.nrLuminance", + "detail.nrDetail", + "detail.nrContrast", + ] { + assert!(lightcraft_develop::controls::find(c).is_some(), "{c}"); + assert!(Overlay::for_control(c).is_some(), "{c}"); + } + assert!(Overlay::ClipShadows.is_clipping() && Overlay::ClipHighlights.is_clipping() && !Overlay::Grayscale.is_clipping()); let all = [Overlay::ClipShadows, Overlay::ClipHighlights, Overlay::Grayscale, Overlay::SharpenDetail, Overlay::SharpenMask]; for o in all { let (k, v) = o.to_parts(); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 5dc96512..be0b6653 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -44,6 +44,8 @@ mod tests_activity; #[cfg(test)] mod tests_album_picker; #[cfg(test)] +mod tests_alt_preview; +#[cfg(test)] mod tests_appearance; #[cfg(test)] mod tests_before_after; diff --git a/crates/ui-egui/src/tests_alt_preview.rs b/crates/ui-egui/src/tests_alt_preview.rs new file mode 100644 index 00000000..7a212869 --- /dev/null +++ b/crates/ui-egui/src/tests_alt_preview.rs @@ -0,0 +1,71 @@ +//! Headless test of the Alt-drag previews (issue #937) through real input: a slider drag, Alt +//! (Option) pressed and released during it, and the loupe's pixels at each step. + +use std::time::Duration; + +use egui::{Event, Modifiers, PointerButton}; +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); + +fn detail() -> Headless { + let services = Services { png: None, ..Default::default() }; + let app = LightcraftApp::new(lightcraft_engine::Session::with_demo(), services); + let mut h = Headless::new(app, [1400.0, 1400.0], 1.0); + let r = h.request("ui.set", json!({"view": "detail", "right": "edit"}), T); + assert_eq!(r["ok"], true, "{r}"); + if !h.app.ui.section_open("light") { + h.app.ui.toggle_section("light"); + } + h.app.renderer.keep_pixels = true; + h.settle(SETTLE); + h +} + +/// Share of the loupe render's pixels whose channels are all 0 or 255 (a clipping view). +fn binary_share(h: &Headless) -> f64 { + let t = h.app.renderer.textures.get(&crate::render::Slot::Main).expect("loupe render"); + let px = &t.pixels.as_ref().expect("pixels kept").pixels; + let binary = px.iter().filter(|c| [c.r(), c.g(), c.b()].iter().all(|v| *v == 0 || *v == 255)).count(); + binary as f64 / px.len().max(1) as f64 +} + +#[test] +fn alt_during_a_blacks_drag_shows_shadow_clipping_until_released() { + let mut h = detail(); + assert!(binary_share(&h) < 0.5, "the photo itself is no clipping view"); + let track = h.app.widgets.iter().find(|(w, _)| w == "slider:light.blacks").map(|(_, r)| *r).expect("Blacks slider on screen"); + let from = track.center(); + let before = h.app.session.develop_of(h.app.session.active().expect("photo")).expect("develop").light.blacks; + // a plain drag to the left, button held: the photo as usual + h.app.synthetic.push(Event::PointerMoved(from)); + h.app.synthetic.push(Event::PointerButton { pos: from, button: PointerButton::Primary, pressed: true, modifiers: Modifiers::NONE }); + for i in 1..=8 { + h.app.synthetic.push(Event::PointerMoved(from - egui::vec2(6.0 * i as f32, 0.0))); + } + h.settle(SETTLE); + assert_eq!(h.app.ui.dragging_control.as_deref(), Some("light.blacks")); + assert!(binary_share(&h) < 0.5, "no preview without Alt"); + // Alt pressed mid-drag: the shadow clipping view + h.app.synthetic.push(Event::ModifiersChanged(Modifiers::ALT)); + h.settle(SETTLE); + assert!(binary_share(&h) > 0.99, "the clipping view: {}", binary_share(&h)); + // Alt released: the photo again, the drag still on + h.app.synthetic.push(Event::ModifiersChanged(Modifiers::NONE)); + h.settle(SETTLE); + assert!(binary_share(&h) < 0.5, "the photo is back"); + h.app.synthetic.push(Event::PointerButton { + pos: from - egui::vec2(48.0, 0.0), + button: PointerButton::Primary, + pressed: false, + modifiers: Modifiers::NONE, + }); + h.settle(SETTLE); + assert_eq!(h.app.ui.dragging_control, None); + let after = h.app.session.develop_of(h.app.session.active().expect("photo")).expect("develop").light.blacks; + assert!(after < before, "the drag itself still moved Blacks: {before} โ†’ {after}"); +} diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 75d2aed2..cfd9248d 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -53,9 +53,9 @@ cited here still exists. Formerly docs/parity.md. | Y. Menus | 84 | 0 | 4 | 8 | 47/47 (100%) | 23/23 (100%) | | Z. Keyboard shortcuts (desktop) | 76 | 3 | 3 | 1 | 49/52 (94%) | 23/24 (96%) | | Lightroom Classic extras | 34 | 16 | 35 | 8 | โ€” | 24/26 (92%) | -| **Total** | 406 | 51 | 73 | 35 | 191/200 (96%) | 145/157 (92%) | +| **Total** | 407 | 52 | 72 | 35 | 192/201 (96%) | 145/157 (92%) | -Weighted completion (โœ… = 1, ๐ŸŸก = ยฝ, ๐Ÿšซ left out): **81.4%** of 530 in-scope rows โ€” P0 97.8% of 200 ยท P1 95.2% of 157 ยท P2 49.7% of 172. +Weighted completion (โœ… = 1, ๐ŸŸก = ยฝ, ๐Ÿšซ left out): **81.5%** of 531 in-scope rows โ€” P0 97.8% of 201 ยท P1 95.2% of 157 ยท P2 50.0% of 172. ## Top gaps and shortcut notes @@ -204,7 +204,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts). | LR-EDIT-EFFECTS-VIGNETTE | Post-crop vignette | P0 | โœ… | `ctl:vignette.*`, `crates/pipeline/src/finish.rs`, `crates/ui-egui/src/panels/edit.rs` | style picker (Highlight / Color / Paint) in the Effects section | | LR-EDIT-EFFECTS-GRAIN | Grain | P1 | โœ… | `ctl:grain.*` | | | LR-EDIT-DETAIL-SHARPEN | Sharpening | P0 | ๐ŸŸก | `ctl:detail.sharpenAmount`, `ctl:detail.sharpenRadius`, `ctl:detail.sharpenDetail`, `ctl:detail.sharpenMasking`, `crates/pipeline/src/visualize.rs` | Amount and Masking work; Radius and Detail are stored (XMP round trip) but do not change the render yet (the band is a fixed 0.18 % of the long edge). Alt/Option-drag previews: Amount greyscale, Radius / Detail the detail band sharpening amplifies, Masking the mask (white = sharpened) | -| LR-EDIT-DETAIL-NR | Luminance noise reduction | P0 | โœ… | `ctl:detail.nrLuminance`, `ctl:detail.nrDetail`, `ctl:detail.nrContrast` | | +| LR-EDIT-DETAIL-NR | Luminance noise reduction | P0 | โœ… | `ctl:detail.nrLuminance`, `ctl:detail.nrDetail`, `ctl:detail.nrContrast` | Alt/Option-drag on Luminance, Detail and Contrast shows the photo in greyscale | | LR-EDIT-DETAIL-CNR | Colour noise reduction | P0 | โœ… | `ctl:detail.nrColor`, `ctl:detail.nrColorDetail`, `ctl:detail.nrColorSmoothness` | | | LR-EDIT-DETAIL-DENOISE | AI denoise | P2 | ๐ŸŸก | cmd:denoise.toggle, cmd:denoise.models.install, cmd:denoise.queue, ctl:enhance.denoise, `crates/denoise/`, `docs/denoise.md` | non-destructive Bayer RAW, opt-in weights, pure-Rust CPU/GPU; GPU convolution uses independently written shared scalars (Metal equivalence regression, issue #479); tiles can also run on another computer's graphics card (`lightcraft-cli denoise-serve`, `crates/denoise/src/remote.rs`); X-Trans/linear RGB, model policy and Lightroom fidelity remain; closing the model setup without installing turns the switch back off (#922) | | LR-EDIT-DETAIL-RAWDETAILS | Improved demosaic toggle | P2 | โฌœ | | | diff --git a/docs/ui-parity.md b/docs/ui-parity.md index 06a728db..6782cc6b 100644 --- a/docs/ui-parity.md +++ b/docs/ui-parity.md @@ -70,9 +70,10 @@ under the UI command that wraps it (a no-op error); the UI command now wins. **Alt/Option-drag previews (issue #937):** hold Alt (Option on macOS) while dragging an Edit slider, before or during the drag. Exposure, Highlights and Whites show highlight clipping (black, clipped channels in their colour, white where all three clip); Shadows and Blacks show shadow clipping (white, black where all three clip). Sharpening Amount shows -greyscale, Radius and Detail the detail band sharpening amplifies, Masking the mask (white = sharpened). Releasing the -key or the slider brings the photo back; the edit and its history are unchanged. Not yet: Noise Reduction, mask range -sliders and the Masking panel's local sliders. On Linux desktops that move windows with Alt+drag (KDE Plasma's +greyscale, Radius and Detail the detail band sharpening amplifies, Masking the mask (white = sharpened); luminance Noise +Reduction (Luminance, Detail, Contrast) shows greyscale. Releasing the key or the slider brings the photo back; the +edit, its history and the histogram are unchanged (on a profiled display too). Not yet: mask range sliders and the +Masking panel's local sliders. On Linux desktops that move windows with Alt+drag (KDE Plasma's default, also some GNOME / Xfce setups) the window manager takes the drag first: set its window-move modifier to Meta. **Still missing / broken:** diff --git a/docs/whats-new.md b/docs/whats-new.md index 5107eae2..8d2329db 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -17,7 +17,8 @@ colour. Shadows and Blacks do the same for clipping to black, on white (issue #937). - In Detail โ–ธ Sharpening the same key shows Amount in greyscale, Radius and Detail as the fine detail being sharpened, and Masking as the mask: white is sharpened, black is protected. Radius and Detail don't change the - render yet, so their preview stays the same while you drag them. + render yet, so their preview stays the same while you drag them. Noise Reduction's Luminance, Detail and Contrast + show the photo in greyscale. - The preview lasts while the key and the slider are held, and never changes the edit, its history or exports. - Linux: KDE Plasma (and some other desktops) move windows with Alt+drag. If Alt+drag moves the window instead, set the window manager's move modifier to Meta.