Render Fill and Stroke via List<Graphic>#4111
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces functionality to convert Fill styles into graphical tables, including a new to_transform method for the Gradient struct and associated unit tests. Feedback suggests refactoring the private fill_to_paint function into an implementation of the IntoGraphicTable trait for Fill to align with existing architectural patterns and optimize performance by avoiding unnecessary clones.
1e575b6 to
28b9fba
Compare
dd79824 to
212e088
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the vector fill system to use a List representation, enabling complex fills such as vectors and rasters via clipping. It updates the SVG and Vello renderers to support the ATTR_FILL_GRAPHIC attribute and introduces a fill_graphic debug node. Review feedback identifies high-severity performance bottlenecks caused by frequent cloning of the graphic list in both render loops. Additionally, there is a discrepancy where the SVG renderer only processes the first fill element compared to Vello's multi-fill support, and a concern regarding SVG bloat from redundant clip path generation in complex meshes.
e045d30 to
12d6335
Compare
4b7a823 to
847b8e9
Compare
15fcaac to
d5f0140
Compare
# Conflicts: # node-graph/libraries/rendering/src/renderer.rs
335430e to
9df8439
Compare
This simplifies the future implementation of clipping-based rendering for strokes, as the stroke does not support the use of a clip path but rather paint sources from a paint server.
9df8439 to
ac3f317
Compare
There was a problem hiding this comment.
1 issue found across 6 files
Confidence score: 3/5
- There is a concrete user-facing rendering risk: in
node-graph/libraries/rendering/src/render_ext.rs, an emptyList<Color>produces an empty string rather thanfill="none", so SVG elements can render black instead of transparent. - Given the high confidence (9/10) and meaningful severity (7/10), this is more than a minor edge case and could cause visible regressions in generated SVG output.
- Pay close attention to
node-graph/libraries/rendering/src/render_ext.rs- ensure empty color lists map to explicit transparent fill behavior (fill="none").
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
This exposes List's attributes to message handlers, enabling them to access the necessary attribute data such as ATTR_STROKE_PAINT_GRAPHIC as `Fill` and `Stroke` will not have paint information in the future.
Also extracts `fill_graphic_list_at` / `stroke_paint_graphic_list_at` to share the row-attribute lookup across the existing call sites.
This reverts commit 4285243
Add `fill_attributes` / `stroke_paint_attributes` to `DocumentMetadata` so the `ExpandFillStrokeOnSelectedLayers` handler can read row paint visibility without exposing entire `List<Vector>`.
There was a problem hiding this comment.
4 issues found across 13 files
Confidence score: 3/5
- There is concrete rendering-behavior risk in
node-graph/libraries/graphic-types/src/graphic.rs:Graphic::is_opaque/is_fully_transparentreportedly ignoreATTR_OPACITYandATTR_OPACITY_FILL, which can misclassify partially transparent graphics as opaque. node-graph/libraries/graphic-types/src/graphic.rsalso appears to evaluate only the first paint entry inList<Graphic>, so multi-layer fill/stroke paint setups may produce incorrect opacity/transparent decisions.- This looks mergeable with caution, but the combined medium-to-high severity issues suggest user-visible rendering mismatches are plausible (including the gradient-bounds mismatch noted in
node-graph/libraries/rendering/src/renderer.rs). - Pay close attention to
node-graph/libraries/graphic-types/src/graphic.rsandnode-graph/libraries/rendering/src/renderer.rs- opacity classification and stroke gradient bounds may diverge from expected/SVG behavior.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Partly closes #2779
This PR adds foundation of the renderer to handle
List<Graphic>as a paint source of fill and stroke.Demo
render-fill-and-stroke-via-list-graphic.mp4
The demo file can be found in below.
render-fill-and-stroke-via-list-graphic.graphite.zip
Notes
Fill/ColortoList<Graphic>to avoid breaking current node flow related to theFillandStrokenode. This should be removed after the future refactoring of the node.ATTR_FILL_GRAPHICandATTR_STROKE_PAINT_GRAPHICattribute keys for storing the paintsList<Graphic>on aList<Vector>item.List<Graphic>as a fill and stroke paint source.fill_graphicandstroke_paint_graphicdebug nodes that stores anyGraphiccastableListinto the corresponding attributes to test the clipping paint pass, as it is not available in any production nodes.RenderExtimplementations forPathStyle,Fillare no longer needed, now we have implementation forList<Graphic>instead. The same forStrokeis now shrunk to only generate the shape related attributes.<pattern>element instead of the<clipPath>.