From 35d46208cff4a6555e1add5edf2a5d5ddbcfa1a8 Mon Sep 17 00:00:00 2001 From: Trevor Brown Date: Wed, 11 Nov 2026 03:48:24 -0500 Subject: [PATCH] M4: Shared open loops all close through their segment; one index per rounding pass (#1951) Two regions over one open chain both got the closing segment when rounded, but reopen_loops gave it to the first loop only, so after an unrelated deletion the second region's curved closing segment dropped out of its fill. The pass now takes the segment for every loop that walks the same chain (same segment set); a different chain still can't share it. closing_segment no longer scans every segment and every loop per call: rounded_smoothed and reopen_loops build one LoopIndex (endpoint pair to segments, and which segments are in a loop) per pass and pass it in. Co-Authored-By: Claude Sonnet 5.5 Model-Effort: Opus 5.5 drove; Opus 5.5 briefed; Sonnet 5.5 wrote at medium --- crates/doc/src/vector.rs | 35 ++++-- crates/engine/src/canvas/vectoredit/ops.rs | 118 +++++++++++++++++++-- 2 files changed, 137 insertions(+), 16 deletions(-) diff --git a/crates/doc/src/vector.rs b/crates/doc/src/vector.rs index 1538de2a6..fd6d48072 100644 --- a/crates/doc/src/vector.rs +++ b/crates/doc/src/vector.rs @@ -184,6 +184,13 @@ fn redirect_last_segment(net: &mut VectorNetwork, loop_segs: &[usize], from: usi } } +/// Which segments are in a loop, and which segments join each pair of points: the lookups +/// [`VectorNetwork::closing_segment`] reads, built once per pass by [`VectorNetwork::loop_index`]. +pub struct LoopIndex { + in_loop: Vec, + by_ends: std::collections::BTreeMap<(usize, usize), Vec>, +} + impl VectorNetwork { /// A network from a polyline / polygon. pub fn from_points(points: &[Point], closed: bool) -> Self { @@ -471,6 +478,7 @@ impl VectorNetwork { // The curves go between the two segments wherever a loop walks through the point, in the // order the loop meets them. Each loop is rebuilt in one pass, so a polygon with many // rounded corners isn't quadratic. + let index = self.loop_index(); for lp in out.regions.iter_mut().flat_map(|r| r.loops.iter_mut()) { if lp.is_empty() { continue; @@ -478,7 +486,7 @@ impl VectorNetwork { let start_v = self.loop_start(lp).unwrap_or(0); let mut rebuilt = Vec::with_capacity(lp.len() + by_vertex.len().min(lp.len()) + 1); let mut cur = start_v; - let closing = self.closing_segment(lp); + let closing = self.closing_segment(lp, &index); for &x in lp.iter().chain(closing.iter()) { rebuilt.push(x); let Some(s) = self.segments.get(x) else { continue }; @@ -506,13 +514,29 @@ impl VectorNetwork { } } + /// The lookups [`Self::closing_segment`] needs, built once for a pass over every loop. + pub fn loop_index(&self) -> LoopIndex { + let mut in_loop = vec![false; self.segments.len()]; + for &x in self.regions.iter().flat_map(|r| r.loops.iter().flatten()) { + if let Some(f) = in_loop.get_mut(x) { + *f = true; + } + } + let mut by_ends: std::collections::BTreeMap<(usize, usize), Vec> = std::collections::BTreeMap::new(); + for (i, s) in self.segments.iter().enumerate() { + by_ends.entry((s.a.min(s.b), s.a.max(s.b))).or_default().push(i); + } + LoopIndex { in_loop, by_ends } + } + /// The segment that joins the two ends of an open loop `lp` when it is in no loop of its own /// (and the only such segment): a corner cut off by a segment drawn before the corner point was /// deleted leaves the loop open there, and the stroke follows that segment. Only a loop that /// walks in order as one continuous chain has one. Rounding walks it as the loop's last /// segment, so the points at the loop's ends are cut by arcs as the stroke's are, and deleting - /// points or segments puts it into the loop (#1951). - pub fn closing_segment(&self, lp: &[usize]) -> Option { + /// points or segments puts it into the loop (#1951). `index` is this network's [`Self::loop_index`], + /// shared by every loop of one pass. + pub fn closing_segment(&self, lp: &[usize], index: &LoopIndex) -> Option { let start_v = self.loop_start(lp)?; let mut cur = start_v; for &x in lp { @@ -528,9 +552,8 @@ impl VectorNetwork { if cur == start_v { return None; } - let in_loop = |i: usize| self.regions.iter().flat_map(|r| r.loops.iter().flatten()).any(|&x| x == i); - let ends = |s: &Segment| (s.a == cur && s.b == start_v) || (s.b == cur && s.a == start_v); - let mut joining = self.segments.iter().enumerate().filter(|(i, s)| ends(s) && !in_loop(*i)).map(|(i, _)| i); + let mut joining = + index.by_ends.get(&(cur.min(start_v), cur.max(start_v)))?.iter().copied().filter(|&i| !index.in_loop.get(i).copied().unwrap_or(false)); let first = joining.next()?; joining.next().is_none().then_some(first) } diff --git a/crates/engine/src/canvas/vectoredit/ops.rs b/crates/engine/src/canvas/vectoredit/ops.rs index 733885de1..f4609dc1e 100644 --- a/crates/engine/src/canvas/vectoredit/ops.rs +++ b/crates/engine/src/canvas/vectoredit/ops.rs @@ -470,17 +470,28 @@ fn reopen_loops(net: &mut VectorNetwork) { } // An open chain whose two ends a segment in no loop already joins (a bridge drawn between two // points before the point between them was deleted) closes through that segment, at the end so - // the loop's winding is kept, as `connect` closes it (#1951). One loop at a time, so two loops - // can't both take one segment. - let places: Vec<(usize, usize)> = net.regions.iter().enumerate().flat_map(|(ri, r)| (0..r.loops.len()).map(move |li| (ri, li))).collect(); - for (ri, li) in places { - let Some(lp) = net.regions.get(ri).and_then(|r| r.loops.get(li)) else { continue }; - if lp.len() < 2 { - continue; + // the loop's winding is kept, as `connect` closes it (#1951). Every loop that walks the same + // chain (two regions over one ring) takes it, as the render does; a different chain can't + // share it. The lookups are built once, so a segment one loop took doesn't hide it from the + // next. + let index = net.loop_index(); + let mut closings: Vec<(usize, usize, usize, Vec)> = Vec::new(); + for (ri, r) in net.regions.iter().enumerate() { + for (li, lp) in r.loops.iter().enumerate() { + if lp.len() < 2 { + continue; + } + if let Some(closing) = net.closing_segment(lp, &index) { + let mut chain = lp.clone(); + chain.sort_unstable(); + if closings.iter().all(|(_, _, c, other)| *c != closing || *other == chain) { + closings.push((ri, li, closing, chain)); + } + } } - if let Some(closing) = net.closing_segment(lp) - && let Some(lp) = net.regions.get_mut(ri).and_then(|r| r.loops.get_mut(li)) - { + } + for (ri, li, closing, _) in closings { + if let Some(lp) = net.regions.get_mut(ri).and_then(|r| r.loops.get_mut(li)) { lp.push(closing); } } @@ -1209,4 +1220,91 @@ mod tests { assert!(ends.iter().any(|x| (x - (centre.x - 0.5)).abs() < 0.01), "{ends:?}"); assert!(ends.iter().any(|x| (x - (centre.x + 0.5)).abs() < 0.01), "{ends:?}"); } + + /// An open chain of four segments over five points whose two ends a fifth segment (in no + /// loop) joins, as a bridge drawn before the point between them was deleted. + fn bridged_open_chain() -> VectorNetwork { + let corners = [ + Point::new(0.0, 0.0), + Point::new(525.0, 525.0), + Point::new(0.0, 525.0), + Point::new(333.0, 0.0), + Point::new(525.0, 183.0), + Point::new(900.0, 900.0), + ]; + let mut n = VectorNetwork::from_points(&corners, false); + n.segments = [(0, 3), (1, 2), (2, 0), (4, 1), (3, 4)].iter().map(|&(a, b)| Segment::line(a, b)).collect(); + n.regions.clear(); + for v in &mut n.vertices { + v.radius = 125.0; + } + n + } + + #[test] + fn regions_over_one_open_chain_all_close_through_the_joining_segment() { + use figcraft_geom::BezPath; + use figcraft_geom::kurbo::ParamCurveArclen; + let mut n = bridged_open_chain(); + for _ in 0..2 { + n.regions.push(Region { loops: vec![vec![3, 1, 2, 0]], winding: WindingRule::NonZero, fills: None, ..Default::default() }); + } + delete_vertices(&mut n, &[5], false); + for (i, r) in n.regions.iter().enumerate() { + assert_eq!(r.loops[0], vec![3, 1, 2, 0, 4], "region {i} closes through the segment: {:?}", n.regions); + } + let length = |path: &BezPath| path.segments().map(|s| s.arclen(1e-9)).sum::(); + let stroke = length(&n.stroke_path()); + let rounded = n.rounded().unwrap(); + for i in 0..2 { + let fill = length(&rounded.region_path(i).unwrap()); + assert!((fill - stroke).abs() < 1e-6, "region {i}: fill {fill} and stroke {stroke} trace the same outline"); + } + } + + #[test] + fn a_segment_joining_two_different_chains_goes_to_the_first_only() { + let mut n = bridged_open_chain(); + n.regions.push(Region { loops: vec![vec![3, 1, 2, 0]], winding: WindingRule::NonZero, fills: None, ..Default::default() }); + n.regions.push(Region { loops: vec![vec![0, 2, 1, 3]], winding: WindingRule::NonZero, fills: None, ..Default::default() }); + reopen_loops(&mut n); + assert_eq!(n.regions[0].loops[0].len(), 5); + assert_eq!(n.regions[1].loops[0].len(), 5, "the same segment set is the same chain"); + let mut m = bridged_open_chain(); + m.segments.push(Segment::line(1, 2)); + m.regions.push(Region { loops: vec![vec![3, 1, 2, 0]], winding: WindingRule::NonZero, fills: None, ..Default::default() }); + m.regions.push(Region { loops: vec![vec![3, 5, 2, 0]], winding: WindingRule::NonZero, fills: None, ..Default::default() }); + reopen_loops(&mut m); + let closed = m.regions.iter().filter(|r| r.loops[0].len() == 5).count(); + assert!(closed <= 1, "different chains don't share the segment: {:?}", m.regions); + } + + #[test] + fn many_open_loops_with_no_closing_segment_round_as_before() { + let mut n = VectorNetwork::from_points(&[Point::new(0.0, 0.0), Point::new(10.0, 0.0)], false); + n.segments.clear(); + n.vertices.clear(); + for k in 0..40 { + let x = f64::from(k) * 100.0; + let base = n.vertices.len(); + for p in [(x, 0.0), (x + 50.0, 0.0), (x + 50.0, 50.0), (x, 50.0)] { + n.vertices.push(Vertex { radius: 8.0, ..Vertex::new(p.0, p.1) }); + } + let first = n.segments.len(); + // Three sides of a square, an open loop with nothing to close it. + for (a, b) in [(0, 1), (1, 2), (2, 3)] { + n.segments.push(Segment::line(base + a, base + b)); + } + n.regions.push(Region { + loops: vec![vec![first, first + 1, first + 2]], + winding: WindingRule::NonZero, + fills: None, + ..Default::default() + }); + } + let r = n.rounded().unwrap(); + assert_eq!(r.regions.len(), 40); + // Each loop gained only its two inner corners' arcs (two corners, one arc each). + assert!(r.regions.iter().all(|reg| reg.loops[0].len() == 5), "{:?}", r.regions[0]); + } }