-
-
Notifications
You must be signed in to change notification settings - Fork 989
Render vector mesh fills more correctly as separate subpaths #3388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -228,6 +228,24 @@ impl BezierHandles { | |
| _ => self, | ||
| } | ||
| } | ||
|
|
||
| /// Returns the tangent vector at the start of the curve (t=0) | ||
| pub fn derivative_at_start(&self, start: DVec2, end: DVec2) -> DVec2 { | ||
| match self { | ||
| BezierHandles::Linear => end - start, | ||
| BezierHandles::Quadratic { handle } => 2.0 * (handle - start), | ||
| BezierHandles::Cubic { handle_start, .. } => 3.0 * (handle_start - start), | ||
| } | ||
| } | ||
|
|
||
| /// Returns the tangent vector at the end of the curve (t=1) | ||
| pub fn derivative_at_end(&self, start: DVec2, end: DVec2) -> DVec2 { | ||
| match self { | ||
| BezierHandles::Linear => end - start, | ||
| BezierHandles::Quadratic { handle } => 2.0 * (end - handle), | ||
| BezierHandles::Cubic { handle_end, .. } => 3.0 * (end - handle_end), | ||
| } | ||
| } | ||
|
Comment on lines
+232
to
+248
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note most of the functionality of this struct has been removed (including a former derivative implementation) in favour of using |
||
| } | ||
|
|
||
| /// Representation of a bezier curve with 2D points. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,8 @@ use crate::vector::vector_types::Vector; | |
| use dyn_any::DynAny; | ||
| use glam::{DAffine2, DVec2}; | ||
| use kurbo::{CubicBez, Line, PathSeg, QuadBez}; | ||
| use std::collections::HashMap; | ||
| use log::debug; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This import is unused. Most crates do It would in fact be very helpful to have some debug logging since I have no clue what the rendering code does. If you want to remove it for performance, you can put it behind a |
||
| use std::collections::{HashMap, HashSet}; | ||
| use std::hash::{Hash, Hasher}; | ||
| use std::iter::zip; | ||
|
|
||
|
|
@@ -678,6 +679,44 @@ impl FoundSubpath { | |
| pub fn contains(&self, segment_id: SegmentId) -> bool { | ||
| self.edges.iter().any(|s| s.id == segment_id) | ||
| } | ||
|
|
||
| pub fn to_bezpath(&self, vector: &Vector) -> kurbo::BezPath { | ||
| let mut bezpath = kurbo::BezPath::new(); | ||
|
|
||
| if let Some(first_edge) = self.edges.first() { | ||
| let start_pos = vector.point_domain.positions()[first_edge.start]; | ||
| bezpath.move_to(dvec2_to_point(start_pos)); | ||
| } | ||
|
|
||
| for edge in &self.edges { | ||
| let segment_index = vector.segment_domain.ids().iter().position(|&id| id == edge.id).expect("Segment ID must exist"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please try to avoid O(n^2) algorithms when you could just store the segment index in the |
||
|
|
||
| let mut handles = vector.segment_domain.handles()[segment_index]; | ||
| if edge.reverse { | ||
| handles = handles.reversed(); | ||
| } | ||
|
|
||
| let end_pos = vector.point_domain.positions()[edge.end]; | ||
|
|
||
| match handles { | ||
| BezierHandles::Linear => { | ||
| bezpath.line_to(dvec2_to_point(end_pos)); | ||
| } | ||
| BezierHandles::Quadratic { handle } => { | ||
| bezpath.quad_to(dvec2_to_point(handle), dvec2_to_point(end_pos)); | ||
| } | ||
| BezierHandles::Cubic { handle_start, handle_end } => { | ||
| bezpath.curve_to(dvec2_to_point(handle_start), dvec2_to_point(handle_end), dvec2_to_point(end_pos)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if self.is_closed() { | ||
| bezpath.close_path(); | ||
| } | ||
|
|
||
| bezpath | ||
| } | ||
| } | ||
|
|
||
| impl Vector { | ||
|
|
@@ -985,6 +1024,134 @@ impl Vector { | |
| self.segment_domain.map_ids(&id_map); | ||
| self.region_domain.map_ids(&id_map); | ||
| } | ||
|
|
||
| pub fn is_branching_mesh(&self) -> bool { | ||
| for point_index in 0..self.point_domain.len() { | ||
| let connection_count = self.segment_domain.connected_count(point_index); | ||
|
|
||
| if connection_count > 2 { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| false | ||
| } | ||
|
|
||
| /// Compute signed area of a face using the shoelace formula. | ||
| /// Negative area indicates clockwise winding, positive indicates counterclockwise. | ||
| fn compute_signed_area(&self, face: &FoundSubpath) -> f64 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| let mut area = 0.0; | ||
| for edge in &face.edges { | ||
| let start_pos = self.point_domain.positions()[edge.start]; | ||
| let end_pos = self.point_domain.positions()[edge.end]; | ||
| area += (end_pos.x - start_pos.x) * (end_pos.y + start_pos.y); | ||
| } | ||
| area * 0.5 | ||
| } | ||
|
|
||
| /// Find all minimal closed regions (faces) in a branching mesh vector. | ||
| pub fn find_closed_regions(&self) -> Vec<FoundSubpath> { | ||
| let mut regions = Vec::new(); | ||
| let mut used_half_edges = HashSet::new(); | ||
|
|
||
| // Build adjacency list sorted by angle for proper face traversal | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does it mean to sort by angle? Angle between which directions? |
||
| let mut adjacency: HashMap<usize, Vec<(SegmentId, usize, bool)>> = HashMap::new(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could be beneficial to add a struct since it is hard to determine what the usize and bool are. |
||
|
|
||
| for (segment_id, start, end, _) in self.segment_domain.iter() { | ||
| adjacency.entry(start).or_default().push((segment_id, end, false)); | ||
| adjacency.entry(end).or_default().push((segment_id, start, true)); | ||
| } | ||
|
|
||
| // Sort neighbors by angle to enable finding the "rightmost" path | ||
| for (&point_idx, neighbors) in adjacency.iter_mut() { | ||
| let start = self.point_domain.positions()[point_idx]; | ||
|
|
||
| neighbors.sort_by(|a, b| { | ||
| let angles: [f64; 2] = [a, b].map(|&(segment_id, end_idx, reversed)| { | ||
| let end = self.point_domain.positions()[end_idx]; | ||
| let curve = self.segment_domain.handles()[self.segment_domain.id_to_index(segment_id).unwrap()]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems a bit wasteful to convert indexes to ids and then back to indexes in the sort by. |
||
| let curve = if reversed { curve.reversed() } else { curve }; | ||
| let direction = curve.derivative_at_start(start, end); | ||
| direction.y.atan2(direction.x) | ||
| }); | ||
| angles[0].partial_cmp(&angles[1]).unwrap_or(std::cmp::Ordering::Equal) | ||
| }); | ||
| } | ||
|
|
||
| for (segment_id, start, end, _) in self.segment_domain.iter() { | ||
| for &reversed in &[false, true] { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no need to do references here since [bool;2] has |
||
| let (from, to) = if reversed { (end, start) } else { (start, end) }; | ||
| let half_edge_key = (segment_id, reversed); | ||
|
|
||
| if used_half_edges.contains(&half_edge_key) { | ||
| continue; | ||
| } | ||
|
|
||
| if let Some(face) = self.find_minimal_face_from_edge(segment_id, from, to, reversed, &adjacency, &mut used_half_edges) { | ||
| regions.push(face); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Filter out outer (counterclockwise) faces, keeping only inner (clockwise) faces | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It isn't really clear why you'd want to do this? |
||
| regions.retain(|face| self.compute_signed_area(face) <= 0.0); | ||
| regions | ||
| } | ||
|
|
||
| /// Helper to find a minimal face (smallest cycle) starting from a half-edge | ||
| fn find_minimal_face_from_edge( | ||
| &self, | ||
| start_segment: SegmentId, | ||
| from_point: usize, | ||
| to_point: usize, | ||
| start_reversed: bool, | ||
| adjacency: &HashMap<usize, Vec<(SegmentId, usize, bool)>>, | ||
| used_half_edges: &mut HashSet<(SegmentId, bool)>, | ||
| ) -> Option<FoundSubpath> { | ||
| let mut path = vec![HalfEdge::new(start_segment, from_point, to_point, start_reversed)]; | ||
| let mut current = to_point; | ||
| let target = from_point; | ||
| let mut prev_segment = start_segment; | ||
|
|
||
| let max_iterations = adjacency.len() * 2; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would this be the max iterations? Surely you should only loop |
||
|
|
||
| // Follow the "rightmost" edge at each vertex to find minimal face | ||
| for _iteration in 1..=max_iterations { | ||
| let neighbors = adjacency.get(¤t)?; | ||
|
|
||
| // Since neighbors are pre-sorted by angle, find the index of the edge we came from | ||
| // and take the next edge in the sorted circular list | ||
| let prev_index = neighbors.iter().position(|(seg, _next, _rev)| *seg == prev_segment)?; | ||
|
|
||
| // The next edge in the sorted list is the minimal face edge (rightmost turn) | ||
| // Wrap around using modulo to handle circular list | ||
| let next = neighbors[(prev_index + 1) % neighbors.len()]; | ||
|
|
||
| let (next_segment, next_point, next_reversed) = next; | ||
|
|
||
| // Check if we've created a cycle (might not be back to start) | ||
| if path.iter().any(|e| e.end == next_point && e.id != next_segment) { | ||
| return None; | ||
| } | ||
|
|
||
| path.push(HalfEdge::new(next_segment, current, next_point, next_reversed)); | ||
|
|
||
| // Check if we've completed the face | ||
| if next_point == target { | ||
| // Mark all half-edges as used | ||
| for edge in &path { | ||
| used_half_edges.insert((edge.id, edge.reverse)); | ||
| } | ||
| return Some(FoundSubpath::new(path)); | ||
| } | ||
|
|
||
| prev_segment = next_segment; | ||
| current = next_point; | ||
| } | ||
|
|
||
| // If we exit the loop without returning, the path didn't close | ||
| None | ||
|
Comment on lines
+1152
to
+1153
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should probably be |
||
| } | ||
| } | ||
|
|
||
| #[derive(Clone, Copy, PartialEq, Eq, Debug, Default)] | ||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This returns a zero vector in the case when the start handle is on top of the start anchor. This is a situation frequently created by the vector tooling in the editor. A zero vector is correct for the derivative, however it isn't correct if you want a tangent (which is what you wrote in the doc comment).