From e5f4f8e260ccfe8266b07c51595294faeb8f4ebd Mon Sep 17 00:00:00 2001 From: David Mann Date: Wed, 21 Oct 2026 12:14:46 +0200 Subject: [PATCH 1/2] Address CodeRabbit review: soft shape frame from the hard render, feather claim A soft shape's transform frame, snap rectangle and Properties X/Y/W/H came from the path plus the full stroke width on every side, so a centred or inside stroke grew the frame when Density or Feather was set. The frame is now the pixels the same shape renders at full Density without Feather (edge strips only, clipped to the canvas), identical to the hard shape's for every stroke alignment. feather_matches_photoshops_ramp now enforces the 1-level claim the scorecard makes (measured max deviation 1 at 8, 16 and 32 bit). Co-Authored-By: Claude Opus 5.5 --- crates/compose/src/lib.rs | 33 ++++++++++++++++++++--- crates/compose/src/soft_shape_tests.rs | 5 ++-- crates/engine/src/snap.rs | 6 ++--- crates/engine/src/transform_cmds.rs | 36 +++++++++++++++++++++++--- crates/ui-egui/src/lib.rs | 17 +++++++++++- 5 files changed, 84 insertions(+), 13 deletions(-) diff --git a/crates/compose/src/lib.rs b/crates/compose/src/lib.rs index e3ce1f2c26..42b3892648 100644 --- a/crates/compose/src/lib.rs +++ b/crates/compose/src/lib.rs @@ -1144,9 +1144,7 @@ pub fn relayout_damage(a: &Document, b: &Document) -> Rect { } /// The bounding box of a shape layer's path and stroke, ignoring its Density and Feather: its -/// path control bounds plus stroke width. It is the frame Free Transform and the Move tool's -/// transform controls show for a soft shape, whose rendering reaches past it (in Photoshop a -/// shape's transform frame follows its path whatever its mask Density and Feather). +/// path control bounds plus stroke width (a conservative reach for any alignment, cap and join). pub fn hard_shape_bounds(sh: &photocraft_doc::ShapeLayer) -> Option { if sh.fill.is_none() && sh.stroke.as_ref().is_none_or(|s| s.width <= 0.0 || s.opacity <= 0.0) { return None; @@ -1157,6 +1155,35 @@ pub fn hard_shape_bounds(sh: &photocraft_doc::ShapeLayer) -> Option { (!r.is_empty()).then_some(r) } +/// The frame Free Transform, the Move tool's transform controls, snapping and Properties show for +/// a soft shape (Density below 100 % or a Feather), whose rendering reaches past its path: the +/// pixels the same shape renders in `format` over `canvas` at full Density without Feather, so +/// the frame is exactly the hard shape's whatever the stroke's alignment, cap and join (in +/// Photoshop a shape's transform frame follows its path and stroke whatever its mask Density and +/// Feather, #5846). An inverted path's frame is its own outline, not the whole canvas. Renders +/// only the strips along the edges of the rasterizers' (1 px conservative) bounds. +pub fn soft_shape_frame(sh: &photocraft_doc::ShapeLayer, format: photocraft_color::PixelFormat, canvas: Rect) -> Option { + let mut path = sh.path.clone(); + path.inverted = false; + let hard = photocraft_doc::ShapeLayer { density: 1.0, feather: 0.0, path, fill: sh.fill.clone(), stroke: sh.stroke.clone(), ..Default::default() }; + let shape = photocraft_vector::CompiledShape::new(&hard, photocraft_vector::DEFAULT_TOLERANCE, canvas); + let mut r = shape.bounds()?.intersect(&canvas); + let painted = |strip: Rect| !shape.render(format, strip).content_bounds().is_empty(); + while !r.is_empty() && !painted(Rect::new(r.x0, r.y0, r.x1, r.y0 + 1)) { + r.y0 += 1; + } + while !r.is_empty() && !painted(Rect::new(r.x0, r.y1 - 1, r.x1, r.y1)) { + r.y1 -= 1; + } + while !r.is_empty() && !painted(Rect::new(r.x0, r.y0, r.x0 + 1, r.y1)) { + r.x0 += 1; + } + while !r.is_empty() && !painted(Rect::new(r.x1 - 1, r.y0, r.x1, r.y1)) { + r.x1 -= 1; + } + (!r.is_empty()).then_some(r) +} + /// The bounding box of a shape layer's vector content: its path control bounds plus stroke width /// (and the reach of a soft fill). pub fn shape_bounds(sh: &photocraft_doc::ShapeLayer) -> Option { diff --git a/crates/compose/src/soft_shape_tests.rs b/crates/compose/src/soft_shape_tests.rs index d506c3e55b..38bd8fa9aa 100644 --- a/crates/compose/src/soft_shape_tests.rs +++ b/crates/compose/src/soft_shape_tests.rs @@ -58,8 +58,9 @@ fn feather_matches_photoshops_ramp() { let r = row(&shape_doc(depth, 1.0, 10.0, None), 100); let ours: Vec = (0..15).map(|i| r[30 + 3 * i]).collect(); let dev = ours.iter().zip(PS_FEATHER_10).map(|(a, b)| (a - i32::from(b)).abs()).max().unwrap_or(0); - // Same Gaussian (sigma = feather) as a vector mask: within 3 levels of Photoshop. - assert!(dev <= 3, "{depth:?}: ours {ours:?} vs Photoshop {PS_FEATHER_10:?} (max {dev})"); + // Same Gaussian (sigma = feather) as a vector mask: within 1 level of Photoshop (as + // scorecard TOOL-214-12 claims; measured max deviation 1 at every depth). + assert!(dev <= 1, "{depth:?}: ours {ours:?} vs Photoshop {PS_FEATHER_10:?} (max {dev})"); eprintln!("{depth:?}: ours {ours:?}, Photoshop {PS_FEATHER_10:?}, max deviation {dev}"); // The path runs between pixels 49 and 50: their mean is half way. assert!((r[49] + r[50] - 255).abs() <= 2, "{depth:?}: half way on the path: {} {}", r[49], r[50]); diff --git a/crates/engine/src/snap.rs b/crates/engine/src/snap.rs index ef0e72f5db..652ee1d747 100644 --- a/crates/engine/src/snap.rs +++ b/crates/engine/src/snap.rs @@ -95,8 +95,8 @@ fn rect_of(doc: &Document, l: &photocraft_doc::Layer) -> Option<[f64; 4]> { /// Bounds of a layer as a Snap To Layers target: all of its content, whatever the selection /// holds of it (#6290: clipped to the selection, every layer's edges and centre were the /// selection's, so a moved selection snapped back to where it started). -fn target_rect_of(l: &photocraft_doc::Layer) -> Option<[f64; 4]> { - snappable(l).then(|| crate::transform_cmds::whole_layer_bounds(l)).and_then(to_rect) +fn target_rect_of(doc: &Document, l: &photocraft_doc::Layer) -> Option<[f64; 4]> { + snappable(l).then(|| crate::transform_cmds::whole_layer_bounds(doc, l)).and_then(to_rect) } fn snappable(l: &photocraft_doc::Layer) -> bool { @@ -132,7 +132,7 @@ impl SnapTargets { if !l.visible || exclude.contains(&l.id) { continue; } - if let Some(r) = target_rect_of(l) { + if let Some(r) = target_rect_of(doc, l) { t.add_rect(r, SnapKind::LayerEdge, SnapKind::LayerCenter); } } diff --git a/crates/engine/src/transform_cmds.rs b/crates/engine/src/transform_cmds.rs index afebf35467..2c85f30ddf 100644 --- a/crates/engine/src/transform_cmds.rs +++ b/crates/engine/src/transform_cmds.rs @@ -29,7 +29,7 @@ pub fn moves_whole(layer: &Layer) -> bool { /// content within the selection, or all of it for a layer that [`moves_whole`] (#2630). /// Content scans are cached per tile: snapping asks for every layer's bounds per Move drag. pub fn transform_bounds(doc: &Document, layer: &Layer) -> Rect { - let content = whole_layer_bounds(layer); + let content = whole_layer_bounds(doc, layer); match &doc.selection { Some(sel) if !moves_whole(layer) => content.intersect(&sel.content_bounds()), _ => content, @@ -54,12 +54,12 @@ fn framed_by_selection(doc: &Document, bounds: Rect) -> Rect { } /// A group moves all its descendants, so its frame must not clip raster children to the selection. -pub(crate) fn whole_layer_bounds(layer: &Layer) -> Rect { +pub(crate) fn whole_layer_bounds(doc: &Document, layer: &Layer) -> Rect { let content = match &layer.content { - LayerContent::Group(g) => g.children.iter().map(whole_layer_bounds).fold(Rect::EMPTY, |a, b| a.union(&b)), + LayerContent::Group(g) => g.children.iter().map(|c| whole_layer_bounds(doc, c)).fold(Rect::EMPTY, |a, b| a.union(&b)), // A soft shape (Density below 100 % or a Feather) renders past its path, over the whole // canvas below 100 %: its frame follows the path and stroke, as in Photoshop (#5846). - LayerContent::Shape(sh) if sh.has_soft_mask() => photocraft_compose::hard_shape_bounds(sh).unwrap_or(Rect::EMPTY), + LayerContent::Shape(sh) if sh.has_soft_mask() => photocraft_compose::soft_shape_frame(sh, doc.pixel_format(), doc.bounds()).unwrap_or(Rect::EMPTY), _ => layer.surface().map_or(Rect::EMPTY, photocraft_compose::bounds::content_bounds), }; if content.is_empty() { layer.mask.as_ref().map_or(Rect::EMPTY, |m| photocraft_compose::bounds::content_bounds(&m.surface)) } else { content } @@ -1539,4 +1539,32 @@ mod tests { assert_eq!((x0.round(), y0.round(), x1.round(), y1.round()), (-10.0, 30.0, 110.0, 70.0)); assert_eq!((sh.density, sh.feather), (1.0, 20.0), "the transform keeps the soft settings"); } + + /// A stroked soft shape's frame is the hard shape's rendered frame for every stroke alignment: + /// a centred stroke reaches half its width past the path, not its full width; both stop at + /// the canvas edge. + #[test] + fn a_stroked_soft_shapes_frame_matches_the_hard_shapes() { + for (align, reach, x) in [("center", 5, 50), ("inside", 0, 50), ("outside", 10, 50), ("center", 5, -20)] { + let mut s = Session::new(); + s.execute("file.new", json!({"width": 200, "height": 200})).unwrap(); + let stroke = json!({"width": 10, "color": "#ff0000", "align": align, "join": "round"}); + let id = s.execute("shape.create", json!({"kind": "rect", "rect": [x, 50, 60, 40], "fill": "#000000", "stroke": stroke})).unwrap()["layer"] + .as_u64() + .unwrap(); + let id = photocraft_doc::LayerId(id); + let boxes = |s: &Session| { + let st = s.active().unwrap(); + let l = st.doc.layer(id).unwrap(); + (transform_bounds(&st.doc, l), transform_frame(&st.doc, l), crate::snap::layer_rect(&st.doc, id)) + }; + let hard = boxes(&s); + // Hand-computed: the path is x..x + 60 x 50..90, the stroke reaches `reach` px past it. + assert_eq!(hard.0, Rect::new((x - reach).max(0), 50 - reach, x + 60 + reach, 90 + reach), "{align} at {x}: the hard shape's frame"); + s.execute("layer.vectorMask.edit", json!({"density": 60, "feather": 8})).unwrap(); + assert_eq!(boxes(&s), hard, "{align} at {x}: Density and Feather leave the frame"); + s.execute("layer.vectorMask.edit", json!({"density": 100, "feather": 8})).unwrap(); + assert_eq!(boxes(&s), hard, "{align} at {x}: Feather alone leaves the frame"); + } + } } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index fe6043cda2..63007122bd 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -2593,7 +2593,22 @@ impl PhotocraftApp { /// transform frame does. pub fn layer_frame_bounds(&mut self, layer: &photocraft_doc::Layer, surface: &photocraft_raster::Surface) -> photocraft_geom::Rect { match &layer.content { - photocraft_doc::LayerContent::Shape(sh) if sh.has_soft_mask() => photocraft_compose::hard_shape_bounds(sh).unwrap_or(photocraft_geom::Rect::EMPTY), + photocraft_doc::LayerContent::Shape(sh) if sh.has_soft_mask() => { + // Cached per rendering, like the pixel bounds: the frame renders strips of the shape. + let (key, rev) = (layer.id.0 | 1 << 63, surface_fingerprint(surface)); + if let Some((r, b)) = self.bounds_cache.get(&key) + && *r == rev + { + return *b; + } + let canvas = self.session.active().map_or_else(|| surface.content_bounds(), |st| st.doc.bounds()); + let b = photocraft_compose::soft_shape_frame(sh, surface.format(), canvas).unwrap_or(photocraft_geom::Rect::EMPTY); + if self.bounds_cache.len() > 256 { + self.bounds_cache.clear(); + } + self.bounds_cache.insert(key, (rev, b)); + b + } _ => self.cached_bounds(layer.id.0, surface), } } From 4bc06b906b08b367a8d5f269935b627539f51e87 Mon Sep 17 00:00:00 2001 From: David Mann Date: Wed, 21 Oct 2026 13:15:11 +0200 Subject: [PATCH 2/2] Address CodeRabbit review: bound the soft shape frame scan Look in from each edge through strips that double in thickness, so a subtracted or intersected subpath's empty margin costs a few renders instead of one per row. Co-Authored-By: Claude Opus 5.5 --- crates/compose/src/lib.rs | 60 +++++++++++++++++++++----- crates/compose/src/soft_shape_tests.rs | 39 +++++++++++++++++ 2 files changed, 89 insertions(+), 10 deletions(-) diff --git a/crates/compose/src/lib.rs b/crates/compose/src/lib.rs index 42b3892648..2e63844c1b 100644 --- a/crates/compose/src/lib.rs +++ b/crates/compose/src/lib.rs @@ -1167,19 +1167,59 @@ pub fn soft_shape_frame(sh: &photocraft_doc::ShapeLayer, format: photocraft_colo path.inverted = false; let hard = photocraft_doc::ShapeLayer { density: 1.0, feather: 0.0, path, fill: sh.fill.clone(), stroke: sh.stroke.clone(), ..Default::default() }; let shape = photocraft_vector::CompiledShape::new(&hard, photocraft_vector::DEFAULT_TOLERANCE, canvas); - let mut r = shape.bounds()?.intersect(&canvas); - let painted = |strip: Rect| !shape.render(format, strip).content_bounds().is_empty(); - while !r.is_empty() && !painted(Rect::new(r.x0, r.y0, r.x1, r.y0 + 1)) { - r.y0 += 1; + let r = shape.bounds()?.intersect(&canvas); + trim_to_painted(r, |strip| shape.render(format, strip).content_bounds()) +} + +/// `r` shrunk to the pixels `painted` reports (the painted bounds within a strip of `r`, empty if +/// none), looking in from each edge through strips that double in thickness: a few renders per +/// edge however wide the empty margin (a subtracted or intersected subpath can leave most of the +/// rasterizers' bounds empty), and none of `r`'s inside rendered at once. +fn trim_to_painted(mut r: Rect, mut painted: impl FnMut(Rect) -> Rect) -> Option { + // Top: the first strip holding paint gives the first painted row. + let mut t = 1; + while !r.is_empty() { + let strip = Rect::new(r.x0, r.y0, r.x1, r.y0.saturating_add(t).min(r.y1)); + let p = painted(strip).intersect(&strip); + if !p.is_empty() { + r.y0 = p.y0; + break; + } + r.y0 = strip.y1; + t = t.saturating_mul(2); } - while !r.is_empty() && !painted(Rect::new(r.x0, r.y1 - 1, r.x1, r.y1)) { - r.y1 -= 1; + let mut t = 1; + while !r.is_empty() { + let strip = Rect::new(r.x0, r.y1.saturating_sub(t).max(r.y0), r.x1, r.y1); + let p = painted(strip).intersect(&strip); + if !p.is_empty() { + r.y1 = p.y1; + break; + } + r.y1 = strip.y0; + t = t.saturating_mul(2); } - while !r.is_empty() && !painted(Rect::new(r.x0, r.y0, r.x0 + 1, r.y1)) { - r.x0 += 1; + let mut t = 1; + while !r.is_empty() { + let strip = Rect::new(r.x0, r.y0, r.x0.saturating_add(t).min(r.x1), r.y1); + let p = painted(strip).intersect(&strip); + if !p.is_empty() { + r.x0 = p.x0; + break; + } + r.x0 = strip.x1; + t = t.saturating_mul(2); } - while !r.is_empty() && !painted(Rect::new(r.x1 - 1, r.y0, r.x1, r.y1)) { - r.x1 -= 1; + let mut t = 1; + while !r.is_empty() { + let strip = Rect::new(r.x1.saturating_sub(t).max(r.x0), r.y0, r.x1, r.y1); + let p = painted(strip).intersect(&strip); + if !p.is_empty() { + r.x1 = p.x1; + break; + } + r.x1 = strip.x0; + t = t.saturating_mul(2); } (!r.is_empty()).then_some(r) } diff --git a/crates/compose/src/soft_shape_tests.rs b/crates/compose/src/soft_shape_tests.rs index 38bd8fa9aa..222b3f3e9a 100644 --- a/crates/compose/src/soft_shape_tests.rs +++ b/crates/compose/src/soft_shape_tests.rs @@ -158,3 +158,42 @@ fn soft_shape_bounds_cover_the_fill() { assert_eq!(layer_bounds(&dense.layers[1], canvas), canvas); assert!(effect_outline(&dense.layers[1]).is_none(), "a soft fill's alpha is its outline"); } + +/// A soft shape's frame is the hard shape's painted pixels, also where a subtracted subpath +/// leaves most of the rasterizers' bounds empty (#5846), at every depth. +#[test] +fn soft_shape_frame_skips_a_subtracted_margin() { + let canvas = Rect::new(0, 0, 200, 200); + let mut cut = Subpath::polygon(&[(40.0, 40.0), (160.0, 40.0), (160.0, 140.0), (40.0, 140.0)]); + cut.op = photocraft_doc::vector::PathOp::Subtract; + let path = Path::new(vec![Subpath::polygon(&[(50.0, 50.0), (150.0, 50.0), (150.0, 150.0), (50.0, 150.0)]), cut]); + for depth in [SampleType::U8, SampleType::U16, SampleType::F32] { + let sh = ShapeLayer { path: path.clone(), fill: Some(Fill::Solid(Color::BLACK)), density: 0.5, feather: 4.0, ..ShapeLayer::default() }; + assert_eq!(soft_shape_frame(&sh, doc(depth).pixel_format(), canvas), Some(Rect::new(50, 140, 150, 150)), "{depth:?}"); + } +} + +/// The frame scan renders a few strips per edge, not one per empty row or column: a 2-px +/// painted spot in a 4000 × 3000 bounds takes a handful of renders and is found exactly. +#[test] +fn soft_shape_frame_scan_is_bounded() { + let spot = Rect::new(2500, 1700, 2502, 1702); + let mut calls = 0; + let got = trim_to_painted(Rect::new(0, 0, 4000, 3000), |strip| { + calls += 1; + strip.intersect(&spot) + }); + assert_eq!(got, Some(spot)); + assert!(calls <= 4 * 14, "{calls} renders"); + let mut calls = 0; + assert_eq!( + trim_to_painted(Rect::new(0, 0, 4000, 3000), |_| { + calls += 1; + Rect::EMPTY + }), + None + ); + assert!(calls <= 14, "an empty shape stops after the top edge: {calls} renders"); + assert_eq!(trim_to_painted(Rect::EMPTY, |r| r), None); + assert_eq!(trim_to_painted(Rect::new(3, 4, 9, 7), |r| r), Some(Rect::new(3, 4, 9, 7)), "a full rect keeps its edges"); +}