From ca5c212d03e007372bdca2b25caf7382c4bfaa0a Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Thu, 8 Oct 2026 12:37:15 -0400 Subject: [PATCH] #323: review fixes: prefetch and stand-ins stay at preview level; render sizes move in 64 px steps An adversarial review found that with Automatic the neighbour prefetch asked for the full source level, whose single cache slot then evicted the open photo's source, and that hover / Before renders and every window-resize pixel produced full-size renders. Prefetch and stand-ins are now capped at the preview source level and the loupe size is rounded up to a 64 px step. Co-Authored-By: Claude Sonnet 5.5 --- crates/ui-egui/src/panels/detail.rs | 9 +++++--- crates/ui-egui/src/state.rs | 22 ++++++++++++++++-- crates/ui-egui/src/tests_preview_limit.rs | 27 ++++++++++++++++++++--- 3 files changed, 50 insertions(+), 8 deletions(-) diff --git a/crates/ui-egui/src/panels/detail.rs b/crates/ui-egui/src/panels/detail.rs index ca74d623..7810b95e 100644 --- a/crates/ui-egui/src/panels/detail.rs +++ b/crates/ui-egui/src/panels/detail.rs @@ -285,6 +285,9 @@ pub fn show(app: &mut LightcraftApp, ui: &mut egui::Ui) { let native_long = native[0].max(native[1]); let want = app.ui.settings.loupe_edge(target_rect.width().max(target_rect.height()) * ppp * scale, native_long); let (rw, rh) = if aspect >= 1.0 { (want, (want as f32 / aspect) as usize) } else { ((want as f32 * aspect) as usize, want) }; + // hover and before renders are stand-ins: the preview size is plenty and keeps them cheap + let se = app.ui.settings.stand_in_edge(want); + let (sw, sh) = if aspect >= 1.0 { (se, (se as f32 / aspect) as usize) } else { ((se as f32 * aspect) as usize, se) }; if let Some(job) = app.session.loupe_job(id, rw.max(8), rh.max(8), !crop_tool) { let job = if interacting { job.draft() } else { job }; let job = job.with_overlay(view_overlay(app, &d)).with_proof(app.ui.soft_proof.then_some(app.ui.proof)); @@ -293,7 +296,7 @@ pub fn show(app: &mut LightcraftApp, ui: &mut egui::Ui) { // hovering a preset or profile: the photo with that look, shown instead of the loupe render // once it is ready (nothing is committed) let hover_key = match app.hover_preview.clone() { - Some(h) if !interacting => app.session.preview_job(id, rw.max(8), rh.max(8), !crop_tool, &h.settings).map(|job| { + Some(h) if !interacting => app.session.preview_job(id, sw.max(8), sh.max(8), !crop_tool, &h.settings).map(|job| { let key = job.key; app.renderer.request(Slot::Hover, job, 105); (key, h.label) @@ -313,7 +316,7 @@ pub fn show(app: &mut LightcraftApp, ui: &mut egui::Ui) { let nf = Frame::with_lens(np.width.max(1) as usize, np.height.max(1) as usize, &np.develop, !crop_tool, np.embedded_lens.as_ref()); let na = nf.aspect() as f32; let nr = fit_rect(main_area, na, app.ui.zoom, [np.width.max(1) as usize, np.height.max(1) as usize], ppp, app.ui.pan); - let nw = app.ui.settings.loupe_edge(nr.width().max(nr.height()) * ppp, np.width.max(np.height).max(1) as usize); + let nw = app.ui.settings.prefetch_edge(nr.width().max(nr.height()) * ppp, np.width.max(np.height).max(1) as usize); let (w, h) = if na >= 1.0 { (nw, (nw as f32 / na) as usize) } else { ((nw as f32 * na) as usize, nw) }; if let Some(job) = app.session.loupe_job(nid, w.max(8), h.max(8), !crop_tool) { app.renderer.prefetch(Slot::Prefetch(n as u8), job, PREFETCH_PRIORITY); @@ -329,7 +332,7 @@ pub fn show(app: &mut LightcraftApp, ui: &mut egui::Ui) { } let show_before = app.ui.before_after == BeforeAfter::Original || ui.input(|i| i.key_down(egui::Key::Backslash)); if (split || show_before || split_view) - && let Some(job) = app.session.render_job(id, rw.max(8), rh.max(8), true, !crop_tool) + && let Some(job) = app.session.render_job(id, sw.max(8), sh.max(8), true, !crop_tool) { app.renderer.request(Slot::Before, job, 90); } diff --git a/crates/ui-egui/src/state.rs b/crates/ui-egui/src/state.rs index e24c6279..589d58e6 100644 --- a/crates/ui-egui/src/state.rs +++ b/crates/ui-egui/src/state.rs @@ -182,16 +182,34 @@ pub const PREVIEW_LIMITS: [u32; 5] = [0, 1600, 2560, 3840, 5120]; /// The most the loupe ever renders in one go (memory and GPU texture size), whatever the setting. pub const LOUPE_EDGE_CEILING: u32 = 8192; +/// Loupe render sizes move in steps of this many pixels. +pub const LOUPE_EDGE_STEP: usize = 64; + /// Long edge of Build Standard-Sized Previews when the limit is Automatic. pub const STANDARD_PREVIEW_EDGE: u32 = 2560; impl AppSettings { /// Long edge (pixels) to render the loupe at when it is drawn `wanted_px` wide on screen: - /// that size, never above the photo's own `native_long_edge`, the user's limit or the ceiling. + /// that size (rounded up to [`LOUPE_EDGE_STEP`], so resizing a window doesn't re-render at + /// every pixel), never above the photo's own `native_long_edge`, the user's limit or the ceiling. pub fn loupe_edge(&self, wanted_px: f32, native_long_edge: usize) -> usize { let ceiling = if self.preview_limit == 0 { LOUPE_EDGE_CEILING } else { self.preview_limit.clamp(512, LOUPE_EDGE_CEILING) } as usize; let wanted = if wanted_px.is_nan() { 8.0 } else { wanted_px.clamp(8.0, ceiling as f32) } as usize; - wanted.min(native_long_edge.max(8)).min(ceiling) + let stepped = wanted.div_ceil(LOUPE_EDGE_STEP) * LOUPE_EDGE_STEP; + stepped.min(native_long_edge.max(8)).min(ceiling) + } + + /// Long edge for warming a neighbouring photo: its loupe size but never above the preview + /// source level. A full-size decode would replace the open photo's single full-resolution + /// source in the cache, and nothing is gained by it. + pub fn prefetch_edge(&self, wanted_px: f32, native_long_edge: usize) -> usize { + self.loupe_edge(wanted_px, native_long_edge).min(lightcraft_engine::SourceLevel::Preview.max_edge()) + } + + /// Long edge for the hover (preset / profile) and Before renders, which are stand-ins drawn + /// over the loupe: capped at the preview source level like the old default. + pub fn stand_in_edge(&self, loupe_edge: usize) -> usize { + loupe_edge.min(lightcraft_engine::SourceLevel::Preview.max_edge()) } /// The edge Build Standard-Sized Previews uses. diff --git a/crates/ui-egui/src/tests_preview_limit.rs b/crates/ui-egui/src/tests_preview_limit.rs index 53476886..1fb32a0e 100644 --- a/crates/ui-egui/src/tests_preview_limit.rs +++ b/crates/ui-egui/src/tests_preview_limit.rs @@ -24,7 +24,28 @@ fn automatic_renders_a_one_to_one_view_at_native_size() { // Given Automatic, a Retina fit view of 3024 px is not held to the old 2560 px cap #[test] fn automatic_does_not_cap_a_fit_view_at_the_old_default() { - assert_eq!(with_limit(0).loupe_edge(3024.0, 6000), 3024); + assert_eq!(with_limit(0).loupe_edge(3024.0, 6000), 3072, "rounded up to the next 64 px step"); +} + +// Dragging a window edge must not re-render (and re-key the caches) on every pixel +#[test] +fn render_sizes_move_in_steps() { + let s = with_limit(0); + assert_eq!(s.loupe_edge(2945.0, 6000), s.loupe_edge(3008.0, 6000)); + assert_ne!(s.loupe_edge(3008.0, 6000), s.loupe_edge(3009.0, 6000)); +} + +// Neighbour prefetch and hover / before stand-ins stay at the preview level: a full-size decode +// would evict the open photo's single full-resolution source (review of #323) +#[test] +fn prefetch_and_stand_ins_never_ask_for_the_full_source_level() { + use lightcraft_engine::SourceLevel; + let s = with_limit(0); + for wanted in [1000.0, 2560.0, 3024.0, 6000.0, 48000.0] { + assert!(s.prefetch_edge(wanted, 6000) <= SourceLevel::Preview.max_edge(), "{wanted}"); + assert!(s.stand_in_edge(s.loupe_edge(wanted, 6000)) <= SourceLevel::Preview.max_edge()); + } + assert_eq!(s.prefetch_edge(1000.0, 6000), 1024, "a small view is not raised"); } // Given Automatic, a small photo zoomed far in is not rendered above its own pixels @@ -43,7 +64,7 @@ fn automatic_is_bounded_by_the_ceiling() { #[test] fn an_explicit_limit_caps_the_render() { assert_eq!(with_limit(1600).loupe_edge(6000.0, 6000), 1600); - assert_eq!(with_limit(1600).loupe_edge(900.0, 6000), 900, "and does not raise a small view"); + assert_eq!(with_limit(1600).loupe_edge(900.0, 6000), 960, "and does not raise a small view past its step"); } // Whatever the inputs (hostile or degenerate), the result is a usable size @@ -54,7 +75,7 @@ fn degenerate_inputs_give_a_usable_size() { let e = s.loupe_edge(wanted, 6000); assert!((8..=LOUPE_EDGE_CEILING as usize).contains(&e), "{wanted}: {e}"); } - assert!((8..=LOUPE_EDGE_CEILING as usize).contains(&s.loupe_edge(1000.0, 0))); + assert!((8..=LOUPE_EDGE_CEILING as usize).contains(&s.loupe_edge(1000.0, 0)) && s.prefetch_edge(f32::NAN, 0) >= 8); assert!(with_limit(7).loupe_edge(1000.0, 6000) >= 8, "a corrupt limit is clamped"); }