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 <noreply@anthropic.com>
This commit is contained in:
Matthew James Sivak
parent
77cfb94c2f
commit
86f4bde867
|
|
@ -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()));
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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}");
|
||||
}
|
||||
|
|
@ -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.
|
||||
<!-- /parity:summary -->
|
||||
|
||||
## 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 | ⬜ | | |
|
||||
|
|
|
|||
|
|
@ -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:**
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Reference in New Issue