Implement text-on-path support with SVG 2 compliance#4055
Draft
jsjgdh wants to merge 5 commits intoGraphiteEditor:masterfrom
Draft
Implement text-on-path support with SVG 2 compliance#4055jsjgdh wants to merge 5 commits intoGraphiteEditor:masterfrom
jsjgdh wants to merge 5 commits intoGraphiteEditor:masterfrom
Conversation
Contributor
Author
Contributor
|
@jsjgdh I have started the AI code review. It will take a few minutes to complete. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements SVG 2 text-on-path layout rules by adding a new text_on_path node and integrating the kurbo library for path calculations. The FontCache was refactored to use Arc for better data sharing, and the PathBuilder was updated to handle glyph drawing for paths. Review feedback identifies several critical issues, including fragile string-based font mapping in SVGs, incorrect glyph horizontal shifting due to absolute coordinate reuse, and a bug in path reversal for complex geometries. Improvements were also suggested for the efficiency of arc length sampling and the correction of a double-radian conversion in font synthesis.
Comment on lines
+232
to
+233
| let final_transform = | ||
| DAffine2::from_translation(DVec2::new(point.x, point.y)) * DAffine2::from_angle(angle) * DAffine2::from_translation(DVec2::new(glyph.x as f64, -glyph.y as f64)); |
Contributor
There was a problem hiding this comment.
The glyph transformation uses glyph.x as a translation after placing the glyph on the path. In parley, glyph.x is the absolute horizontal position within the run. Since the path is already sampled at a specific distance (mid), applying glyph.x again will cause incorrect horizontal shifting along the path. The translation should only account for the offset relative to the sampled point (e.g., centering the glyph horizontally).
Suggested change
| let final_transform = | |
| DAffine2::from_translation(DVec2::new(point.x, point.y)) * DAffine2::from_angle(angle) * DAffine2::from_translation(DVec2::new(glyph.x as f64, -glyph.y as f64)); | |
| let final_transform = | |
| DAffine2::from_translation(DVec2::new(point.x, point.y)) * DAffine2::from_angle(angle) * DAffine2::from_translation(DVec2::new(-glyph.advance as f64 / 2.0, -glyph.y as f64)); |
| insert_index: usize, | ||
| graphite_gradient_stops: &HashMap<String, GradientStops>, | ||
| ) { | ||
| log::error!("DIAGNOSTIC: Visiting node root: {:?}", node); |
Contributor
There was a problem hiding this comment.
This diagnostic message uses log::error!, which is typically reserved for actual error conditions that require developer or operator attention. For diagnostic or tracing information, log::trace! or log::debug! would be more appropriate.
| for i in 1..=samples_per_segment { | ||
| let t = i as f64 / samples_per_segment as f64; | ||
| let sub_len = seg.subsegment(0.0..t).arclen(accuracy); | ||
| lengths.push(cumulative + sub_len); |
Contributor
There was a problem hiding this comment.
Calling seg.subsegment(0.0..t).arclen(accuracy) inside a loop is inefficient because calculating the arc length of a Bezier curve involves numerical integration. This results in
| line.items().for_each(|item| { | ||
| if let PositionedLayoutItem::GlyphRun(glyph_run) = item { | ||
| let run = glyph_run.run(); | ||
| let style_skew = run.synthesis().skew().map(|angle| DAffine2::from_cols_array(&[1., 0., -angle.to_radians().tan() as f64, 1., 0., 0.])); |
Contributor
There was a problem hiding this comment.
In parley 0.6, RunSynthesis::skew returns the angle in radians. Calling to_radians() on a value that is already in radians will result in an incorrect skew angle.
Suggested change
| let style_skew = run.synthesis().skew().map(|angle| DAffine2::from_cols_array(&[1., 0., -angle.to_radians().tan() as f64, 1., 0., 0.])); | |
| let style_skew = run.synthesis().skew().map(|angle| DAffine2::from_cols_array(&[1., 0., -(angle as f64).tan(), 1., 0., 0.])); |
Member
|
!build (Run ID 24950870909) |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
10 issues found across 13 files
Confidence score: 3/5
- There is moderate merge risk because several medium-severity, high-confidence text rendering/import issues could affect user-visible output, especially around fallback font selection and text-on-path placement behavior.
- The most severe functional concerns are in
node-graph/nodes/text/src/text_on_path.rsandeditor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs, where anchoring math and chunk assignment can misplace or corrupt multi-line/multi-chunk SVG text layout. - Additional correctness regressions in
node-graph/nodes/text/src/path_builder.rsandnode-graph/nodes/text/src/font_cache.rs(synthetic skew handling, nondeterministic fallback key choice, and style-name changes for 950 weight) increase the chance of visual inconsistencies; the PR title convention issue appears process-related rather than runtime-breaking. - Pay close attention to
node-graph/nodes/text/src/text_on_path.rs,editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs,node-graph/nodes/text/src/font_cache.rs, andnode-graph/nodes/text/src/path_builder.rs- these contain the highest-impact rendering and import behavior risks.
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:53">
P2: Removing the explicit 950 font-weight name changes generated style strings and can break exact style-name matching for 950 fonts.</violation>
<violation number="2" location="node-graph/nodes/text/src/font_cache.rs:126">
P2: Missing-font fallback chooses an arbitrary `HashMap` key, making font resolution nondeterministic when default font is unavailable.</violation>
</file>
<file name="node-graph/nodes/text/src/text_context.rs">
<violation number="1" location="node-graph/nodes/text/src/text_context.rs:64">
P2: `layout_text` was made fully public, unintentionally expanding the external API and exposing backend-specific `Layout` details; `pub(crate)` is sufficient for the new internal caller.</violation>
</file>
<file name="node-graph/nodes/text/src/text_on_path.rs">
<violation number="1" location="node-graph/nodes/text/src/text_on_path.rs:125">
P2: `reverse_bezpath` collects all segments across subpaths and reverses them as a single continuous path. Any `MoveTo` elements between distinct subpaths are lost, causing separate subpaths to be incorrectly merged into one. For paths with multiple subpaths, this produces incorrect geometry with unexpected connecting lines.</violation>
<violation number="2" location="node-graph/nodes/text/src/text_on_path.rs:207">
P2: Multiline text-on-path anchoring uses layout block width (`full_width`) while placement advances cursor across all lines, causing middle/end anchor misalignment and visibility errors.</violation>
</file>
<file name="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs:555">
P2: Unconditional error-level diagnostic log in normal SVG import path causes noisy false error reports.</violation>
<violation number="2" location="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs:715">
P2: Text-on-path import incorrectly applies all chunk text to the first path chunk, breaking multi-chunk/mixed-flow SVG text layout.</violation>
</file>
<file name="node-graph/nodes/text/src/path_builder.rs">
<violation number="1" location="node-graph/nodes/text/src/path_builder.rs:38">
P2: `origin` is dead state: it is assigned but never read, and no longer affects point generation, making transform state misleading.</violation>
<violation number="2" location="node-graph/nodes/text/src/path_builder.rs:79">
P2: `render_glyph_run` drops font synthesis skew by tying `style_skew` to `tilt` instead of `run.synthesis().skew()`, which can remove synthetic italic/slant styling in rendered output.</violation>
</file>
<file name="node-graph/nodes/gstd/src/text.rs">
<violation number="1" location="node-graph/nodes/gstd/src/text.rs:81">
P2: Custom agent: **PR title enforcement**
PR title is not in imperative mood and lacks a leading action verb as required by PR title conventions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Wasm: 23.10 MB — JS: 0.43 MB — CSS: 0.09 MB — Fonts: 0.30 MB — Images: 0.09 MB — All Assets: 24.01 MB |
Keavon
force-pushed
the
master
branch
2 times, most recently
from
f07c79b to
76938eb
Compare
jsjgdh
force-pushed
the
text-on-path
branch
2 times, most recently
from
ef54a07 to
2c13faa
Compare
Contributor
|
@jsjgdh I have started the AI code review. It will take a few minutes to complete. |
Contributor
Author
Contributor
|
@jsjgdh I have started the AI code review. It will take a few minutes to complete. |
jsjgdh
changed the title
Contributor
There was a problem hiding this comment.
6 issues found across 18 files
Confidence score: 2/5
- There are concrete runtime-failure risks:
node-graph/nodes/text/src/text_on_path.rscan panic on invalid font parsing, andeditor/src/messages/portfolio/document/graph_operation/utility_types.rscan panic when Text On Path node registration is missing, which makes this high risk to merge as-is. editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rsappears to parse TextPath attributes from the wrong SVG scope and collides anonymous<text>elements on an empty key, so layout can be applied to the wrong node (clear user-facing regression risk).node-graph/nodes/text/src/path_builder.rsbakes per-glyph placement into geometry instead ofTableRow.transform, which can break downstream assumptions about row-level transforms; the duplication note ineditor/src/messages/portfolio/document/graph_operation/utility_types.rsis lower severity but increases future drift risk.- Pay close attention to
node-graph/nodes/text/src/text_on_path.rs,editor/src/messages/portfolio/document/graph_operation/utility_types.rs,editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs, andnode-graph/nodes/text/src/path_builder.rs- they contain panic paths and text-on-path mapping/transform behaviors most likely to cause regressions.
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/text_on_path.rs">
<violation number="1" location="node-graph/nodes/text/src/text_on_path.rs:294">
P1: Unwrap on font parsing can panic on invalid font data/index, crashing text-on-path rendering instead of skipping the affected run.</violation>
</file>
<file name="editor/src/messages/portfolio/document/graph_operation/utility_types.rs">
<violation number="1" location="editor/src/messages/portfolio/document/graph_operation/utility_types.rs:293">
P3: Custom agent: **PR title enforcement**
PR title is a noun phrase instead of an imperative command.</violation>
<violation number="2" location="editor/src/messages/portfolio/document/graph_operation/utility_types.rs:312">
P2: Panics if Text On Path node registration is missing instead of failing gracefully.</violation>
<violation number="3" location="editor/src/messages/portfolio/document/graph_operation/utility_types.rs:354">
P3: This new method duplicates the existing transform/stroke/fill chain setup logic, which increases the risk of the different insertion paths drifting apart over time.</violation>
</file>
<file name="node-graph/nodes/text/src/path_builder.rs">
<violation number="1" location="node-graph/nodes/text/src/path_builder.rs:58">
P2: Per-glyph placement is baked into geometry instead of preserved in `TableRow.transform`, breaking the row-level transform contract for downstream consumers.</violation>
</file>
<file name="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs:554">
P2: TextPath attrs are parsed out of the wrong SVG scope and anonymous `<text>` elements collide on the empty-string key, so text-on-path layout can be applied to the wrong node.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ) { | ||
| let path_vector = Table::new_from_element(Vector::from_subpaths(path_subpaths, true)); | ||
| let text_on_path_node = resolve_proto_node_type(graphene_std::text::text_on_path::IDENTIFIER) | ||
| .expect("Text On Path node does not exist") |
Contributor
There was a problem hiding this comment.
P2: Panics if Text On Path node registration is missing instead of failing gracefully.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/graph_operation/utility_types.rs, line 312:
<comment>Panics if Text On Path node registration is missing instead of failing gracefully.</comment>
<file context>
@@ -289,6 +290,82 @@ impl<'a> ModifyInputsContext<'a> {
+ ) {
+ let path_vector = Table::new_from_element(Vector::from_subpaths(path_subpaths, true));
+ let text_on_path_node = resolve_proto_node_type(graphene_std::text::text_on_path::IDENTIFIER)
+ .expect("Text On Path node does not exist")
+ .node_template_input_override([
+ Some(NodeInput::scope("editor-api")),
</file context>
| ..Default::default() | ||
| }); | ||
| let mut vector = Vector::from_subpaths(subpaths, false); | ||
| vector.transform(transform); |
Contributor
There was a problem hiding this comment.
P2: Per-glyph placement is baked into geometry instead of preserved in TableRow.transform, breaking the row-level transform contract for downstream consumers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/text/src/path_builder.rs, line 58:
<comment>Per-glyph placement is baked into geometry instead of preserved in `TableRow.transform`, breaking the row-level transform contract for downstream consumers.</comment>
<file context>
@@ -1,158 +1,149 @@
- ..Default::default()
- });
+ let mut vector = Vector::from_subpaths(subpaths, false);
+ vector.transform(transform);
+ self.vector_table.push(TableRow::new_from_element(vector));
} else {
</file context>
| @@ -3,7 +3,7 @@ use crate::messages::portfolio::document::node_graph::document_node_definitions: | |||
| use crate::messages::portfolio::document::utility_types::document_metadata::LayerNodeIdentifier; | |||
Contributor
There was a problem hiding this comment.
P3: This new method duplicates the existing transform/stroke/fill chain setup logic, which increases the risk of the different insertion paths drifting apart over time.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/graph_operation/utility_types.rs, line 354:
<comment>This new method duplicates the existing transform/stroke/fill chain setup logic, which increases the risk of the different insertion paths drifting apart over time.</comment>
<file context>
@@ -289,6 +290,82 @@ impl<'a> ModifyInputsContext<'a> {
+ self.network_interface.insert_node(transform_id, transform_node, &[]);
+ self.network_interface.move_node_to_chain_start(&transform_id, layer, &[], self.import);
+
+ let stroke = resolve_proto_node_type(graphene_std::vector_nodes::stroke::IDENTIFIER)
+ .expect("Stroke node does not exist")
+ .default_node_template();
</file context>
Contributor
There was a problem hiding this comment.
1 issue found across 10 files
Confidence score: 5/5
- This PR is low risk to merge because the only reported issue is a title-convention mismatch, not a code behavior defect.
- The most severe item is in
node-graph/nodes/text/src/text_on_path.rsat 3/10 severity and points to PR title phrasing (noun phrase vs imperative), so impact is process/compliance rather than user-facing functionality. - Pay close attention to
node-graph/nodes/text/src/text_on_path.rs- ensure the associated PR title is updated to match the required imperative convention.
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/text_on_path.rs">
<violation number="1" location="node-graph/nodes/text/src/text_on_path.rs:1">
P3: Custom agent: **PR title enforcement**
PR title is a noun phrase rather than an imperative command, so it does not satisfy the required title convention.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
Contributor
|
@jsjgdh I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
6 issues found across 18 files
Confidence score: 2/5
- Several high-confidence, user-facing regressions are likely:
unwrapcalls innode-graph/nodes/text/src/path_builder.rscan panic on malformed fonts, and the boolean parameter semantic change can silently break existing call sites with incorrect text scaling/rendering. - The fallback behavior in
node-graph/nodes/text/src/font_cache.rscan choose an arbitrary cached font when the default is missing, creating nondeterministic and incorrect text appearance across runs. - Export/cache correctness is at risk because
text_on_path_metadatais not invalidated onVectormutation and is omitted fromHashinnode-graph/libraries/vector-types/src/vector/vector_types.rs, which can leave stale SVG output or missed change detection. - Pay close attention to
node-graph/nodes/text/src/path_builder.rs,node-graph/libraries/vector-types/src/vector/vector_types.rs, andnode-graph/nodes/text/src/font_cache.rs- panic risk, silent API mismatch, and stale/nondeterministic text output need targeted fixes before merge.
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/path_builder.rs">
<violation number="1" location="node-graph/nodes/text/src/path_builder.rs:20">
P1: Changing the first `bool` parameter to mean `is_text_on_path` creates a silent API mismatch with existing call sites that still pass `per_glyph_instances`, causing incorrect scaling/rendering.</violation>
<violation number="2" location="node-graph/nodes/text/src/path_builder.rs:41">
P1: Unwraps in the text rendering path can panic on malformed/unsupported font data.</violation>
</file>
<file name="node-graph/nodes/text/src/font_cache.rs">
<violation number="1" location="node-graph/nodes/text/src/font_cache.rs:126">
P2: Missing-font resolution now falls back to an arbitrary cached font when the default font is unavailable, which can silently render text with the wrong face/style and vary across runs.</violation>
</file>
<file name="node-graph/libraries/vector-types/src/vector/vector_types.rs">
<violation number="1" location="node-graph/libraries/vector-types/src/vector/vector_types.rs:70">
P2: `text_on_path_metadata` is export-significant but is not invalidated when a `Vector` is mutated, so stale text-on-path metadata can produce incorrect SVG output after geometry changes.</violation>
<violation number="2" location="node-graph/libraries/vector-types/src/vector/vector_types.rs:97">
P2: `Vector`'s `Hash` implementation omits the new export-relevant `text_on_path_metadata`, so hash-based caches/change detection can miss text-on-path updates.</violation>
</file>
<file name="node-graph/nodes/text/src/text_on_path.rs">
<violation number="1" location="node-graph/nodes/text/src/text_on_path.rs:56">
P2: `ArcLengthLut::build` accepts `0` and silently creates a degenerate LUT, so later `at()` lookups collapse to the path start instead of being validated or clamped.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
Contributor
|
@jsjgdh I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
7 issues found across 20 files
Confidence score: 2/5
- High-confidence, user-impacting risks remain:
editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rscan create extra sibling layers on multi-chunk text import, breaking child-extents and potentially crashing position assignment. node-graph/nodes/text/src/path_builder.rshas two notable regression vectors—constant collision seeding can reintroduce duplicate point IDs across glyphs, and baseline anchoring changes can shift non-per-glyph text layout.node-graph/libraries/vector-types/src/vector/vector_types.rsandnode-graph/nodes/text/src/text_on_path.rsshow consistency gaps (stale text-on-path metadata invalidation, partial hashing, ignored RTL parameter) that can produce incorrect exports or directionality behavior after edits.- Pay close attention to
editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs,node-graph/nodes/text/src/path_builder.rs,node-graph/libraries/vector-types/src/vector/vector_types.rs,node-graph/nodes/text/src/text_on_path.rs- invariants, ID uniqueness, and text-on-path state/placement can regress in real documents.
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/path_builder.rs">
<violation number="1" location="node-graph/nodes/text/src/path_builder.rs:78">
P1: Concatenating per-glyph vectors with a constant collision seed can reintroduce duplicate point IDs across glyphs, breaking the unique-ID invariant in the final vector.</violation>
<violation number="2" location="node-graph/nodes/text/src/path_builder.rs:113">
P2: Synthetic skew is no longer anchored to the run baseline for non-per-glyph text runs, which can shift glyph placement and regress normal text layout.</violation>
</file>
<file name="node-graph/libraries/vector-types/src/vector/vector_types.rs">
<violation number="1" location="node-graph/libraries/vector-types/src/vector/vector_types.rs:98">
P2: Text-on-path metadata is only partially hashed, and `font_size` is truncated to `u64`, creating avoidable collisions and a hash that does not reflect the full equality-relevant state.</violation>
<violation number="2" location="node-graph/libraries/vector-types/src/vector/vector_types.rs:110">
P2: Text-on-path metadata is only invalidated on one mutation path, so edited vectors can keep stale `<textPath>` export state.</violation>
</file>
<file name="node-graph/nodes/text/src/text_on_path.rs">
<violation number="1" location="node-graph/nodes/text/src/text_on_path.rs:200">
P2: RTL support is exposed by the API but not implemented here; the `rtl` parameter is ignored, so right-to-left text is placed like LTR text.</violation>
</file>
<file name="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs:563">
P2: Synthesized textPath path ids are not checked for collisions with existing SVG ids, which can create duplicate ids and misdirect href resolution during import.</violation>
<violation number="2" location="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs:817">
P1: Multi-chunk text import can create extra sibling layers that break the child-extents invariant and crash position assignment.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Not to be merged, only for testing.
Known Issue: Font