Skip to content

Render List<String> as raw paths in SVG and Vello mode instead of ever becoming List<Vector>#4141

Open
Annonnymmousss wants to merge 3 commits into
GraphiteEditor:masterfrom
Annonnymmousss:feat/raw_path-text-rendering
Open

Render List<String> as raw paths in SVG and Vello mode instead of ever becoming List<Vector>#4141
Annonnymmousss wants to merge 3 commits into
GraphiteEditor:masterfrom
Annonnymmousss:feat/raw_path-text-rendering

Conversation

@Annonnymmousss
Copy link
Copy Markdown
Contributor

@Annonnymmousss Annonnymmousss commented May 12, 2026

Currently :
image
image

Now:

Screen.Recording.2026-05-13.at.3.01.06.AM.mov

@Annonnymmousss Annonnymmousss force-pushed the feat/raw_path-text-rendering branch from cfdd328 to 2f9fd96 Compare May 12, 2026 21:41
@Annonnymmousss
Copy link
Copy Markdown
Contributor Author

@cubic-dev

@cubic-dev-ai
Copy link
Copy Markdown
Contributor

cubic-dev-ai Bot commented May 12, 2026

@cubic-dev

@Annonnymmousss I have started the AI code review. It will take a few minutes to complete.

Copy link
Copy Markdown
Contributor

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

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 implements text rendering support by introducing a Text variant to the Graphic enum and providing rendering implementations for SVG and Vello using the parley and skrifa crates. The changes involve updating the node graph runtime to handle font registration, defining new font attributes, and extending the Render and BoundingBox traits. Feedback from the review recommends removing the unused available_fonts field from RenderParams, simplifying font registration using parley's Blob::from_bytes, and refactoring the text layout logic into a shared helper to eliminate code duplication between the SVG and Vello rendering paths.

Comment thread node-graph/libraries/rendering/src/renderer.rs Outdated
Comment thread node-graph/libraries/rendering/src/renderer.rs
Comment thread node-graph/libraries/rendering/src/renderer.rs
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

3 issues found across 12 files

Confidence score: 3/5

  • There is concrete user-facing regression risk in text rendering: node-graph/libraries/rendering/src/renderer.rs is reported to miss group opacity/blend-mode compositing in Vello, which can visibly darken overlapping glyphs and ignore expected blending/opacity behavior.
  • Interaction correctness is also at risk in node-graph/libraries/rendering/src/renderer.rs because hit-testing uses a fixed rectangle (font_size * 6. × font_size) instead of real text bounds, which can break clicking and selection.
  • node-graph/libraries/graphic-types/src/graphic.rs returning RenderBoundingBox::None for Graphic::Text adds medium risk, since that value is treated as "skip this element" in list bound computation and may cause text to be excluded from bounds-related logic.
  • Pay close attention to node-graph/libraries/rendering/src/renderer.rs and node-graph/libraries/graphic-types/src/graphic.rs - text visual compositing, hit-testing, and bounding behavior may regress user-visible editor behavior.
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/graphic-types/src/graphic.rs">

<violation number="1" location="node-graph/libraries/graphic-types/src/graphic.rs:377">
P2: `Graphic::Text` returns `RenderBoundingBox::None` for both `bounding_box` and `thumbnail_bounding_box`, but it represents renderable path content. `RenderBoundingBox::None` means "skip this element" in list bound computation, which will cause culling, thumbnail, and hit-testing issues for text/path-only groups.</violation>
</file>

<file name="node-graph/libraries/rendering/src/renderer.rs">

<violation number="1" location="node-graph/libraries/rendering/src/renderer.rs:2286">
P1: Vello text rendering missing group opacity/blend-mode compositing: overlapping glyphs double-darken, blend modes ignored, and outline mode ignores opacity</violation>

<violation number="2" location="node-graph/libraries/rendering/src/renderer.rs:2363">
P1: Text click targets use a fixed-size rectangle (`font_size * 6.` × `font_size`) that doesn't match actual text dimensions, breaking hit-testing and selection.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread node-graph/libraries/rendering/src/renderer.rs
Comment thread node-graph/libraries/rendering/src/renderer.rs
Comment thread node-graph/libraries/graphic-types/src/graphic.rs Outdated
@Annonnymmousss Annonnymmousss marked this pull request as ready for review May 12, 2026 22:29
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

1 issue found across 12 files

Confidence score: 3/5

  • There is a concrete performance risk in node-graph/nodes/text/src/font_cache.rs: iter_fonts copies full font bytes into a new Arc<[u8]> on each call, which can add avoidable CPU and memory pressure during rendering.
  • Given the high confidence (9/10) and medium-high severity (7/10), this is more than a cosmetic issue and could regress responsiveness in text-heavy workloads.
  • Pay close attention to node-graph/nodes/text/src/font_cache.rs - repeated per-call font byte cloning in iter_fonts may degrade render performance.
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/nodes/text/src/font_cache.rs">

<violation number="1" location="node-graph/nodes/text/src/font_cache.rs:138">
P1: `iter_fonts` allocates and copies full font bytes into a new `Arc<[u8]>` for every font on every call, causing unnecessary memory/CPU overhead during rendering.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread node-graph/nodes/text/src/font_cache.rs Outdated
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.

1 participant