Text on path#4055
Conversation
|
@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}
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.
| let svg = svg.replace("font-family=\"sans-serif\"", "font-family=\"Source Sans Pro\""); | ||
| let svg = svg.replace("font-family='sans-serif'", "font-family='Source Sans Pro'"); |
There was a problem hiding this comment.
Performing string replacements on the SVG XML to handle font families is fragile. It doesn't account for variations in whitespace, single vs. double quotes, or font-family lists. It also fails to handle fonts specified via CSS styles. A more robust approach would be to configure usvg to map generic font families to the desired local font via its font database options.
| 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)); |
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).
| 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); |
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); |
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.])); |
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.
| 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.])); |
|
!build (Run ID 24950870909) |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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.
| .font_file_data | ||
| .keys() | ||
| .find(|font| font.font_family == core_types::consts::DEFAULT_FONT_FAMILY && font.font_style == core_types::consts::DEFAULT_FONT_STYLE) | ||
| .or_else(|| self.font_file_data.keys().next()); |
There was a problem hiding this comment.
P2: Missing-font fallback chooses an arbitrary HashMap key, making font resolution nondeterministic when default font is unavailable.
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/font_cache.rs, line 126:
<comment>Missing-font fallback chooses an arbitrary `HashMap` key, making font resolution nondeterministic when default font is unavailable.</comment>
<file context>
@@ -51,81 +50,97 @@ impl Font {
+ .font_file_data
.keys()
.find(|font| font.font_family == core_types::consts::DEFAULT_FONT_FAMILY && font.font_style == core_types::consts::DEFAULT_FONT_STYLE)
+ .or_else(|| self.font_file_data.keys().next());
+ fallback
}
</file context>
| @@ -1,6 +1,7 @@ | |||
| use core_types::{Ctx, table::Table}; | |||
There was a problem hiding this comment.
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.
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 81:
<comment>PR title is not in imperative mood and lacks a leading action verb as required by PR title conventions.</comment>
<file context>
@@ -74,3 +75,48 @@ fn text<'i: 'n>(
+
+/// Flows text glyphs along a vector path following the SVG 2 text-on-path layout rules (§11.8).
+#[node_macro::node(category("Text"))]
+fn text_on_path<'i: 'n>(
+ _: impl Ctx,
+ #[scope("editor-api")] editor_resources: &'i WasmEditorApi,
</file context>
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 |
2 participants
Not to be merged, only for testing.
Known Issue: Font