Skip to content

Refactor: Unified Paint Abstraction & Stroke Gradient Support [FEATURE]#3975

Open
Chetansahney wants to merge 3 commits intoGraphiteEditor:masterfrom
Chetansahney:refactor/unified-paint-stroke-gradients
Open

Refactor: Unified Paint Abstraction & Stroke Gradient Support [FEATURE]#3975
Chetansahney wants to merge 3 commits intoGraphiteEditor:masterfrom
Chetansahney:refactor/unified-paint-stroke-gradients

Conversation

@Chetansahney
Copy link
Copy Markdown

This PR initiates the architectural refactor for the "Generalized Graphical Data Rendering" project. It replaces the legacy, color-only stroke model with an extensible system capable of rendering gradients. This serves as the foundational "plumbing" to eventually support patterns, image fills, and canvas-spanning effects across all vector attributes.

Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for gradient strokes by adding a gradient field to the Stroke struct and updating the rendering logic for both SVG and Vello-based outputs. The review feedback identifies several improvement opportunities, including refactoring duplicated gradient brush creation into a shared helper function, implementing smoother transitions for gradient interpolation in the lerp method, and standardizing the with_color API return type for consistency. It is also suggested to use unreachable! in branches that are logically covered by prior checks to improve code clarity and error detection.

I am having trouble creating individual review comments. Click here to see my feedback.

node-graph/libraries/rendering/src/render_ext.rs (125)

medium

This branch seems unreachable. The if !self.has_renderable_stroke() check on line 110 should handle the case where both gradient and color are None, causing an early return. Using unreachable!() here would make this assumption explicit and catch potential logic errors in the future if has_renderable_stroke changes.

			_ => unreachable!("Stroke should have a color or gradient to be rendered, as checked by has_renderable_stroke"),

node-graph/libraries/rendering/src/renderer.rs (1135-1193)

medium

The logic for creating a gradient brush here is very similar to the logic for fill gradients in the do_fill_path closure (lines 1037-1088). To improve maintainability and reduce code duplication, consider extracting this gradient-to-brush conversion logic into a shared helper function. This function could take a &Gradient and the necessary transforms, and return a (peniko::Brush, Option<kurbo::Affine>) tuple.

node-graph/libraries/vector-types/src/vector/style.rs (364-369)

medium

The interpolation logic for gradients here uses a hard switch at time = 0.5 when one of the strokes has a gradient and the other doesn't. This will result in an abrupt visual change rather than a smooth transition.

For a better user experience, consider implementing a smoother interpolation, for example by fading the gradient's alpha to transparent. You could look at Fill::lerp for inspiration on how to handle transitions between different paint types (e.g., solid color to gradient).

node-graph/libraries/vector-types/src/vector/style.rs (433-440)

medium

The function with_color returns an Option<Self> but always returns Some(self). For consistency with with_gradient and with_weight which return Self, and to simplify the API, consider changing the return type to Self.

	pub fn with_color(mut self, color: &Option<Color>) -> Self {
		self.color = *color;
		if color.is_some() {
			self.gradient = None;
		}

		self
	}

@Chetansahney Chetansahney marked this pull request as ready for review April 12, 2026 12:59
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

3 issues found across 5 files

Confidence score: 3/5

  • There is a concrete rendering-risk issue in node-graph/libraries/vector-types/src/vector/style.rs: has_renderable_stroke() currently ORs gradient/color alpha checks, which can incorrectly mark a transparent gradient stroke as renderable due to ignored color state.
  • The new stroke-gradient path in node-graph/libraries/rendering/src/renderer.rs duplicates complex fill-gradient transform/construction logic; this is not an immediate break but raises medium regression risk from future fill/stroke divergence.
  • The PR title policy violation in node-graph/libraries/rendering/src/renderer.rs appears process-related rather than runtime-impacting, so it lowers confidence only slightly compared with the rendering behavior concerns.
  • Pay close attention to node-graph/libraries/vector-types/src/vector/style.rs and node-graph/libraries/rendering/src/renderer.rs - stroke renderability precedence and duplicated gradient logic could cause visible rendering inconsistencies.
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/rendering/src/renderer.rs">

