From 39978e59b3ca80b01920648e5061e8346f17edde Mon Sep 17 00:00:00 2001 From: Skyler Lehmkuhl Date: Sun, 21 Jun 2026 14:45:47 -0400 Subject: [PATCH] Unify selection systems and make region/lasso cut robust Collapse the two parallel selection systems into one. The RegionSelect tool (rect + lasso) now cuts the geometry along the region outline and selects the resulting sub-pieces into the standard `Selection` ID-sets, exactly like every other tool. The vestigial floating `RegionSelection` (drag never wired; commit/delete/copy were stubbed) and all its plumbing are removed, so Group, Convert-to-Movie-Clip, Delete, and Properties all operate uniformly from lasso, rect, marquee, and click selections. Region cutting is reworked onto a robust planar arrangement: - Replace fragile incremental "split a fill by one cut edge" logic with planar face re-tracing (`retrace_fills_after_cut` + `trace_faces`), which correctly handles arbitrary holed/concave fills. - `extract_subgraph` no longer frees vertices still referenced by kept boundary edges (fixed Group leaving freed-but-referenced vertices that a later alloc reused and corrupted). - `split_fill_by_*` direction fix (was producing disconnected boundaries rendered as stray diagonals). - `fill_interior_point` (area-centroid + inward-step fallback) for reliable inside/outside classification of non-convex pieces. - Coincident-edge dedupe + degenerate-fill removal (edge-adjacent shapes no longer make zero-area sliver fills). - Dangling-edge pruning, near-coincident endpoint welding, induced- subgraph expansion, and tracking of `split_edge` sub-edges, so self-intersecting freehand lassos cut correctly. Region-select capture is available behind LIGHTNINGBEAM_DUMP_REGION=1 for turning a misbehaving cut into a deterministic test. Extensive regression tests added in vector_graph/tests/region_cut_select.rs. --- .../src/actions/clip_from_geometry.rs | 76 +- .../lightningbeam-core/src/selection.rs | 39 +- .../src/vector_graph/mod.rs | 658 +++++++++++++++--- .../src/vector_graph/tests/mod.rs | 2 + .../vector_graph/tests/region_cut_select.rs | 609 ++++++++++++++++ .../lightningbeam-editor/src/main.rs | 98 +-- .../lightningbeam-editor/src/panes/mod.rs | 6 +- .../lightningbeam-editor/src/panes/stage.rs | 329 +++------ 8 files changed, 1364 insertions(+), 453 deletions(-) create mode 100644 lightningbeam-ui/lightningbeam-core/src/vector_graph/tests/region_cut_select.rs diff --git a/lightningbeam-ui/lightningbeam-core/src/actions/clip_from_geometry.rs b/lightningbeam-ui/lightningbeam-core/src/actions/clip_from_geometry.rs index fdfd36b..bbd418e 100644 --- a/lightningbeam-ui/lightningbeam-core/src/actions/clip_from_geometry.rs +++ b/lightningbeam-ui/lightningbeam-core/src/actions/clip_from_geometry.rs @@ -4,15 +4,61 @@ //! //! A *group* (`is_group = true`) is a static container; a *movie clip* (`is_group = //! false`) has its own timeline. Both are tweenable via the clip instance's transform. +//! +//! Both the Select tool and the (cut-and-select) RegionSelect tool populate the same +//! `Selection` ID sets, so a single entry point — [`extract_geometry_to_clip`] — handles +//! Group and Convert-to-Movie-Clip from whatever is selected. use std::collections::HashSet; use crate::clip::{ClipInstance, VectorClip}; use crate::document::Document; use crate::layer::{AnyLayer, ShapeKeyframe, VectorLayer}; +use crate::object::Transform; use crate::vector_graph::{EdgeId, FillId, VectorGraph}; use uuid::Uuid; +/// Build a clip holding `sub_graph` and place a `ClipInstance` (with `transform`) on the +/// layer. Shared by both extraction paths. Does **not** touch the source graph — the +/// caller is responsible for having removed the moved geometry first. +fn assemble_clip_from_graph( + document: &mut Document, + layer_id: Uuid, + time: f64, + sub_graph: VectorGraph, + transform: Transform, + clip_id: Uuid, + instance_id: Uuid, + is_group: bool, + clip_name: &str, +) { + let (doc_w, doc_h, doc_dur) = (document.width, document.height, document.duration.max(1.0)); + + // A vector layer whose single keyframe holds the extracted graph (in the source's + // coordinate space, so an identity/translation placement renders it in place). + let mut inner = VectorLayer::new("Layer 1"); + let mut kf = ShapeKeyframe::new(0.0); + kf.graph = sub_graph; + inner.keyframes.push(kf); + let mut clip = VectorClip::with_id(clip_id, clip_name, doc_w, doc_h, doc_dur); + clip.is_group = is_group; + clip.layers.add_root(AnyLayer::Vector(inner)); + document.add_vector_clip(clip); + + let mut instance = ClipInstance::with_id(instance_id, clip_id); + instance.transform = transform; + if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { + // Groups gate visibility by the active keyframe's clip_instance_ids; movie + // clips render unconditionally. + if is_group { + if let Some(kf) = vl.keyframe_at_mut(time) { + kf.clip_instance_ids.push(instance_id); + } + } + vl.clip_instances.push(instance); + } +} + /// Extract the selected geometry into a new clip + place a `ClipInstance`. Returns the /// pre-extraction graph snapshot for undo. `clip_id`/`instance_id` are caller-provided /// so undo/redo is stable. The selection sets come straight from the editor selection @@ -32,7 +78,6 @@ pub fn extract_geometry_to_clip( if fills.is_empty() && edges.is_empty() { return Err("No geometry selected".to_string()); } - let (doc_w, doc_h, doc_dur) = (document.width, document.height, document.duration.max(1.0)); // 1. Extract from the source graph (extract_subgraph removes the moved geometry). let (graph_before, sub_graph) = { @@ -48,29 +93,11 @@ pub fn extract_geometry_to_clip( (before, sub) }; - // 2. Build the clip: a vector layer whose single keyframe holds the extracted graph - // (in the source's coordinate space, so identity placement renders it in place). - let mut inner = VectorLayer::new("Layer 1"); - let mut kf = ShapeKeyframe::new(0.0); - kf.graph = sub_graph; - inner.keyframes.push(kf); - let mut clip = VectorClip::with_id(clip_id, clip_name, doc_w, doc_h, doc_dur); - clip.is_group = is_group; - clip.layers.add_root(AnyLayer::Vector(inner)); - document.add_vector_clip(clip); - - // 3. Place a ClipInstance (identity transform → geometry stays put). - let instance = ClipInstance::with_id(instance_id, clip_id); - if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) { - // Groups gate visibility by the active keyframe's clip_instance_ids; movie - // clips render unconditionally. - if is_group { - if let Some(kf) = vl.keyframe_at_mut(time) { - kf.clip_instance_ids.push(instance_id); - } - } - vl.clip_instances.push(instance); - } + // 2 & 3. Build the clip + place an identity-transform instance (geometry stays put). + assemble_clip_from_graph( + document, layer_id, time, sub_graph, Transform::default(), + clip_id, instance_id, is_group, clip_name, + ); Ok(graph_before) } @@ -96,3 +123,4 @@ pub fn undo_extract_geometry( } } } + diff --git a/lightningbeam-ui/lightningbeam-core/src/selection.rs b/lightningbeam-ui/lightningbeam-core/src/selection.rs index 0da1b5b..af4da81 100644 --- a/lightningbeam-ui/lightningbeam-core/src/selection.rs +++ b/lightningbeam-ui/lightningbeam-core/src/selection.rs @@ -4,9 +4,8 @@ use crate::vector_graph::{VectorGraph, EdgeId, FillId, VertexId}; use serde::{Deserialize, Serialize}; -use std::collections::{HashMap, HashSet}; +use std::collections::HashSet; use uuid::Uuid; -use vello::kurbo::{Affine, BezPath}; /// Shape of a raster pixel selection, in canvas pixel coordinates. #[derive(Clone, Debug)] @@ -440,42 +439,6 @@ impl Selection { } } -/// Represents a temporary region-based selection. -/// -/// When a region select is active, the region boundary is inserted into the -/// DCEL as invisible edges, splitting existing geometry. Faces inside the -/// region are added to the normal `Selection`. If the user performs an -/// operation, the selection is committed; if they deselect, the DCEL is -/// restored from the snapshot. -#[derive(Clone, Debug)] -pub struct RegionSelection { - /// The clipping region as a closed BezPath (polygon or rect) - pub region_path: BezPath, - /// Layer containing the affected elements - pub layer_id: Uuid, - /// Keyframe time - pub time: f64, - /// Snapshot of the graph before region boundary insertion, for revert - pub graph_snapshot: VectorGraph, - /// The extracted graph containing geometry inside the region - pub selected_graph: VectorGraph, - /// Transform applied to the selected graph (e.g. from dragging) - pub transform: Affine, - /// Whether the selection has been committed (via an operation on the selection) - pub committed: bool, - /// IDs of the invisible edges inserted for the region boundary stroke. - /// These exist in the main graph (remainder side). Deleted during merge-back. - 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, - /// selected_graph VID → main graph VID for boundary vertices (shared between both graphs). - pub boundary_vertex_map: HashMap, - /// selected_graph boundary EID → main graph boundary EID (duplicated edges to skip on merge). - pub boundary_edge_map: HashMap, -} - #[cfg(test)] mod tests { use super::*; diff --git a/lightningbeam-ui/lightningbeam-core/src/vector_graph/mod.rs b/lightningbeam-ui/lightningbeam-core/src/vector_graph/mod.rs index 4aeb980..451d6e2 100644 --- a/lightningbeam-ui/lightningbeam-core/src/vector_graph/mod.rs +++ b/lightningbeam-ui/lightningbeam-core/src/vector_graph/mod.rs @@ -417,6 +417,91 @@ impl VectorGraph { self.boundary_to_bezpath(&fill.boundary) } + /// A point guaranteed to lie inside the fill — for point-in-region classification + /// (e.g. deciding whether a fill is inside a lasso). Prefers the polygon area-centroid, + /// but for a non-convex fill (e.g. an L-shape, where the area-centroid can fall in the + /// concavity *outside* the shape) it steps just inward from a boundary edge instead. + /// The naive average of boundary-edge midpoints is NOT reliable here — it can land + /// outside a non-convex fill and misclassify it. + pub fn fill_interior_point(&self, fill_id: FillId) -> Point { + let boundary = self.fills[fill_id.idx()].boundary.clone(); + self.boundary_interior_point(&boundary) + } + + /// A point guaranteed to lie inside the region enclosed by a `(edge, direction)` + /// boundary loop. See [`fill_interior_point`]. + pub fn boundary_interior_point(&self, boundary: &[(EdgeId, Direction)]) -> Point { + use kurbo::{ParamCurve, Shape, Vec2}; + let path = self.boundary_to_bezpath(boundary); + + // Ordered polygon corners: the directed start point of each boundary edge. + let mut pts: Vec = Vec::new(); + for &(eid, dir) in boundary { + if eid.is_none() { + continue; + } + let c = self.edges[eid.idx()].curve; + pts.push(match dir { + Direction::Forward => c.p0, + Direction::Backward => c.p3, + }); + } + if pts.len() < 3 { + if pts.is_empty() { + return Point::ZERO; + } + let (sx, sy) = pts.iter().fold((0.0, 0.0), |(x, y), p| (x + p.x, y + p.y)); + return Point::new(sx / pts.len() as f64, sy / pts.len() as f64); + } + + // Shoelace area-centroid. + let (mut a2, mut cx, mut cy) = (0.0, 0.0, 0.0); + for i in 0..pts.len() { + let p0 = pts[i]; + let p1 = pts[(i + 1) % pts.len()]; + let cross = p0.x * p1.y - p1.x * p0.y; + a2 += cross; + cx += (p0.x + p1.x) * cross; + cy += (p0.y + p1.y) * cross; + } + if a2.abs() > 1e-9 { + let c = Point::new(cx / (3.0 * a2), cy / (3.0 * a2)); + if path.winding(c) != 0 { + return c; + } + } + + // Fallback: step a small distance inward from a boundary edge midpoint. + let (mut minx, mut miny, mut maxx, mut maxy) = (f64::MAX, f64::MAX, f64::MIN, f64::MIN); + for p in &pts { + minx = minx.min(p.x); + miny = miny.min(p.y); + maxx = maxx.max(p.x); + maxy = maxy.max(p.y); + } + let eps = ((maxx - minx).min(maxy - miny) * 1e-3).max(1e-4); + for &(eid, _) in boundary { + if eid.is_none() { + continue; + } + let c = self.edges[eid.idx()].curve; + let mid = c.eval(0.5); + let tangent = c.eval(0.5001) - c.eval(0.4999); + let len = tangent.hypot(); + if len < 1e-12 { + continue; + } + let n = Vec2::new(-tangent.y / len, tangent.x / len); + for s in [1.0_f64, -1.0] { + let cand = mid + n * (s * eps); + if path.winding(cand) != 0 { + return cand; + } + } + } + pts[0] + } + // ------------------------------------------------------------------- // Vertex editing // ------------------------------------------------------------------- @@ -653,6 +738,10 @@ impl VectorGraph { } let mut all_new_edges = Vec::new(); + // Sub-edges produced when a stroke segment splits an existing edge (incl. an earlier + // segment of this same stroke). Tracked separately so they reach the fill re-tracer + // without polluting the returned edge list. + let mut split_products: Vec = Vec::new(); let mut prev_end_vertex: Option = None; for (seg_idx, seg) in expanded_segments.iter().enumerate() { @@ -706,7 +795,14 @@ impl VectorGraph { let remapped_t = original_edge_t / head_end; let remapped_t = remapped_t.clamp(ENDPOINT_T_MARGIN, 1.0 - ENDPOINT_T_MARGIN); - let (mid_v, _sub_a, _sub_b) = self.split_edge(eid, remapped_t); + let (mid_v, sub_a, sub_b) = self.split_edge(eid, remapped_t); + // Track the split products so the fill re-tracer sees them: when a later + // stroke segment crosses an earlier one, these sub-edges are part of the + // stroke's arrangement but would otherwise be invisible to the re-trace. + // (Kept out of `all_new_edges` so the returned edge list stays the stroke's + // own edges only.) + split_products.push(sub_a); + split_products.push(sub_b); // Snap vertex to intersection point self.vertices[mid_v.idx()].position = point; // Merge with nearby existing vertex if within snap distance @@ -791,127 +887,460 @@ impl VectorGraph { prev_end_vertex = Some(end_v); } - // Fill splitting pass: for each new edge, check if both endpoints - // lie on any fill's boundary — if so, split that fill. - let edges_to_check = all_new_edges.clone(); - for &eid in &edges_to_check { - let v0 = self.edges[eid.idx()].vertices[0]; - let v1 = self.edges[eid.idx()].vertices[1]; + // Weld dangling stroke endpoints onto a near-coincident existing vertex. A + // self-intersecting freehand stroke can create an intersection vertex a fraction of + // a pixel away from a segment endpoint that should be the same point; if they don't + // merge, the stroke's loop is broken by a degree-1 stub and the cut is lost. + self.weld_dangling_endpoints(&all_new_edges, snap_epsilon.max(1.0)); - // Find fills where both v0 and v1 appear as boundary vertices - let fill_ids: Vec = self.fills - .iter() - .enumerate() - .filter(|(_, f)| !f.deleted) - .filter(|(_, f)| { - let has_v0 = f.boundary.iter().any(|&(be, _)| { - let e = &self.edges[be.idx()]; - e.vertices[0] == v0 || e.vertices[1] == v0 - }); - let has_v1 = f.boundary.iter().any(|&(be, _)| { - let e = &self.edges[be.idx()]; - e.vertices[0] == v1 || e.vertices[1] == v1 - }); - has_v0 && has_v1 - }) - .map(|(i, _)| FillId(i as u32)) - .collect(); + // Coincident-edge cleanup: a new edge that lands exactly on an existing edge + // between the same two vertices (e.g. drawing a shape whose edge snaps onto an + // existing one) must not be duplicated — duplicates produce zero-area "sliver" + // fills. Merge such duplicates before splitting/filling. + self.dedupe_coincident_new_edges(&all_new_edges); - for fid in fill_ids { - self.split_fill_by_edge(fid, eid); - } - } + // Re-derive the fills touched by the new edges. Rather than incrementally splitting + // along single cut edges (which can't handle a lasso whose path is interrupted by a + // hole/notch in a non-convex fill), we re-trace the planar faces of the affected + // sub-arrangement and rebuild the fills from them. This is robust for arbitrary + // holed/concave fills (e.g. cutting across geometry left behind by a prior group). + // Include split products so a self-crossing stroke's full arrangement is seen. + let mut retrace_edges = all_new_edges.clone(); + retrace_edges.extend(split_products); + self.retrace_fills_after_cut(&retrace_edges); + + // Drop any zero-area fills (e.g. slivers left between coincident edges). + self.remove_degenerate_fills(); all_new_edges } - /// When a new edge splits a fill (both endpoints on the fill's boundary), - /// split the fill into two fills. - pub fn split_fill_by_edge( - &mut self, - fill_id: FillId, - splitting_edge: EdgeId, - ) -> Option<(FillId, FillId)> { - let fill = &self.fills[fill_id.idx()]; - if fill.deleted { - return None; - } - - let split_v0 = self.edges[splitting_edge.idx()].vertices[0]; - let split_v1 = self.edges[splitting_edge.idx()].vertices[1]; - - // Find the positions in the boundary where the splitting edge's - // endpoint vertices appear as the "arrival" vertex of a directed edge. - let boundary = fill.boundary.clone(); - - // Helper: get the "end" vertex of a directed boundary edge - let end_vertex = |eid: EdgeId, dir: Direction| -> VertexId { - match dir { - Direction::Forward => self.edges[eid.idx()].vertices[1], - Direction::Backward => self.edges[eid.idx()].vertices[0], + /// Merge edges from the latest stroke that are geometrically coincident with an + /// existing edge between the same two vertices (drawing a shape whose edge lands exactly + /// on an existing edge). Keeps the existing edge, redirects fill references, and frees + /// the duplicate — preventing zero-area "sliver" fills between the two copies. + /// Weld degree-1 endpoints of freshly inserted edges onto a near-coincident existing + /// vertex. A self-intersecting freehand stroke can create an intersection vertex a + /// fraction of a pixel from a segment endpoint that should be the same point; if they + /// don't merge, the stroke's loop is broken by a degree-1 stub and the cut is lost. + fn weld_dangling_endpoints(&mut self, new_edges: &[EdgeId], eps: f64) { + // Repeatedly weld the closest dangling new endpoint to its nearest neighbour. + loop { + let mut to_merge: Option<(VertexId, VertexId)> = None; // (keep, merge) + 'scan: for &e in new_edges { + if self.edges[e.idx()].deleted { + continue; + } + for &v in &self.edges[e.idx()].vertices { + if self.vertices[v.idx()].deleted || self.edges_at_vertex(v).len() != 1 { + continue; + } + let pv = self.vertices[v.idx()].position; + let mut best: Option<(f64, VertexId)> = None; + for ui in 0..self.vertices.len() { + if ui == v.idx() || self.vertices[ui].deleted { + continue; + } + let pu = self.vertices[ui].position; + let d = (pu.x - pv.x).hypot(pu.y - pv.y); + if d < eps && best.map_or(true, |(bd, _)| d < bd) { + best = Some((d, VertexId(ui as u32))); + } + } + if let Some((_, keep)) = best { + to_merge = Some((keep, v)); + break 'scan; + } + } } + match to_merge { + Some((keep, merge)) => self.merge_vertices(keep, merge), + None => break, + } + } + // Drop any edges that collapsed to zero length (both endpoints welded together). + let degenerate: Vec = self + .edges + .iter() + .enumerate() + .filter(|(_, e)| !e.deleted && e.vertices[0] == e.vertices[1]) + .map(|(i, _)| EdgeId(i as u32)) + .collect(); + for e in degenerate { + for fill in &mut self.fills { + if !fill.deleted { + fill.boundary.retain(|&(fe, _)| fe != e); + } + } + self.free_edge(e); + } + } + + fn dedupe_coincident_new_edges(&mut self, new_edges: &[EdgeId]) { + for &ne in new_edges { + if self.edges[ne.idx()].deleted { + continue; + } + let va = self.edges[ne.idx()].vertices[0]; + let vb = self.edges[ne.idx()].vertices[1]; + let candidates: Vec = self + .edges_at_vertex(va) + .into_iter() + .filter(|&e| e != ne && !self.edges[e.idx()].deleted) + .filter(|&e| { + let v = self.edges[e.idx()].vertices; + (v[0] == va && v[1] == vb) || (v[0] == vb && v[1] == va) + }) + .collect(); + for c in candidates { + if self.edges[c.idx()].deleted { + continue; + } + if self.curves_coincident(ne, c) { + self.redirect_edge_in_fills(ne, c); + self.free_edge(ne); + break; + } + } + } + } + + /// Whether two edges (already known to share both endpoints) trace the same path. + /// Coincident duplicates in practice are straight collinear segments (a shape edge + /// snapping onto an existing edge), so we treat "both are straight lines between the + /// same endpoints" as coincident. Comparing curve `eval(t)` directly is unreliable — + /// split sub-edges are non-uniformly parameterised, so equal `t` ≠ equal point. + fn curves_coincident(&self, a: EdgeId, b: EdgeId) -> bool { + let is_straight = |c: kurbo::CubicBez| { + let chord = c.p3 - c.p0; + let len = chord.hypot(); + if len < 1e-9 { + return true; // zero-length chord — degenerate, treat as coincident + } + // Perpendicular distance of each control point from the p0→p3 chord. + let dist = |p: Point| ((p - c.p0).cross(chord)).abs() / len; + dist(c.p1) < 1e-2 && dist(c.p2) < 1e-2 }; + is_straight(self.edges[a.idx()].curve) && is_straight(self.edges[b.idx()].curve) + } - // Find positions where boundary edges arrive at split_v0 and split_v1 - let mut pos_v0: Option = None; - let mut pos_v1: Option = None; - - for (i, &(eid, dir)) in boundary.iter().enumerate() { - let ev = end_vertex(eid, dir); - if ev == split_v0 && pos_v0.is_none() { - pos_v0 = Some(i); + /// Replace every `from` boundary reference with `to`, preserving traversal direction. + fn redirect_edge_in_fills(&mut self, from: EdgeId, to: EdgeId) { + let f = self.edges[from.idx()].vertices; + let t = self.edges[to.idx()].vertices; + let same_dir = t[0] == f[0] && t[1] == f[1]; + for fill in &mut self.fills { + if fill.deleted { + continue; } - if ev == split_v1 && pos_v1.is_none() { - pos_v1 = Some(i); + for entry in &mut fill.boundary { + if entry.0 == from { + entry.1 = match (entry.1, same_dir) { + (Direction::Forward, true) | (Direction::Backward, false) => { + Direction::Forward + } + (Direction::Backward, true) | (Direction::Forward, false) => { + Direction::Backward + } + }; + entry.0 = to; + } } } + } - let pos_v0 = pos_v0?; - let pos_v1 = pos_v1?; + /// Drop fills that enclose ~zero area (degenerate slivers from coincident edges). + fn remove_degenerate_fills(&mut self) { + for i in 0..self.fills.len() { + if self.fills[i].deleted { + continue; + } + let mut pts: Vec = Vec::new(); + for &(eid, dir) in &self.fills[i].boundary { + if eid.is_none() { + continue; + } + let c = self.edges[eid.idx()].curve; + pts.push(match dir { + Direction::Forward => c.p0, + Direction::Backward => c.p3, + }); + } + let area = if pts.len() < 3 { + 0.0 + } else { + let mut a2 = 0.0; + for k in 0..pts.len() { + let p0 = pts[k]; + let p1 = pts[(k + 1) % pts.len()]; + a2 += p0.x * p1.y - p1.x * p0.y; + } + (a2 * 0.5).abs() + }; + if area < 1e-6 { + self.fills[i].deleted = true; + self.free_fills.push(i as u32); + } + } + } - // Ensure we have two distinct positions - if pos_v0 == pos_v1 { - return None; + /// Directed end vertex of a `(edge, direction)` boundary entry. + #[inline] + fn entry_end_vertex(&self, eid: EdgeId, dir: Direction) -> VertexId { + match dir { + Direction::Forward => self.edges[eid.idx()].vertices[1], + Direction::Backward => self.edges[eid.idx()].vertices[0], + } + } + + /// Re-derive the fills touched by a freshly inserted stroke by re-tracing the planar + /// faces of the affected sub-arrangement. This replaces incremental "split a fill by a + /// cut edge" logic, which can't handle a cut whose path is interrupted by a hole/notch + /// in a non-convex fill. Each affected fill is deleted and rebuilt from the traced + /// faces that lie inside it (inheriting its colour/rule); faces outside it — or in a + /// hole — are dropped. + fn retrace_fills_after_cut(&mut self, new_edges: &[EdgeId]) { + use kurbo::{ParamCurve, Shape}; + let new_set: HashSet = new_edges + .iter() + .filter(|&&e| !e.is_none() && !self.edges[e.idx()].deleted) + .copied() + .collect(); + if new_set.is_empty() { + return; + } + let new_verts: HashSet = + new_set.iter().flat_map(|&e| self.edges[e.idx()].vertices).collect(); + + // Affected fills: any non-deleted fill that shares a vertex with a new edge. + let affected: Vec = (0..self.fills.len()) + .filter(|&i| !self.fills[i].deleted) + .filter(|&i| { + self.fills[i].boundary.iter().any(|&(e, _)| { + !e.is_none() + && self.edges[e.idx()].vertices.iter().any(|v| new_verts.contains(v)) + }) + }) + .map(|i| FillId(i as u32)) + .collect(); + if affected.is_empty() { + return; } - // Walk boundary in two halves: - // Half A: from pos_v0+1 to pos_v1 (inclusive), then splitting_edge Forward - // Half B: from pos_v1+1 to pos_v0 (wrapping), then splitting_edge Backward - let n = boundary.len(); - let color = fill.color; - let fill_rule = fill.fill_rule; + // Snapshot each affected fill's path + attributes before we delete them. + let originals: Vec<(kurbo::BezPath, Option, FillRule)> = affected + .iter() + .map(|&f| { + ( + self.fill_to_bezpath(f), + self.fills[f.idx()].color, + self.fills[f.idx()].fill_rule, + ) + }) + .collect(); - let mut half_a = Vec::new(); - let mut idx = (pos_v0 + 1) % n; + // Edge set for the local arrangement: every affected fill's boundary edges plus the + // ENTIRE inserted stroke. We include the whole stroke (not just the segments whose + // midpoint is inside a fill) because a wiggly freehand lasso has segments that dip + // just outside the fill; excluding them would break the inside-arc chain into + // dangling fragments that then get pruned away, losing the cut entirely. The stroke + // forms closed loops, so it contributes no dangling edges; faces that end up outside + // every affected fill are discarded by the classification below. + let mut edge_set: HashSet = HashSet::new(); + for &f in &affected { + for &(e, _) in &self.fills[f.idx()].boundary { + if !e.is_none() && !self.edges[e.idx()].deleted { + edge_set.insert(e); + } + } + } + edge_set.extend(new_set.iter().copied()); + + // Expand to the induced subgraph on the covered vertices. A self-intersecting + // freehand stroke splits its own edges via `split_edge`, whose sub-edges aren't in + // `new_edges`; without them the local arrangement has gaps and the stroke's loop + // looks like dangling fragments. Adding every edge whose endpoints are both already + // covered closes those gaps (the sub-edges connect already-covered stroke vertices). loop { - half_a.push(boundary[idx]); - if idx == pos_v1 { + let verts: HashSet = edge_set + .iter() + .flat_map(|&e| self.edges[e.idx()].vertices) + .collect(); + let added: Vec = (0..self.edges.len()) + .map(|i| EdgeId(i as u32)) + .filter(|&e| !self.edges[e.idx()].deleted && !edge_set.contains(&e)) + .filter(|&e| { + let [a, b] = self.edges[e.idx()].vertices; + verts.contains(&a) && verts.contains(&b) + }) + .collect(); + if added.is_empty() { break; } - idx = (idx + 1) % n; + edge_set.extend(added); } - half_a.push((splitting_edge, Direction::Forward)); - let mut half_b = Vec::new(); - idx = (pos_v1 + 1) % n; + // Prune dangling edges (a vertex with degree < 2 in the local arrangement). They + // form spikes, never real face boundaries — a freehand lasso that wiggles or nearly + // self-touches leaves such stubs, and tracing them produces a face that runs out and + // back along the same edge (a degenerate self-touching boundary). Iterate, since + // removing one stub can expose another. loop { - half_b.push(boundary[idx]); - if idx == pos_v0 { + let mut degree: HashMap = HashMap::new(); + for &e in &edge_set { + for &v in &self.edges[e.idx()].vertices { + *degree.entry(v).or_default() += 1; + } + } + let dangling: Vec = edge_set + .iter() + .copied() + .filter(|&e| { + let [a, b] = self.edges[e.idx()].vertices; + a == b || degree[&a] < 2 || degree[&b] < 2 + }) + .collect(); + if dangling.is_empty() { break; } - idx = (idx + 1) % n; + for e in dangling { + edge_set.remove(&e); + } } - half_b.push((splitting_edge, Direction::Backward)); - // Delete the original fill - self.fills[fill_id.idx()].deleted = true; - self.free_fills.push(fill_id.0); + let faces = self.trace_faces(&edge_set); - // Create two new fills - let fill_a = self.alloc_fill(half_a, color, fill_rule); - let fill_b = self.alloc_fill(half_b, color, fill_rule); + // Replace the affected fills with the re-traced bounded faces that fall inside them. + for &f in &affected { + self.fills[f.idx()].deleted = true; + self.free_fills.push(f.0); + } + for face in faces { + // Only bounded (counter-clockwise, positive-area) faces are real regions; the + // outer face is clockwise/negative. + if self.face_signed_area(&face) <= 1e-6 { + continue; + } + let sample = self.boundary_interior_point(&face); + if let Some((_, color, rule)) = + originals.iter().find(|(p, _, _)| p.winding(sample) != 0) + { + self.alloc_fill(face, *color, *rule); + } + } + } - Some((fill_a, fill_b)) + /// Trace all faces of the planar arrangement formed by `edge_set`, using the standard + /// angular next-edge rule (turn to the clockwise-adjacent dart of the twin at each + /// vertex). Returns each face as an ordered `(edge, direction)` loop. Bounded faces + /// come out counter-clockwise (positive signed area); the outer face clockwise. + fn trace_faces(&self, edge_set: &HashSet) -> Vec> { + // Outgoing darts per vertex, sorted by outgoing angle (CCW). + let mut out: HashMap> = HashMap::new(); + for &e in edge_set { + if self.edges[e.idx()].deleted { + continue; + } + let [a, b] = self.edges[e.idx()].vertices; + out.entry(a) + .or_default() + .push((self.dart_angle(e, Direction::Forward), (e, Direction::Forward))); + out.entry(b) + .or_default() + .push((self.dart_angle(e, Direction::Backward), (e, Direction::Backward))); + } + for darts in out.values_mut() { + darts.sort_by(|x, y| x.0.partial_cmp(&y.0).unwrap_or(std::cmp::Ordering::Equal)); + } + + let mut visited: HashSet<(EdgeId, Direction)> = HashSet::new(); + let mut faces: Vec> = Vec::new(); + let cap = edge_set.len() * 2 + 4; + for &e in edge_set { + if self.edges[e.idx()].deleted { + continue; + } + for dir in [Direction::Forward, Direction::Backward] { + let start = (e, dir); + if visited.contains(&start) { + continue; + } + let mut face: Vec<(EdgeId, Direction)> = Vec::new(); + let mut d = start; + loop { + visited.insert(d); + face.push(d); + // Next dart: at the end vertex, the dart clockwise-adjacent to the twin. + let end_v = self.entry_end_vertex(d.0, d.1); + let twin = ( + d.0, + match d.1 { + Direction::Forward => Direction::Backward, + Direction::Backward => Direction::Forward, + }, + ); + let darts = match out.get(&end_v) { + Some(d) => d, + None => break, + }; + let Some(idx) = darts.iter().position(|&(_, dd)| dd == twin) else { + break; + }; + let next = darts[(idx + darts.len() - 1) % darts.len()].1; + if next == start { + break; + } + if visited.contains(&next) || face.len() > cap { + break; + } + d = next; + } + if face.len() >= 3 { + faces.push(face); + } + } + } + faces + } + + /// Outgoing direction angle of a dart at its start vertex. + fn dart_angle(&self, e: EdgeId, dir: Direction) -> f64 { + let c = self.edges[e.idx()].curve; + let (base, cands) = match dir { + Direction::Forward => (c.p0, [c.p1, c.p2, c.p3]), + Direction::Backward => (c.p3, [c.p2, c.p1, c.p0]), + }; + for cand in cands { + let d = cand - base; + if d.hypot() > 1e-9 { + return d.y.atan2(d.x); + } + } + 0.0 + } + + /// Signed area of a face given as an ordered `(edge, direction)` loop (CCW positive). + fn face_signed_area(&self, face: &[(EdgeId, Direction)]) -> f64 { + let pts: Vec = face + .iter() + .map(|&(e, dir)| { + let c = self.edges[e.idx()].curve; + match dir { + Direction::Forward => c.p0, + Direction::Backward => c.p3, + } + }) + .collect(); + if pts.len() < 3 { + return 0.0; + } + let mut a2 = 0.0; + for i in 0..pts.len() { + let p0 = pts[i]; + let p1 = pts[(i + 1) % pts.len()]; + a2 += p0.x * p1.y - p1.x * p0.y; + } + a2 * 0.5 } /// Merge two fills that share a boundary edge (e.g., after edge deletion). @@ -1479,6 +1908,32 @@ impl VectorGraph { let mut vtx_map: HashMap = HashMap::new(); let mut edge_map: HashMap = HashMap::new(); + // Augment the inside set with every boundary edge of an extracted fill. A + // selection might not enumerate them all (e.g. lasso/region selection populates + // edges differently than `select_fill`); without this an extracted fill would be + // copied with `EdgeId::NONE` standing in for a missing edge, and that NONE later + // panics any code that indexes `fill.boundary` (e.g. `insert_stroke`). + let inside_edges: HashSet = { + let mut s = inside_edges.clone(); + for &fid in inside_fills { + if fid.is_none() { + continue; + } + if let Some(fill) = self.fills.get(fid.idx()) { + if fill.deleted { + continue; + } + for &(eid, _) in &fill.boundary { + if !eid.is_none() { + s.insert(eid); + } + } + } + } + s + }; + let inside_edges = &inside_edges; + // Boundary = `explicit_boundary` (e.g. a region/lasso cut the caller knows about) // UNION any inside edge still referenced by a fill we're NOT extracting (a shared // DCEL edge — must be duplicated, not moved, or that fill dangles). Deriving the @@ -1511,16 +1966,21 @@ impl VectorGraph { } } - // Determine which vertices are interior (exclusively owned by the - // extracted subgraph) vs boundary (shared with remaining geometry). - // A vertex is interior if ALL of its incident edges are either in - // inside_edges or boundary_edge_ids. + // Determine which vertices are safe to free from `self`. A vertex can only be + // freed if EVERY one of its incident edges is actually being removed from `self`, + // i.e. is an inside edge that is NOT a boundary edge. Boundary edges are kept + // (duplicated) in `self`, so a vertex touching one is still referenced and must + // remain — otherwise it becomes a freed-but-referenced vertex whose slot a later + // `alloc_vertex` reuses, corrupting the remaining fill. let mut interior_vertices: HashSet = HashSet::new(); let mut boundary_vertices: HashSet = HashSet::new(); for &vid in &referenced_vids { let incident = self.edges_at_vertex(vid); - let all_inside = incident.iter().all(|&eid| edges_to_copy.contains(&eid)); - if all_inside { + let all_removed = !incident.is_empty() + && incident + .iter() + .all(|&eid| inside_edges.contains(&eid) && !boundary_edge_ids.contains(&eid)); + if all_removed { interior_vertices.insert(vid); } else { boundary_vertices.insert(vid); diff --git a/lightningbeam-ui/lightningbeam-core/src/vector_graph/tests/mod.rs b/lightningbeam-ui/lightningbeam-core/src/vector_graph/tests/mod.rs index 742aa64..a1af2b4 100644 --- a/lightningbeam-ui/lightningbeam-core/src/vector_graph/tests/mod.rs +++ b/lightningbeam-ui/lightningbeam-core/src/vector_graph/tests/mod.rs @@ -10,3 +10,5 @@ mod editing; mod gap_close; #[cfg(test)] mod region; +#[cfg(test)] +mod region_cut_select; diff --git a/lightningbeam-ui/lightningbeam-core/src/vector_graph/tests/region_cut_select.rs b/lightningbeam-ui/lightningbeam-core/src/vector_graph/tests/region_cut_select.rs new file mode 100644 index 0000000..3a6a48f --- /dev/null +++ b/lightningbeam-ui/lightningbeam-core/src/vector_graph/tests/region_cut_select.rs @@ -0,0 +1,609 @@ +//! Reproduces the unified region-select behaviour at the graph level: cutting a shape +//! along a lasso outline (`insert_stroke`) and classifying which resulting fills/edges +//! fall inside the lasso — the same logic the editor's `execute_region_select` runs. +//! +//! The shape is built exactly as the editor builds a rectangle: `insert_stroke` of a +//! closed rect loop, then `paint_bucket` to create the fill (NOT a hand-rolled +//! `alloc_fill`), because the bug is in how that traced fill's boundary survives being +//! split by the lasso cut. + +use super::super::*; +use kurbo::{BezPath, CubicBez, ParamCurve, Point, Shape}; + +/// Straight-line cubic from a to b. +fn line(a: Point, b: Point) -> CubicBez { + CubicBez::new( + a, + Point::new(a.x + (b.x - a.x) / 3.0, a.y + (b.y - a.y) / 3.0), + Point::new(a.x + 2.0 * (b.x - a.x) / 3.0, a.y + 2.0 * (b.y - a.y) / 3.0), + b, + ) +} + +fn closed_rect_path(min_x: f64, min_y: f64, max_x: f64, max_y: f64) -> BezPath { + let mut p = BezPath::new(); + p.move_to((min_x, min_y)); + p.line_to((max_x, min_y)); + p.line_to((max_x, max_y)); + p.line_to((min_x, max_y)); + p.close_path(); + p +} + +/// Draw a filled rectangle into an existing graph the way the editor's rectangle tool does. +fn draw_filled_rect(g: &mut VectorGraph, min_x: f64, min_y: f64, max_x: f64, max_y: f64) { + let style = StrokeStyle { width: 1.0, ..Default::default() }; + let color = ShapeColor::rgb(0, 0, 0); + for segs in bezpath_to_cubic_segments(&closed_rect_path(min_x, min_y, max_x, max_y)) { + g.insert_stroke(&segs, Some(style.clone()), Some(color), 0.5); + } + let centroid = Point::new((min_x + max_x) / 2.0, (min_y + max_y) / 2.0); + g.paint_bucket(centroid, ShapeColor::rgb(255, 0, 0), FillRule::NonZero, 0.0) + .expect("paint_bucket should create a fill"); +} + +/// Build a filled rectangle the way the editor's rectangle tool does. +fn editor_filled_rect(min_x: f64, min_y: f64, max_x: f64, max_y: f64) -> VectorGraph { + let mut g = VectorGraph::new(); + draw_filled_rect(&mut g, min_x, min_y, max_x, max_y); + g +} + +/// Closed rectangular lasso: cubic segments (for insert_stroke) + BezPath (for winding). +fn rect_lasso(min_x: f64, min_y: f64, max_x: f64, max_y: f64) -> (Vec, BezPath) { + let p = [ + Point::new(min_x, min_y), + Point::new(max_x, min_y), + Point::new(max_x, max_y), + Point::new(min_x, max_y), + ]; + let segs = vec![line(p[0], p[1]), line(p[1], p[2]), line(p[2], p[3]), line(p[3], p[0])]; + let mut path = BezPath::new(); + path.move_to(p[0]); + for pt in &p[1..] { + path.line_to(*pt); + } + path.close_path(); + (segs, path) +} + +// Classify-against-lasso uses the same guaranteed-interior probe point the editor uses. +fn fill_centroid(g: &VectorGraph, fid: FillId) -> Point { + g.fill_interior_point(fid) +} + +/// Directed start/end points of a boundary entry. +fn dir_start(g: &VectorGraph, eid: EdgeId, dir: Direction) -> Point { + let c = g.edge(eid).curve; + match dir { + Direction::Forward => c.p0, + Direction::Backward => c.p3, + } +} +fn dir_end(g: &VectorGraph, eid: EdgeId, dir: Direction) -> Point { + let c = g.edge(eid).curve; + match dir { + Direction::Forward => c.p3, + Direction::Backward => c.p0, + } +} + +/// Describe a fill's boundary as an ordered list of (start → end) segments for diagnostics. +fn describe_boundary(g: &VectorGraph, fid: FillId) -> Vec<(EdgeId, Point, Point)> { + g.fill(fid) + .boundary + .iter() + .filter(|(e, _)| !e.is_none()) + .map(|&(e, d)| (e, dir_start(g, e, d), dir_end(g, e, d))) + .collect() +} + +/// Assert no fill boundary uses the same edge twice (the signature of a degenerate +/// "spike" where the trace runs out and back along a dangling edge). +fn assert_no_spikes(g: &VectorGraph) { + for (fi, f) in g.fills.iter().enumerate() { + if f.deleted { continue; } + let mut seen = std::collections::HashSet::new(); + for &(e, _) in &f.boundary { + if e.is_none() { continue; } + assert!(seen.insert(e.0), "fill {fi} uses edge {} twice (spike)", e.0); + } + } +} + +/// Assert every non-deleted fill's boundary is a connected closed loop. +fn assert_all_fills_connected(g: &VectorGraph) { + for (i, f) in g.fills.iter().enumerate() { + if f.deleted { + continue; + } + let fid = FillId(i as u32); + let b = describe_boundary(g, fid); + if b.is_empty() { + continue; + } + let n = b.len(); + for k in 0..n { + let (_, _, end) = b[k]; + let (_, next_start, _) = b[(k + 1) % n]; + assert!( + (end.x - next_start.x).abs() < 1e-6 && (end.y - next_start.y).abs() < 1e-6, + "fill {i} boundary disconnected between edge {k} (ends {end:?}) and \ + edge {} (starts {next_start:?}); boundary = {b:#?}", + (k + 1) % n + ); + } + } +} + +#[test] +fn single_vertical_cut_produces_two_connected_fills() { + // Rectangle (0,0)-(200,100), one vertical cut at x=100. + let mut g = editor_filled_rect(0.0, 0.0, 200.0, 100.0); + let cut = vec![line(Point::new(100.0, -10.0), Point::new(100.0, 110.0))]; + g.insert_stroke(&cut, None, None, 1.0); + + let live = g.fills.iter().filter(|f| !f.deleted).count(); + assert_eq!(live, 2, "one cut should split the rect into 2 fills, got {live}"); + assert_all_fills_connected(&g); +} + +#[test] +fn lasso_over_second_of_two_rects_splits_that_rect() { + // Two separate rectangles; lasso a vertical strip over the SECOND one. + let mut g = editor_filled_rect(0.0, 0.0, 200.0, 100.0); + draw_filled_rect(&mut g, 0.0, 150.0, 200.0, 250.0); + assert_eq!(g.fills.iter().filter(|f| !f.deleted).count(), 2, "two rects → two fills"); + + let (segs, lasso_path) = rect_lasso(50.0, 120.0, 150.0, 300.0); // crosses rect B only + g.insert_stroke(&segs, None, None, 1.0); + g.gc_invisible_edges(); + + // Rect B should now be split into 3 fills (left/middle/right strips); rect A is untouched. + // So 1 (rect A) + 3 (rect B pieces) = 4 fills total. + let live = g.fills.iter().filter(|f| !f.deleted).count(); + let inside_fills: Vec = g + .fills + .iter() + .enumerate() + .filter(|(_, f)| !f.deleted) + .map(|(i, _)| FillId(i as u32)) + .filter(|&fid| lasso_path.winding(fill_centroid(&g, fid)) != 0) + .collect(); + + let dump = || { + g.fills + .iter() + .enumerate() + .filter(|(_, f)| !f.deleted) + .map(|(i, _)| (i, fill_centroid(&g, FillId(i as u32)))) + .collect::>() + }; + + assert_eq!(live, 4, "rect B should split into 3 (total 4 fills); got {live}: {:#?}", dump()); + assert_eq!( + inside_fills.len(), + 1, + "exactly the center strip of rect B should be inside; got {:#?}", + dump() + ); + assert_all_fills_connected(&g); +} + +#[test] +fn lasso_strip_through_side_by_side_second_rect() { + // Two rectangles side by side; lasso a vertical strip through the SECOND (right) one. + let mut g = editor_filled_rect(0.0, 0.0, 100.0, 100.0); + draw_filled_rect(&mut g, 150.0, 0.0, 250.0, 100.0); + + let (segs, lasso_path) = rect_lasso(180.0, -50.0, 220.0, 150.0); + g.insert_stroke(&segs, None, None, 1.0); + g.gc_invisible_edges(); + assert_all_fills_connected(&g); + + let inside: Vec = g.fills.iter().enumerate() + .filter(|(_, f)| !f.deleted).map(|(i, _)| FillId(i as u32)) + .filter(|&fid| lasso_path.winding(fill_centroid(&g, fid)) != 0).collect(); + let dump: Vec<_> = g.fills.iter().enumerate().filter(|(_, f)| !f.deleted) + .map(|(i, _)| (i, fill_centroid(&g, FillId(i as u32)), describe_boundary(&g, FillId(i as u32)).len())).collect(); + assert_eq!(inside.len(), 1, "one strip of the right rect should be inside; got {dump:#?}"); +} + +/// No live fill may reference a freed edge or vertex (whose slot a later alloc reuses). +fn assert_no_freed_but_referenced(g: &VectorGraph) { + let freed_v: std::collections::HashSet = g.free_vertices.iter().copied().collect(); + let freed_e: std::collections::HashSet = g.free_edges.iter().copied().collect(); + for (fi, f) in g.fills.iter().enumerate() { + if f.deleted { continue; } + for &(e, _) in &f.boundary { + if e.is_none() { continue; } + assert!(!freed_e.contains(&e.0), "live fill {fi} references freed edge {}", e.0); + for &v in &g.edge(e).vertices { + assert!(!freed_v.contains(&v.0), "live fill {fi} references freed vertex {}", v.0); + } + } + } +} + +#[test] +fn extract_after_cut_keeps_remaining_fill_intact() { + // Cut a rectangle's corner with a lasso, then extract (Group) the clipped corner. The + // extraction must not free vertices/edges still referenced by the L-shaped remainder — + // previously it freed the shared cut vertices, and a later `alloc_vertex` reused those + // slots and corrupted the remainder (the "second lasso deletes all faces" bug). + let mut g = editor_filled_rect(0.0, 0.0, 200.0, 100.0); + let (segs, lasso) = rect_lasso(100.0, -50.0, 300.0, 50.0); // clips the top-right corner + g.insert_stroke(&segs, None, None, 1.0); + g.gc_invisible_edges(); + + // Pick the fill inside the lasso (the clipped corner) and extract it. + let inside: Vec = g.fills.iter().enumerate() + .filter(|(_, f)| !f.deleted).map(|(i, _)| FillId(i as u32)) + .filter(|&fid| lasso.winding(g.fill_interior_point(fid)) != 0).collect(); + assert_eq!(inside.len(), 1, "one corner fill should be inside the lasso"); + let inside_fills: HashSet = inside.iter().copied().collect(); + let inside_edges: HashSet = g.fill(inside[0]).boundary.iter() + .filter_map(|&(e, _)| (!e.is_none()).then_some(e)).collect(); + + let _ = g.extract_subgraph(&inside_edges, &inside_fills, &HashSet::new()); + + assert_no_freed_but_referenced(&g); + assert_all_fills_connected(&g); + + // Simulate the next lasso allocating vertices, then re-validate: a corrupted (freed but + // referenced) vertex would now be at a bogus position. + let (segs2, _) = rect_lasso(20.0, 20.0, 60.0, 80.0); + g.insert_stroke(&segs2, None, None, 1.0); + g.gc_invisible_edges(); + assert_all_fills_connected(&g); +} + +#[test] +fn second_stroke_crossing_first_splits_into_quadrants() { + // A later stroke that crosses an earlier stroke's edge inside a fill triggers an + // edge-domain `split_edge`, whose sub-edges aren't tracked in the second stroke's new + // edges. The retrace must still see them (induced-subgraph expansion) or the cut breaks + // and unravels. Two crossing cuts through a rectangle must yield four connected quadrants. + let mut g = editor_filled_rect(0.0, 0.0, 100.0, 100.0); + g.insert_stroke(&[line(Point::new(-10.0, 50.0), Point::new(110.0, 50.0))], None, None, 1.0); // horizontal + assert_eq!(g.fills.iter().filter(|f| !f.deleted).count(), 2, "horizontal cut → 2"); + g.insert_stroke(&[line(Point::new(50.0, -10.0), Point::new(50.0, 110.0))], None, None, 1.0); // vertical, crosses it + g.gc_invisible_edges(); + + let live: Vec = g.fills.iter().enumerate() + .filter(|(_, f)| !f.deleted).map(|(i, _)| FillId(i as u32)).collect(); + assert_eq!(live.len(), 4, "two crossing cuts → four quadrants; got {}", live.len()); + assert_all_fills_connected(&g); + assert_no_spikes(&g); + for &fid in &live { + assert!((polygon_area(&g, fid) - 2500.0).abs() < 1.0, "each quadrant is 50x50, got {}", polygon_area(&g, fid)); + } +} + +#[test] +fn stroke_dead_ending_inside_fill_does_not_spike() { + // A stroke (e.g. a freehand lasso that wiggles or nearly self-touches) can leave a + // dangling stub: an edge whose inner endpoint has degree 1. Re-tracing must not run out + // and back along it (a self-touching "spike" boundary); the fill stays a clean loop. + let mut g = editor_filled_rect(0.0, 0.0, 200.0, 100.0); + // Open stroke entering the top edge and dead-ending inside at (100,50). + let stub = vec![line(Point::new(50.0, -20.0), Point::new(100.0, 50.0))]; + g.insert_stroke(&stub, None, None, 1.0); + g.gc_invisible_edges(); + + let live: Vec = g.fills.iter().enumerate() + .filter(|(_, f)| !f.deleted).map(|(i, _)| FillId(i as u32)).collect(); + assert_eq!(live.len(), 1, "a dead-end stub must not split the fill; got {}", live.len()); + assert_all_fills_connected(&g); + assert_no_spikes(&g); + // The fill is still the full rectangle. + assert!((polygon_area(&g, live[0]) - 20000.0).abs() < 1.0, + "fill should remain the 200x100 rectangle, area {}", polygon_area(&g, live[0])); +} + +#[test] +fn groupfail_two_notched_fills_second_lasso() { + // Faithful reconstruction of the captured post-group state (dump_1): two adjacent + // fills sharing the x=337 column, each with an invisible notch (the hole left by the + // first lasso+group). A second lasso (398,249)-(495,327) clips fill 3's corner. + use std::collections::HashMap; + let mut g = VectorGraph::new(); + let vp = [ + (130.0, 232.0), (337.0, 232.0), (337.0, 362.0), (130.0, 362.0), (337.0, 297.0), + (535.0, 297.0), (535.0, 417.0), (337.0, 417.0), (0.0, 0.0), (337.0, 315.0), + (220.0, 315.0), (456.0, 315.0), (456.0, 345.0), (337.0, 345.0), (220.0, 345.0), + ]; + let vs: Vec<_> = vp.iter().map(|&(x, y)| g.alloc_vertex(Point::new(x, y))).collect(); + let style = StrokeStyle { width: 3.0, ..Default::default() }; + let color = ShapeColor::rgb(0, 0, 0); + // (name, v_start, v_end, visible) + let edge_defs: &[(&str, usize, usize, bool)] = &[ + ("e0", 0, 1, true), ("e1", 4, 5, true), ("e2", 2, 3, true), ("e3", 3, 0, true), + ("e4", 1, 4, true), ("e5", 7, 2, true), ("e6", 5, 6, true), ("e7", 6, 7, true), + ("e8", 10, 9, false), ("e9", 12, 13, false), ("e11", 4, 9, true), ("e12", 9, 11, false), + ("e13", 11, 12, false), ("e15", 13, 2, true), ("e16", 13, 14, false), ("e17", 14, 10, false), + ]; + let mut em: HashMap<&str, EdgeId> = HashMap::new(); + for &(name, a, b, vis) in edge_defs { + let (ss, sc) = if vis { (Some(style.clone()), Some(color)) } else { (None, None) }; + let e = g.alloc_edge(line(vp[a].into(), vp[b].into()), vs[a], vs[b], ss, sc); + em.insert(name, e); + } + let mk = |spec: &[(&str, Direction)]| -> Vec<(EdgeId, Direction)> { + spec.iter().map(|&(n, d)| (em[n], d)).collect() + }; + use Direction::{Backward as B, Forward as F}; + g.alloc_fill(mk(&[("e11", B), ("e4", B), ("e0", B), ("e3", B), ("e2", B), ("e15", B), ("e16", F), ("e17", F), ("e8", F)]), + ShapeColor::rgb(100, 100, 255), FillRule::NonZero); + g.alloc_fill(mk(&[("e15", F), ("e5", B), ("e7", B), ("e6", B), ("e1", B), ("e11", F), ("e12", F), ("e13", F), ("e9", F)]), + ShapeColor::rgb(100, 100, 255), FillRule::NonZero); + + assert_eq!(g.fills.iter().filter(|f| !f.deleted).count(), 2, "starts with 2 fills"); + let (segs, lasso) = rect_lasso(398.0, 249.0, 495.0, 327.0); + g.insert_stroke(&segs, None, None, 1.0); + g.gc_invisible_edges(); + + let live = g.fills.iter().filter(|f| !f.deleted).count(); + let inside: Vec = g.fills.iter().enumerate() + .filter(|(_, f)| !f.deleted).map(|(i, _)| FillId(i as u32)) + .filter(|&fid| lasso.winding(g.fill_interior_point(fid)) != 0).collect(); + eprintln!("groupfail: {live} live fills, {} inside lasso", inside.len()); + for (i, f) in g.fills.iter().enumerate() { + if f.deleted { continue; } + let fid = FillId(i as u32); + eprintln!(" fill {i}: bbox {:?} area {:.0}", fill_bbox(&g, fid), polygon_area(&g, fid)); + } + assert_all_fills_connected(&g); + // The two original shapes must survive (no faces wrongly deleted); the lasso adds a cut. + assert!(live >= 2, "must keep both shapes plus the cut; got {live}"); + assert_eq!(inside.len(), 1, "exactly the clipped corner of fill 3 should be selected"); +} + +#[test] +fn lasso_across_notched_fill_never_corrupts_boundaries() { + // A rectangle with a rectangular notch bitten out of its top edge — the shape a fill + // is left as after grouping a lasso selection (the notch is the extracted region's + // hole). A second lasso crossing the notch must never produce disconnected/corrupt + // fill boundaries (it previously incorporated the lasso's out-of-shape edges, drawing + // stray diagonals). It may under-select on such complex geometry, but must stay valid. + let mut g = VectorGraph::new(); + let pts = [ + Point::new(0.0, 0.0), Point::new(80.0, 0.0), Point::new(80.0, 40.0), + Point::new(120.0, 40.0), Point::new(120.0, 0.0), Point::new(200.0, 0.0), + Point::new(200.0, 100.0), Point::new(0.0, 100.0), + ]; + let style = StrokeStyle { width: 1.0, ..Default::default() }; + let color = ShapeColor::rgb(0, 0, 0); + let v: Vec<_> = pts.iter().map(|&p| g.alloc_vertex(p)).collect(); + let mut boundary = Vec::new(); + for i in 0..pts.len() { + let e = g.alloc_edge(line(pts[i], pts[(i + 1) % pts.len()]), v[i], v[(i + 1) % pts.len()], + Some(style.clone()), Some(color)); + boundary.push((e, Direction::Forward)); + } + g.alloc_fill(boundary, ShapeColor::rgb(255, 0, 0), FillRule::NonZero); + + // Lasso a rectangle that straddles the notch and the shape body. Inside the fill it + // covers (60..140, 0..60) minus the notch (80..120, 0..40) — an arch shape — which the + // cut must isolate as one connected fill, with the remainder outside. + let (segs, lasso) = rect_lasso(60.0, -20.0, 140.0, 60.0); + g.insert_stroke(&segs, None, None, 1.0); + g.gc_invisible_edges(); + + // Every resulting fill is a valid connected loop (never corrupt). + assert_all_fills_connected(&g); + + // Exactly one fill lies inside the lasso, and it is the arch (area = 80*60 - 40*40). + let inside: Vec = g.fills.iter().enumerate() + .filter(|(_, f)| !f.deleted).map(|(i, _)| FillId(i as u32)) + .filter(|&fid| lasso.winding(g.fill_interior_point(fid)) != 0).collect(); + assert_eq!(inside.len(), 1, "the arch (lasso ∩ notched fill) should be one selected fill"); + let area = polygon_area(&g, inside[0]); + assert!((area - 3200.0).abs() < 1.0, "arch area should be 3200, got {area}"); +} + +/// Absolute polygon area of a fill from its boundary corner points. +fn polygon_area(g: &VectorGraph, fid: FillId) -> f64 { + let pts: Vec = g.fill(fid).boundary.iter() + .filter(|(e, _)| !e.is_none()) + .map(|&(e, d)| dir_start(g, e, d)) + .collect(); + if pts.len() < 3 { return 0.0; } + let mut a2 = 0.0; + for i in 0..pts.len() { + let p0 = pts[i]; let p1 = pts[(i + 1) % pts.len()]; + a2 += p0.x * p1.y - p1.x * p0.y; + } + (a2 * 0.5).abs() +} + +#[test] +fn two_edge_adjacent_rects_make_two_clean_fills() { + // Two rectangles sharing the x=100 edge. The second's left edge lands exactly on the + // first's right edge; without coincident-edge cleanup this produced duplicate edges and + // zero-area "sliver" fills (4 fills instead of 2) before any lasso. + let mut g = editor_filled_rect(0.0, 0.0, 100.0, 100.0); + draw_filled_rect(&mut g, 100.0, 0.0, 200.0, 100.0); + let live: Vec<_> = g.fills.iter().enumerate().filter(|(_, f)| !f.deleted) + .map(|(i, _)| (i, fill_bbox(&g, FillId(i as u32)))).collect(); + assert_eq!(live.len(), 2, "two adjacent rects should make exactly two fills; got {live:?}"); + assert_all_fills_connected(&g); +} + +#[test] +fn lasso_strip_through_adjacent_second_rect() { + // Two rectangles sharing an edge (snapped adjacent), lasso a strip through the second. + let mut g = editor_filled_rect(0.0, 0.0, 100.0, 100.0); + draw_filled_rect(&mut g, 100.0, 0.0, 200.0, 100.0); // shares the x=100 edge with the first + + let (segs, lasso_path) = rect_lasso(130.0, -50.0, 170.0, 150.0); // strip through 2nd + g.insert_stroke(&segs, None, None, 1.0); + g.gc_invisible_edges(); + assert_all_fills_connected(&g); + + let inside: Vec = g.fills.iter().enumerate() + .filter(|(_, f)| !f.deleted).map(|(i, _)| FillId(i as u32)) + .filter(|&fid| lasso_path.winding(fill_centroid(&g, fid)) != 0).collect(); + let dump: Vec<_> = g.fills.iter().enumerate().filter(|(_, f)| !f.deleted) + .map(|(i, _)| (i, fill_centroid(&g, FillId(i as u32)), describe_boundary(&g, FillId(i as u32)).len())).collect(); + assert_eq!(inside.len(), 1, "one strip of the 2nd rect should be inside; got {dump:#?}"); +} + +#[test] +fn lasso_strip_through_overlapping_second_rect() { + // Second rectangle overlaps the first; lasso a strip through the second's free part. + let mut g = editor_filled_rect(0.0, 0.0, 100.0, 100.0); + draw_filled_rect(&mut g, 60.0, 60.0, 200.0, 160.0); + + let (segs, _lasso) = rect_lasso(120.0, 40.0, 160.0, 180.0); + g.insert_stroke(&segs, None, None, 1.0); + g.gc_invisible_edges(); + assert_all_fills_connected(&g); +} + +/// Bounding box of a fill's boundary edge endpoints. +fn fill_bbox(g: &VectorGraph, fid: FillId) -> (f64, f64, f64, f64) { + let (mut minx, mut miny, mut maxx, mut maxy) = (f64::MAX, f64::MAX, f64::MIN, f64::MIN); + for &(eid, _) in &g.fill(fid).boundary { + if eid.is_none() { + continue; + } + for &vid in &g.edge(eid).vertices { + let p = g.vertex(vid).position; + minx = minx.min(p.x); + miny = miny.min(p.y); + maxx = maxx.max(p.x); + maxy = maxy.max(p.y); + } + } + (minx, miny, maxx, maxy) +} + +#[test] +fn lasso_corner_clip_of_second_rect_splits_off_corner() { + // Captured repro (/tmp dump): two separate rects; the lasso clips the top-left CORNER + // of the second rect, so the cut path turns a corner at a vertex INTERIOR to the rect. + let mut g = editor_filled_rect(128.37, 255.97, 290.91, 336.55); // rect 1 + draw_filled_rect(&mut g, 417.39, 311.62, 620.25, 442.55); // rect 2 + let (segs, lasso) = rect_lasso(360.12, 277.92, 537.32, 397.14); // clips rect-2 corner + g.insert_stroke(&segs, None, None, 1.0); + g.gc_invisible_edges(); + assert_all_fills_connected(&g); + + let inside: Vec = g.fills.iter().enumerate() + .filter(|(_, f)| !f.deleted).map(|(i, _)| FillId(i as u32)) + .filter(|&fid| lasso.winding(fill_centroid(&g, fid)) != 0).collect(); + let dump: Vec<_> = g.fills.iter().enumerate().filter(|(_, f)| !f.deleted) + .map(|(i, _)| (i, fill_centroid(&g, FillId(i as u32)), fill_bbox(&g, FillId(i as u32)), describe_boundary(&g, FillId(i as u32)).len())).collect(); + assert_eq!(inside.len(), 1, "exactly the clipped corner should be inside; fills = {dump:#?}"); + + // The selected fill must be the clipped CORNER, not the whole rect 2. + let (minx, miny, maxx, maxy) = fill_bbox(&g, inside[0]); + let eps = 1.0; + assert!( + (minx - 417.39).abs() < eps && (miny - 311.62).abs() < eps + && (maxx - 537.32).abs() < eps && (maxy - 397.14).abs() < eps, + "expected clipped-corner bbox (417.4,311.6)-(537.3,397.1), got ({minx:.1},{miny:.1})-({maxx:.1},{maxy:.1}) \ + — whole rect2 is (417,311)-(620,442); the fill wasn't split" + ); +} + +#[test] +fn lasso_over_middle_of_rect_selects_clean_center_strip() { + // Rectangle (0,0)-(200,100); lasso a vertical strip (50,-50)-(150,150). + let mut g = editor_filled_rect(0.0, 0.0, 200.0, 100.0); + let (segs, lasso_path) = rect_lasso(50.0, -50.0, 150.0, 150.0); + + g.insert_stroke(&segs, None, None, 1.0); + // The editor runs this right after the cut: the lasso's top/bottom edges and the + // out-of-shape extensions of its sides are invisible and unreferenced — they must be + // collected so they can't be selected/edited later. + g.gc_invisible_edges(); + for (i, e) in g.edges.iter().enumerate() { + if e.deleted || e.stroke_style.is_some() || e.stroke_color.is_some() { + continue; + } + let eid = EdgeId(i as u32); + let in_a_fill = g + .fills + .iter() + .any(|f| !f.deleted && f.boundary.iter().any(|&(fe, _)| fe == eid)); + assert!( + in_a_fill, + "stray invisible edge {i} ({:?}) survived gc — not part of any fill", + e.curve + ); + } + + // Classify fills by centroid winding (the editor's logic). + let inside_fills: Vec = g + .fills + .iter() + .enumerate() + .filter(|(_, f)| !f.deleted) + .map(|(i, _)| FillId(i as u32)) + .filter(|&fid| lasso_path.winding(fill_centroid(&g, fid)) != 0) + .collect(); + + let dump = || { + g.fills + .iter() + .enumerate() + .filter(|(_, f)| !f.deleted) + .map(|(i, _)| { + let fid = FillId(i as u32); + (i, fill_centroid(&g, fid), describe_boundary(&g, fid)) + }) + .collect::>() + }; + + assert_eq!( + inside_fills.len(), + 1, + "expected exactly one inside fill (the center strip); live fills = {:#?}", + dump() + ); + + let fid = inside_fills[0]; + let boundary = describe_boundary(&g, fid); + + // The center strip is a rectangle: it must have 4 boundary edges that form a + // *connected closed loop* (each edge's directed end == the next edge's directed start). + // Before the fix, the split produced a 2-edge boundary (left cut + top segment) whose + // bbox is still (50,0)-(150,100) but which `fill_to_bezpath` renders as left+top edges + // plus a diagonal close — the artifact the user reported. + assert_eq!( + boundary.len(), + 4, + "center strip should have 4 boundary edges, got {}: {:#?}", + boundary.len(), + boundary + ); + + let eps = 1e-6; + let n = boundary.len(); + for i in 0..n { + let (_, _, end) = boundary[i]; + let (_, next_start, _) = boundary[(i + 1) % n]; + assert!( + (end.x - next_start.x).abs() < eps && (end.y - next_start.y).abs() < eps, + "boundary is disconnected between edge {i} (ends {end:?}) and edge {} \ + (starts {next_start:?}); full boundary = {boundary:#?}", + (i + 1) % n + ); + } + + // And the loop should visit the four expected corners. + let corners = [ + Point::new(50.0, 0.0), + Point::new(150.0, 0.0), + Point::new(150.0, 100.0), + Point::new(50.0, 100.0), + ]; + for c in corners { + let hit = boundary + .iter() + .any(|&(_, s, _)| (s.x - c.x).abs() < 0.5 && (s.y - c.y).abs() < 0.5); + assert!(hit, "center strip boundary should pass through corner {c:?}: {boundary:#?}"); + } +} diff --git a/lightningbeam-ui/lightningbeam-editor/src/main.rs b/lightningbeam-ui/lightningbeam-editor/src/main.rs index 1405ad8..57bfed8 100644 --- a/lightningbeam-ui/lightningbeam-editor/src/main.rs +++ b/lightningbeam-ui/lightningbeam-editor/src/main.rs @@ -984,8 +984,9 @@ struct EditorApp { snap_enabled: bool, // Whether to snap to geometry (default: true) paint_bucket_gap_tolerance: f64, // Fill gap tolerance for paint bucket (default: 5.0) polygon_sides: u32, // Number of sides for polygon tool (default: 5) - // Region select state - region_selection: Option, + /// Undo depth before the current region-select session's first cut (see + /// `resolve_pending_region_cut`). `None` when no region selection is pending. + pending_region_cut_base: Option, region_select_mode: lightningbeam_core::tool::RegionSelectMode, lasso_mode: lightningbeam_core::tool::LassoMode, @@ -1303,7 +1304,7 @@ impl EditorApp { snap_enabled: true, // Default to snapping paint_bucket_gap_tolerance: 5.0, // Default gap tolerance polygon_sides: 5, // Default to pentagon - region_selection: None, + pending_region_cut_base: None, region_select_mode: lightningbeam_core::tool::RegionSelectMode::default(), lasso_mode: lightningbeam_core::tool::LassoMode::default(), input_level: 0.0, @@ -2629,27 +2630,7 @@ impl EditorApp { None => return, }; - // Region selection case: faces are selected but no edges. - // The inside geometry was already extracted from the live DCEL; - // commit the current state (outside + boundary) using the - // pre-boundary snapshot as the "before" for undo. - if self.selection.selected_edges().is_empty() { - if let Some(region_sel) = self.region_selection.take() { - // dcel_snapshot = state before boundary was inserted. - // Current document DCEL = outside portion only (boundary edges present). - // We commit the snapshot as "before" and the current state as "after", - // then drop the region selection so it is not merged back. - // TODO: Region selection delete requires converting Dcel snapshot - // to VectorGraph for ModifyGraphAction. Deferred until RegionSelection - // is migrated from Dcel to VectorGraph. - eprintln!("Region selection delete: not yet ported to VectorGraph"); - // region_sel is dropped; the stage pane will see region_selection == None. - } - self.selection.clear_geometry_selection(); - return; - } - - // Select-tool case: delete the selected edges. + // Delete the selected edges (region/marquee/click all populate the same sets). let edge_ids: Vec = self.selection.selected_edges().iter().copied().collect(); @@ -3036,38 +3017,31 @@ impl EditorApp { } } - /// Revert an uncommitted region selection, restoring original shapes - fn revert_region_selection( - region_selection: &mut Option, - action_executor: &mut lightningbeam_core::action::ActionExecutor, - selection: &mut lightningbeam_core::selection::Selection, - ) { - use lightningbeam_core::layer::AnyLayer; - - let region_sel = match region_selection.take() { - Some(rs) => rs, - None => return, - }; - - if region_sel.committed { + /// Resolve a pending region-select cut once its selection has been cleared. + /// + /// A region/lasso selection cuts the geometry (an undoable `"Region select"` action) + /// and selects the resulting sub-pieces. When that selection is deselected: + /// - if nothing was edited in between, the cut(s) are healed (undone) so the lasso + /// was non-destructive — the shape returns to whole; + /// - if anything was edited, everything is kept (the edits — and the cut they rely + /// on — stay on the undo stack), i.e. the change is "merged" in place. + /// We tell the two apart by walking the undo stack down to the recorded base depth and + /// only undoing while the top action is itself a region-select cut; the first non-cut + /// action (an edit) stops the heal and leaves everything intact. + fn resolve_pending_region_cut(&mut self) { + let Some(base) = self.pending_region_cut_base else { return }; + // Only resolve once the region selection has actually been deselected. + if self.selection.has_geometry_selection() { return; } - - let doc = action_executor.document_mut(); - let layer = match doc.get_layer_mut(®ion_sel.layer_id) { - Some(l) => l, - None => return, - }; - let vector_layer = match layer { - AnyLayer::Vector(vl) => vl, - _ => return, - }; - - // TODO: DCEL - region selection revert disabled during migration - // (was: remove/add_shape_from/to_keyframe for splits) - let _ = vector_layer; - - selection.clear(); + while self.action_executor.undo_depth() > base { + if self.action_executor.undo_description().as_deref() == Some("Region select") { + let _ = self.action_executor.undo(); + } else { + break; // an edit sits on top → the user changed something; keep it all. + } + } + self.pending_region_cut_base = None; } fn handle_menu_action(&mut self, action: MenuAction) { @@ -6563,7 +6537,7 @@ impl eframe::App for EditorApp { graph_topology_generation: &mut self.graph_topology_generation, script_to_edit: &mut self.script_to_edit, script_saved: &mut self.script_saved, - region_selection: &mut self.region_selection, + pending_region_cut_base: &mut self.pending_region_cut_base, region_select_mode: &mut self.region_select_mode, lasso_mode: &mut self.lasso_mode, pending_graph_loads: &self.pending_graph_loads, @@ -7002,21 +6976,21 @@ impl eframe::App for EditorApp { } } - // Escape key: cancel floating raster selection or revert uncommitted region selection + // Escape key: cancel floating raster selection, or clear the geometry selection if !wants_keyboard && ctx.input(|i| self.keymap.action_pressed(keymap::AppAction::CancelAction, i)) { if self.selection.raster_floating.is_some() { self.cancel_raster_floating(); } else if self.selection.raster_selection.is_some() { self.selection.raster_selection = None; - } else if self.region_selection.is_some() { - Self::revert_region_selection( - &mut self.region_selection, - &mut self.action_executor, - &mut self.selection, - ); + } else if self.selection.has_geometry_selection() { + self.selection.clear_geometry_selection(); } } + // After all input is processed, if a region selection was deselected without any + // intervening edit, heal (undo) its cut so the lasso is non-destructive. + self.resolve_pending_region_cut(); + // F3 debug overlay toggle (works even when text input is active) if ctx.input(|i| self.keymap.action_pressed(keymap::AppAction::ToggleDebugOverlay, i)) { self.debug_overlay_visible = !self.debug_overlay_visible; diff --git a/lightningbeam-ui/lightningbeam-editor/src/panes/mod.rs b/lightningbeam-ui/lightningbeam-editor/src/panes/mod.rs index 66ba40d..ae78e67 100644 --- a/lightningbeam-ui/lightningbeam-editor/src/panes/mod.rs +++ b/lightningbeam-ui/lightningbeam-editor/src/panes/mod.rs @@ -325,8 +325,10 @@ pub struct SharedPaneState<'a> { pub script_to_edit: &'a mut Option, /// Script ID that was just saved (triggers auto-recompile of nodes using it) pub script_saved: &'a mut Option, - /// Active region selection (temporary split state) - pub region_selection: &'a mut Option, + /// Undo-stack depth recorded before the first region-select cut of the current + /// selection session. `Some` while a region selection is live and unchanged; lets a + /// later deselect heal (undo) the cut if nothing was edited. See `resolve_pending_region_cut`. + pub pending_region_cut_base: &'a mut Option, /// Region select mode (Rectangle or Lasso) pub region_select_mode: &'a mut lightningbeam_core::tool::RegionSelectMode, /// Lasso select sub-mode (Freehand / Polygonal / Magnetic) diff --git a/lightningbeam-ui/lightningbeam-editor/src/panes/stage.rs b/lightningbeam-ui/lightningbeam-editor/src/panes/stage.rs index 88bd8c4..823b094 100644 --- a/lightningbeam-ui/lightningbeam-editor/src/panes/stage.rs +++ b/lightningbeam-ui/lightningbeam-editor/src/panes/stage.rs @@ -472,8 +472,6 @@ struct VelloRenderContext { editing_instance_id: Option, /// The parent layer ID containing the clip instance being edited editing_parent_layer_id: Option, - /// Active region selection state (for rendering boundary overlay) - region_selection: Option, /// Mouse position in document-local (clip-local) world coordinates, for hover hit testing mouse_world_pos: Option, /// Latest webcam frame for live preview (if any camera is active) @@ -1901,44 +1899,10 @@ 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 = overlay_transform * region_sel.transform; - lightningbeam_core::renderer::render_vector_graph( - ®ion_sel.selected_graph, - &mut scene, - sel_transform, - 1.0, - &self.ctx.document, - &mut image_cache, - ); - } - drop(image_cache); 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_vector_graph( - ®ion_sel.selected_graph, - &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) { @@ -2118,50 +2082,6 @@ impl egui_wgpu::CallbackTrait for VelloCallback { } } - // 1a. Draw stipple overlay on region-selected graph - if let Some(ref region_sel) = self.ctx.region_selection { - use lightningbeam_core::vector_graph::FillId; - let sel_graph = ®ion_sel.selected_graph; - let sel_transform = overlay_transform * region_sel.transform; - let stipple_brush = selection_stipple_brush(); - let inv_zoom = 1.0 / self.ctx.zoom as f64; - let brush_xform = Some(Affine::scale(inv_zoom)); - - // Stipple fills with visible content - for (i, fill) in sel_graph.fills.iter().enumerate() { - if fill.deleted { continue; } - if fill.color.is_none() && fill.image_fill.is_none() && fill.gradient_fill.is_none() { continue; } - let fill_id = FillId(i as u32); - let path = sel_graph.fill_to_bezpath(fill_id); - scene.fill( - vello::peniko::Fill::NonZero, - sel_transform, - stipple_brush, - brush_xform, - &path, - ); - } - - // Stipple edges with visible stroke - for edge in &sel_graph.edges { - if edge.deleted { continue; } - if edge.stroke_style.is_none() && edge.stroke_color.is_none() { continue; } - let width = edge.stroke_style.as_ref() - .map(|s| s.width) - .unwrap_or(2.0); - let mut path = vello::kurbo::BezPath::new(); - path.move_to(edge.curve.p0); - path.curve_to(edge.curve.p1, edge.curve.p2, edge.curve.p3); - scene.stroke( - &vello::kurbo::Stroke::new(width), - sel_transform, - stipple_brush, - brush_xform, - &path, - ); - } - } - // 1b. Draw stipple hover highlight on the curve under the mouse // During active curve editing, lock highlight to the edited curve if matches!(self.ctx.selected_tool, Tool::Select | Tool::BezierEdit) { @@ -3738,12 +3658,6 @@ 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, @@ -5076,6 +4990,7 @@ impl StagePane { _ui: &mut egui::Ui, response: &egui::Response, world_pos: egui::Vec2, + shift_held: bool, shared: &mut SharedPaneState, ) { use lightningbeam_core::tool::{ToolState, RegionSelectMode}; @@ -5089,12 +5004,13 @@ impl StagePane { None => return, }; - // Mouse down: start region selection + // Mouse down: start region selection. Clear the existing selection unless shift + // is held (additive), mirroring the marquee. Region select populates the same + // `Selection` ID sets as every other tool. if self.rsp_drag_started(response) { - // 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(); + if !shift_held { + shared.selection.clear(); + } match *shared.region_select_mode { RegionSelectMode::Rectangle => { @@ -5165,7 +5081,10 @@ impl StagePane { } } - /// Execute region selection: snapshot DCEL, insert region boundary, extract inside geometry + /// Execute region selection (rect or lasso): cut the geometry along the region + /// outline (an undoable edit) and select the resulting inside sub-pieces into the + /// normal `Selection`. This is the single, unified selection path — there is no + /// separate floating "region selection" anymore. fn execute_region_select( shared: &mut SharedPaneState, region_path: vello::kurbo::BezPath, @@ -5177,8 +5096,8 @@ impl StagePane { let time = *shared.playback_time; - use lightningbeam_core::vector_graph::{EdgeId, FillId, VertexId}; - use std::collections::{HashMap, HashSet}; + use lightningbeam_core::vector_graph::{EdgeId, FillId}; + use std::collections::HashSet; use vello::kurbo::{ParamCurve, Shape as _}; // Convert region path line segments to CubicBez for insert_stroke @@ -5219,179 +5138,134 @@ impl StagePane { return; } - // Do all graph work in a block so the mutable borrow of shared ends - // before we assign to shared.region_selection. - let extraction_result = { - let document = shared.action_executor.document_mut(); - let graph = match document.get_layer_mut(&layer_id) { - Some(AnyLayer::Vector(vl)) => match vl.graph_at_time_mut(time) { + // Cut a copy of the graph along the region outline, then classify which resulting + // pieces lie inside. We work on a clone so the cut can be committed as a single + // undoable action (or discarded entirely if the region caught nothing). + let (graph_before, graph_after, inside_edges, inside_fills) = { + let document = shared.action_executor.document(); + let graph = match document.get_layer(&layer_id) { + Some(AnyLayer::Vector(vl)) => match vl.graph_at_time(time) { Some(d) => d, None => return, }, _ => return, }; - let snapshot = graph.clone(); + let graph_before = graph.clone(); + let mut graph_after = graph_before.clone(); - // Insert region boundary as invisible edges (no stroke style/color) - let region_edge_ids = graph.insert_stroke(&segments, None, None, 1.0); + // Debug capture (set LIGHTNINGBEAM_DUMP_REGION=1): write the exact pre-cut graph + // + lasso segments to a numbered JSON file under the temp dir, so a misbehaving + // region select can be replayed deterministically as a regression test. + if std::env::var_os("LIGHTNINGBEAM_DUMP_REGION").is_some() { + use std::sync::atomic::{AtomicUsize, Ordering}; + static DUMP_N: AtomicUsize = AtomicUsize::new(0); + let n = DUMP_N.fetch_add(1, Ordering::Relaxed); + let seg_pts: Vec<[[f64; 2]; 4]> = segments + .iter() + .map(|c| [[c.p0.x, c.p0.y], [c.p1.x, c.p1.y], [c.p2.x, c.p2.y], [c.p3.x, c.p3.y]]) + .collect(); + if let Ok(graph_json) = serde_json::to_value(&graph_before) { + let dump = serde_json::json!({ "graph": graph_json, "segments": seg_pts }); + let path = std::env::temp_dir().join(format!("lightningbeam_region_dump_{n}.json")); + match serde_json::to_string_pretty(&dump).map(|s| std::fs::write(&path, s)) { + Ok(Ok(())) => eprintln!("[region dump] wrote {}", path.display()), + e => eprintln!("[region dump] failed: {e:?}"), + } + } + } + // Insert the region boundary as invisible edges (no stroke style/color) so any + // shape the region crosses is split into inside/outside sub-fills. + let region_edge_ids = graph_after.insert_stroke(&segments, None, None, 1.0); let region_edge_set: HashSet = region_edge_ids.iter().copied().collect(); - // Classify edges: inside vs outside by midpoint winding - let mut inside_edges = HashSet::new(); - for (i, edge) in graph.edges.iter().enumerate() { + // The lasso outline portions that DON'T cross geometry leave dangling invisible + // edges (no stroke, not part of any fill). Drop them so they can't be selected + // or edited later; the cut edges (now part of a fill boundary) are kept. + graph_after.gc_invisible_edges(); + + // Classify edges: inside by midpoint winding (excluding the cut edges themselves). + let mut inside_edges: Vec = Vec::new(); + for (i, edge) in graph_after.edges.iter().enumerate() { let eid = EdgeId(i as u32); if edge.deleted || region_edge_set.contains(&eid) { continue; } let mid = edge.curve.eval(0.5); if region_path.winding(mid) != 0 { - inside_edges.insert(eid); + inside_edges.push(eid); } } - // Classify fills: inside vs outside by boundary centroid winding - let mut inside_fills = HashSet::new(); - for (i, fill) in graph.fills.iter().enumerate() { + // Classify fills: inside if a guaranteed-interior point of the fill is inside + // the lasso. (A non-convex sub-fill's edge-midpoint average can fall inside the + // lasso even when the shape is mostly outside, so use fill_interior_point.) + let mut inside_fills: Vec = Vec::new(); + for (i, fill) in graph_after.fills.iter().enumerate() { let fid = FillId(i as u32); if fill.deleted { continue; } - let centroid = Self::compute_fill_centroid(graph, fid); - if region_path.winding(centroid) != 0 { - inside_fills.insert(fid); + let probe = graph_after.fill_interior_point(fid); + if region_path.winding(probe) != 0 { + inside_fills.push(fid); } } - // If nothing is inside, restore snapshot and bail - if inside_edges.is_empty() && inside_fills.is_empty() { - *graph = snapshot; - None - } else { - // Extract subgraph (boundary edges get duplicated into both graphs) - let (selected_graph, vtx_map, edge_map) = graph.extract_subgraph( - &inside_edges, - &inside_fills, - ®ion_edge_set, - ); - - // Build boundary maps for merge-back - let boundary_vertex_map: HashMap = vtx_map - .iter() - .filter(|(&old_vid, _)| !graph.vertex(old_vid).deleted) - .map(|(&old, &new)| (new, old)) - .collect(); - - let boundary_edge_map: HashMap = edge_map - .iter() - .filter(|(old_eid, _)| region_edge_set.contains(old_eid)) - .map(|(&old, &new)| (new, old)) - .collect(); - - Some((snapshot, selected_graph, region_edge_ids, boundary_vertex_map, boundary_edge_map)) - } + (graph_before, graph_after, inside_edges, inside_fills) }; - let Some((snapshot, selected_graph, region_edge_ids, boundary_vertex_map, boundary_edge_map)) = extraction_result else { + // Nothing inside: don't mutate the graph (avoid littering it with stray cut + // edges), leaving the selection as cleared/extended on drag-start. + if inside_edges.is_empty() && inside_fills.is_empty() { #[cfg(debug_assertions)] shared.test_mode.clear_pending_geometry(); return; - }; + } - *shared.region_selection = Some(lightningbeam_core::selection::RegionSelection { - region_path: region_path.clone(), + // Record the undo depth before the first cut of this selection session, so a + // later deselect can heal (undo) the cut(s) if nothing was changed in between. + // A shift-additive series of cuts keeps the same base (it accumulates). + if shared.pending_region_cut_base.is_none() { + *shared.pending_region_cut_base = Some(shared.action_executor.undo_depth()); + } + + // Commit the cut as one undoable edit. + let action = lightningbeam_core::actions::ModifyGraphAction::new( layer_id, time, - graph_snapshot: snapshot, - selected_graph, - transform: vello::kurbo::Affine::IDENTITY, - committed: false, - region_edge_ids, - action_epoch_at_selection: shared.action_executor.epoch(), - boundary_vertex_map, - boundary_edge_map, - }); - - shared.selection.clear_geometry_selection(); - - #[cfg(debug_assertions)] - shared.test_mode.clear_pending_geometry(); - } - - /// Compute the centroid of a fill's boundary edge midpoints. - fn compute_fill_centroid( - graph: &lightningbeam_core::vector_graph::VectorGraph, - fid: lightningbeam_core::vector_graph::FillId, - ) -> vello::kurbo::Point { - use vello::kurbo::{ParamCurve, Point}; - let fill = graph.fill(fid); - let mut sum_x = 0.0; - let mut sum_y = 0.0; - let mut count = 0; - for &(eid, _) in &fill.boundary { - if eid.is_none() { - continue; - } - let mid = graph.edge(eid).curve.eval(0.5); - sum_x += mid.x; - sum_y += mid.y; - count += 1; - } - if count > 0 { - Point::new(sum_x / count as f64, sum_y / count as f64) - } else { - Point::ZERO - } - } - - /// Revert an uncommitted region selection, restoring the DCEL from snapshot - fn revert_region_selection_static(shared: &mut SharedPaneState) { - use lightningbeam_core::layer::AnyLayer; - - let region_sel = match shared.region_selection.take() { - Some(rs) => rs, - None => return, - }; - - if region_sel.committed { - // Already committed via action system, nothing to revert + graph_before, + graph_after, + "Region select", + ); + if let Err(e) = shared.action_executor.execute(Box::new(action)) { + eprintln!("Region select cut failed: {}", e); + #[cfg(debug_assertions)] + shared.test_mode.clear_pending_geometry(); return; } - let no_actions_taken = - shared.action_executor.epoch() == region_sel.action_epoch_at_selection; - let no_transform = region_sel.transform == vello::kurbo::Affine::IDENTITY; - - let doc = shared.action_executor.document_mut(); - if let Some(AnyLayer::Vector(vl)) = doc.get_layer_mut(®ion_sel.layer_id) { - if let Some(graph) = vl.graph_at_time_mut(region_sel.time) { - if no_actions_taken && no_transform { - // Fast path: nothing changed, restore from snapshot - *graph = region_sel.graph_snapshot; - } else { - // Delete the main graph's copy of boundary edges - for &eid in ®ion_sel.region_edge_ids { - if !graph.edge(eid).deleted { - graph.free_edge(eid); - } - } - - // Merge the (possibly transformed) selected_graph back - graph.merge_subgraph( - ®ion_sel.selected_graph, - region_sel.transform, - ®ion_sel.boundary_vertex_map, - ®ion_sel.boundary_edge_map, - ); - - // Clean up invisible edges left from the boundary - graph.gc_invisible_edges(); + // Select the inside sub-pieces from the now-current (post-cut) graph. The fill/edge + // ids are stable because the committed graph is exactly `graph_after`. + if let Some(AnyLayer::Vector(vl)) = shared.action_executor.document().get_layer(&layer_id) { + if let Some(graph) = vl.graph_at_time(time) { + for fid in &inside_fills { + shared.selection.select_fill(*fid, graph); + } + for eid in &inside_edges { + shared.selection.select_edge(*eid, graph); } } } - shared.selection.clear_geometry_selection(); + if shared.selection.has_geometry_selection() { + *shared.focus = + lightningbeam_core::selection::FocusSelection::Geometry { layer_id, time }; + } + + #[cfg(debug_assertions)] + shared.test_mode.clear_pending_geometry(); } /// Create a rectangle path centered at origin (easier for curve editing later) @@ -10874,7 +10748,7 @@ impl StagePane { self.handle_eyedropper_tool(ui, &response, mouse_pos, shared); } Tool::RegionSelect => { - self.handle_region_select_tool(ui, &response, world_pos, shared); + self.handle_region_select_tool(ui, &response, world_pos, shift_held, shared); } Tool::Warp => { self.handle_raster_warp_tool(ui, &response, world_pos, shared); @@ -12112,7 +11986,6 @@ impl PaneRenderer for StagePane { editing_clip_id: shared.editing_clip_id, editing_instance_id: shared.editing_instance_id, editing_parent_layer_id: shared.editing_parent_layer_id, - region_selection: shared.region_selection.clone(), mouse_world_pos, webcam_frame: shared.webcam_frame.clone(), pending_raster_dabs: self.pending_raster_dabs.take(),