Render Fill via Graphic#4111
Conversation
There was a problem hiding this comment.
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.
| 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))) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| 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
- 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.
WIP! This PR adds foundation of the renderer to handle
Graphicenum as a paint source.FilltoTable<Graphic>to do not break current node flow related toFillTable<Graphic>as a fill paint source, start fromTable<Color>andTable<GradientStops>Graphictypes