<violation number="1" location="node-graph/libraries/rendering/src/renderer.rs:1197">
P1: Custom agent: **PR title enforcement**

PR title format violates the title policy: it uses a colon-prefixed `Refactor:` tag and title case wording instead of a sentence-case imperative title.</violation>

<violation number="2" location="node-graph/libraries/rendering/src/renderer.rs:1221">
P2: New stroke-gradient code duplicates complex fill-gradient transform/construction logic, increasing risk of fill/stroke rendering divergence.</violation>
</file>

<file name="node-graph/libraries/vector-types/src/vector/style.rs">

<violation number="1" location="node-graph/libraries/vector-types/src/vector/style.rs:523">
P2: `has_renderable_stroke()` violates gradient-over-color precedence by OR-ing both alpha checks, so an ignored color can make a transparent gradient stroke appear renderable.</violation>
</file>

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

@@ -1194,34 +1194,90 @@ impl Render for Table<Vector> {
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1: Custom agent: PR title enforcement

PR title format violates the title policy: it uses a colon-prefixed Refactor: tag and title case wording instead of a sentence-case imperative title.

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 1197:

<comment>PR title format violates the title policy: it uses a colon-prefixed `Refactor:` tag and title case wording instead of a sentence-case imperative title.</comment>

<file context>
@@ -1194,34 +1194,90 @@ impl Render for Table<Vector> {
-						None => peniko::Color::TRANSPARENT,
-					};
-					let cap = match stroke.cap {
+				if let Some(stroke_style) = row.element.style.stroke() {
+					let cap = match stroke_style.cap {
 						StrokeCap::Butt => Cap::Butt,
</file context>

let has_color_alpha = self.color.is_some_and(|color| color.a() != 0.);
let has_gradient_alpha = self.gradient.as_ref().is_some_and(|gradient| gradient.stops.color.iter().any(|color| color.a() != 0.));

has_color_alpha || has_gradient_alpha
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: has_renderable_stroke() violates gradient-over-color precedence by OR-ing both alpha checks, so an ignored color can make a transparent gradient stroke appear renderable.

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/style.rs, line 523:

<comment>`has_renderable_stroke()` violates gradient-over-color precedence by OR-ing both alpha checks, so an ignored color can make a transparent gradient stroke appear renderable.</comment>

<file context>
@@ -488,7 +513,14 @@ impl Stroke {
+		let has_color_alpha = self.color.is_some_and(|color| color.a() != 0.);
+		let has_gradient_alpha = self.gradient.as_ref().is_some_and(|gradient| gradient.stops.color.iter().any(|color| color.a() != 0.));
+
+		has_color_alpha || has_gradient_alpha
 	}
 }
</file context>
Suggested change
has_color_alpha || has_gradient_alpha
if self.gradient.is_some() { has_gradient_alpha } else { has_color_alpha }

scene.stroke(&stroke, kurbo::Affine::new(element_transform.to_cols_array()), color, None, &path);
if kurbo_stroke.width > 0. {
let (brush, brush_transform) = if let Some(gradient) = stroke_style.gradient.as_ref() {
let mut stops = peniko::ColorStops::new();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: New stroke-gradient code duplicates complex fill-gradient transform/construction logic, increasing risk of fill/stroke rendering divergence.

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 1221:

<comment>New stroke-gradient code duplicates complex fill-gradient transform/construction logic, increasing risk of fill/stroke rendering divergence.</comment>

<file context>
@@ -1194,34 +1194,90 @@ impl Render for Table<Vector> {
-						scene.stroke(&stroke, kurbo::Affine::new(element_transform.to_cols_array()), color, None, &path);
+					if kurbo_stroke.width > 0. {
+						let (brush, brush_transform) = if let Some(gradient) = stroke_style.gradient.as_ref() {
+							let mut stops = peniko::ColorStops::new();
+							for (position, color, _) in gradient.stops.interpolated_samples() {
+								stops.push(peniko::ColorStop {
</file context>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant