Skip to content

Bump linesweeper to 0.4#4273

Merged
Keavon merged 2 commits into
GraphiteEditor:masterfrom
jneem:bump-linesweeper
Jun 23, 2026
Merged

Bump linesweeper to 0.4#4273
Keavon merged 2 commits into
GraphiteEditor:masterfrom
jneem:bump-linesweeper

Conversation

@jneem

@jneem jneem commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

As linesweeper 0.4 reversed the path direction, this removes the temporary reversal.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the linesweeper dependency from version 0.3 to 0.4, allowing the removal of a workaround that reversed subpaths. It also implements the of_tag method for WindingNumber to support the updated API. The review feedback highlights a potential out-of-bounds panic in the new of_tag implementation when indexing self.elems[tag], suggesting a defensive check using .get(tag) instead.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread node-graph/nodes/path-bool/src/lib.rs
@Keavon

Keavon commented Jun 22, 2026

Copy link
Copy Markdown
Member

Thanks! What's the of_tag function?

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1 issue found across 3 files

Confidence score: 3/5

  • In node-graph/nodes/path-bool/src/lib.rs, indexing self.elems[tag] can panic when self.elems is shorter than tag + 1 (such as default/shorter initialization), which could crash graph execution at runtime for valid inputs—switch to bounds-safe access like .get(tag).copied().unwrap_or(0) before merging.

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread node-graph/nodes/path-bool/src/lib.rs Outdated
@jneem

jneem commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

It's an annoying consequence of the fact that linesweeper abstracts over the definition of winding numbers. So there's a WindingNumber trait that implements a collection of winding numbers, one for each set that you care about, and the type WindingNumber::Tag identifies a single set. For example, for binary ops linesweeper provides BinaryWindingNumber and its tag type takes 2 values: "set a" or "set b".

WindingNumber::of_tag just restricts the winding number to a single set. So if w is a binary winding number representing -5 for "set a" and 3 for "set b" then w.of_tag(SetA) is (-5, 0) and w.of_tag(SetB) is (0, 3).

Since Graphite supports n-ary set ops, the winding number type has dynamic length and of_tag uses dynamic indexing. If the tag is (tag, out_of) then out_of is always the length of the array and tag is the index of an element. It's definitely a bug if the array access is out of bounds, and I'd be inclined to just panic in that case. But if you want to be defensive about it, I'd say that returning the all-zero array is acceptable.

@Keavon

Keavon commented Jun 22, 2026

Copy link
Copy Markdown
Member

I see, thanks for the explanation. And yes, we try to avoid crashing the app if a bug occurs, since broken behavior with a console error (log::error!("...");) is better than making the user potentially lose their work.

@Keavon Keavon force-pushed the bump-linesweeper branch from 52a468a to ccb52b0 Compare June 23, 2026 00:10
@Keavon Keavon enabled auto-merge June 23, 2026 00:10
@Keavon Keavon added this pull request to the merge queue Jun 23, 2026
Merged via the queue into GraphiteEditor:master with commit 57ee2b0 Jun 23, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants