Skip to content

Render Fill via Graphic#4111

Draft
YohYamasaki wants to merge 1 commit intoGraphiteEditor:masterfrom
YohYamasaki:render-fill-via-graphic
Draft

Render Fill via Graphic#4111
YohYamasaki wants to merge 1 commit intoGraphiteEditor:masterfrom
YohYamasaki:render-fill-via-graphic

Conversation

@YohYamasaki
Copy link
Copy Markdown
Contributor

WIP! This PR adds foundation of the renderer to handle Graphic enum as a paint source.

  • Add converter from Fill to Table<Graphic> to do not break current node flow related to Fill
  • Refactor renderer to use Table<Graphic> as a fill paint source, start from Table<Color> and Table<GradientStops>
  • Add implementation of clip-based rendering for other Graphic types

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 functionality to convert Fill styles into graphical tables, including a new to_transform method for the Gradient struct and associated unit tests. Feedback suggests refactoring the private fill_to_paint function into an implementation of the IntoGraphicTable trait for Fill to align with existing architectural patterns and optimize performance by avoiding unnecessary clones.

Comment on lines +197 to +211
fn fill_to_paint(fill: Fill) -> Option<Table<Graphic>> {
match fill {
Fill::None => None,
Fill::Solid(color) => Some(Table::new_from_element(color.into())),
Fill::Gradient(gradient) => {
let gradient_row = TableRow::new_from_element(gradient.stops.clone())
.with_attribute(ATTR_TRANSFORM, gradient.to_transform())
.with_attribute(ATTR_GRADIENT_TYPE, gradient.gradient_type)
.with_attribute(ATTR_SPREAD_METHOD, gradient.spread_method);
let gradient_table = Table::new_from_row(gradient_row);

Some(Table::new_from_element(Graphic::Gradient(gradient_table)))
}
}
}
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.

medium

Instead of a standalone private function, consider implementing the IntoGraphicTable trait for Fill. This aligns with the existing patterns in this file for converting various types into a Table<Graphic> and makes the functionality public and accessible for the renderer refactor mentioned in the PR description. Additionally, since the function takes fill by value, you can avoid cloning the gradient stops by reordering the calls. Note that the attributes field in Table and TableRow is intentionally not serialized, so these attributes will only be available at runtime.

Suggested change
fn fill_to_paint(fill: Fill) -> Option<Table<Graphic>> {
match fill {
Fill::None => None,
Fill::Solid(color) => Some(Table::new_from_element(color.into())),
Fill::Gradient(gradient) => {
let gradient_row = TableRow::new_from_element(gradient.stops.clone())
.with_attribute(ATTR_TRANSFORM, gradient.to_transform())
.with_attribute(ATTR_GRADIENT_TYPE, gradient.gradient_type)
.with_attribute(ATTR_SPREAD_METHOD, gradient.spread_method);
let gradient_table = Table::new_from_row(gradient_row);
Some(Table::new_from_element(Graphic::Gradient(gradient_table)))
}
}
}
impl IntoGraphicTable for Fill {
fn into_graphic_table(self) -> Table<Graphic> {
match self {
Fill::None => Table::new(),
Fill::Solid(color) => Table::new_from_element(color.into()),
Fill::Gradient(gradient) => {
let transform = gradient.to_transform();
let gradient_row = TableRow::new_from_element(gradient.stops)
.with_attribute(ATTR_TRANSFORM, transform)
.with_attribute(ATTR_GRADIENT_TYPE, gradient.gradient_type)
.with_attribute(ATTR_SPREAD_METHOD, gradient.spread_method);
let gradient_table = Table::new_from_row(gradient_row);
Table::new_from_element(Graphic::Gradient(gradient_table))
}
}
}
}
References
  1. The attributes field in Table and TableRow is intentionally not serialized because it contains type-erased data, and serialization for this is not currently implemented.

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