Render vector mesh fills more correctly as separate subpaths #3388

iamthenoname · 2025-11-16T17:13:38Z

Resolves #3378

Using the SVG renderer:

Using the Vello renderer:

The render_svg and render_to_vello functions have become a bit complex and hard to follow, but I plan on improving this after this change has been settled.

…or#3378)

liunicholas6

Mostly nitpicks. Making sure stuff works with multiple edges on the same 2 vertices I think would be a major improvement though (assuming that I'm right that the current code actually doesn't work for that).

liunicholas6 · 2025-11-16T19:54:51Z

node-graph/gcore/src/vector/vector_attributes.rs

+
+			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];


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()];

liunicholas6 · 2025-11-16T19:57:10Z

node-graph/gcore/src/vector/vector_attributes.rs

+		let max_iterations = adjacency.len() * 2;
+
+		// Follow the "rightmost" edge at each vertex to find minimal face
+		loop {


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

liunicholas6 · 2025-11-16T20:02:49Z

node-graph/gcore/src/vector/vector_attributes.rs

+			current = next_point;
+
+			// Prevent infinite loops
+			if path.len() > adjacency.len() {


nit: Redundant with maximum iteration check

liunicholas6 · 2025-11-16T20:44:06Z

node-graph/gcore/src/vector/vector_attributes.rs

+		}
+
+		// Sort neighbors by angle to enable finding the "rightmost" path
+		for (point_idx, neighbors) in adjacency.iter_mut() {


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.

liunicholas6 · 2025-11-16T21:33:12Z

node-graph/gcore/src/vector/vector_attributes.rs

+			}
+
+			// 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) {


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.

liunicholas6 · 2025-11-16T21:34:42Z

node-graph/gcore/src/vector/vector_attributes.rs

+
+			let (next_segment, next_point, next_reversed) = *next;
+
+			if next_point == target {


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

Keavon · 2025-11-17T05:12:41Z

Could you please estimate (as a percentage, or feel free to explain in more detail) how much of this is LLM-authored versus authored directly by you?

iamthenoname · 2025-11-17T05:23:15Z

I'd say around 60% was AI, and 40% was from me.

I didn't use any LLM assistance whatsoever for the rendering code.

For the algorithm itself I implemented it myself first, then had it go through an LLM to check for optimization issues (although I'll admit the LLM did most of the work here).

Admittedly I also got lazy with the follow-up changes and used the LLM to implement those as well. I've been experimenting with these tools recently and wanted to see how effective they would be.

If there's any concerns with the LLM use I'll be toning it down and redoing it myself. I apologize for not making this clear.

Keavon · 2025-11-17T05:28:44Z

Ok, thanks for clarifying and explaining your process. I noticed the prompt files you committed and got concerned that a greater share of the implementation was written without human control than I'd initially expected. The important part is that the actual thought process is yours and the code that's produced is a representation of your engineering competency, so it's not done without a human in the loop. I'm not as concerned about its help doing trivial things like writing the actual characters of code or applying code review style change requests or tab-completing the rest of your lines of code. But beyond trivial uses like tab-completing, we ask that you always disclose LLM authorship so it is clear how that may affect quality, which is crucial information to have when approaching a code review.

Keavon · 2025-11-17T05:33:42Z

package-lock.json

 {
 	"name": "Graphite",
-	"lockfileVersion": 2,
+	"lockfileVersion": 3,


Please remove this file.

Keavon · 2025-11-17T05:33:51Z

!build

github-actions · 2025-11-17T05:46:02Z

📦 Build Complete for `169ddb1`
https://1b86b5c3.graphite.pages.dev

liunicholas6 · 2025-11-17T05:46:09Z

Update: I should have sketched out a few more examples and read the blog post before reviewing.

Derivative angle should actually be the only value used for the sort key for determining being "rightmost", not just the tiebreaker like I initially assumed.

I should have sketched out a few more examples and read the blog post before reviewing.

Additionally, the hack using the derivative at the end instead of the start breaks as many self-intersecting cases and can be ignored.

Apologies for the error

iamthenoname · 2025-11-17T06:36:41Z

Update: I should have sketched out a few more examples and read the blog post before reviewing.

Derivative angle should actually be the only value used for the sort key for determining being "rightmost", not just the tiebreaker like I initially assumed.

I should have sketched out a few more examples and read the blog post before reviewing.

Additionally, the hack using the derivative at the end instead of the start breaks as many self-intersecting cases and can be ignored.

Apologies for the error

I see, I'll fix this as well. I'll be working on making tests to make sure that the algorithm is working properly.

Keavon · 2025-11-17T06:38:47Z

@iamthenoname would you be willing to open up commit access to @liunicholas6 who was requested to collaborate with you on this PR's code? You can do that by inviting him to have write access to your fork's repo in its settings page. If you're willing, of course.

iamthenoname · 2025-11-17T06:43:29Z

Alright, I've added @liunicholas6 as a collaborator.

Ordering of "rightmost" half edge should actually always use the local direction at the vertex rather than the straight line segment direction 2 vertex case is handled by using <= instead of < on retain in the back face cull

0HyperCube

A trivial case for which this does not work:

broken_fill.graphite.txt

Another (without bézier handles):

broken_fill2.graphite.txt

0HyperCube · 2025-11-23T10:32:12Z

node-graph/gcore/src/subpath/structs.rs

+	/// 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),
+		}
+	}


Note most of the functionality of this struct has been removed (including a former derivative implementation) in favour of using kurbo::PathSeg. It is therefore undesirable to add this back.

0HyperCube · 2025-11-23T10:34:21Z

node-graph/gcore/src/subpath/structs.rs

+		match self {
+			BezierHandles::Linear => end - start,
+			BezierHandles::Quadratic { handle } => 2.0 * (handle - start),
+			BezierHandles::Cubic { handle_start, .. } => 3.0 * (handle_start - start),


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).

0HyperCube · 2025-11-23T10:35:23Z

node-graph/gcore/src/vector/vector_attributes.rs

 use glam::{DAffine2, DVec2};
 use kurbo::{CubicBez, Line, PathSeg, QuadBez};
-use std::collections::HashMap;
+use log::debug;


This import is unused. Most crates do #[macro_use] extern crate log; but not this one.

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 #[cfg(feature = "logging")] or something.

0HyperCube · 2025-11-23T10:45:39Z

node-graph/gcore/src/vector/vector_attributes.rs

+
+	/// 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 {


This ignores all of the bézier paths, leading to a completely incorrect area (and potentially misleading winding direction). However you/your LLM never mentions this.

0HyperCube · 2025-11-23T10:46:32Z

node-graph/gcore/src/vector/vector_attributes.rs

+		let mut used_half_edges = HashSet::new();
+
+		// Build adjacency list sorted by angle for proper face traversal
+		let mut adjacency: HashMap<usize, Vec<(SegmentId, usize, bool)>> = HashMap::new();


It could be beneficial to add a struct since it is hard to determine what the usize and bool are.

0HyperCube · 2025-11-23T11:02:39Z

node-graph/gsvg-renderer/src/renderer.rs

-			let path_is_closed = vector.stroke_bezier_paths().all(|path| path.closed());
-			let can_draw_aligned_stroke = path_is_closed && vector.style.stroke().is_some_and(|stroke| stroke.has_renderable_stroke() && stroke.align.is_not_centered());
-			let can_use_paint_order = !(row.element.style.fill().is_none() || !row.element.style.fill().is_opaque() || mask_type == MaskType::Clip);
+			// Helper closure to render a single path with given path string and closed status


Please just make this a separate function. Very long closures are hard to reason about.

0HyperCube · 2025-11-23T11:07:48Z

node-graph/gsvg-renderer/src/renderer.rs


-				let fill_and_stroke = style.render(defs, element_transform, applied_stroke_transform, bounds_matrix, transformed_bounds_matrix, &render_params);
+						let mut style = row.element.style.clone();
+						style.clear_fill(); // Critical: Remove fill to only render stroke


What does it mean to be "Critical"? That rather implies that the rest of the code isn't critical and so can be removed.

This is rather a junk comment since it is very obvious that style.clear_fill() will remove the fill.

0HyperCube · 2025-11-23T11:12:37Z

node-graph/gsvg-renderer/src/renderer.rs

-							scene.push_layer(peniko::Mix::Normal, 1., kurbo::Affine::IDENTITY, &rect);
-							vector_table.render_to_vello(scene, parent_transform, _context, &render_params.for_alignment(applied_stroke_transform));
-							scene.push_layer(peniko::BlendMode::new(peniko::Mix::Normal, compose), 1., kurbo::Affine::IDENTITY, &rect);
+							// Use the existing rendering logic for this fallback case


What do you mean by the "existing rendering logic"? In the diff, it appears to be completely new code.

node-graph/gsvg-renderer/src/renderer.rs

0HyperCube · 2025-11-23T11:38:10Z

node-graph/gcore/src/vector/vector_attributes.rs

+		}
+
+		for edge in &self.edges {
+			let segment_index = vector.segment_domain.ids().iter().position(|&id| id == edge.id).expect("Segment ID must exist");


Please try to avoid O(n^2) algorithms when you could just store the segment index in the HalfEdge struct.

Render vector mesh fills correctly as separate subpaths (GraphiteEdit…

5dedb08

…or#3378)

iamthenoname marked this pull request as ready for review November 16, 2025 17:14

liunicholas6 approved these changes Nov 16, 2025

View reviewed changes

Use derivative tiebreaker

326e00c

iamthenoname force-pushed the master branch from 335c5c1 to 169ddb1 Compare November 17, 2025 05:27

Keavon changed the title ~~Render vector mesh fills correctly as separate subpaths (#3378)~~ Render vector mesh fills more correctly as separate subpaths Nov 17, 2025

Keavon reviewed Nov 17, 2025

View reviewed changes

package-lock.json Outdated

{

"name": "Graphite",

"lockfileVersion": 2,

"lockfileVersion": 3,

Copy link

Member

Keavon Nov 17, 2025

Choose a reason for hiding this comment

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

Please remove this file.

Miscellaneous improvements

9ec1dec

iamthenoname force-pushed the master branch from 169ddb1 to 9ec1dec Compare November 17, 2025 06:33

Fix ordering/modify retain logic

384dccf

Ordering of "rightmost" half edge should actually always use the local direction at the vertex rather than the straight line segment direction 2 vertex case is handled by using <= instead of < on retain in the back face cull

0HyperCube reviewed Nov 23, 2025

View reviewed changes


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

		if next_point == target {

Uh oh!

Render vector mesh fills more correctly as separate subpaths #3388

Are you sure you want to change the base?

Render vector mesh fills more correctly as separate subpaths #3388

Uh oh!

Conversation

iamthenoname commented Nov 16, 2025

Uh oh!

liunicholas6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liunicholas6 Nov 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Keavon commented Nov 17, 2025

Uh oh!

iamthenoname commented Nov 17, 2025

Uh oh!

Keavon commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Keavon commented Nov 17, 2025

Uh oh!

github-actions bot commented Nov 17, 2025

Uh oh!

liunicholas6 commented Nov 17, 2025

Uh oh!

iamthenoname commented Nov 17, 2025

Uh oh!

Keavon commented Nov 17, 2025

Uh oh!

iamthenoname commented Nov 17, 2025

Uh oh!

0HyperCube left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

liunicholas6 Nov 16, 2025 •

edited

Loading

Keavon commented Nov 17, 2025 •

edited

Loading

0HyperCube left a comment •

edited

Loading