Skip to content

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

Draft
Chetansahney wants to merge 2 commits intoGraphiteEditor:masterfrom
Chetansahney:refactor/unified-paint-stroke-gradients
Draft

Refactor: Unified Paint Abstraction & Stroke Gradient Support [FEATURE]#3975
Chetansahney wants to merge 2 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
	}

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