shifathosn
/
lightcraft
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

#323: wipe, pane clipping and identical-looks fixes for the Before/After windows 

An adversarial review found that (1) in a wipe the Before window was drawn
before, so under, the low-resolution Before stand-in; (2) the windows of side by
side and stacked views were clipped to the canvas, so their margins spilled into
the neighbouring pane; (3) with identical Before and After looks (a fresh,
unedited photo) both windows had the same job key and the After tile replaced the
Before one in the tile table. Windows are now drawn over the stand-ins and under
the wipe line, clipped to their pane or their side of the line (region::window_clip,
pure and tested), and tiles are keyed by side and job key.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This commit is contained in:
Nathaniel Ledford 2026-10-08 17:40:16 -04:00
parent eb06b65fdb
commit 8a5ecebc18
No known key found for this signature in database
4 changed files with 107 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
crates/ui-egui/src/lib.rs
View File

@ -207,7 +207,10 @@ pub struct LightcraftApp {
pub region_before_view: Option<region::RegionView>,
/// The loupe's render sizes while a pinch or two-finger scroll runs.
pub(crate) size_hold: region::SizeHold,
pub(crate) region_tiles: std::collections::HashMap<u64, region::RegionView>,
/// What the loupe drew last frame, in order (tests check layering).
#[cfg(test)]
pub(crate) draw_order: Vec<&'static str>,
pub(crate) region_tiles: std::collections::HashMap<(bool, u64), region::RegionView>,
/// Photo Merge dialog previews and background merges.
pub merge: merge::MergeState,
/// An import in progress (the import review dialog's batches).
@ -278,6 +281,8 @@ impl LightcraftApp {
region_view: None,
region_before_view: None,
size_hold: Default::default(),
#[cfg(test)]
draw_order: Vec::new(),
region_tiles: Default::default(),
merge: merge::MergeState::default(),
import: None,

62
crates/ui-egui/src/panels/detail.rs
View File

@ -267,8 +267,8 @@ fn request_window(app: &mut LightcraftApp, c: &WindowCtx, v: &WindowView) -> Opt
let look = job.settings.hash64();
let view = crate::region::RegionView { photo: c.id, before: v.before, key: job.key, full: (fw, fh), window: win, settings: look };
// windows of other photos, looks and zooms of this side can't be drawn any more
app.region_tiles.retain(|_, t| t.before != v.before || t.is_current(c.id, (fw, fh), look, c.interacting));
app.region_tiles.insert(job.key, view);
app.region_tiles.retain(|(before, _), t| *before != v.before || t.is_current(c.id, (fw, fh), look, c.interacting));
app.region_tiles.insert((v.before, job.key), view);
app.renderer.request(v.slot, job, 99);
Some(view)
}
@ -277,9 +277,7 @@ fn request_window(app: &mut LightcraftApp, c: &WindowCtx, v: &WindowView) -> Opt
/// drag, an earlier draft of it).
fn draw_window(p: &egui::Painter, app: &LightcraftApp, c: &WindowCtx, v: &WindowView, wanted: &crate::region::RegionView) {
let Some(tex) = app.renderer.textures.get(&v.slot).filter(|t| t.photo == c.id) else { return };
let Some(tile) =
app.region_tiles.get(&tex.key).filter(|t| t.before == v.before && t.is_current(c.id, wanted.full, wanted.settings, c.interacting))
else {
let Some(tile) = app.region_tiles.get(&(v.before, tex.key)).filter(|t| t.is_current(c.id, wanted.full, wanted.settings, c.interacting)) else {
return;
};
let (fw, fh) = (tile.full.0 as f32, tile.full.1 as f32);
@ -487,7 +485,7 @@ pub fn show(app: &mut LightcraftApp, ui: &mut egui::Ui) {
app.region_view = shown;
}
if shown.is_none() {
app.region_tiles.retain(|_, v| v.before != before);
app.region_tiles.retain(|(b, _), _| *b != before);
app.renderer.release(slot);
}
}
@ -555,35 +553,43 @@ pub fn show(app: &mut LightcraftApp, ui: &mut egui::Ui) {
}
}
}
// the window renders, drawn over the whole-frame ones where they belong in the frame (the
// Before side of a wipe only on its side of the line)
for (v, view) in &windows {
let clip = match (app.ui.before_after, v.before) {
(BeforeAfter::Split, true) => Rect::from_min_max(canvas.min, pos2(img_rect.center().x, canvas.bottom())),
(BeforeAfter::SplitTopBottom, true) => Rect::from_min_max(canvas.min, pos2(canvas.right(), img_rect.center().y)),
(BeforeAfter::Split, false) => Rect::from_min_max(pos2(img_rect.center().x, canvas.top()), canvas.max),
(BeforeAfter::SplitTopBottom, false) => Rect::from_min_max(pos2(canvas.left(), img_rect.center().y), canvas.max),
_ => canvas,
};
draw_window(&p.with_clip_rect(clip.intersect(canvas)), app, &window_ctx, v, view);
}
app.loupe_shown = Some((id, shown));
#[cfg(test)]
app.draw_order.clear();
// a wipe shows the Before whole-frame render on its side of the line…
if app.ui.before_after == BeforeAfter::Split
&& let Some(tex) = app.renderer.textures.get(&Slot::Before).filter(|t| t.photo == id)
{
let before_rect = fit_texture_rect(img_rect, tex.size);
let left = Rect::from_min_max(before_rect.min, pos2(before_rect.center().x, before_rect.bottom()));
p.image(tex.tex.id(), left, Rect::from_min_max(pos2(0.0, 0.0), pos2(0.5, 1.0)), Color32::WHITE);
}
if app.ui.before_after == BeforeAfter::SplitTopBottom
&& let Some(tex) = app.renderer.textures.get(&Slot::Before).filter(|t| t.photo == id)
{
let before_rect = fit_texture_rect(img_rect, tex.size);
let top = Rect::from_min_max(before_rect.min, pos2(before_rect.right(), before_rect.center().y));
p.image(tex.tex.id(), top, Rect::from_min_max(pos2(0.0, 0.0), pos2(1.0, 0.5)), Color32::WHITE);
}
#[cfg(test)]
if matches!(app.ui.before_after, BeforeAfter::Split | BeforeAfter::SplitTopBottom) {
app.draw_order.push("wipe stand-in");
}
// …and the window renders go over the whole-frame ones, all of them, where they belong in the
// frame (clipped to their pane, or their side of the line)
for (v, view) in &windows {
let clip = crate::region::window_clip(app.ui.before_after, v.before, v.visible, canvas, img_rect);
draw_window(&p.with_clip_rect(clip), app, &window_ctx, v, view);
#[cfg(test)]
app.draw_order.push(if v.before { "window RegionBefore" } else { "window Region" });
}
// the line of a wipe over both
if app.ui.before_after == BeforeAfter::Split {
let mid = img_rect.center().x;
if let Some(tex) = app.renderer.textures.get(&Slot::Before).filter(|t| t.photo == id) {
let before_rect = fit_texture_rect(img_rect, tex.size);
let left = Rect::from_min_max(before_rect.min, pos2(before_rect.center().x, before_rect.bottom()));
p.image(tex.tex.id(), left, Rect::from_min_max(pos2(0.0, 0.0), pos2(0.5, 1.0)), Color32::WHITE);
}
p.line_segment([pos2(mid, img_rect.top()), pos2(mid, img_rect.bottom())], Stroke::new(1.5, Color32::WHITE));
}
if app.ui.before_after == BeforeAfter::SplitTopBottom {
let mid = img_rect.center().y;
if let Some(tex) = app.renderer.textures.get(&Slot::Before).filter(|t| t.photo == id) {
let before_rect = fit_texture_rect(img_rect, tex.size);
let top = Rect::from_min_max(before_rect.min, pos2(before_rect.right(), before_rect.center().y));
p.image(tex.tex.id(), top, Rect::from_min_max(pos2(0.0, 0.0), pos2(1.0, 0.5)), Color32::WHITE);
}
p.line_segment([pos2(img_rect.left(), mid), pos2(img_rect.right(), mid)], Stroke::new(1.5, Color32::WHITE));
}
if app.ui.show_clipping && !show_before && !fullscreen {

38
crates/ui-egui/src/region.rs
View File

@ -142,6 +142,23 @@ impl SizeHold {
}
}
/// Where a window of one side of a Before/After view may be drawn: its own pane in side by side
/// and stacked views (a window carries margin beyond what its pane shows, which would otherwise
/// spill into the neighbour), its side of the line in a wipe, else the canvas.
pub fn window_clip(mode: crate::state::BeforeAfter, before_side: bool, pane: egui::Rect, canvas: egui::Rect, img: egui::Rect) -> egui::Rect {
use crate::state::BeforeAfter;
let (min, max) = (canvas.min, canvas.max);
let clip = match (mode, before_side) {
(BeforeAfter::Split, true) => egui::Rect::from_min_max(min, egui::pos2(img.center().x, max.y)),
(BeforeAfter::SplitTopBottom, true) => egui::Rect::from_min_max(min, egui::pos2(max.x, img.center().y)),
(BeforeAfter::Split, false) => egui::Rect::from_min_max(egui::pos2(img.center().x, min.y), max),
(BeforeAfter::SplitTopBottom, false) => egui::Rect::from_min_max(egui::pos2(min.x, img.center().y), max),
(BeforeAfter::SideBySide | BeforeAfter::TopBottom, _) => pane,
_ => canvas,
};
clip.intersect(canvas)
}
/// Whether the picture on screen has the frame's aspect (to the one pixel a texture's whole-pixel
/// size is off by): a window is a part of the frame, so it can only be placed over a picture that
/// is the frame (an unsupported raw's embedded JPEG may be cropped differently).
@ -313,6 +330,27 @@ mod tests {
assert_eq!(hold.apply(PhotoId(2), 0.1, true, zoomed), zoomed);
}
// Given side by side, a window is drawn in its pane only; given a wipe, on its side of the line
#[test]
fn a_window_is_clipped_to_its_pane_or_its_side_of_the_wipe() {
use crate::state::BeforeAfter::*;
let canvas = egui::Rect::from_min_max(egui::pos2(0.0, 0.0), egui::pos2(1000.0, 600.0));
let (left, right) = (
egui::Rect::from_min_max(egui::pos2(24.0, 24.0), egui::pos2(494.0, 576.0)),
egui::Rect::from_min_max(egui::pos2(506.0, 24.0), egui::pos2(976.0, 576.0)),
);
assert_eq!(window_clip(SideBySide, true, left, canvas, left), left);
assert_eq!(window_clip(SideBySide, false, right, canvas, right), right);
let img = egui::Rect::from_min_max(egui::pos2(100.0, 50.0), egui::pos2(900.0, 550.0));
let before = window_clip(Split, true, canvas, canvas, img);
let after = window_clip(Split, false, canvas, canvas, img);
assert_eq!((before.max.x, after.min.x), (500.0, 500.0), "the line is the image's middle");
assert_eq!(window_clip(SplitTopBottom, true, canvas, canvas, img).max.y, 300.0);
assert_eq!(window_clip(SplitTopBottom, false, canvas, canvas, img).min.y, 300.0);
assert_eq!(window_clip(Off, false, canvas, canvas, img), canvas);
assert_eq!(window_clip(Original, true, canvas, canvas, img), canvas);
}
// Hostile numbers give a plan, never a panic
#[test]
fn hostile_sizes_give_a_usable_plan() {

29
crates/ui-egui/src/tests_preview_limit.rs
View File

@ -526,4 +526,33 @@ mod in_the_loupe {
assert!(h.app.renderer.textures.contains_key(&Slot::Region), "a window arrived");
assert!(h.app.session.memory_report().full_source.bytes > 0, "the original stayed in the cache while the drag went on");
}
// Given a wipe at 1:1, the Before window is drawn after (over) the low-resolution Before
// stand-in on its side of the line, never under it
#[test]
fn a_wipe_draws_the_before_window_over_its_stand_in() {
for mode in [crate::state::BeforeAfter::Split, crate::state::BeforeAfter::SplitTopBottom] {
let (mut h, _) = detail();
h.app.ui.before_after = mode;
h.request("engine.execute", json!({"command": "view.zoom100"}), T);
h.settle(SETTLE);
h.step();
let order = &h.app.draw_order;
let pos = |what: &str| order.iter().position(|d| *d == what);
let (stand_in, before, after) = (pos("wipe stand-in"), pos("window RegionBefore"), pos("window Region"));
assert!(stand_in.is_some() && before.is_some() && after.is_some(), "{mode:?}: {order:?}");
assert!(before > stand_in, "{mode:?}: the Before window is under its stand-in: {order:?}");
}
}
// Given a Before and an After with the same look (an unedited photo), both windows are kept
// apart: the job key is the same, and one tile must not replace the other
#[test]
fn identical_before_and_after_keep_a_tile_each() {
let (mut h, _) = detail();
h.app.ui.before_after = crate::state::BeforeAfter::SideBySide;
h.request("engine.execute", json!({"command": "view.zoom100"}), T);
h.settle(SETTLE);
assert_eq!(h.app.region_tiles.len(), 2, "{:?}", h.app.region_tiles.keys().collect::<Vec<_>>());
}
}