Skip to content

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.

Sign up for GitHub

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

Open
iamthenoname wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Render vector mesh fills more correctly as separate subpaths #3388

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5dedb08
Render vector mesh fills correctly as separate subpaths (#3378)
iamthenoname Nov 16, 2025
326e00c
Use derivative tiebreaker
iamthenoname Nov 17, 2025
9ec1dec
Miscellaneous improvements
iamthenoname Nov 17, 2025
384dccf
Fix ordering/modify retain logic
liunicholas6 Nov 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
168 changes: 167 additions & 1 deletion node-graph/gcore/src/vector/vector_attributes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ 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 std::collections::{HashMap, HashSet};
use std::hash::{Hash, Hasher};
use std::iter::zip;

Expand Down Expand Up @@ -678,6 +678,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");
Copy link
Contributor

@0HyperCube 0HyperCube Nov 23, 2025

Choose a reason for hiding this comment

The 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 HalfEdge struct.


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 {
Expand Down Expand Up @@ -985,6 +1023,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
}

/// 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
Copy link
Contributor

@0HyperCube 0HyperCube Nov 23, 2025

Choose a reason for hiding this comment

The 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();
Copy link
Contributor

@0HyperCube 0HyperCube Nov 23, 2025

Choose a reason for hiding this comment

The 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() {
Copy link

@liunicholas6 liunicholas6 Nov 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't any tiebreaker here for cases where two edges share the same endpoints. In such cases, you should sort by the derivative of the bezier curve. I think it should look something like this:

Inside impl block of BezierHandles

	pub fn derivative(&mut self, start: DVec2, end: DVec2, t: f64) -> DVec2 {
		let p0 = start;
		match self {
			BezierHandles::Cubic { handle_start, handle_end } => {
				let p1 = *handle_start;
				let p2 = *handle_end;
				let p3 = end;
				3.0 * (1.0 - t).powi(2) * (p1 - p0) + 6.0 * (1.0 - t) * t * (p2 - p1) + 3.0 * t.powi(2) * (p3 - p2)
			}
			BezierHandles::Quadratic { handle } => {
				let p1 = *handle;
				let p2 = end;
				2.0 * (1.0 - t) * (p1 - p0) + 2.0 * t * (p2 - p1)
			}
			BezierHandles::Linear => end - start,
		}
	}

Here

neighbors.sort(|a, b| {
    let segment_angles = [a; b].map(|(_, end_idx, _)| {
        let end = self.point_domain.positions()[end_idx];
        let segment_direction = end - start;
        segment_direction.y.atan2(segment_direction.x)
    });

    let segment_angle_cmp = segment_angles[0].partial_cmp(&segment_angles[1]).unwrap();
    if segment_angle_cmp != std::cmp::Ordering::Equal {
        return segment_angle_cmp;
    }

    let use_source_for_derivative_order = self.segment_domain.id_to_index(a.0) < self.segment_domain.id_to_index(b.0);
    let t : f64 = if use_source_for_derivative_order { 0.0 } else { 1.0 };
    let curve_angles = [a; b].map(|(segment_id, end_idx, reversed)| {
        let end = self.point_domain.positions()[end_idx];
        let forward_curve = self.segment_domain.handles()[self.segment_domain.id_to_index(segment_id)].unwrap();
        let curve_direction = if (reversed) {
            forward_curve.reversed().derivative(start, end, t);
        } else {
            forward_curve.derivative(start, end, t);
        };
        curve_direction.y.atan2(curve_direction.x)
    });
    
    curve_angles[0].partial_cmp(&curve_angles[1]).unwrap()
});

Note that if there is no self-intersecting geometry, then given vertex A and vertex B, the "rightmost" edge order of their shared edges should be the same. However this will not be the case if there is self-intersection -- hence the use_source_for_derivative_order flag, which should prevent breakage. That can be removed once intersection testing is done.

let point_pos = self.point_domain.positions()[*point_idx];
neighbors.sort_by(|a, b| {
let pos_a = self.point_domain.positions()[a.1];
let pos_b = self.point_domain.positions()[b.1];
let angle_a = (pos_a - point_pos).y.atan2((pos_a - point_pos).x);
let angle_b = (pos_b - point_pos).y.atan2((pos_b - point_pos).x);
angle_a.partial_cmp(&angle_b).unwrap_or(std::cmp::Ordering::Equal)
});
}

for (segment_id, start, end, _) in self.segment_domain.iter() {
for &reversed in &[false, true] {
Copy link
Contributor

@0HyperCube 0HyperCube Nov 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to do references here since [bool;2] has into_iter().

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);
}
}
}

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 mut iteration = 0;
let max_iterations = adjacency.len() * 2;
Copy link
Contributor

@0HyperCube 0HyperCube Nov 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would this be the max iterations? Surely you should only loop adjacency.len() times (since you guarantee to not use the same segment multiple times).


// Follow the "rightmost" edge at each vertex to find minimal face
loop {
Copy link

@liunicholas6 liunicholas6 Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (style): for iteration in 1..=max_iterations

iteration += 1;

if iteration > max_iterations {
return None;
}

let neighbors = adjacency.get(&current)?;
// Find the next edge in counterclockwise order (rightmost turn)
let prev_direction = self.point_domain.positions()[current] - self.point_domain.positions()[path.last()?.start];
Copy link

@liunicholas6 liunicholas6 Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neighbors is already sorted; no need to do all this work to find next. If neighbors sorting is more involved (see previous comment on derivatives) then this actually matters.

let prev_index = neighbors.iter().position(|seg, _next, _rev| {*seg == prev_segment})?;
let next = neighbors[(prev_index + 1) % neighbors.size()];


let angle_between = |v1: DVec2, v2: DVec2| -> f64 {
let angle = v2.y.atan2(v2.x) - v1.y.atan2(v1.x);
if angle < 0.0 { angle + 2.0 * std::f64::consts::PI } else { angle }
};

let next = neighbors.iter().filter(|(seg, _next, _rev)| *seg != prev_segment).min_by(|a, b| {
let dir_a = self.point_domain.positions()[a.1] - self.point_domain.positions()[current];
let dir_b = self.point_domain.positions()[b.1] - self.point_domain.positions()[current];
let angle_a = angle_between(prev_direction, dir_a);
let angle_b = angle_between(prev_direction, dir_b);
angle_a.partial_cmp(&angle_b).unwrap_or(std::cmp::Ordering::Equal)
})?;

let (next_segment, next_point, next_reversed) = *next;

if next_point == target {
Copy link

@liunicholas6 liunicholas6 Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Move after line 1144 to reuse the path-push for clarity

// Completed the cycle
path.push(HalfEdge::new(next_segment, current, next_point, next_reversed));

// Mark all half-edges as used
for edge in &path {
used_half_edges.insert((edge.id, edge.reverse));
}

return Some(FoundSubpath::new(path));
}

// 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) {
Copy link

@liunicholas6 liunicholas6 Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I don't think you should need to check that e.id != next_segment. As far as I can tell this check will only occur when traversing the outer face of a mesh a with a bridge, in which case the first part of the condition will trigger.

return None;
}

path.push(HalfEdge::new(next_segment, current, next_point, next_reversed));
prev_segment = next_segment;
current = next_point;

// Prevent infinite loops
if path.len() > adjacency.len() {
Copy link

@liunicholas6 liunicholas6 Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Redundant with maximum iteration check

return None;
}
}
}
}

#[derive(Clone, Copy, PartialEq, Eq, Debug, Default)]
Expand Down
Loading