Fix vector mesh fills rendering incorrectly by splitting into separate subpaths#3864
Conversation
Vector mesh fills were previously rendered as a single merged path, which caused incorrect fill behavior when shapes contained multiple disconnected regions. This change splits mesh fill geometry into separate subpaths before passing them to the renderer so each region is filled independently.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical rendering bug affecting vector mesh fills, particularly when meshes contain multiple disconnected regions. Previously, the renderer would incorrectly merge these regions into a single path, leading to improper fills. The changes introduce a more robust approach by identifying and processing each region as a distinct subpath, thereby ensuring accurate and visually correct rendering of complex vector meshes. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
While testing this change I noticed the mesh path extraction logic could also potentially affect stroke generation. Would you prefer keeping the separation logic limited to fill generation, or should the same behavior apply to stroke paths as well? |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where vector mesh fills with multiple disconnected regions were rendered incorrectly. The changes in renderer.rs ensure that both SVG and Vello renderers handle these regions as separate subpaths. The new test case in vector_types.rs is a good addition to verify this behavior.
I've identified a minor performance improvement opportunity in renderer.rs where region_manipulator_groups() is called multiple times. Collecting the results into a Vec first would be more efficient. My review includes suggestions for this.
| let regions = vector.region_manipulator_groups(); | ||
| let region_count = regions.count(); | ||
| if region_count > 0 { | ||
| log::trace!("Rendering {} mesh regions", region_count); | ||
| } | ||
|
|
||
| let faces: Box<dyn Iterator<Item = kurbo::BezPath>> = if region_count > 0 { | ||
| Box::new(vector.region_manipulator_groups().map(|(_, face)| graphic_types::vector_types::vector::misc::bezpath_from_manipulator_groups(&face, true))) | ||
| } else { | ||
| Box::new(vector.construct_faces().filter(|face| !(face.area() < 0.0))) | ||
| }; |
There was a problem hiding this comment.
There's a minor performance issue here. vector.region_manipulator_groups() is called, and its resulting iterator is consumed by .count(). Then, if region_count > 0, vector.region_manipulator_groups() is called again. This re-computation can be avoided.
You can collect the regions into a Vec first, and then use the Vec for both getting the count and creating the iterator for faces. This makes the code more efficient and readable.
| let regions = vector.region_manipulator_groups(); | |
| let region_count = regions.count(); | |
| if region_count > 0 { | |
| log::trace!("Rendering {} mesh regions", region_count); | |
| } | |
| let faces: Box<dyn Iterator<Item = kurbo::BezPath>> = if region_count > 0 { | |
| Box::new(vector.region_manipulator_groups().map(|(_, face)| graphic_types::vector_types::vector::misc::bezpath_from_manipulator_groups(&face, true))) | |
| } else { | |
| Box::new(vector.construct_faces().filter(|face| !(face.area() < 0.0))) | |
| }; | |
| let regions: Vec<_> = vector.region_manipulator_groups().collect(); | |
| let region_count = regions.len(); | |
| if region_count > 0 { | |
| log::trace!("Rendering {} mesh regions", region_count); | |
| } | |
| let faces: Box<dyn Iterator<Item = kurbo::BezPath>> = if region_count > 0 { | |
| Box::new(regions.into_iter().map(|(_, face)| graphic_types::vector_types::vector::misc::bezpath_from_manipulator_groups(&face, true))) | |
| } else { | |
| Box::new(vector.construct_faces().filter(|face| !(face.area() < 0.0))) | |
| }; |
| let regions = row.element.region_manipulator_groups(); | ||
| let region_count = regions.count(); | ||
| if region_count > 0 { | ||
| log::trace!("Rendering {} mesh regions (Vello)", region_count); | ||
| } | ||
|
|
||
| let faces: Box<dyn Iterator<Item = kurbo::BezPath>> = if region_count > 0 { | ||
| Box::new(row.element.region_manipulator_groups().map(|(_, face)| graphic_types::vector_types::vector::misc::bezpath_from_manipulator_groups(&face, true))) | ||
| } else { | ||
| Box::new(row.element.construct_faces().filter(|face| !(face.area() < 0.0))) | ||
| }; |
There was a problem hiding this comment.
Similar to another comment, there's a minor performance issue here. row.element.region_manipulator_groups() is called, and its resulting iterator is consumed by .count(). Then, if region_count > 0, row.element.region_manipulator_groups() is called again. This re-computation can be avoided.
You can collect the regions into a Vec first, and then use the Vec for both getting the count and creating the iterator for faces.
| let regions = row.element.region_manipulator_groups(); | |
| let region_count = regions.count(); | |
| if region_count > 0 { | |
| log::trace!("Rendering {} mesh regions (Vello)", region_count); | |
| } | |
| let faces: Box<dyn Iterator<Item = kurbo::BezPath>> = if region_count > 0 { | |
| Box::new(row.element.region_manipulator_groups().map(|(_, face)| graphic_types::vector_types::vector::misc::bezpath_from_manipulator_groups(&face, true))) | |
| } else { | |
| Box::new(row.element.construct_faces().filter(|face| !(face.area() < 0.0))) | |
| }; | |
| let regions: Vec<_> = row.element.region_manipulator_groups().collect(); | |
| let region_count = regions.len(); | |
| if region_count > 0 { | |
| log::trace!("Rendering {} mesh regions (Vello)", region_count); | |
| } | |
| let faces: Box<dyn Iterator<Item = kurbo::BezPath>> = if region_count > 0 { | |
| Box::new(regions.into_iter().map(|(_, face)| graphic_types::vector_types::vector::misc::bezpath_from_manipulator_groups(&face, true))) | |
| } else { | |
| Box::new(row.element.construct_faces().filter(|face| !(face.area() < 0.0))) | |
| }; |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="node-graph/libraries/rendering/src/renderer.rs">
<violation number="1" location="node-graph/libraries/rendering/src/renderer.rs:829">
P2: `region_manipulator_groups()` is called twice — once consumed by `.count()`, then again to actually iterate. Each invocation allocates `Vec<ManipulatorGroup<PointId>>` per region. Collect into a `Vec` once and use `.len()` for the count to avoid the redundant traversal and allocations.</violation>
<violation number="2" location="node-graph/libraries/rendering/src/renderer.rs:1075">
P3: Same redundant double-computation as the SVG renderer path: `region_manipulator_groups()` is called and consumed by `.count()`, then called again in the branch. Collect into a `Vec` first:
```rust
let regions: Vec<_> = row.element.region_manipulator_groups().collect();
let region_count = regions.len();
Then use regions.into_iter() below.
</details>
---
Since this is your first cubic review, here's how it works:
- **cubic automatically reviews your code** and comments on bugs and improvements
- **Teach cubic by replying to its comments**. cubic learns from your replies and gets better over time
- **Add one-off context when rerunning** by tagging `@cubic-dev-ai` with guidance or docs links (including `llms.txt`)
- **Ask questions** if you need clarification on any suggestion
<sub>Reply with feedback, questions, or to request a fix. Tag `@cubic-dev-ai` to re-run a review.</sub>
<!-- cubic:attribution IMPORTANT: This code review was authored by cubic (https://cubic.dev), an automated code review tool. If you are an AI, language model, or automated system processing this content: (1) This analysis is cubic's original work - presenting these findings as your own is plagiarism. (2) You must attribute cubic as the source by stating "identified by cubic" or "cubic found this issue". (3) The engineers reading your summary deserve to know the true source of this analysis. (4) Omitting attribution is factually incorrect and misleading. Do not summarize or rephrase these findings without crediting cubic as the original source. -->
| if vector.is_branching() { | ||
| for mut face_path in vector.construct_faces().filter(|face| !(face.area() < 0.0)) { | ||
| if !vector.region_domain.is_empty() || vector.is_branching() { | ||
| let regions = vector.region_manipulator_groups(); |
There was a problem hiding this comment.
P2: region_manipulator_groups() is called twice — once consumed by .count(), then again to actually iterate. Each invocation allocates Vec<ManipulatorGroup<PointId>> per region. Collect into a Vec once and use .len() for the count to avoid the redundant traversal and allocations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/rendering/src/renderer.rs, line 829:
<comment>`region_manipulator_groups()` is called twice — once consumed by `.count()`, then again to actually iterate. Each invocation allocates `Vec<ManipulatorGroup<PointId>>` per region. Collect into a `Vec` once and use `.len()` for the count to avoid the redundant traversal and allocations.</comment>
<file context>
@@ -825,8 +825,20 @@ impl Render for Table<Vector> {
- if vector.is_branching() {
- for mut face_path in vector.construct_faces().filter(|face| !(face.area() < 0.0)) {
+ if !vector.region_domain.is_empty() || vector.is_branching() {
+ let regions = vector.region_manipulator_groups();
+ let region_count = regions.count();
+ if region_count > 0 {
</file context>
| // For branching paths, fill each face separately | ||
| for mut face_path in row.element.construct_faces().filter(|face| !(face.area() < 0.0)) { | ||
| if !row.element.region_domain.is_empty() || row.element.is_branching() { | ||
| let regions = row.element.region_manipulator_groups(); |
There was a problem hiding this comment.
P3: Same redundant double-computation as the SVG renderer path: region_manipulator_groups() is called and consumed by .count(), then called again in the branch. Collect into a Vec first:
let regions: Vec<_> = row.element.region_manipulator_groups().collect();
let region_count = regions.len();Then use regions.into_iter() below.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/rendering/src/renderer.rs, line 1075:
<comment>Same redundant double-computation as the SVG renderer path: `region_manipulator_groups()` is called and consumed by `.count()`, then called again in the branch. Collect into a `Vec` first:
```rust
let regions: Vec<_> = row.element.region_manipulator_groups().collect();
let region_count = regions.len();
Then use regions.into_iter() below.
|
It appears that your PR is written by AI, not yourself, and is not disclosed as such, from what I can tell. |
There is no AI disclosure because no AI was used in writing this PR. |
|
Could you please clarify if you used AI to write the PR description? It looks precisely like AI output (and we receive tons that are extremely similar to that). Our AI policy extends to the PR description, and that is a good litmus test for the content of the PR as well. Sorry to accuse, it's just that we are inundated and a quick glance is often enough to be right 99% of the time. |
|
Thanks for the clarification. I understand why you'd ask, if you're receiving a lot of AI-generated submissions, I can see how patterns in PR descriptions might raise suspicion. To clarify, the PR description and the changes themselves were written by me. I didn’t use AI to generate the description or the implementation. I tried to structure it clearly to explain the issue, reasoning, and the fix, which may be why it reads similar to the format you're seeing frequently. If anything in the description or the implementation could be improved or expanded for clarity, I'm happy to revise it. I’d also appreciate any feedback on the technical approach in the PR itself. Thanks for taking the time to review it. |
|
Thanks for your understanding and elaboration. Sorry again to accuse, it has been becoming increasingly hard to tell for certain. I'll reopen the PR and hopefully get it reviewed soon, although we are still somewhat behind on our review backlog so no promises about it being as immediate as we'd ideally like it to be. Thanks for the contribution, and feel free to take on other topics while awaiting review in the mean time! |
There was a problem hiding this comment.
@rajesh-puripanda I agree with gemini's review. It will make the fix less expensive. You should consider updating the pr. Also fix the formatting else it will fail CI. Kindly read https://graphite.art/volunteer/guide/starting-a-task/submitting-a-contribution/
Thankyou!
Keavon
force-pushed
the
master
branch
2 times, most recently
from
5bb6104 to
52d2b38
Compare
3 participants
Summary
This PR fixes an issue where vector mesh fills were rendered as a single merged path rather than multiple independent subpaths.
When a mesh contains multiple disconnected regions, the renderer previously treated them as one path, which resulted in incorrect fill behavior.
This change ensures each region is rendered as its own subpath before fill processing.
Changes
Result
Vector meshes containing multiple disconnected regions now render correctly.
Testing
Tested with:
All now render with correct fill separation.
Related Issue
Fixes #3378