Implement text-on-path support with SVG 2 compliance - #4055
Open
jsjgdh wants to merge 4 commits into
Open
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
April 29, 2026 12:16
f07c79b to
76938eb
Compare
jsjgdh
force-pushed
the
text-on-path
branch
2 times, most recently
from
May 2, 2026 22:27
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>
| @@ -1,9 +1,9 @@ | |||
| use super::transform_utils; | |||
| use crate::messages::portfolio::document::node_graph::document_node_definitions::{DefinitionIdentifier, resolve_document_node_type, resolve_network_node_type, resolve_proto_node_type}; | |||
| 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.
Keavon
force-pushed
the
master
branch
2 times, most recently
from
May 17, 2026 14:37
4b7a823 to
847b8e9
Compare
timon-schelling
force-pushed
the
master
branch
2 times, most recently
from
May 17, 2026 15:37
15fcaac to
d5f0140
Compare
jsjgdh
marked this pull request as ready for review
August 13, 2026 07:32
Contributor
There was a problem hiding this comment.
19 issues found across 18 files
Confidence score: 2/5
- The highest-risk blocker is in
editor/src/messages/portfolio/document/graph_operation/utility_types.rs: text-on-path insertion can panic becauseNodeNetworkInterface::insert_noderejects aNodeInput::Node, which can break the feature at creation time—insert the node first and then wire it in a separate step. node-graph/nodes/text/src/text_on_path.rshas multiple layout/parameter consistency risks (segment-boundary stalling, multiline overlap, andSome(0)length metadata divergence) that can produce visibly wrong placement and SVG/render mismatches—fix boundary interpolation and normalize/reject unsupported inputs to keep geometry and metadata aligned.- Export fidelity in
node-graph/libraries/rendering/src/renderer.rsis currently fragile: missingfill="none", incomplete font properties (font-weight/font-stretch), and dropped blend mode can change appearance in downstream SVG viewers—emit explicit style attributes from the same layout metadata used for rendering. node-graph/libraries/rendering/src/renderer.rsandeditor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rsshow semantic handling gaps (clip-path bypass in clipping runs and missed inherited RTL direction on import), which can invert ordering or ignore clipping in real documents—route text-on-path through clip-mask state and resolvedirectionthrough ancestors.
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/vector-types/src/vector/vector_attributes.rs">
<violation number="1" location="node-graph/libraries/vector-types/src/vector/vector_attributes.rs:1206">
P2: The Vector::transform now clears text_on_path_metadata when geometry changes, but the sibling Vector::bake_transform (vector_types.rs line 126) also mutates geometry (point positions and segment_domain) yet does not clear the metadata. A text-on-path vector passed through the Bake Transform node therefore keeps a stale metadata.path_d and start_offset that no longer match its transformed geometry, so SVG export emits a <textPath> placed along the original path while the editor shows the transformed outlines. Make the two code paths consistent so a geometry transform always invalidates the text-on-path metadata.</violation>
</file>
<file name="node-graph/nodes/gstd/src/text.rs">
<violation number="1" location="node-graph/nodes/gstd/src/text.rs:130">
P2: The Text on Path node does not expose the font selector used by the existing Text node. Add the `text_font` widget override so this input can select loaded typefaces rather than using the generic resource control.</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:292">
P1: Every text-on-path insertion panics here because `text_on_path_node` contains a `NodeInput::Node`, which `NodeNetworkInterface::insert_node` explicitly rejects. Insert the node without this wire and connect it with `set_input` afterward, or use the supported group-insertion path.</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:118">
P1: At every segment boundary, `ArcLengthLut::at` pins the point to the previous segment for one LUT interval instead of interpolating into the next segment. Text therefore stalls at each corner on paths with multiple segments; select `seg_idx1` and interpolate `t1` when the interval crosses segments.</violation>
<violation number="2" location="node-graph/nodes/text/src/text_on_path.rs:227">
P3: For closed paths, `curvature_spacing_adjustment` samples `mid +/- half` via `at_with_extension`, which extends along the tangent when `s` is outside `[0, total_length]` instead of wrapping like `point_on_path` does for placement. Near the seam of a closed path, Auto spacing therefore uses the wrong curvature and creates a discontinuity where the text starts. Route these samples through `point_on_path` (which wraps for closed paths) so Auto spacing matches the placement.</violation>
<violation number="3" location="node-graph/nodes/text/src/text_on_path.rs:317">
P2: When an optional length is enabled with the UI-allowed value `0`, geometry ignores it but the attached metadata preserves `Some(0)`, making SVG export disagree with the rendered result. Normalize or reject non-positive `text_length` and `path_length` before using them for both layout and metadata.</violation>
<violation number="4" location="node-graph/nodes/text/src/text_on_path.rs:327">
P2: When the input contains a newline, every layout line is placed at the same path positions, causing the lines to overlap. Apply a per-line path/baseline offset or reject multiline text explicitly instead of laying all lines over one another.</violation>
<violation number="5" location="node-graph/nodes/text/src/text_on_path.rs:388">
P2: When the selected font has a bold or variable weight, the outlines use that weight but the exported `<text>` metadata does not preserve it. Store and emit the font weight so SVG export selects the same face as the rendered geometry.</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:856">
P2: When RTL is declared on the parent `<text>`, this lookup misses the inherited value and imports the text in LTR order. Resolve `direction` through the textPath ancestor chain or use the resolved span direction.</violation>
<violation number="2" location="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs:1043">
P2: When a text path uses italic or bold styling, this line discards the resolved span style and always selects `Regular (400)`. Derive the Graphite font style from the span’s resolved weight/style.</violation>
<violation number="3" location="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs:1061">
P2: When a text chunk contains multiple styled `<tspan>` runs, this single text-on-path node applies the first span’s styling to the combined text. Create separate runs/layers or preserve each span’s font and paint.</violation>
</file>
<file name="node-graph/libraries/rendering/src/renderer.rs">
<violation number="1" location="node-graph/libraries/rendering/src/renderer.rs:1407">
P2: When a font family name contains CSS metacharacters, XML escaping alone does not make this `style` value safe and the exported SVG can receive extra or malformed declarations. Serialize the family and style with CSS escaping or emit them as separately escaped presentation attributes.</violation>
<violation number="2" location="node-graph/libraries/rendering/src/renderer.rs:1407">
P2: When the source font has a non-normal weight or width, the exported `<text>` uses different glyph metrics because this CSS omits `font-weight` and `font-stretch`. Carry the layout font weight/width in the metadata and emit both properties.</violation>
<violation number="3" location="node-graph/libraries/rendering/src/renderer.rs:1439">
P1: When a text-on-path vector has no fill, this emits no `fill` attribute and SVG renders the text with its default black fill. Emit `fill="none"` when `graphic_list_at` returns no paint, matching the normal vector path renderer.</violation>
<violation number="4" location="node-graph/libraries/rendering/src/renderer.rs:1445">
P2: When text-on-path content contains significant whitespace, the exported text no longer matches the outline layout because the `<text>` element uses SVG's default whitespace handling. Add `xml:space="preserve"` (or equivalent `white-space: pre`) to the generated text element.</violation>
<violation number="5" location="node-graph/libraries/rendering/src/renderer.rs:1445">
P2: When a text-on-path layer uses a non-default blend mode, this replacement omits it and exports source-over rendering. Preserve the vector's blend-mode style on the generated `<text>` element.</violation>
<violation number="6" location="node-graph/libraries/rendering/src/renderer.rs:1446">
P2: When a text-on-path vector participates in a clipping run, this early `continue` bypasses `clip_mask_state`, so the vector is rendered without the clip or cannot serve as the clip source. Route the emitted text through the same clipping logic as ordinary vectors.</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:32">
P2: TextOnPathMetadata stores several values whose valid domains are small closed sets (text_anchor: start/middle/end, side: left/right, method: align/stretch, spacing: exact/auto, length_adjust: spacing/spacingAndGlyphs). Storing them as free String makes the type admit invalid states that the renderer then emits verbatim into SVG attributes, and the renderer cannot match on them. Model these as Rust enums so invalid values are unrepresentable.</violation>
</file>
<file name="node-graph/nodes/text/src/path_builder.rs">
<violation number="1" location="node-graph/nodes/text/src/path_builder.rs:119">
P3: The two new methods panic with `glyph.draw(...).unwrap()` and `element_mut(0).unwrap()`. A draw error or an empty list would abort node-graph evaluation instead of failing locally. Prefer handling the `Result`/`Option` (e.g. log and return) rather than unwrapping on this call path.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ]); | ||
|
|
||
| let text_on_path_id = NodeId::new(); | ||
| self.network_interface.insert_node(text_on_path_id, text_on_path_node, &[]); |
Contributor
There was a problem hiding this comment.
P1: Every text-on-path insertion panics here because text_on_path_node contains a NodeInput::Node, which NodeNetworkInterface::insert_node explicitly rejects. Insert the node without this wire and connect it with set_input afterward, or use the supported group-insertion path.
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 292:
<comment>Every text-on-path insertion panics here because `text_on_path_node` contains a `NodeInput::Node`, which `NodeNetworkInterface::insert_node` explicitly rejects. Insert the node without this wire and connect it with `set_input` afterward, or use the supported group-insertion path.</comment>
<file context>
@@ -234,6 +235,98 @@ impl<'a> ModifyInputsContext<'a> {
+ ]);
+
+ let text_on_path_id = NodeId::new();
+ self.network_interface.insert_node(text_on_path_id, text_on_path_node, &[]);
+ self.network_interface.move_node_to_chain_start(&text_on_path_id, layer, &[], self.import);
+
</file context>
| let (seg_idx1, t1) = self.params[next_idx]; | ||
|
|
||
| // Interpolate t within the segment | ||
| let t = if seg_idx0 == seg_idx1 && (l1 - l0) > 1e-9 { t0 + (t1 - t0) * (s - l0) / (l1 - l0) } else { t0 }; |
Contributor
There was a problem hiding this comment.
P1: At every segment boundary, ArcLengthLut::at pins the point to the previous segment for one LUT interval instead of interpolating into the next segment. Text therefore stalls at each corner on paths with multiple segments; select seg_idx1 and interpolate t1 when the interval crosses segments.
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/text_on_path.rs, line 118:
<comment>At every segment boundary, `ArcLengthLut::at` pins the point to the previous segment for one LUT interval instead of interpolating into the next segment. Text therefore stalls at each corner on paths with multiple segments; select `seg_idx1` and interpolate `t1` when the interval crosses segments.</comment>
<file context>
@@ -0,0 +1,432 @@
+ let (seg_idx1, t1) = self.params[next_idx];
+
+ // Interpolate t within the segment
+ let t = if seg_idx0 == seg_idx1 && (l1 - l0) > 1e-9 { t0 + (t1 - t0) * (s - l0) / (l1 - l0) } else { t0 };
+
+ let seg = self.segs.get(seg_idx0)?;
</file context>
| PaintTarget::Fill, | ||
| ) | ||
| }) | ||
| .unwrap_or_default(); |
Contributor
There was a problem hiding this comment.
P1: When a text-on-path vector has no fill, this emits no fill attribute and SVG renders the text with its default black fill. Emit fill="none" when graphic_list_at returns no paint, matching the normal vector path renderer.
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 1439:
<comment>When a text-on-path vector has no fill, this emits no `fill` attribute and SVG renders the text with its default black fill. Emit `fill="none"` when `graphic_list_at` returns no paint, matching the normal vector path renderer.</comment>
<file context>
@@ -1383,10 +1391,61 @@ fn render_vector_item_svg(list: &List<Vector>, index: usize, vector: &Vector, re
+ PaintTarget::Fill,
+ )
+ })
+ .unwrap_or_default();
+ let opacity_attr = self.attribute_cloned_or(ATTR_OPACITY, index, 1.);
+ let opacity_fill_attr = self.attribute_cloned_or(ATTR_OPACITY_FILL, index, 1.);
</file context>
Suggested change
| .unwrap_or_default(); | |
| .unwrap_or_else(|| r#" fill="none""#.to_string()); |
| pub fn transform(&mut self, transform: DAffine2) { | ||
| self.point_domain.transform(transform); | ||
| self.segment_domain.transform(transform); | ||
| self.text_on_path_metadata = None; |
Contributor
There was a problem hiding this comment.
P2: The Vector::transform now clears text_on_path_metadata when geometry changes, but the sibling Vector::bake_transform (vector_types.rs line 126) also mutates geometry (point positions and segment_domain) yet does not clear the metadata. A text-on-path vector passed through the Bake Transform node therefore keeps a stale metadata.path_d and start_offset that no longer match its transformed geometry, so SVG export emits a placed along the original path while the editor shows the transformed outlines. Make the two code paths consistent so a geometry transform always invalidates the text-on-path metadata.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/vector-types/src/vector/vector_attributes.rs, line 1206:
<comment>The Vector::transform now clears text_on_path_metadata when geometry changes, but the sibling Vector::bake_transform (vector_types.rs line 126) also mutates geometry (point positions and segment_domain) yet does not clear the metadata. A text-on-path vector passed through the Bake Transform node therefore keeps a stale metadata.path_d and start_offset that no longer match its transformed geometry, so SVG export emits a <textPath> placed along the original path while the editor shows the transformed outlines. Make the two code paths consistent so a geometry transform always invalidates the text-on-path metadata.</comment>
<file context>
@@ -1203,6 +1203,7 @@ impl Vector {
pub fn transform(&mut self, transform: DAffine2) {
self.point_domain.transform(transform);
self.segment_domain.transform(transform);
+ self.text_on_path_metadata = None;
}
</file context>
| /// The vector path that glyphs follow. | ||
| path: Item<Vector>, | ||
| /// The loaded font file used to draw the text. The editor resolves the chosen typeface to these bytes via the resource system. | ||
| font: Item<Resource>, |
Contributor
There was a problem hiding this comment.
P2: The Text on Path node does not expose the font selector used by the existing Text node. Add the text_font widget override so this input can select loaded typefaces rather than using the generic resource control.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/gstd/src/text.rs, line 130:
<comment>The Text on Path node does not expose the font selector used by the existing Text node. Add the `text_font` widget override so this input can select loaded typefaces rather than using the generic resource control.</comment>
<file context>
@@ -115,3 +116,75 @@ fn text_to_vector_glyphs(
+ /// The vector path that glyphs follow.
+ path: Item<Vector>,
+ /// The loaded font file used to draw the text. The editor resolves the chosen typeface to these bytes via the resource system.
+ font: Item<Resource>,
+ /// The font size in pixels.
+ #[unit(" px")]
</file context>
Suggested change
| font: Item<Resource>, | |
| #[widget(ParsedWidgetOverride::Custom = "text_font")] | |
| font: Item<Resource>, |
| let opacity_attr_str = if opacity < 1. { format!(r#" opacity="{opacity}""#) } else { String::new() }; | ||
|
|
||
| render.leaf_node(format!(r##"<text style="{font_style_css} {anchor_style}"{transform_attr}{direction_attr}{opacity_attr_str}{fill_attr}><textPath href="#{path_id}" startOffset="{start_offset_attr}" method="{method}" spacing="{spacing}"{side_attr}{text_length_attr}{path_length_attr}>{text}</textPath></text>"##)); | ||
| continue; |
Contributor
There was a problem hiding this comment.
P2: When a text-on-path vector participates in a clipping run, this early continue bypasses clip_mask_state, so the vector is rendered without the clip or cannot serve as the clip source. Route the emitted text through the same clipping logic as ordinary vectors.
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 1446:
<comment>When a text-on-path vector participates in a clipping run, this early `continue` bypasses `clip_mask_state`, so the vector is rendered without the clip or cannot serve as the clip source. Route the emitted text through the same clipping logic as ordinary vectors.</comment>
<file context>
@@ -1383,10 +1391,61 @@ fn render_vector_item_svg(list: &List<Vector>, index: usize, vector: &Vector, re
+ let opacity_attr_str = if opacity < 1. { format!(r#" opacity="{opacity}""#) } else { String::new() };
+
+ render.leaf_node(format!(r##"<text style="{font_style_css} {anchor_style}"{transform_attr}{direction_attr}{opacity_attr_str}{fill_attr}><textPath href="#{path_id}" startOffset="{start_offset_attr}" method="{method}" spacing="{spacing}"{side_attr}{text_length_attr}{path_length_attr}>{text}</textPath></text>"##));
+ continue;
+ }
+
</file context>
| let opacity = opacity_attr * opacity_fill_attr; | ||
| let opacity_attr_str = if opacity < 1. { format!(r#" opacity="{opacity}""#) } else { String::new() }; | ||
|
|
||
| render.leaf_node(format!(r##"<text style="{font_style_css} {anchor_style}"{transform_attr}{direction_attr}{opacity_attr_str}{fill_attr}><textPath href="#{path_id}" startOffset="{start_offset_attr}" method="{method}" spacing="{spacing}"{side_attr}{text_length_attr}{path_length_attr}>{text}</textPath></text>"##)); |
Contributor
There was a problem hiding this comment.
P2: When a text-on-path layer uses a non-default blend mode, this replacement omits it and exports source-over rendering. Preserve the vector's blend-mode style on the generated <text> element.
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 1445:
<comment>When a text-on-path layer uses a non-default blend mode, this replacement omits it and exports source-over rendering. Preserve the vector's blend-mode style on the generated `<text>` element.</comment>
<file context>
@@ -1383,10 +1391,61 @@ fn render_vector_item_svg(list: &List<Vector>, index: usize, vector: &Vector, re
+ let opacity = opacity_attr * opacity_fill_attr;
+ let opacity_attr_str = if opacity < 1. { format!(r#" opacity="{opacity}""#) } else { String::new() };
+
+ render.leaf_node(format!(r##"<text style="{font_style_css} {anchor_style}"{transform_attr}{direction_attr}{opacity_attr_str}{fill_attr}><textPath href="#{path_id}" startOffset="{start_offset_attr}" method="{method}" spacing="{spacing}"{side_attr}{text_length_attr}{path_length_attr}>{text}</textPath></text>"##));
+ continue;
+ }
</file context>
| pub start_offset: f64, | ||
| pub start_offset_percent: bool, | ||
| /// "start" | "middle" | "end" | ||
| pub text_anchor: String, |
Contributor
There was a problem hiding this comment.
P2: TextOnPathMetadata stores several values whose valid domains are small closed sets (text_anchor: start/middle/end, side: left/right, method: align/stretch, spacing: exact/auto, length_adjust: spacing/spacingAndGlyphs). Storing them as free String makes the type admit invalid states that the renderer then emits verbatim into SVG attributes, and the renderer cannot match on them. Model these as Rust enums so invalid values are unrepresentable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/vector-types/src/vector/vector_types.rs, line 32:
<comment>TextOnPathMetadata stores several values whose valid domains are small closed sets (text_anchor: start/middle/end, side: left/right, method: align/stretch, spacing: exact/auto, length_adjust: spacing/spacingAndGlyphs). Storing them as free String makes the type admit invalid states that the renderer then emits verbatim into SVG attributes, and the renderer cannot match on them. Model these as Rust enums so invalid values are unrepresentable.</comment>
<file context>
@@ -13,6 +13,35 @@ use dyn_any::StaticType;
+ pub start_offset: f64,
+ pub start_offset_percent: bool,
+ /// "start" | "middle" | "end"
+ pub text_anchor: String,
+ /// "left" | "right"
+ pub side: String,
</file context>
|
|
||
| fn curvature_spacing_adjustment(lut: &ArcLengthLut, mid: f64, advance: f64) -> f64 { | ||
| let half = advance / 2.0; | ||
| let (_, a0) = at_with_extension(lut, mid - half); |
Contributor
There was a problem hiding this comment.
P3: For closed paths, curvature_spacing_adjustment samples mid +/- half via at_with_extension, which extends along the tangent when s is outside [0, total_length] instead of wrapping like point_on_path does for placement. Near the seam of a closed path, Auto spacing therefore uses the wrong curvature and creates a discontinuity where the text starts. Route these samples through point_on_path (which wraps for closed paths) so Auto spacing matches the placement.
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/text_on_path.rs, line 227:
<comment>For closed paths, `curvature_spacing_adjustment` samples `mid +/- half` via `at_with_extension`, which extends along the tangent when `s` is outside `[0, total_length]` instead of wrapping like `point_on_path` does for placement. Near the seam of a closed path, Auto spacing therefore uses the wrong curvature and creates a discontinuity where the text starts. Route these samples through `point_on_path` (which wraps for closed paths) so Auto spacing matches the placement.</comment>
<file context>
@@ -0,0 +1,432 @@
+
+fn curvature_spacing_adjustment(lut: &ArcLengthLut, mid: f64, advance: f64) -> f64 {
+ let half = advance / 2.0;
+ let (_, a0) = at_with_extension(lut, mid - half);
+ let (_, a1) = at_with_extension(lut, mid + half);
+ let angle_delta = (a1 - a0 + std::f64::consts::PI).rem_euclid(std::f64::consts::TAU) - std::f64::consts::PI;
</file context>
|
|
||
| let location_ref = LocationRef::new(normalized_coords); | ||
| let settings = DrawSettings::unhinted(Size::new(size), location_ref); | ||
| glyph.draw(settings, self).unwrap(); |
Contributor
There was a problem hiding this comment.
P3: The two new methods panic with glyph.draw(...).unwrap() and element_mut(0).unwrap(). A draw error or an empty list would abort node-graph evaluation instead of failing locally. Prefer handling the Result/Option (e.g. log and return) rather than unwrapping on this call path.
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 119:
<comment>The two new methods panic with `glyph.draw(...).unwrap()` and `element_mut(0).unwrap()`. A draw error or an empty list would abort node-graph evaluation instead of failing locally. Prefer handling the `Result`/`Option` (e.g. log and return) rather than unwrapping on this call path.</comment>
<file context>
@@ -105,6 +105,64 @@ impl PathBuilder {
+
+ let location_ref = LocationRef::new(normalized_coords);
+ let settings = DrawSettings::unhinted(Size::new(size), location_ref);
+ glyph.draw(settings, self).unwrap();
+
+ self.origin = saved_origin;
</file context>
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.
CLOSES #978