Add additional boolean operations#4122
Add additional boolean operations#4122devviktoria wants to merge 1 commit intoGraphiteEditor:masterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces 'Trim' and 'Crop' boolean operations, including UI menu entries and a new cascading subtraction logic for multi-layer vector paths. Feedback focuses on optimizing the implementation by removing redundant transformation logic, preventing the creation of empty layers when operations yield no geometry, and improving performance in the 'Crop' operation by reusing the existing topology instead of re-calculating it.
| for i in 0..result_vector_table.len() { | ||
| let transform: DAffine2 = result_vector_table.attribute_cloned_or_default(ATTR_TRANSFORM, i); | ||
| result_vector_table.set_attribute(ATTR_TRANSFORM, i, DAffine2::IDENTITY); | ||
|
|
||
| let result_vector = result_vector_table.element_mut(0).unwrap(); | ||
| let result_vector = result_vector_table.element_mut(i).unwrap(); | ||
| Vector::transform(result_vector, transform); | ||
| result_vector.style.set_stroke_transform(DAffine2::IDENTITY); | ||
|
|
||
| // Snapshot the input layers as the `editor:merged_layers` attribute so the renderer can recurse into them | ||
| // for editor click-target preservation. | ||
| result_vector_table.set_attribute(ATTR_EDITOR_MERGED_LAYERS, 0, content.clone()); | ||
| result_vector_table.set_attribute(ATTR_EDITOR_MERGED_LAYERS, i, content.clone()); | ||
|
|
||
| // Clean up the boolean operation result by merging duplicated points | ||
| let merge_transform: DAffine2 = result_vector_table.attribute_cloned_or_default(ATTR_TRANSFORM, 0); | ||
| result_vector_table.element_mut(0).unwrap().merge_by_distance_spatial(merge_transform, 0.0001); | ||
| let merge_transform: DAffine2 = result_vector_table.attribute_cloned_or_default(ATTR_TRANSFORM, i); | ||
| result_vector_table.element_mut(i).unwrap().merge_by_distance_spatial(merge_transform, 0.0001); | ||
| } |
There was a problem hiding this comment.
The transformation logic in this loop is redundant because the helper functions (boolean_operation_on_vector_table and cascading_subtract) already set the ATTR_TRANSFORM attribute to DAffine2::IDENTITY and bake the original transforms into the geometry. Consequently, transform will always be identity here, making Vector::transform a no-op.
Additionally, fetching merge_transform from the table after it has been set to identity is unnecessary; you can simply pass DAffine2::IDENTITY to merge_by_distance_spatial.
for i in 0..result_vector_table.len() {
// Snapshot the input layers as the editor:merged_layers attribute so the renderer can recurse into them
// for editor click-target preservation.
result_vector_table.set_attribute(ATTR_EDITOR_MERGED_LAYERS, i, content.clone());
let result_vector = result_vector_table.element_mut(i).unwrap();
result_vector.style.set_stroke_transform(DAffine2::IDENTITY);
// Clean up the boolean operation result by merging duplicated points
result_vector.merge_by_distance_spatial(DAffine2::IDENTITY, 0.0001);
}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.
- When a Vello render is requested, the GPU executor is guaranteed to be available, making the use of unwrap() acceptable in this context.
| let contours = top.contours(|winding| winding.is_inside(boolean_operation)); | ||
|
|
||
| append_linesweeper_contours(row.element_mut(), &contours); | ||
|
|
||
| table.push(row); |
There was a problem hiding this comment.
This code pushes a row to the result table even if the boolean operation produced no geometry (empty contours). It is better to skip adding the row if the result is empty to avoid polluting the document with invisible layers.
| let contours = top.contours(|winding| winding.is_inside(boolean_operation)); | |
| append_linesweeper_contours(row.element_mut(), &contours); | |
| table.push(row); | |
| let contours = top.contours(|winding| winding.is_inside(boolean_operation)); | |
| if contours.contours().next().is_some() { | |
| append_linesweeper_contours(row.element_mut(), &contours); | |
| table.push(row); | |
| } |
| for i in 0..end_index { | ||
| let contours = top.contours(|w| predicate(w, i)); | ||
|
|
||
| let source = match vector.element(i) { | ||
| Some(source) => source, | ||
| None => continue, | ||
| }; | ||
|
|
||
| let mut attributes = vector.clone_row_attributes(i); | ||
| attributes.insert(ATTR_TRANSFORM, DAffine2::IDENTITY); | ||
|
|
||
| let mut element = Vector { | ||
| style: source.style.clone(), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| append_linesweeper_contours(&mut element, &contours); | ||
|
|
||
| let row = TableRow::from_parts(element, attributes); | ||
| table.push(row); | ||
| } |
There was a problem hiding this comment.
Similar to the general boolean operation, cascading_subtract should avoid adding empty layers to the result table if a layer is completely removed by the operation.
for i in 0..end_index {
let contours = top.contours(|w| predicate(w, i));
if contours.contours().next().is_none() {
continue;
}
let source = match vector.element(i) {
Some(source) => source,
None => continue,
};
let mut attributes = vector.clone_row_attributes(i);
attributes.insert(ATTR_TRANSFORM, DAffine2::IDENTITY);
let mut element = Vector {
style: source.style.clone(),
..Default::default()
};
append_linesweeper_contours(&mut element, &contours);
let row = TableRow::from_parts(element, attributes);
table.push(row);
}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.
| if boolean_operation == BooleanOperation::Crop { | ||
| let top_remainder = boolean_operation_on_vector_table(vector, BooleanOperation::SubtractBack); | ||
| if let Some(mut row) = top_remainder.clone_row(0) { | ||
| let result_vector = row.element_mut(); | ||
| result_vector.style.clear_fill(); | ||
| result_vector.style.clear_stroke(); | ||
| table.push(row); | ||
| } | ||
| } |
There was a problem hiding this comment.
Calling boolean_operation_on_vector_table here is inefficient because it re-calculates the entire topology and re-runs the sweep-line algorithm. Since the topology top is already available in this function, you can extract the SubtractBack contours directly.
| if boolean_operation == BooleanOperation::Crop { | |
| let top_remainder = boolean_operation_on_vector_table(vector, BooleanOperation::SubtractBack); | |
| if let Some(mut row) = top_remainder.clone_row(0) { | |
| let result_vector = row.element_mut(); | |
| result_vector.style.clear_fill(); | |
| result_vector.style.clear_stroke(); | |
| table.push(row); | |
| } | |
| } | |
| if boolean_operation == BooleanOperation::Crop { | |
| let contours = top.contours(|w| w.is_inside(BooleanOperation::SubtractBack)); | |
| if contours.contours().next().is_some() { | |
| let index = vector.len().saturating_sub(1); | |
| let mut attributes = vector.clone_row_attributes(index); | |
| attributes.insert(ATTR_TRANSFORM, DAffine2::IDENTITY); | |
| let source = vector.element(index).unwrap(); | |
| let mut element = Vector { | |
| style: source.style.clone(), | |
| ..Default::default() | |
| }; | |
| element.style.clear_fill(); | |
| element.style.clear_stroke(); | |
| append_linesweeper_contours(&mut element, &contours); | |
| table.push(TableRow::from_parts(element, attributes)); | |
| } | |
| } |
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.
- When a Vello render is requested, the GPU executor is guaranteed to be available, making the use of unwrap() acceptable in this context.
|
So far I had tested only the basic functionality so I would like to test it with edge cases to make sure everything is working fine. And also I will check Geminis code review and creating tests for the Crop and Trim. |
This pull requests adds the Crop and Trim boolean operations to Grahpite.
The Trim operation is best described in the following video:
Adobe Illustrator Pathfinder Panel Explained! Understand Your Shape-Merging Options
https://youtu.be/Me5oOw8r0Is?t=370
The Crop operation is best described in this part of the video:
Adobe Illustrator Pathfinder Panel Explained! Understand Your Shape-Merging Options
https://youtu.be/Me5oOw8r0Is?t=428
Part of #4103