Skip to content

Fix SVG even-odd fill rule imports (#4332)#4337

Open
NicTanghe wants to merge 4 commits into
GraphiteEditor:masterfrom
NicTanghe:evenodd
Open

Fix SVG even-odd fill rule imports (#4332)#4337
NicTanghe wants to merge 4 commits into
GraphiteEditor:masterfrom
NicTanghe:evenodd

Conversation

@NicTanghe

Copy link
Copy Markdown

• ## Summary

Fixes SVG imports ignoring fill-rule="evenodd".

  • Preserves the parsed fill rule as a vector attribute.
  • Applies it consistently in SVG and Vello rendering.
  • Keeps existing branching-vector fill behavior unchanged.
  • Migrates existing Fill nodes with NonZero as the default.
  • Adds regression tests for importing, propagation, and rendering.

Testing

  • cargo test -p vector-nodes -p rendering -p graphite-editor
  • 144 tests passed; 1 existing test ignored.
  • cargo fmt --all -- --check
  • git diff --check

Closes #4332.

@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 introduces support for explicit vector fill rules (NonZero and EvenOdd) across the application. This includes defining the FillRule enum, updating the fill node to accept a fill rule input, implementing SVG and canvas rendering support for even-odd fills, and adding document migration logic to upgrade legacy fill nodes. Feedback suggests optimizing the document migration logic to avoid redundant node replacements when upgrading legacy fill nodes.

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.

@@ -1667,6 +1667,22 @@ fn migrate_node(node_id: &NodeId, node: &DocumentNode, network_path: &[NodeId],
inputs_count = 7;

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.

medium

By directly setting the default FillRule input and updating inputs_count to 8 at the end of the inputs_count == 4 migration block, we can avoid entering the subsequent inputs_count == 7 block. This prevents redundant calls to replace_implementation and replace_inputs for the same node during a single migration pass, improving performance and reducing overhead.

Suggested change
inputs_count = 7;
document
.network_interface
.set_input(&InputConnector::node(*node_id, 7), NodeInput::value(TaggedValue::FillRule(FillRule::NonZero), false), network_path);
inputs_count = 8;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This was done to prevent other tests from failing.

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

All reported issues were addressed

Confidence score: 5/5

  • Safe to merge after the addressed issues were fixed.

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

Re-trigger cubic

Comment thread node-graph/libraries/rendering/src/renderer.rs
Comment thread editor/src/messages/portfolio/document_migration.rs
NicTanghe and others added 2 commits July 15, 2026 18:56
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
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.

SVG importer ignores fill-rule="evenodd"

1 participant