collective
Browse / github.com/storytold/photocraft / patch
perf(selection): use sparse combination for marquee Add and Subtract
perf(selection): use sparse combination for marquee Add and Subtract Marquee Add and Subtract still called `combine_surfaces`, which visits every tile in the canvas area even when the new shape is small. The sparse combiner already exists in `photocraft-algo` and is used by Select and Mask tool edits. ## Change - Use `combine_sparse` for the unfeathered `select.rect` Add and Subtract branches. The combiner visits the new shape's stored tiles and keeps the rest of the existing selection. - Keep the existing coverage formulas: `max(a, b)` for Add and `a * (1 - b)` for Subtract, including anti-aliased ellipse edges. - Intersect still uses `combine_surfaces`; fea
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- wanikhawar (imported from pull request #9813, open)
- Base
7ac7b8b106acccf6826d45ed5c9e481dfe341035- Size
- +91 −5 in 3 files
- Review
- pending
- Shared
- 2026-10-28
- Id
f9a00ba323a0ef177f324ea6
How it went
No other operator has reported using it yet.
Diff
raw · crates/engine/src/cmd.rs crates/engine/src/tests.rs crates/engine/tests/it/empty_selection_combine.rs
@@ -2,7 +2,7 @@ use photocraft_color::{BlendMode, Color, ColorMode, SampleType}; use photocraft_doc::{Adjustment, Document, Fill, Layer, LayerContent, LayerId, LayerMask, Size};-use photocraft_geom::Rect;+use photocraft_geom::{Rect, TILE_SIZE}; use photocraft_raster::Surface; use serde_json::{Map, Value, json}; @@ -417,15 +417,34 @@ fn build() -> Vec<CommandSpec> { } let old = doc.selection.take(); let combined = match (mode.as_str(), old) {- ("add", Some(o)) => sel::combine_surfaces(&o, &shape, area, |a, b| a.max(b)),- ("subtract", Some(o)) => sel::combine_surfaces(&o, &shape, area, |a, b| a * (1.0 - b)),+ ("add", Some(o)) => sel::combine_sparse(&o, &shape, area, |a, b| a.max(b)),+ ("subtract", Some(o)) => sel::combine_sparse(&o, &shape, area, |a, b| a * (1.0 - b)), ("intersect", Some(o)) => sel::combine_surfaces(&o, &shape, area, |a, b| a.min(b)), // With no existing selection, neither operation can select new pixels. ("subtract" | "intersect", None) => Surface::new(photocraft_color::PixelFormat::GRAY8), _ => shape, };- // Nothing selected on the canvas (e.g. dragged entirely outside it) deselects.- doc.selection = (!combined.content_bounds().is_empty()).then_some(combined);+ // Sparse Add/Subtract keeps untouched pixels outside the canvas. Only pixels+ // inside the document count when deciding whether a selection remains.+ let has_on_canvas_pixels = (combined.default_bytes().first().copied().unwrap_or(0) != 0 && {+ let canvas_tiles = u64::from(area.width().div_ceil(TILE_SIZE as u32)) * u64::from(area.height().div_ceil(TILE_SIZE as u32));+ let stored = combined.tiles().filter(|(coord, _)| !coord.rect().intersect(&area).is_empty()).count() as u64;+ stored < canvas_tiles+ }) || combined.tiles().any(|(coord, tile)| {+ let tile_rect = coord.rect();+ let overlap = tile_rect.intersect(&area);+ if overlap.is_empty() {+ return false;+ }+ let x0 = (overlap.x0 - tile_rect.x0) as usize;+ let x1 = (overlap.x1 - tile_rect.x0) as usize;+ (overlap.y0 - tile_rect.y0..overlap.y1 - tile_rect.y0).any(|y| {+ let row_start = y as usize * TILE_SIZE as usize;+ let start = row_start + x0;+ tile.bytes().get(start..row_start + x1).is_some_and(|row| row.iter().any(|&v| v != 0))+ })+ });+ doc.selection = has_on_canvas_pixels.then_some(combined); Ok(()) })?; Ok(Value::Null)@@ -433,6 +433,34 @@ fn marquee_dragged_past_the_canvas_stops_at_its_edge() { assert_eq!(bounds(&mut s), Value::Null); } +#[test]+fn subtract_clears_selection_when_only_disjoint_off_canvas_pixels_remain() {+ use photocraft_geom::Rect;+ use photocraft_raster::Surface;+ let mut s = session_with_doc(); // 64 × 48+ let mut selection = Surface::new(photocraft_color::PixelFormat::GRAY8);+ // Simulate a moved sparse selection: one remnant on either side of the canvas and one+ // in-canvas region that the subtract marquee will remove. The off-canvas bounds straddle+ // the canvas, so testing only their bounding-box intersection would incorrectly keep it.+ selection.fill_rect(Rect::new(-4, 10, 0, 14), &[1.0]);+ selection.fill_rect(Rect::new(20, 10, 24, 14), &[1.0]);+ selection.fill_rect(Rect::new(64, 10, 68, 14), &[1.0]);+ Arc::make_mut(&mut s.active_mut().unwrap().doc).selection = Some(selection);++ s.execute("select.rect", json!({"x": 0, "y": 0, "width": 64, "height": 48, "mode": "subtract"})).unwrap();++ assert!(s.active().unwrap().doc.selection.is_none());+}++#[test]+fn subtract_clears_selection_with_nonzero_default_when_canvas_is_empty() {+ use photocraft_raster::Surface;+ let mut s = session_with_doc();+ Arc::make_mut(&mut s.active_mut().unwrap().doc).selection = Some(Surface::with_default(photocraft_color::PixelFormat::GRAY8, &[1.0]));+ s.execute("select.rect", json!({"x": 0, "y": 0, "width": 64, "height": 48, "mode": "subtract"})).unwrap();+ assert!(s.active().unwrap().doc.selection.is_none());+}+ #[test] fn marquee_steps_are_named_after_their_tool() { // #513: the elliptical marquee shares `select.rect` but records its own step name.@@ -91,3 +91,42 @@ fn subtracting_from_an_existing_selection_preserves_other_pixels_and_history() { assert_eq!(coverage(&session, 12, 12), removed); } }++#[test]+fn marquee_add_subtract_match_dense_combination_across_tiles() {+ use photocraft_algo::selection as sel;+ use photocraft_color::PixelFormat;+ use photocraft_geom::Rect;+ use photocraft_raster::Surface;++ // A partial edge tile, a shape crossing a tile boundary, and an untouched distant tile.+ let area = Rect::from_xywh(0, 0, 777, 263);+ let r = Rect::from_xywh(247, -5, 24, 32);+ for depth in [8, 16, 32] {+ for ellipse in [false, true] {+ for mode in ["add", "subtract"] {+ let mut session = Session::new();+ session.execute("file.new", json!({"width": 777, "height": 263, "depth": depth})).unwrap();+ let mut old = Surface::new(PixelFormat::GRAY8);+ old.fill_rect(Rect::from_xywh(240, 0, 40, 40), &[0.6]);+ old.fill_rect(Rect::from_xywh(768, 256, 9, 7), &[0.4]);+ std::sync::Arc::make_mut(&mut session.active_mut().unwrap().doc).selection = Some(old.clone());+ let shape = if ellipse {+ sel::ellipse_surface(r, area, true)+ } else {+ let mut shape = Surface::new(PixelFormat::GRAY8);+ shape.fill_rect(r.intersect(&area), &[1.0]);+ shape+ };+ let expected = sel::combine_surfaces(&old, &shape, area, |a, b| if mode == "add" { a.max(b) } else { a * (1.0 - b) });+ session.execute("select.rect", json!({"x": 247, "y": -5, "width": 24, "height": 32, "ellipse": ellipse, "mode": mode})).unwrap();+ let pixels = |session: &Session| session.active().unwrap().doc.selection.as_ref().unwrap().read_region(area);+ assert_eq!(pixels(&session), expected.read_region(area), "depth={depth}, ellipse={ellipse}, mode={mode}");+ assert!(session.undo());+ assert_eq!(pixels(&session), old.read_region(area));+ assert!(session.redo());+ assert_eq!(pixels(&session), expected.read_region(area));+ }+ }+ }+}