diff --git a/lightningbeam-ui/lightningbeam-core/src/action.rs b/lightningbeam-ui/lightningbeam-core/src/action.rs index ce9ae4e..a0c4321 100644 --- a/lightningbeam-ui/lightningbeam-core/src/action.rs +++ b/lightningbeam-ui/lightningbeam-core/src/action.rs @@ -132,6 +132,10 @@ pub struct ActionExecutor { /// Maximum number of actions to keep in undo stack max_undo_depth: usize, + + /// Monotonically increasing counter, bumped on every `execute` call. + /// Used to detect whether any actions were taken during a region selection. + epoch: u64, } impl ActionExecutor { @@ -144,6 +148,7 @@ impl ActionExecutor { undo_stack: Vec::new(), redo_stack: Vec::new(), max_undo_depth: 100, // Default: keep last 100 actions + epoch: 0, } } @@ -188,6 +193,9 @@ impl ActionExecutor { // Clear redo stack (new action invalidates redo history) self.redo_stack.clear(); + // Bump epoch so region selections can detect that an action occurred + self.epoch = self.epoch.wrapping_add(1); + // Add to undo stack self.undo_stack.push(action); @@ -289,6 +297,16 @@ impl ActionExecutor { self.redo_stack.len() } + /// Return the current action epoch. + /// + /// The epoch is a monotonically increasing counter that is bumped every + /// time `execute` is called. It is never decremented on undo/redo, so + /// callers can record it at a point in time and later compare to detect + /// whether any action was executed in the interim. + pub fn epoch(&self) -> u64 { + self.epoch + } + /// Clear all undo/redo history pub fn clear_history(&mut self) { self.undo_stack.clear(); @@ -335,6 +353,7 @@ impl ActionExecutor { // 3. Push to undo stack (both succeeded) self.redo_stack.clear(); + self.epoch = self.epoch.wrapping_add(1); self.undo_stack.push(action); // Limit undo stack size diff --git a/lightningbeam-ui/lightningbeam-core/src/dcel2/region.rs b/lightningbeam-ui/lightningbeam-core/src/dcel2/region.rs index 522ead8..5c25b15 100644 --- a/lightningbeam-ui/lightningbeam-core/src/dcel2/region.rs +++ b/lightningbeam-ui/lightningbeam-core/src/dcel2/region.rs @@ -9,7 +9,7 @@ //! Faces are classified by which vertices they touch — no sampling needed. use super::{Dcel, EdgeId, FaceId, VertexId}; -use kurbo::{BezPath, Point, Shape}; +use kurbo::{BezPath, CubicBez, Point, Shape}; /// Vertex classification relative to the region boundary. #[derive(Clone, Copy, PartialEq, Eq, Debug)] @@ -171,6 +171,88 @@ impl Dcel { } } } + + /// Merge a modified `selected` DCEL back into `self` (the snapshot DCEL). + /// + /// Call this when deselecting after uncommitted modifications (e.g. a move). + /// `self` is the snapshot; inside-vertex positions and edge curves are + /// updated from `selected`, then the invisible region boundary edges are + /// removed to restore a unified DCEL. + pub fn merge_back_from_selected( + &mut self, + selected: &Dcel, + inside_vertices: &[VertexId], + _boundary_vertices: &[VertexId], + region_edge_ids: &[EdgeId], + ) { + // Step 1: Copy inside vertex positions from selected_dcel. + for &vid in inside_vertices { + if vid.idx() < selected.vertices.len() && !selected.vertices[vid.idx()].deleted { + if vid.idx() < self.vertices.len() && !self.vertices[vid.idx()].deleted { + self.vertices[vid.idx()].position = selected.vertices[vid.idx()].position; + } + } + } + + // Step 2: Update curves for edges whose endpoints changed. + let inside_set: std::collections::HashSet = + inside_vertices.iter().copied().collect(); + + for i in 0..self.edges.len() { + let e = &self.edges[i]; + if e.deleted { continue; } + let [fwd, bwd] = e.half_edges; + let v1 = self.half_edges[fwd.idx()].origin; + let v2 = self.half_edges[bwd.idx()].origin; + let v1_inside = inside_set.contains(&v1); + let v2_inside = inside_set.contains(&v2); + + if v1_inside && v2_inside { + // Fully inside — copy transformed curve from selected. + if i < selected.edges.len() && !selected.edges[i].deleted { + self.edges[i].curve = selected.edges[i].curve; + } + } else if v1_inside || v2_inside { + // Mixed edge: one endpoint moved, one is boundary (fixed). + // Approximate with a straight line to preserve connectivity. + let p0 = self.vertices[v1.idx()].position; + let p3 = self.vertices[v2.idx()].position; + self.edges[i].curve = line_to_cubic_pts(p0, p3); + } + } + + // Step 3: Remove invisible region boundary edges to heal face splits. + for &eid in region_edge_ids { + if eid.idx() < self.edges.len() && !self.edges[eid.idx()].deleted { + self.remove_edge(eid); + } + } + + // Step 4: Clean up stale inner half-edge references left by remove_edge. + self.repair_stale_inner_half_edges(); + } + + /// Remove stale `inner_half_edges` entries that point to deleted half-edges. + /// + /// After `remove_edge` calls, some faces' hole-boundary references can point + /// to half-edges that no longer exist. This cleans them up. + pub fn repair_stale_inner_half_edges(&mut self) { + for face in self.faces.iter_mut() { + face.inner_half_edges.retain(|&he| { + he.idx() < self.half_edges.len() && !self.half_edges[he.idx()].deleted + }); + } + } +} + +/// Convert two points into a degenerate cubic (straight-line cubic bezier). +fn line_to_cubic_pts(p0: Point, p3: Point) -> CubicBez { + CubicBez::new( + p0, + Point::new(p0.x + (p3.x - p0.x) / 3.0, p0.y + (p3.y - p0.y) / 3.0), + Point::new(p0.x + 2.0 * (p3.x - p0.x) / 3.0, p0.y + 2.0 * (p3.y - p0.y) / 3.0), + p3, + ) } #[cfg(test)] @@ -178,6 +260,46 @@ mod tests { use super::*; use kurbo::{CubicBez, Point}; + /// After `extract_region`, verify no face in `dcel` has half-edges from + /// both Inside-classified and Outside-classified vertices. + fn assert_no_face_straddles(dcel: &Dcel, region: &BezPath, boundary_verts: &[VertexId]) { + #[derive(Clone, Copy, PartialEq, Eq, Debug)] + enum VClass { Inside, Outside, Boundary } + + let classes: Vec = dcel.vertices.iter().enumerate().map(|(i, v)| { + if v.deleted { return VClass::Outside; } + let vid = VertexId(i as u32); + if boundary_verts.contains(&vid) { + VClass::Boundary + } else if region.winding(v.position) != 0 { + VClass::Inside + } else { + VClass::Outside + } + }).collect(); + + for i in 1..dcel.faces.len() { + let face = &dcel.faces[i]; + if face.deleted || face.outer_half_edge.is_none() { continue; } + let boundary = dcel.face_boundary(FaceId(i as u32)); + let mut has_inside = false; + let mut has_outside = false; + for he_id in &boundary { + let vid = dcel.half_edges[he_id.idx()].origin; + match classes[vid.idx()] { + VClass::Inside => has_inside = true, + VClass::Outside => has_outside = true, + VClass::Boundary => {} + } + } + assert!( + !(has_inside && has_outside), + "face {} straddles boundary (has both Inside and Outside vertices)", + i + ); + } + } + fn line_cubic(a: Point, b: Point) -> CubicBez { CubicBez::new( a, @@ -190,6 +312,41 @@ mod tests { ) } + #[test] + fn extract_region_no_face_straddles() { + let mut dcel = Dcel::new(); + + // Two horizontal lines spanning the region boundary. + // After insert_stroke splits them, boundary vertices appear at x=50. + let a0 = Point::new(0.0, 30.0); + let a1 = Point::new(100.0, 30.0); + let b0 = Point::new(0.0, 70.0); + let b1 = Point::new(100.0, 70.0); + let va0 = dcel.alloc_vertex(a0); + let va1 = dcel.alloc_vertex(a1); + let vb0 = dcel.alloc_vertex(b0); + let vb1 = dcel.alloc_vertex(b1); + dcel.insert_edge(va0, va1, FaceId(0), line_cubic(a0, a1)); + dcel.insert_edge(vb0, vb1, FaceId(0), line_cubic(b0, b1)); + + let mut region = BezPath::new(); + region.move_to(Point::new(-10.0, -10.0)); + region.line_to(Point::new(50.0, -10.0)); + region.line_to(Point::new(50.0, 110.0)); + region.line_to(Point::new(-10.0, 110.0)); + region.close_path(); + + // Simulate boundary splitting: mid-points at x=50 + let v_amid = dcel.alloc_vertex(Point::new(50.0, 30.0)); + let v_bmid = dcel.alloc_vertex(Point::new(50.0, 70.0)); + let boundary_verts = vec![v_amid, v_bmid]; + + let extracted = dcel.extract_region(®ion, &boundary_verts); + + assert_no_face_straddles(&dcel, ®ion, &boundary_verts); + assert_no_face_straddles(&extracted, ®ion, &boundary_verts); + } + #[test] fn extract_region_basic() { let mut dcel = Dcel::new(); @@ -232,6 +389,97 @@ mod tests { assert_eq!(ext_edges, 0, "edges span boundary → removed from extracted"); } + /// Replicate: draw filled rect (0,0)-(100,100), region-select top-left corner + /// with selection (-50,-50)-(50,50). Goal: validate passes on both DCELs. + #[test] + fn rect_corner_region_select() { + let mut dcel = Dcel::new(); + + // Draw rectangle: W=(0,0) X=(100,0) Y=(100,100) Z=(0,100) + let w = Point::new(0.0, 0.0); + let x = Point::new(100.0, 0.0); + let y = Point::new(100.0, 100.0); + let z = Point::new(0.0, 100.0); + + dcel.insert_stroke(&[ + line_cubic(w, x), + line_cubic(x, y), + line_cubic(y, z), + line_cubic(z, w), + ], None, None, 1.0); + + // Simulate paint-bucket: find the inner face cycle and create F1 + let face_query = dcel.find_face_at_point(Point::new(50.0, 50.0)); + eprintln!("paint bucket face query: {:?}, he: {:?}", face_query.face, face_query.cycle_he); + if !face_query.cycle_he.is_none() && face_query.face.0 == 0 { + let f1 = dcel.create_face_at_cycle(face_query.cycle_he); + eprintln!("created F{} for interior", f1.0); + } + + dcel.validate(); + eprintln!("DCEL valid after rect+paint"); + + // Selection boundary: A=(-50,-50) B=(50,-50) C=(50,50) D=(-50,50) + let a = Point::new(-50.0, -50.0); + let b = Point::new(50.0, -50.0); + let c = Point::new(50.0, 50.0); + let d = Point::new(-50.0, 50.0); + + let mut region_path = BezPath::new(); + region_path.move_to(a); + region_path.line_to(b); + region_path.line_to(c); + region_path.line_to(d); + region_path.close_path(); + + let stroke_result = dcel.insert_stroke(&[ + line_cubic(a, b), + line_cubic(b, c), + line_cubic(c, d), + line_cubic(d, a), + ], None, None, 1.0); + + eprintln!("after insert_stroke: {} new edges, {} new faces", + stroke_result.new_edges.len(), stroke_result.new_faces.len()); + dcel.validate(); + eprintln!("DCEL valid after selection boundary insert"); + + // Dump face structure after insert_stroke + for (i, face) in dcel.faces.iter().enumerate() { + if face.deleted { continue; } + let ohe = face.outer_half_edge; + if ohe.is_none() { + eprintln!(" F{i}: outer_he=NONE (unbounded)"); + } else { + let cycle = dcel.walk_cycle(ohe); + let origins: Vec<_> = cycle.iter().map(|&he| { + let vid = dcel.half_edges[he.idx()].origin; + dcel.vertices[vid.idx()].position + }).collect(); + eprintln!(" F{i}: {} HEs, vertices: {:?}", cycle.len(), origins); + } + } + + let boundary_verts: Vec = stroke_result.new_vertices.clone(); + let mut extracted = dcel.extract_region(®ion_path, &boundary_verts); + + eprintln!("after extract_region"); + // Check face_boundary on all non-F0 faces + for (i, face) in dcel.faces.iter().enumerate() { + if face.deleted || i == 0 { continue; } + let b = dcel.face_boundary(FaceId(i as u32)); + eprintln!(" self F{i}: {} boundary HEs", b.len()); + } + for (i, face) in extracted.faces.iter().enumerate() { + if face.deleted || i == 0 { continue; } + let b = extracted.face_boundary(FaceId(i as u32)); + eprintln!(" extracted F{i}: {} boundary HEs", b.len()); + } + dcel.validate(); + extracted.validate(); + eprintln!("both DCELs valid after extract_region"); + } + #[test] fn extract_region_with_boundary_vertices() { let mut dcel = Dcel::new(); @@ -269,4 +517,228 @@ mod tests { assert_eq!(ext_edges, 1, "extracted should have left edge"); assert_eq!(self_edges, 1, "self should have right edge"); } + + /// Replicate: two rectangles, region-select an area that doesn't intersect either. + /// The selected area is to the left of both rectangles, selecting empty space. + /// Goal: validate passes on both DCELs and walk_cycle doesn't infinite-loop. + #[test] + fn rect_empty_region_select() { + let mut dcel = Dcel::new(); + + // Rect 1: (220,353)-(290,379) + let r1a = Point::new(220.0, 353.0); + let r1b = Point::new(290.0, 353.0); + let r1c = Point::new(290.0, 379.0); + let r1d = Point::new(220.0, 379.0); + dcel.insert_stroke(&[ + line_cubic(r1a, r1b), line_cubic(r1b, r1c), + line_cubic(r1c, r1d), line_cubic(r1d, r1a), + ], None, None, 1.0); + + // Rect 2: (304,383)-(551,483) + let r2a = Point::new(304.0, 383.0); + let r2b = Point::new(551.0, 383.0); + let r2c = Point::new(551.0, 483.0); + let r2d = Point::new(304.0, 483.0); + dcel.insert_stroke(&[ + line_cubic(r2a, r2b), line_cubic(r2b, r2c), + line_cubic(r2c, r2d), line_cubic(r2d, r2a), + ], None, None, 1.0); + + dcel.validate(); + + // Selection: (136,332)-(208,347) — entirely to the left of both rectangles + let sel_min = Point::new(136.0, 332.0); + let sel_max = Point::new(208.0, 347.0); + let a = sel_min; + let b = Point::new(sel_max.x, sel_min.y); + let c = sel_max; + let d = Point::new(sel_min.x, sel_max.y); + + let mut region_path = BezPath::new(); + region_path.move_to(a); + region_path.line_to(b); + region_path.line_to(c); + region_path.line_to(d); + region_path.close_path(); + + let stroke_result = dcel.insert_stroke(&[ + line_cubic(a, b), line_cubic(b, c), + line_cubic(c, d), line_cubic(d, a), + ], None, None, 1.0); + + dcel.validate(); + + let boundary_verts = stroke_result.new_vertices.clone(); + let extracted = dcel.extract_region(®ion_path, &boundary_verts); + + // Walk all face boundaries in both DCELs — must not infinite-loop + for (i, face) in dcel.faces.iter().enumerate() { + if face.deleted { continue; } + let _b = dcel.face_boundary(FaceId(i as u32)); + for &ihe in &face.inner_half_edges { + if !ihe.is_none() && !dcel.half_edges[ihe.idx()].deleted { + let _c = dcel.walk_cycle(ihe); + } + } + } + for (i, face) in extracted.faces.iter().enumerate() { + if face.deleted { continue; } + let _b = extracted.face_boundary(FaceId(i as u32)); + for &ihe in &face.inner_half_edges { + if !ihe.is_none() && !extracted.half_edges[ihe.idx()].deleted { + let _c = extracted.walk_cycle(ihe); + } + } + } + + dcel.validate(); + extracted.validate(); + } + + /// Selection entirely inside a filled face: the original face must survive. + /// + /// Before this fix, `insert_edge_both_connected` would overwrite F1's + /// `outer_half_edge` with the spur back-chain, causing F1 to lose its + /// rectangle boundary and visually disappear. + #[test] + fn rect_inner_region_select_preserves_face() { + use crate::shape::ShapeColor; + let red = ShapeColor::rgb(255, 0, 0); + let mut dcel = Dcel::new(); + + // Build a rectangle: (100,100)-(300,200) + let tl = Point::new(100.0, 100.0); + let tr = Point::new(300.0, 100.0); + let br = Point::new(300.0, 200.0); + let bl = Point::new(100.0, 200.0); + dcel.insert_stroke(&[ + line_cubic(tl, tr), line_cubic(tr, br), + line_cubic(br, bl), line_cubic(bl, tl), + ], None, None, 1.0); + + // Simulate paint-bucket: create face for rectangle interior and assign fill. + let fq = dcel.find_face_at_point(Point::new(200.0, 150.0)); + let f1 = if !fq.cycle_he.is_none() && fq.face.0 == 0 { + dcel.create_face_at_cycle(fq.cycle_he) + } else { + fq.face + }; + dcel.faces[f1.idx()].fill_color = Some(red); + dcel.validate(); + + // Confirm F1 has fill + assert!(dcel.faces.iter().skip(1).any(|f| !f.deleted && f.fill_color.is_some()), + "rect face should have fill before selection"); + + // Selection: (150,130)-(250,170) — entirely inside the rectangle + let sa = Point::new(150.0, 130.0); + let sb = Point::new(250.0, 130.0); + let sc = Point::new(250.0, 170.0); + let sd = Point::new(150.0, 170.0); + let mut region_path = BezPath::new(); + region_path.move_to(sa); + region_path.line_to(sb); + region_path.line_to(sc); + region_path.line_to(sd); + region_path.close_path(); + + let sel_result = dcel.insert_stroke(&[ + line_cubic(sa, sb), line_cubic(sb, sc), + line_cubic(sc, sd), line_cubic(sd, sa), + ], None, None, 1.0); + dcel.validate(); + + let boundary_verts = sel_result.new_vertices.clone(); + let extracted = dcel.extract_region(®ion_path, &boundary_verts); + + dcel.validate(); + extracted.validate(); + + // The live DCEL must still have a filled face (the original rectangle). + let live_filled = dcel.faces.iter().enumerate() + .filter(|(i, f)| *i > 0 && !f.deleted && f.fill_color.is_some()) + .count(); + assert!(live_filled >= 1, + "live DCEL should still have at least one filled face after selection; got {live_filled}"); + + // The extracted DCEL must also have a filled face (selection interior, inherits fill). + let extracted_filled = extracted.faces.iter().enumerate() + .filter(|(i, f)| *i > 0 && !f.deleted && f.fill_color.is_some()) + .count(); + assert!(extracted_filled >= 1, + "extracted DCEL should have at least one filled face; got {extracted_filled}"); + } + + /// Replicate: multiple consecutive region-selects on two rectangles, + /// some of which select empty space. Verifies no crash accumulates. + #[test] + fn multiple_region_selects() { + fn make_rect_dcel() -> Dcel { + let mut dcel = Dcel::new(); + let r1a = Point::new(220.0, 353.0); + let r1b = Point::new(290.0, 353.0); + let r1c = Point::new(290.0, 379.0); + let r1d = Point::new(220.0, 379.0); + dcel.insert_stroke(&[ + line_cubic(r1a, r1b), line_cubic(r1b, r1c), + line_cubic(r1c, r1d), line_cubic(r1d, r1a), + ], None, None, 1.0); + let r2a = Point::new(304.0, 383.0); + let r2b = Point::new(551.0, 383.0); + let r2c = Point::new(551.0, 483.0); + let r2d = Point::new(304.0, 483.0); + dcel.insert_stroke(&[ + line_cubic(r2a, r2b), line_cubic(r2b, r2c), + line_cubic(r2c, r2d), line_cubic(r2d, r2a), + ], None, None, 1.0); + dcel + } + + let selects: &[(f64, f64, f64, f64)] = &[ + (438.0, 326.0, 614.0, 445.0), // intersects rect 2 + (181.0, 312.0, 407.0, 448.0), // intersects both + (460.0, 293.0, 516.0, 507.0), // intersects rect 2 + (167.0, 311.0, 380.0, 437.0), // intersects both + (690.0, 360.0, 703.0, 448.0), // empty (right) + (136.0, 332.0, 208.0, 347.0), // empty (left) — crashes in original bug + ]; + + for (min_x, min_y, max_x, max_y) in selects { + let mut dcel = make_rect_dcel(); + let a = Point::new(*min_x, *min_y); + let b = Point::new(*max_x, *min_y); + let c = Point::new(*max_x, *max_y); + let d = Point::new(*min_x, *max_y); + + let mut region_path = BezPath::new(); + region_path.move_to(a); + region_path.line_to(b); + region_path.line_to(c); + region_path.line_to(d); + region_path.close_path(); + + let stroke_result = dcel.insert_stroke(&[ + line_cubic(a, b), line_cubic(b, c), + line_cubic(c, d), line_cubic(d, a), + ], None, None, 1.0); + dcel.validate(); + + let boundary_verts = stroke_result.new_vertices.clone(); + let extracted = dcel.extract_region(®ion_path, &boundary_verts); + + // Walk all face boundaries and inner cycles — must not infinite-loop + for (i, face) in extracted.faces.iter().enumerate() { + if face.deleted { continue; } + let _b = extracted.face_boundary(FaceId(i as u32)); + for &ihe in &face.inner_half_edges { + if !ihe.is_none() && !extracted.half_edges[ihe.idx()].deleted { + let _ = extracted.walk_cycle(ihe); + } + } + } + dcel.validate(); + extracted.validate(); + } + } } diff --git a/lightningbeam-ui/lightningbeam-core/src/dcel2/topology.rs b/lightningbeam-ui/lightningbeam-core/src/dcel2/topology.rs index edbb937..fc63d5b 100644 --- a/lightningbeam-ui/lightningbeam-core/src/dcel2/topology.rs +++ b/lightningbeam-ui/lightningbeam-core/src/dcel2/topology.rs @@ -122,7 +122,7 @@ impl Dcel { self.insert_edge_both_connected(he_fwd, he_bwd, v1, v2, edge_id, &curve, face) } _ => { - self.insert_edge_one_isolated(he_fwd, he_bwd, v1, v2, edge_id, &curve, v1_isolated) + self.insert_edge_one_isolated(he_fwd, he_bwd, v1, v2, edge_id, &curve, v1_isolated, face) } } } @@ -169,6 +169,7 @@ impl Dcel { edge_id: EdgeId, curve: &CubicBez, v1_is_isolated: bool, + face_hint: FaceId, ) -> (EdgeId, FaceId) { let (connected, isolated) = if v1_is_isolated { (v2, v1) } else { (v1, v2) }; @@ -187,7 +188,21 @@ impl Dcel { }; let ccw_succ = self.find_ccw_successor(connected, out_angle); let he_into = self.half_edges[ccw_succ.idx()].prev; - let actual_face = self.half_edges[he_into.idx()].face; + let angular_face = self.half_edges[he_into.idx()].face; + + // If angular ordering disagrees with face_hint, try to find the correct + // sector using find_predecessor_on_face — same logic as insert_edge_both_connected. + let (he_into, ccw_succ, actual_face) = if angular_face != face_hint { + if let Some((alt_into, alt_ccw)) = + self.find_predecessor_on_face(connected, out_angle, face_hint) + { + (alt_into, alt_ccw, face_hint) + } else { + (he_into, ccw_succ, angular_face) + } + } else { + (he_into, ccw_succ, angular_face) + }; // Splice: ... → he_into → [he_out → he_back] → ccw_succ → ... self.half_edges[he_into.idx()].next = he_out; @@ -247,6 +262,25 @@ impl Dcel { debug_assert_eq!(face_v1, face_v2); (into_v1, into_v2, ccw_v1, ccw_v2, face_v1) } + } else if face_v1 != face_v2 { + // Angular ordering disagrees between the two endpoints. + // Trust face_hint (midpoint probe) as the authoritative face — + // it correctly determines which face the edge's interior lies in, + // regardless of which angular sector each vertex landed in. + let target = face_hint; + let fix_v1 = if face_v1 == target { + (into_v1, ccw_v1) + } else { + self.find_predecessor_on_face(v1, fwd_angle, target) + .unwrap_or((into_v1, ccw_v1)) + }; + let fix_v2 = if face_v2 == target { + (into_v2, ccw_v2) + } else { + self.find_predecessor_on_face(v2, bwd_angle, target) + .unwrap_or((into_v2, ccw_v2)) + }; + (fix_v1.0, fix_v2.0, fix_v1.1, fix_v2.1, target) } else { debug_assert_eq!( face_v1, face_v2, @@ -297,6 +331,25 @@ impl Dcel { // the old face's outer_half_edge. let old_ohe = self.faces[actual_face.idx()].outer_half_edge; let fwd_has_old = !old_ohe.is_none() && self.cycle_contains(he_fwd, old_ohe); + let bwd_has_old = !fwd_has_old && !old_ohe.is_none() && self.cycle_contains(he_bwd, old_ohe); + + if !fwd_has_old && !bwd_has_old { + // Neither new cycle contains the face's existing outer boundary. + // This happens when the edge closes a loop entirely within the face + // (all selection vertices are isolated/floating, never connected to the + // face's real boundary). Don't overwrite outer_half_edge — it still + // correctly points to the face's real boundary (e.g. the rectangle). + // he_fwd = the enclosed interior (new face F2); he_bwd = reverse + // traversal of the selection boundary (stays in actual_face). + self.assign_cycle_face(he_bwd, actual_face); + let new_face = self.alloc_face(); + self.faces[new_face.idx()].fill_color = self.faces[actual_face.idx()].fill_color; + self.faces[new_face.idx()].image_fill = self.faces[actual_face.idx()].image_fill; + self.faces[new_face.idx()].fill_rule = self.faces[actual_face.idx()].fill_rule; + self.faces[new_face.idx()].outer_half_edge = he_fwd; + self.assign_cycle_face(he_fwd, new_face); + return (edge_id, new_face); + } let (he_old_cycle, he_new_cycle) = if fwd_has_old { (he_fwd, he_bwd) diff --git a/lightningbeam-ui/lightningbeam-core/src/selection.rs b/lightningbeam-ui/lightningbeam-core/src/selection.rs index 6facb2a..08247a7 100644 --- a/lightningbeam-ui/lightningbeam-core/src/selection.rs +++ b/lightningbeam-ui/lightningbeam-core/src/selection.rs @@ -201,6 +201,16 @@ impl Selection { } } + /// Select a face by ID only, without adding boundary edges or vertices. + /// + /// Use this when the geometry lives in a separate DCEL (e.g. region selection's + /// `selected_dcel`) so we don't add stale edge/vertex IDs to the selection. + pub fn select_face_id_only(&mut self, face_id: FaceId) { + if !face_id.is_none() && face_id.0 != 0 { + self.selected_faces.insert(face_id); + } + } + /// Select a face and all its boundary edges + vertices. pub fn select_face(&mut self, face_id: FaceId, dcel: &Dcel) { if face_id.is_none() || face_id.0 == 0 || dcel.face(face_id).deleted { @@ -429,6 +439,17 @@ pub struct RegionSelection { pub transform: Affine, /// Whether the selection has been committed (via an operation on the selection) pub committed: bool, + /// Non-boundary vertices that are strictly inside the region (for merge-back). + pub inside_vertices: Vec, + /// Region boundary intersection vertices (for merge-back and fill propagation). + pub boundary_vertices: Vec, + /// IDs of the invisible edges inserted for the region boundary stroke. + /// Removing these during merge-back heals the face splits they created. + pub region_edge_ids: Vec, + /// Action epoch recorded when this selection was created. + /// Compared against `ActionExecutor::epoch()` on deselect to decide + /// whether merge-back is needed or a clean snapshot restore suffices. + pub action_epoch_at_selection: u64, } #[cfg(test)] diff --git a/lightningbeam-ui/lightningbeam-core/src/test_mode.rs b/lightningbeam-ui/lightningbeam-core/src/test_mode.rs index ae88c54..0f70cc0 100644 --- a/lightningbeam-ui/lightningbeam-core/src/test_mode.rs +++ b/lightningbeam-ui/lightningbeam-core/src/test_mode.rs @@ -88,6 +88,10 @@ pub struct TestCase { pub ended_with_panic: bool, pub panic_message: Option, pub panic_backtrace: Option, + /// Serialized geometry context at the time of the crash (e.g. DCEL + region path). + /// Populated by set_pending_geometry before risky operations. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub geometry_context: Option, } impl TestCase { @@ -102,6 +106,7 @@ impl TestCase { ended_with_panic: false, panic_message: None, panic_backtrace: None, + geometry_context: None, } } diff --git a/lightningbeam-ui/lightningbeam-editor/src/main.rs b/lightningbeam-ui/lightningbeam-editor/src/main.rs index 73c96c7..aca75bf 100644 --- a/lightningbeam-ui/lightningbeam-editor/src/main.rs +++ b/lightningbeam-ui/lightningbeam-editor/src/main.rs @@ -194,17 +194,23 @@ fn main() -> eframe::Result { let test_mode_is_replaying: std::sync::Arc = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); #[cfg(debug_assertions)] + let test_mode_pending_geometry: std::sync::Arc>> = + std::sync::Arc::new(std::sync::Mutex::new(None)); + #[cfg(debug_assertions)] let test_mode_panic_snapshot_for_app = test_mode_panic_snapshot.clone(); #[cfg(debug_assertions)] let test_mode_pending_event_for_app = test_mode_pending_event.clone(); #[cfg(debug_assertions)] let test_mode_is_replaying_for_app = test_mode_is_replaying.clone(); + #[cfg(debug_assertions)] + let test_mode_pending_geometry_for_app = test_mode_pending_geometry.clone(); #[cfg(debug_assertions)] { let panic_snapshot = test_mode_panic_snapshot.clone(); let pending_event = test_mode_pending_event.clone(); let is_replaying = test_mode_is_replaying.clone(); + let pending_geometry = test_mode_pending_geometry.clone(); let test_dir = directories::ProjectDirs::from("", "", "lightningbeam") .map(|dirs| dirs.data_dir().join("test_cases")) .unwrap_or_else(|| std::path::PathBuf::from("test_cases")); @@ -219,7 +225,7 @@ fn main() -> eframe::Result { format!("{}", info) }; let backtrace = format!("{}", std::backtrace::Backtrace::force_capture()); - test_mode::TestModeState::record_panic(&panic_snapshot, &pending_event, &is_replaying, msg, backtrace, &test_dir); + test_mode::TestModeState::record_panic(&panic_snapshot, &pending_event, &is_replaying, &pending_geometry, msg, backtrace, &test_dir); default_hook(info); })); } @@ -229,7 +235,7 @@ fn main() -> eframe::Result { options, Box::new(move |cc| { #[cfg(debug_assertions)] - let app = EditorApp::new(cc, layouts, theme, test_mode_panic_snapshot_for_app, test_mode_pending_event_for_app, test_mode_is_replaying_for_app); + let app = EditorApp::new(cc, layouts, theme, test_mode_panic_snapshot_for_app, test_mode_pending_event_for_app, test_mode_is_replaying_for_app, test_mode_pending_geometry_for_app); #[cfg(not(debug_assertions))] let app = EditorApp::new(cc, layouts, theme); Ok(Box::new(app)) @@ -936,6 +942,7 @@ impl EditorApp { #[cfg(debug_assertions)] panic_snapshot: std::sync::Arc>>, #[cfg(debug_assertions)] pending_event: std::sync::Arc>>, #[cfg(debug_assertions)] is_replaying: std::sync::Arc, + #[cfg(debug_assertions)] pending_geometry: std::sync::Arc>>, ) -> Self { let current_layout = layouts[0].layout.clone(); @@ -1144,7 +1151,7 @@ impl EditorApp { // Debug test mode (F5) #[cfg(debug_assertions)] - test_mode: test_mode::TestModeState::new(panic_snapshot, pending_event, is_replaying), + test_mode: test_mode::TestModeState::new(panic_snapshot, pending_event, is_replaying, pending_geometry), // Debug overlay (F3) cursor_cache: custom_cursor::CursorCache::new(), diff --git a/lightningbeam-ui/lightningbeam-editor/src/panes/stage.rs b/lightningbeam-ui/lightningbeam-editor/src/panes/stage.rs index 2d111fe..0e2d97d 100644 --- a/lightningbeam-ui/lightningbeam-editor/src/panes/stage.rs +++ b/lightningbeam-ui/lightningbeam-editor/src/panes/stage.rs @@ -1138,7 +1138,7 @@ impl egui_wgpu::CallbackTrait for VelloCallback { // Render selected DCEL from active region selection (with transform) if let Some(ref region_sel) = self.ctx.region_selection { - let sel_transform = camera_transform * region_sel.transform; + let sel_transform = overlay_transform * region_sel.transform; lightningbeam_core::renderer::render_dcel( ®ion_sel.selected_dcel, &mut scene, @@ -1153,6 +1153,27 @@ impl egui_wgpu::CallbackTrait for VelloCallback { scene }; + // Render region selection fill into the overlay scene. + // In HDR mode the main scene-building block returns an empty scene (only layer content + // goes through the HDR pipeline), so we must add the selected-DCEL fill here so it + // appears underneath the stipple overlay. In legacy mode the render_dcel call inside + // the block already handled this, but running it again is harmless since `scene` would + // be a fresh empty scene only in HDR mode. + if USE_HDR_COMPOSITING { + if let Some(ref region_sel) = self.ctx.region_selection { + let sel_transform = overlay_transform * region_sel.transform; + let mut image_cache = shared.image_cache.lock().unwrap(); + lightningbeam_core::renderer::render_dcel( + ®ion_sel.selected_dcel, + &mut scene, + sel_transform, + 1.0, + &self.ctx.document, + &mut image_cache, + ); + } + } + // Render drag preview objects with transparency if let (Some(delta), Some(active_layer_id)) = (self.ctx.drag_delta, self.ctx.active_layer_id) { if let Some(layer) = self.ctx.document.get_layer(&active_layer_id) { @@ -1526,18 +1547,6 @@ impl egui_wgpu::CallbackTrait for VelloCallback { _ => {} } - // 2c. Draw active region selection boundary - if let Some(ref region_sel) = self.ctx.region_selection { - // Draw the region boundary as a dashed outline - let boundary_color = Color::from_rgba8(255, 150, 0, 150); - scene.stroke( - &Stroke::new(1.0).with_dashes(0.0, &[6.0, 4.0]), - overlay_transform, - boundary_color, - None, - ®ion_sel.region_path, - ); - } // 3. Draw rectangle creation preview if let lightningbeam_core::tool::ToolState::CreatingRectangle { ref start_point, ref current_point, centered, constrain_square, .. } = self.ctx.tool_state { @@ -2796,6 +2805,12 @@ impl StagePane { None => return, // No active layer }; + // Revert any active region selection on mouse press before borrowing the document + // immutably, so the two selection modes don't coexist. + if self.rsp_primary_pressed(ui) { + Self::revert_region_selection_static(shared); + } + let active_layer = match shared.action_executor.document().get_layer(&active_layer_id) { Some(layer) => layer, None => return, @@ -4121,8 +4136,10 @@ impl StagePane { // Mouse down: start region selection if self.rsp_drag_started(response) { - // Revert any existing uncommitted region selection + // Revert any existing uncommitted region selection, and clear the + // regular selection so both selection modes don't coexist. Self::revert_region_selection_static(shared); + shared.selection.clear(); match *shared.region_select_mode { RegionSelectMode::Rectangle => { @@ -4255,9 +4272,28 @@ impl StagePane { return; } + // Capture DCEL snapshot + region path for crash diagnosis (debug builds only) + #[cfg(debug_assertions)] + { + use vello::kurbo::PathEl; + let path_elems: Vec = region_path.elements().iter().map(|el| match el { + PathEl::MoveTo(p) => serde_json::json!({"type": "M", "x": p.x, "y": p.y}), + PathEl::LineTo(p) => serde_json::json!({"type": "L", "x": p.x, "y": p.y}), + PathEl::QuadTo(p1, p2) => serde_json::json!({"type": "Q", "x1": p1.x, "y1": p1.y, "x2": p2.x, "y2": p2.y}), + PathEl::CurveTo(p1, p2, p3) => serde_json::json!({"type": "C", "x1": p1.x, "y1": p1.y, "x2": p2.x, "y2": p2.y, "x3": p3.x, "y3": p3.y}), + PathEl::ClosePath => serde_json::json!({"type": "Z"}), + }).collect(); + let geom = serde_json::json!({ + "region_path": path_elems, + "dcel_snapshot": serde_json::to_value(&snapshot).unwrap_or(serde_json::Value::Null), + }); + shared.test_mode.set_pending_geometry(geom); + } + // Insert region boundary as invisible edges (no stroke style/color) let stroke_result = dcel.insert_stroke(&segments, None, None, 1.0); - let boundary_verts = stroke_result.new_vertices; + let boundary_verts: Vec<_> = stroke_result.new_vertices.clone(); + let region_edge_ids: Vec<_> = stroke_result.new_edges.clone(); // Extract the inside portion; self (dcel) keeps the outside + boundary. let mut selected_dcel = dcel.extract_region(®ion_path, &boundary_verts); @@ -4276,11 +4312,38 @@ impl StagePane { if !has_visible { // Nothing visible inside — restore snapshot and bail *dcel = snapshot; + #[cfg(debug_assertions)] + shared.test_mode.clear_pending_geometry(); return; } + // Compute inside_vertices: non-deleted verts in selected_dcel that aren't boundary. + let inside_vertices: Vec<_> = selected_dcel + .vertices + .iter() + .enumerate() + .filter_map(|(i, v)| { + if v.deleted { return None; } + let vid = lightningbeam_core::dcel::VertexId(i as u32); + if !boundary_verts.contains(&vid) { Some(vid) } else { None } + }) + .collect(); + + let action_epoch = shared.action_executor.epoch(); + shared.selection.clear(); + // Populate global selection with the faces from the extracted DCEL so + // property panels and other tools can see what is selected. We add face + // IDs only (no boundary edges/vertices) because the boundary geometry + // lives in selected_dcel, not in the live DCEL. + for (i, face) in selected_dcel.faces.iter().enumerate() { + if face.deleted || i == 0 { continue; } + if face.fill_color.is_some() || face.image_fill.is_some() { + shared.selection.select_face_id_only(lightningbeam_core::dcel::FaceId(i as u32)); + } + } + // Store region selection state with extracted DCEL *shared.region_selection = Some(lightningbeam_core::selection::RegionSelection { region_path, @@ -4290,7 +4353,14 @@ impl StagePane { selected_dcel, transform: vello::kurbo::Affine::IDENTITY, committed: false, + inside_vertices, + boundary_vertices: boundary_verts, + region_edge_ids, + action_epoch_at_selection: action_epoch, }); + + #[cfg(debug_assertions)] + shared.test_mode.clear_pending_geometry(); } /// Revert an uncommitted region selection, restoring the DCEL from snapshot @@ -4307,11 +4377,26 @@ impl StagePane { return; } - // Restore the DCEL from the snapshot taken before boundary insertion + let no_actions_taken = + shared.action_executor.epoch() == region_sel.action_epoch_at_selection; + let doc = shared.action_executor.document_mut(); if let Some(AnyLayer::Vector(vl)) = doc.get_layer_mut(®ion_sel.layer_id) { if let Some(dcel) = vl.dcel_at_time_mut(region_sel.time) { - *dcel = region_sel.dcel_snapshot; + if no_actions_taken { + // Nothing changed: restore snapshot cleanly (undo boundary insertion) + *dcel = region_sel.dcel_snapshot; + } else { + // Actions were applied to the selection: merge selected_dcel back + let mut merged = region_sel.dcel_snapshot; + merged.merge_back_from_selected( + ®ion_sel.selected_dcel, + ®ion_sel.inside_vertices, + ®ion_sel.boundary_vertices, + ®ion_sel.region_edge_ids, + ); + *dcel = merged; + } } } diff --git a/lightningbeam-ui/lightningbeam-editor/src/test_mode.rs b/lightningbeam-ui/lightningbeam-editor/src/test_mode.rs index b5fc6ce..0872fe7 100644 --- a/lightningbeam-ui/lightningbeam-editor/src/test_mode.rs +++ b/lightningbeam-ui/lightningbeam-editor/src/test_mode.rs @@ -192,6 +192,9 @@ pub struct TestModeState { /// Current in-flight event, set before processing. If a panic occurs during /// processing, the panic hook appends this to the saved test case. pub pending_event: Arc>>, + /// Geometry context set before risky operations (e.g. region select). + /// Shared with the panic hook so crashes include the relevant geometry. + pub pending_geometry: Arc>>, /// Always-on ring buffer of last N events (for crash capture outside test mode) pub event_ring: VecDeque, pub ring_start_time: Instant, @@ -205,7 +208,7 @@ pub struct TestModeState { } impl TestModeState { - pub fn new(panic_snapshot: Arc>>, pending_event: Arc>>, is_replaying: Arc) -> Self { + pub fn new(panic_snapshot: Arc>>, pending_event: Arc>>, is_replaying: Arc, pending_geometry: Arc>>) -> Self { let test_dir = directories::ProjectDirs::from("", "", "lightningbeam") .map(|dirs| dirs.data_dir().join("test_cases")) .unwrap_or_else(|| PathBuf::from("test_cases")); @@ -219,6 +222,7 @@ impl TestModeState { status_message: None, panic_snapshot, pending_event, + pending_geometry, event_ring: VecDeque::with_capacity(RING_BUFFER_SIZE), ring_start_time: Instant::now(), ring_event_count: 0, @@ -228,6 +232,22 @@ impl TestModeState { } } + /// Store geometry context for panic capture. + /// Called before risky operations (e.g. region select) so the panic hook + /// can include it in the crash file for easier reproduction. + pub fn set_pending_geometry(&self, context: serde_json::Value) { + if let Ok(mut guard) = self.pending_geometry.try_lock() { + *guard = Some(context); + } + } + + /// Clear the pending geometry context (call after the operation succeeds). + pub fn clear_pending_geometry(&self) { + if let Ok(mut guard) = self.pending_geometry.try_lock() { + *guard = None; + } + } + /// Store the current in-flight event for panic capture. /// Called before processing so the panic hook can grab it if processing panics. pub fn set_pending_event(&self, kind: TestEventKind) { @@ -339,6 +359,7 @@ impl TestModeState { panic_snapshot: &Arc>>, pending_event: &Arc>>, is_replaying: &Arc, + pending_geometry: &Arc>>, msg: String, backtrace: String, test_dir: &PathBuf, @@ -372,6 +393,11 @@ impl TestModeState { } } + // Attach geometry context if one was set before the crash + if let Ok(mut geom_guard) = pending_geometry.try_lock() { + test_case.geometry_context = geom_guard.take(); + } + test_case.ended_with_panic = true; test_case.panic_message = Some(msg); test_case.panic_backtrace = Some(backtrace);