From bf8d52ca746890718fba0608937c58ceefe4346c Mon Sep 17 00:00:00 2001 From: harrisonoconnorhover <130776285+harrisonoconnorhover@users.noreply.github.com> Date: Thu, 8 Oct 2026 09:25:19 -0400 Subject: [PATCH] Fix Paint Bucket sampling across visible layers --- crates/algo/src/paint.rs | 28 ++- crates/engine/src/paint_cmds.rs | 176 +++++++++++++++++- crates/ui-egui/src/canvas.rs | 2 +- .../ui-egui/tests/paint_bucket_all_layers.rs | 119 ++++++++++++ 4 files changed, 317 insertions(+), 8 deletions(-) create mode 100644 crates/ui-egui/tests/paint_bucket_all_layers.rs diff --git a/crates/algo/src/paint.rs b/crates/algo/src/paint.rs index 489e11f6..9376102e 100644 --- a/crates/algo/src/paint.rs +++ b/crates/algo/src/paint.rs @@ -186,10 +186,23 @@ pub fn bucket_fill_src( let img = crate::selection::rgba8_image(s, area); let Some(region) = crate::selection::wand_region(&img, area, seed, tolerance, contiguous, anti_alias) else { return false }; drop(img); + bucket_fill_region_src(s, ®ion, opacity, selection, lock_transparency, src); + true +} + +/// Paints a bucket region sampled before the edit, so its boundary can come from the visible +/// composite while only `s` receives the colour or pattern. Selection coverage still clips it. +pub fn bucket_fill_region_src( + s: &mut Surface, + region: &crate::selection::Region, + opacity: f32, + selection: Option<&Surface>, + lock_transparency: bool, + src: impl Fn(i32, i32) -> [f32; 4] + Sync, +) { // Composite only over the filled region's box. let b = region.bbox; composite_area(s, b, BlendMode::Normal, |x, y| region.at(x, y) * opacity * selection.map_or(1.0, |m| m.sample_channel(x, y, 0)), src, lock_transparency); - true } #[cfg(test)] @@ -244,4 +257,17 @@ mod tests { assert_eq!(s.pixel(8, 2), vec![1.0, 1.0, 1.0, 1.0]); assert!(!bucket_fill(&mut s, a, (50, 1), 10.0, true, false, [1.0, 0.0, 0.0, 1.0], 1.0, None, false)); } + + #[test] + fn bucket_region_uses_document_coordinates_and_multiplies_coverage() { + let mut s = Surface::new(PixelFormat::RGBA8); + let region = crate::selection::Region { bbox: Rect::new(-3, 4, 0, 5), mask: vec![255, 128, 255] }; + let mut selection = Surface::new(PixelFormat::GRAY8); + selection.fill_rect(Rect::new(-3, 4, -1, 5), &[0.5]); + bucket_fill_region_src(&mut s, ®ion, 0.5, Some(&selection), false, |_, _| [1.0, 0.0, 0.0, 1.0]); + assert!((s.rgba(-3, 4)[3] - 0.25).abs() < 0.005); + assert!((s.rgba(-2, 4)[3] - 0.125).abs() < 0.005); + assert_eq!(s.rgba(-1, 4), [0.0; 4], "outside the selection"); + assert_eq!(s.rgba(-4, 4), [0.0; 4], "outside the region"); + } } diff --git a/crates/engine/src/paint_cmds.rs b/crates/engine/src/paint_cmds.rs index 52918bd9..7366758e 100644 --- a/crates/engine/src/paint_cmds.rs +++ b/crates/engine/src/paint_cmds.rs @@ -1,6 +1,6 @@ //! Paint helpers: Paint Bucket and Gradient tool. -use photocraft_algo::paint::{GradientShape, bucket_fill, bucket_fill_src, paint_gradient}; +use photocraft_algo::paint::{GradientShape, bucket_fill, bucket_fill_region_src, bucket_fill_src, paint_gradient}; use serde_json::{Value, json}; use crate::commands::{CommandSpec, blend_from_str}; @@ -40,6 +40,16 @@ fn bucket(s: &mut Session, p: &Value) -> Result { let c = color(p.get("color"), fg); let (x, y) = (f(p, "x", 0.0).floor() as i32, f(p, "y", 0.0).floor() as i32); let (tol, contiguous, aa, opacity) = (f(p, "tolerance", 32.0), b(p, "contiguous", true), b(p, "antiAlias", true), f(p, "opacity", 100.0) / 100.0); + let all = b(p, "sampleAllLayers", false) && crate::channel_cmds::target_of(p) == crate::channel_cmds::Target::Pixels; + // Other visible layers can define the region, but the fill still writes only to the target. + let region = if all { + let (area, img) = crate::selection_cmds::sample_rgba8(s, true)?; + let region = photocraft_algo::selection::wand_region(&img, area, (x, y), tol, contiguous, aa); + drop(img); + region + } else { + None + }; // Fill source: foreground colour (default) or a pattern (the Paint Bucket "Fill" dropdown). let source = p.get("contents").or_else(|| p.get("source")).and_then(Value::as_str).unwrap_or("foreground"); let pattern = if source == "pattern" { @@ -55,14 +65,23 @@ fn bucket(s: &mut Session, p: &Value) -> Result { let area = doc.bounds(); let sel = doc.selection.clone(); let (surf, lock) = crate::channel_cmds::target_surface(doc, *active, p)?; - let ok = if let Some((tile, scale, angle, phase)) = &pattern { + let ok = if all && region.is_none() { + false + } else if let Some((tile, scale, angle, phase)) = &pattern { // Render the pattern over the canvas once, then sample it at each filled pixel. let place = photocraft_compose::pattern::Placement::new(photocraft_geom::Rect::EMPTY, false, *phase, *scale, *angle); let rendered = photocraft_compose::pattern::render(tile, &place, area); let w = area.width() as usize; - bucket_fill_src(surf, area, (x, y), tol, contiguous, aa, opacity, sel.as_ref(), lock, |px, py| { - rendered[(py - area.y0) as usize * w + (px - area.x0) as usize] - }) + let src = |px: i32, py: i32| rendered[(py - area.y0) as usize * w + (px - area.x0) as usize]; + if let Some(region) = ®ion { + bucket_fill_region_src(surf, region, opacity, sel.as_ref(), lock, src); + true + } else { + bucket_fill_src(surf, area, (x, y), tol, contiguous, aa, opacity, sel.as_ref(), lock, src) + } + } else if let Some(region) = ®ion { + bucket_fill_region_src(surf, region, opacity, sel.as_ref(), lock, |_, _| c); + true } else { bucket_fill(surf, area, (x, y), tol, contiguous, aa, c, opacity, sel.as_ref(), lock) }; @@ -117,7 +136,7 @@ pub fn specs() -> Vec { label: "Paint Bucket", menu: &[], shortcut: None, - params: r##"{"x":px,"y":px,"tolerance":0..255=32,"contiguous":bool=true,"antiAlias":bool=true,"contents":"foreground|pattern"="foreground","color":"#rrggbb"=foreground,"pattern":id|name (contents=pattern),"scale":%=100,"angle":deg,"opacity":1..100=100,"target":"pixels"|"mask"|"quickMask"|{"channel":i}=Channels panel target}"##, + params: r##"{"x":px,"y":px,"tolerance":0..255=32,"contiguous":bool=true,"antiAlias":bool=true,"sampleAllLayers":bool=false,"contents":"foreground|pattern"="foreground","color":"#rrggbb"=foreground,"pattern":id|name (contents=pattern),"scale":%=100,"angle":deg,"opacity":1..100=100,"target":"pixels"|"mask"|"quickMask"|{"channel":i}=Channels panel target}"##, enabled: crate::commands::has_paintable, run: bucket, journal: true, @@ -151,6 +170,151 @@ mod tests { d.doc.layer(d.active_layer.unwrap()).unwrap().surface().unwrap().pixel(x, y) } + /// A visible wall on the Background with an empty, active layer above it. + fn bucket_layers(depth: u32) -> (Session, photocraft_doc::LayerId) { + let mut s = Session::new(); + s.execute("file.new", json!({"width": 20, "height": 10, "depth": depth, "background": "white"})).unwrap(); + let bottom = s.active().unwrap().active_layer.unwrap(); + s.edit("wall", |doc, _| { + doc.layer_mut(bottom).unwrap().surface_mut().unwrap().fill_rect(Rect::new(10, 0, 11, 10), &[0.0, 0.0, 0.0, 1.0]); + Ok(()) + }) + .unwrap(); + s.execute("layer.new.layer", json!({})).unwrap(); + (s, bottom) + } + + #[test] + fn bucket_sample_all_layers_uses_visible_boundaries_at_all_depths() { + for depth in [8, 16, 32] { + for all in [None, Some(false), Some(true)] { + let (mut s, bottom) = bucket_layers(depth); + let before = s.active().unwrap().doc.layer(bottom).unwrap().clone(); + let mut p = json!({"x": 2, "y": 2, "color": "#ff0000", "antiAlias": false, "tolerance": 0}); + if let Some(all) = all { + p["sampleAllLayers"] = json!(all); + } + assert_eq!(s.execute("paint.bucket", p.clone()).unwrap()["filled"], true); + assert_eq!(px(&s, 5, 5), vec![1.0, 0.0, 0.0, 1.0], "depth {depth}, all {all:?}"); + let beyond = if all == Some(true) { vec![0.0; 4] } else { vec![1.0, 0.0, 0.0, 1.0] }; + assert_eq!(px(&s, 15, 5), beyond, "depth {depth}, all {all:?}: sample flag controls the boundary"); + assert_eq!(s.active().unwrap().doc.layer(bottom).unwrap(), &before, "only the active layer is written"); + if all == Some(true) { + assert_eq!(px(&s, 10, 5), vec![0.0; 4], "wall itself is untouched"); + assert!(s.undo()); + p["contiguous"] = json!(false); + s.execute("paint.bucket", p).unwrap(); + assert_eq!(px(&s, 15, 5), vec![1.0, 0.0, 0.0, 1.0], "noncontiguous finds both white regions"); + assert_eq!(px(&s, 10, 5), vec![0.0; 4]); + assert!(s.undo()); + assert_eq!(s.execute("paint.bucket", json!({"x": -1, "y": 2, "sampleAllLayers": true})).unwrap()["filled"], false); + assert_eq!(px(&s, 5, 5), vec![0.0; 4], "an off-canvas sample does not fill the layer"); + } + } + } + } + + #[test] + fn bucket_sample_all_layers_ignores_hidden_layers() { + let (mut s, _) = bucket_layers(8); + s.edit("hidden wall", |doc, active| { + let l = doc.layer_mut(active.unwrap()).unwrap(); + l.surface_mut().unwrap().fill_rect(Rect::new(4, 0, 5, 10), &[0.0, 0.0, 0.0, 1.0]); + l.visible = false; + Ok(()) + }) + .unwrap(); + s.execute("layer.new.layer", json!({})).unwrap(); + s.execute("paint.bucket", json!({"x": 2, "y": 2, "color": "#ff0000", "antiAlias": false, "tolerance": 0, "sampleAllLayers": true})).unwrap(); + assert_eq!(px(&s, 4, 5), vec![1.0, 0.0, 0.0, 1.0], "hidden wall does not bound the fill"); + assert_eq!(px(&s, 8, 5), vec![1.0, 0.0, 0.0, 1.0]); + assert_eq!(px(&s, 15, 5), vec![0.0; 4], "visible wall still bounds the fill"); + } + + #[test] + fn bucket_sample_all_layers_keeps_selection_opacity_and_history() { + for depth in [8, 16, 32] { + let (mut s, _) = bucket_layers(depth); + s.execute("select.rect", json!({"x": 0, "y": 2, "width": 20, "height": 6})).unwrap(); + s.edit("feather", |doc, _| { + doc.selection.as_mut().unwrap().fill_rect(Rect::new(0, 2, 4, 8), &[0.5]); + Ok(()) + }) + .unwrap(); + let selection = s.active().unwrap().doc.selection.clone(); + s.execute("paint.bucket", json!({"x": 2, "y": 5, "color": "#ff0000", "antiAlias": false, "tolerance": 0, "opacity": 50, "sampleAllLayers": true})) + .unwrap(); + let (feathered, full) = (px(&s, 2, 5), px(&s, 6, 5)); + assert!((feathered[3] - 0.25).abs() < 0.005 && (full[3] - 0.5).abs() < 0.005, "depth {depth}: selection coverage multiplies opacity"); + assert_eq!(full[..3], [1.0, 0.0, 0.0]); + assert_eq!(px(&s, 6, 1), vec![0.0; 4], "outside selection"); + assert_eq!(px(&s, 15, 5), vec![0.0; 4], "outside sampled region"); + assert_eq!(s.active().unwrap().history.undo_label(), Some("Paint Bucket")); + assert!(s.undo()); + assert_eq!(px(&s, 2, 5), vec![0.0; 4]); + assert_eq!(s.active().unwrap().doc.selection, selection); + assert!(s.redo()); + assert_eq!(px(&s, 2, 5), feathered); + assert_eq!(px(&s, 6, 5), full); + assert_eq!(s.active().unwrap().doc.selection, selection); + } + } + + #[test] + fn bucket_sample_all_layers_pattern_uses_the_same_region() { + let (mut s, _) = bucket_layers(8); + s.execute( + "paint.bucket", + json!({"x": 2, "y": 2, "contents": "pattern", "pattern": "Diagonal Lines", "antiAlias": false, "tolerance": 0, "sampleAllLayers": true}), + ) + .unwrap(); + let left: Vec = (0..10).map(|x| px(&s, x, 0)[0]).collect(); + assert!(left.iter().any(|&r| r < 0.3) && left.iter().any(|&r| r > 0.9), "pattern varies inside the region: {left:?}"); + assert_eq!(px(&s, 10, 5), vec![0.0; 4], "wall untouched"); + assert_eq!(px(&s, 15, 5), vec![0.0; 4], "region past the wall untouched"); + } + + #[test] + fn bucket_sample_all_layers_mask_and_channel_targets_keep_their_own_boundaries() { + for target in [json!("mask"), json!({"channel": 0}), json!("quickMask")] { + for all in [false, true] { + let (mut s, _) = bucket_layers(8); + match crate::channel_cmds::target_of(&json!({"target": target})) { + crate::channel_cmds::Target::Mask => { + s.execute("layer.layerMask.revealAll", json!({})).unwrap(); + } + crate::channel_cmds::Target::Alpha(_) => { + s.execute("channel.new", json!({"fill": "white"})).unwrap(); + } + crate::channel_cmds::Target::QuickMask => { + s.execute("select.editInQuickMaskMode", json!({"on": true})).unwrap(); + } + _ => panic!("expected a grayscale target"), + } + let p = json!({"x": 2, "y": 2, "color": "#000000", "antiAlias": false, "tolerance": 0, "target": target, "sampleAllLayers": all}); + s.edit("target wall", |doc, active| { + let area = doc.bounds(); + let (surf, _) = crate::channel_cmds::target_surface(doc, *active, &p).unwrap(); + surf.fill_rect(area, &[1.0]); + surf.fill_rect(Rect::new(5, 0, 6, 10), &[0.0]); + Ok(()) + }) + .unwrap(); + s.execute("paint.bucket", p.clone()).unwrap(); + let d = &s.active().unwrap().doc; + let surf = match crate::channel_cmds::target_of(&p) { + crate::channel_cmds::Target::Mask => &d.layer(s.active().unwrap().active_layer.unwrap()).unwrap().mask.as_ref().unwrap().surface, + crate::channel_cmds::Target::Alpha(i) => &d.channels[i].surface, + crate::channel_cmds::Target::QuickMask => &d.quick_mask.as_ref().unwrap().surface, + _ => panic!("expected a grayscale target"), + }; + assert_eq!(surf.sample_channel(2, 5, 0), 0.0, "{target}, all {all}: filled target"); + assert_eq!(surf.sample_channel(7, 5, 0), 1.0, "{target}, all {all}: stopped at target wall"); + assert_eq!(px(&s, 2, 5), vec![0.0; 4], "layer pixels remain untouched"); + } + } + } + #[test] fn bucket_fill_contiguous() { let mut s = session(); diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 35989d3d..7259b181 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -2983,7 +2983,7 @@ pub fn tool_event(app: &mut PhotocraftApp, ev: ToolEvent, mods: egui::Modifiers) Tool::PaintBucket => { let o = app.ui.tool_options.clone(); let contents = if o.bucket_fill_pattern { "pattern" } else { "foreground" }; - let _ = app.run("paint.bucket", json!({"x": x.floor(), "y": y.floor(), "tolerance": o.tolerance, "contiguous": o.contiguous, "antiAlias": o.anti_alias, "opacity": o.fill_opacity, "contents": contents, "target": paint_target(app)})); + let _ = app.run("paint.bucket", json!({"x": x.floor(), "y": y.floor(), "tolerance": o.tolerance, "contiguous": o.contiguous, "antiAlias": o.anti_alias, "sampleAllLayers": o.sample_all_layers, "opacity": o.fill_opacity, "contents": contents, "target": paint_target(app)})); return; } Tool::RedEye => { diff --git a/crates/ui-egui/tests/paint_bucket_all_layers.rs b/crates/ui-egui/tests/paint_bucket_all_layers.rs new file mode 100644 index 00000000..789a00ee --- /dev/null +++ b/crates/ui-egui/tests/paint_bucket_all_layers.rs @@ -0,0 +1,119 @@ +//! #1118: Paint Bucket's All Layers checkbox must sample the visible image while painting +//! only the active layer. Drive the checkbox and canvas through real egui pointer events. + +use egui::{Modifiers, PointerButton, Pos2}; +use egui_kittest::kittest::Queryable; +use photocraft_doc::LayerId; +use photocraft_geom::Rect; +use photocraft_ui_egui::PhotocraftApp; +use photocraft_ui_egui::canvas::ViewXform; +use photocraft_ui_egui::control::{ControlRequest, Outcome, handle}; +use serde_json::json; + +type Harness = egui_kittest::Harness<'static, PhotocraftApp>; + +fn harness() -> Option { + let built = std::panic::catch_unwind(|| { + egui_kittest::Harness::builder().with_size(egui::vec2(1100.0, 720.0)).with_pixels_per_point(1.0).with_max_steps(64).wgpu().build_eframe(|cc| { + PhotocraftApp::setup_context(&cc.egui_ctx, Default::default()); + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), Default::default()); + app.session.prefs.edit(|p| p.interface.language = "en".into()); + if let Some(rs) = cc.wgpu_render_state.as_ref() { + app.set_wgpu(rs.clone()); + } + app + }) + }); + match built { + Ok(h) => Some(h), + Err(_) => { + eprintln!("skipping: no GPU adapter"); + None + } + } +} + +fn setup(h: &mut Harness) -> LayerId { + let background = { + let app = h.state_mut(); + while app.session.active().is_some() { + app.run("file.close", json!({"discard": true})).expect("close"); + } + app.run("file.new", json!({"width": 400, "height": 300, "background": "white"})).expect("new"); + let background = app.session.active().expect("document").active_layer.expect("background"); + app.run("select.rect", json!({"x": 0, "y": 0, "width": 200, "height": 300})).expect("select left half"); + app.run("edit.fill", json!({"contents": "black"})).expect("black left half"); + app.run("select.deselect", json!({})).expect("deselect"); + app.run("layer.new.layer", json!({"name": "Bucket fill"})).expect("empty active layer"); + app.run("tools.setColors", json!({"foreground": [1.0, 0.0, 0.0, 1.0]})).expect("red foreground"); + app.sync_views(); + app.ui.extras.rulers = false; + let o = &mut app.ui.tool_options; + o.tolerance = 0.0; + o.anti_alias = false; + o.contiguous = true; + o.sample_all_layers = false; + o.bucket_fill_pattern = false; + o.fill_opacity = 100.0; + background + }; + let ctx = h.ctx.clone(); + let (req, _rx) = ControlRequest::new("ui.set", json!({"tool": "paintBucket", "zoom": 1.0, "center": [200, 150]})); + assert!(matches!(handle(h.state_mut(), &ctx, &req), Outcome::Done(_))); + h.run_steps(6); + background +} + +fn screen(h: &Harness, x: f32, y: f32) -> Pos2 { + let app = h.state(); + let v = &app.ui.views[0]; + ViewXform { rect: photocraft_ui_egui::rulers::content_rect(app, app.last_canvas_rect), zoom: v.zoom, center: v.center, flip: app.ui.view.flip_horizontal } + .to_screen(x, y) +} + +fn pixels(h: &Harness, layer: LayerId) -> Vec<[f32; 4]> { + let st = h.state().session.active().expect("document"); + let surface = st.doc.layer(layer).expect("layer").surface().expect("pixel layer"); + let mut pixels = vec![[0.0; 4]; 400 * 300]; + surface.read_rgba_into(Rect::new(0, 0, 400, 300), &mut pixels); + pixels +} + +#[test] +fn all_layers_checkbox_limits_bucket_fill_to_the_visible_color() { + let Some(mut h) = harness() else { return }; + h.run_steps(4); + for all_layers in [false, true] { + let background = setup(&mut h); + let before = pixels(&h, background); + if all_layers { + h.get_by_label("All Layers").click(); + h.run_steps(3); + } + assert_eq!(h.state().ui.tool_options.sample_all_layers, all_layers, "checkbox changed the option"); + + let pos = screen(&h, 300.0, 150.0); + h.event(egui::Event::PointerMoved(pos)); + h.step(); + for pressed in [true, false] { + h.event(egui::Event::PointerButton { pos, button: PointerButton::Primary, pressed, modifiers: Modifiers::NONE }); + h.step(); + } + h.run_steps(6); + + // Optional real UI evidence, saved before assertions so a failing baseline has a frame. + if let Some(dir) = std::env::var_os("PHOTOCRAFT_BUCKET_EVIDENCE_DIR") { + std::fs::create_dir_all(&dir).expect("evidence directory"); + let name = if all_layers { "all-layers-on.png" } else { "all-layers-off.png" }; + h.render().expect("render UI").save(std::path::Path::new(&dir).join(name)).expect("save UI evidence"); + } + + assert_eq!(pixels(&h, background), before, "bucket must leave the sampled background unchanged"); + let active = h.state().session.active().expect("document").active_layer.expect("active layer"); + for (i, pixel) in pixels(&h, active).into_iter().enumerate() { + let x = i % 400; + let expected = if all_layers && x < 200 { [0.0; 4] } else { [1.0, 0.0, 0.0, 1.0] }; + assert_eq!(pixel, expected, "All Layers={all_layers}, active-layer pixel ({x}, {})", i / 400); + } + } +}