Add fill tool support on strokes#4032
Add fill tool support on strokes#4032seam0s-dev wants to merge 6 commits intoGraphiteEditor:masterfrom
Conversation
![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 introduces a 'Fillable Indicator' overlay for the Fill tool, enabling color previews when hovering over layers, and adds a StrokeColorSet message to the graph operation system. Key improvements include updated overlay rendering logic for complex stroke alignments and patterns. However, several issues need addressing: commented-out conditional compilation attributes break cross-platform support, incorrect debug formatting is used for CSS color strings, and redundant clones and duplicate functions were introduced. Additionally, leftover debug logs should be removed, and native web-sys bindings should be used instead of manual JavaScript reflection.
| // #[cfg(not(target_family = "wasm"))] | ||
| // pub mod utility_types_native; | ||
| // #[cfg(not(target_family = "wasm"))] | ||
| // pub use utility_types_native as utility_types; | ||
|
|
||
| // WebAssembly | ||
| #[cfg(target_family = "wasm")] | ||
| // #[cfg(target_family = "wasm")] | ||
| pub mod utility_types_web; | ||
| #[cfg(target_family = "wasm")] | ||
| // #[cfg(target_family = "wasm")] | ||
| pub use utility_types_web as utility_types; |
There was a problem hiding this comment.
The conditional compilation attributes (#[cfg(...)])) for utility_types have been commented out, forcing the use of utility_types_web even on non-WASM targets. This will cause compilation errors when building for native platforms because utility_types_web depends on web-sys and wasm-bindgen. These should be restored to maintain cross-platform support.
| let color_str = format!("#{:?}", color.to_rgba_hex_srgb()); | ||
| self.render_context.set_fill_style_str(&color_str.as_str()); |
There was a problem hiding this comment.
Using the {:?} debug formatter inside format! for a String will include surrounding quotes (e.g., #"ff0000ff"), which is not a valid CSS color string. Use the {} display formatter instead. Additionally, the .as_str() call is redundant as &String will deref to &str.
| let color_str = format!("#{:?}", color.to_rgba_hex_srgb()); | |
| self.render_context.set_fill_style_str(&color_str.as_str()); | |
| let color_str = format!("#{}", color.to_rgba_hex_srgb()); | |
| self.render_context.set_fill_style_str(&color_str); |
| let subpath = subpath.borrow().clone(); | ||
| let mut bezpath = subpath.to_bezpath(); |
There was a problem hiding this comment.
Cloning the Subpath here is unnecessary and potentially expensive as it involves duplicating the underlying vector of manipulators. Subpath::to_bezpath() can be called directly on the borrowed reference.
| let subpath = subpath.borrow().clone(); | |
| let mut bezpath = subpath.to_bezpath(); | |
| let mut bezpath = subpath.borrow().to_bezpath(); |
| let set_pattern_transform = |pattern: &CanvasPattern, matrix: &DomMatrix| { | ||
| // Get the JS function: pattern.setTransform | ||
| let func = Reflect::get(pattern, &JsValue::from_str("setTransform"))?; | ||
| // Pass it the matrix | ||
| Reflect::apply(&func.into(), pattern, &Array::of1(matrix))?; | ||
| Ok::<(), JsValue>(()) | ||
| }; | ||
| set_pattern_transform(&pattern, &dom_matrix); |
There was a problem hiding this comment.
Since web-sys version 0.3.77 is being used (as seen in Cargo.toml), the CanvasPattern struct already provides a set_transform method. Using Reflect to manually call the JS function is less efficient and harder to maintain than using the provided bindings.
pattern.set_transform(&dom_matrix);| pub fn vector_data_from_layer(&self, layer: LayerNodeIdentifier) -> Option<Vector> { | ||
| self.document_metadata.layer_vector_data.get(&layer).map(|arc| arc.as_ref().clone()) | ||
| } |
There was a problem hiding this comment.
The function vector_data_from_layer is an exact duplicate of the logic in the function immediately preceding it (lines 3085-3087). This redundancy should be removed.
| let wants_stroke_below = vector.style.stroke().map(|s| s.paint_order) == Some(PaintOrder::StrokeBelow); | ||
|
|
||
| if needs_separate_alignment_fill && !wants_stroke_below { | ||
| log::info!("Entering needs_separate_alignment_fill && !wants_stroke_below"); |
There was a problem hiding this comment.
This log::info! call appears to be a leftover from debugging and should be removed before merging.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 18 files
Confidence score: 3/5
- Fill-only overlays in
editor/src/messages/portfolio/document/overlays/utility_types_web.rscan callfill()without constructing a path, which risks rendering against an empty/stale canvas path and visible misbehavior. fill_stroke()ineditor/src/messages/portfolio/document/overlays/utility_types_native.rscan panic whencolor: None, a concrete runtime crash risk in native overlays.- Score reflects several user-impacting issues (rendering bugs and potential panic) alongside UI binding mistakes, so there is some merge risk despite most being localized.
- Pay close attention to
editor/src/messages/portfolio/document/overlays/utility_types_web.rs,editor/src/messages/portfolio/document/overlays/utility_types_native.rs,editor/src/messages/portfolio/document/document_message_handler.rs- rendering correctness, panic safety, and inverted checkbox binding.
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="editor/src/messages/portfolio/document/overlays/utility_types_native.rs">
<violation number="1" location="editor/src/messages/portfolio/document/overlays/utility_types_native.rs:1070">
P2: `fill_stroke()` panics on a stroke with `color: None` instead of handling the optional color safely.</violation>
</file>
<file name="editor/src/messages/portfolio/document/document_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/document_message_handler.rs:2323">
P2: Pivot and Origin overlay checkboxes are bound to the wrong visibility fields, so the checked state and toggle action are inverted.</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:431">
P3: Hardcodes the stroke color input index instead of using the stroke node's `ColorInput::INDEX` constant, making this helper brittle if the node schema changes.</violation>
</file>
<file name="editor/src/messages/tool/tool_messages/fill_tool.rs">
<violation number="1" location="editor/src/messages/tool/tool_messages/fill_tool.rs:145">
P3: Substantial stroke/fill targeting logic is duplicated in the overlay preview and commit paths, which increases maintenance burden and risks preview/action divergence.</violation>
</file>
<file name="editor/src/messages/portfolio/document/overlays/utility_types_web.rs">
<violation number="1" location="editor/src/messages/portfolio/document/overlays/utility_types_web.rs:18">
P2: Custom agent: **PR title enforcement**
PR title uses lowercase "fill tool" but tool names must be capitalized as proper nouns per the style guide (e.g., "Select tool", "Path tool"). It should be "Fill tool".
Suggested title: **Add Fill tool support on strokes**</violation>
<violation number="2" location="editor/src/messages/portfolio/document/overlays/utility_types_web.rs:956">
P1: fill-only overlays can call `fill()` without constructing the path, so they may render against an empty/stale canvas path.</violation>
<violation number="3" location="editor/src/messages/portfolio/document/overlays/utility_types_web.rs:1039">
P2: Uses Debug formatting for a CSS hex color, which adds quotes/escaping and makes the fill style invalid.</violation>
</file>
<file name="node-graph/libraries/rendering/src/renderer.rs">
<violation number="1" location="node-graph/libraries/rendering/src/renderer.rs:765">
P2: This `log::info!` call appears to be a leftover from debugging. Remove it before merging to avoid noisy log output in production.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| if subpath.closed() { | ||
| self.render_context.close_path(); | ||
| } else { |
There was a problem hiding this comment.
P1: fill-only overlays can call fill() without constructing the path, so they may render against an empty/stale canvas 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/overlays/utility_types_web.rs, line 956:
<comment>fill-only overlays can call `fill()` without constructing the path, so they may render against an empty/stale canvas path.</comment>
<file context>
@@ -925,14 +953,17 @@ impl OverlayContext {
if subpath.closed() {
self.render_context.close_path();
+ } else {
+ is_closed = false;
}
</file context>
|
|
||
| pub fn fill_stroke(&mut self, subpaths: impl Iterator<Item = impl Borrow<Subpath<PointId>>>, transform: DAffine2, overlay_stroke: &Stroke) { | ||
| let path = self.path_from_subpaths(subpaths, transform); | ||
| let brush = peniko::Brush::Image(self.fill_canvas_pattern_image(&overlay_stroke.color.expect("Color should be set for fill_stroke()"))); |
There was a problem hiding this comment.
P2: fill_stroke() panics on a stroke with color: None instead of handling the optional color safely.
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/overlays/utility_types_native.rs, line 1070:
<comment>`fill_stroke()` panics on a stroke with `color: None` instead of handling the optional color safely.</comment>
<file context>
@@ -1054,12 +1045,33 @@ impl OverlayContextInternal {
+ pub fn fill_stroke(&mut self, subpaths: impl Iterator<Item = impl Borrow<Subpath<PointId>>>, transform: DAffine2, overlay_stroke: &Stroke) {
+ let path = self.path_from_subpaths(subpaths, transform);
+ let brush = peniko::Brush::Image(self.fill_canvas_pattern_image(&overlay_stroke.color.expect("Color should be set for fill_stroke()")));
+
+ self.scene.stroke(&kurbo::Stroke::new(overlay_stroke.weight), self.get_transform(), &brush, None, &path);
</file context>
| let brush = peniko::Brush::Image(self.fill_canvas_pattern_image(&overlay_stroke.color.expect("Color should be set for fill_stroke()"))); | |
| let Some(color) = overlay_stroke.color else { | |
| return; | |
| }; | |
| let brush = peniko::Brush::Image(self.fill_canvas_pattern_image(&color)); |
| @@ -1063,6 +1063,7 @@ impl MessageHandler<DocumentMessage, DocumentMessageContext<'_>> for DocumentMes | |||
| responses.add(PortfolioMessage::UpdateDocumentWidgets); | |||
There was a problem hiding this comment.
P2: Pivot and Origin overlay checkboxes are bound to the wrong visibility fields, so the checked state and toggle action are inverted.
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/document_message_handler.rs, line 2323:
<comment>Pivot and Origin overlay checkboxes are bound to the wrong visibility fields, so the checked state and toggle action are inverted.</comment>
<file context>
@@ -2319,7 +2320,7 @@ impl DocumentMessageHandler {
let checkbox_id = CheckboxId::new();
vec![
- CheckboxInput::new(self.overlays_visibility_settings.pivot)
+ CheckboxInput::new(self.overlays_visibility_settings.origin)
.on_update(|optional_input: &CheckboxInput| {
DocumentMessage::SetOverlaysVisibility {
</file context>
| self.draw_path_from_subpaths(subpaths, transform); | ||
|
|
||
| self.render_context.set_fill_style_canvas_pattern(&pattern); | ||
| let color_str = format!("#{:?}", color.to_rgba_hex_srgb()); |
There was a problem hiding this comment.
P2: Uses Debug formatting for a CSS hex color, which adds quotes/escaping and makes the fill style invalid.
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/overlays/utility_types_web.rs, line 1039:
<comment>Uses Debug formatting for a CSS hex color, which adds quotes/escaping and makes the fill style invalid.</comment>
<file context>
@@ -992,14 +1016,193 @@ impl OverlayContext {
+ self.draw_path_from_subpaths(subpaths, transform);
- self.render_context.set_fill_style_canvas_pattern(&pattern);
+ let color_str = format!("#{:?}", color.to_rgba_hex_srgb());
+ self.render_context.set_fill_style_str(&color_str.as_str());
self.render_context.fill();
</file context>
| let color_str = format!("#{:?}", color.to_rgba_hex_srgb()); | |
| let color_str = format!("#{}", color.to_rgba_hex_srgb()); |
| let wants_stroke_below = vector.style.stroke().map(|s| s.paint_order) == Some(PaintOrder::StrokeBelow); | ||
|
|
||
| if needs_separate_alignment_fill && !wants_stroke_below { | ||
| log::info!("Entering needs_separate_alignment_fill && !wants_stroke_below"); |
There was a problem hiding this comment.
P2: This log::info! call appears to be a leftover from debugging. Remove it before merging to avoid noisy log output in production.
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 765:
<comment>This `log::info!` call appears to be a leftover from debugging. Remove it before merging to avoid noisy log output in production.</comment>
<file context>
@@ -761,6 +762,7 @@ impl Render for Table<Vector> {
let wants_stroke_below = vector.style.stroke().map(|s| s.paint_order) == Some(PaintOrder::StrokeBelow);
if needs_separate_alignment_fill && !wants_stroke_below {
+ log::info!("Entering needs_separate_alignment_fill && !wants_stroke_below");
render.leaf_tag("path", |attributes| {
attributes.push("d", path.clone());
</file context>
| @@ -15,11 +15,13 @@ use graphene_std::math::quad::Quad; | |||
| use graphene_std::subpath::Subpath; | |||
There was a problem hiding this comment.
P2: Custom agent: PR title enforcement
PR title uses lowercase "fill tool" but tool names must be capitalized as proper nouns per the style guide (e.g., "Select tool", "Path tool"). It should be "Fill tool".
Suggested title: Add Fill tool support on strokes
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/overlays/utility_types_web.rs, line 18:
<comment>PR title uses lowercase "fill tool" but tool names must be capitalized as proper nouns per the style guide (e.g., "Select tool", "Path tool"). It should be "Fill tool".
Suggested title: **Add Fill tool support on strokes**</comment>
<file context>
@@ -15,11 +15,13 @@ use graphene_std::math::quad::Quad;
use graphene_std::subpath::Subpath;
use graphene_std::vector::click_target::ClickTargetType;
use graphene_std::vector::misc::{dvec2_to_point, point_to_dvec2};
+use graphene_std::vector::style::{PaintOrder, Stroke, StrokeAlign};
use graphene_std::vector::{PointId, SegmentId, Vector};
+use js_sys::{Array, Reflect};
</file context>
| }; | ||
|
|
||
| let stroke_color = if let Some(color) = color { Table::new_from_element(color) } else { Table::new() }; | ||
| let input_connector = InputConnector::node(stroke_node_id, 1); |
There was a problem hiding this comment.
P3: Hardcodes the stroke color input index instead of using the stroke node's ColorInput::INDEX constant, making this helper brittle if the node schema changes.
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 431:
<comment>Hardcodes the stroke color input index instead of using the stroke node's `ColorInput::INDEX` constant, making this helper brittle if the node schema changes.</comment>
<file context>
@@ -423,6 +422,16 @@ impl<'a> ModifyInputsContext<'a> {
+ };
+
+ let stroke_color = if let Some(color) = color { Table::new_from_element(color) } else { Table::new() };
+ let input_connector = InputConnector::node(stroke_node_id, 1);
+ self.set_input_with_refresh(input_connector, NodeInput::value(TaggedValue::Color(stroke_color), false), false);
+ }
</file context>
| let input_connector = InputConnector::node(stroke_node_id, 1); | |
| let input_connector = InputConnector::node(stroke_node_id, graphene_std::vector::stroke::ColorInput::INDEX); |
| @@ -1,7 +1,11 @@ | |||
| use super::tool_prelude::*; | |||
There was a problem hiding this comment.
P3: Substantial stroke/fill targeting logic is duplicated in the overlay preview and commit paths, which increases maintenance burden and risks preview/action divergence.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/tool_messages/fill_tool.rs, line 145:
<comment>Substantial stroke/fill targeting logic is duplicated in the overlay preview and commit paths, which increases maintenance burden and risks preview/action divergence.</comment>
<file context>
@@ -108,9 +132,36 @@ impl Fsm for FillToolFsmState {
+ let graph_layer = graph_modification_utils::NodeGraphLayer::new(layer, &document.network_interface);
+
+ // Stroke
+ let stroke_node = graph_layer.upstream_node_id_from_name(&STROKE_ID);
+ let stroke_exists_and_visible = stroke_node.is_some_and(|stroke| document.network_interface.is_visible(&stroke, &[]));
+ let stroke = vector_data.style.stroke();
</file context>
1 participant
This PR adds support for stroke overlays for the fill tool as requested in #2615 and supercedes #2664.