Skip to content

Add additional boolean operations#4122

Draft
devviktoria wants to merge 1 commit intoGraphiteEditor:masterfrom
devviktoria:additional_booleans
Draft

Add additional boolean operations#4122
devviktoria wants to merge 1 commit intoGraphiteEditor:masterfrom
devviktoria:additional_booleans

Conversation

@devviktoria
Copy link
Copy Markdown

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

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 '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.

Comment on lines +45 to 60
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);
}
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

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
  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.
  2. When a Vello render is requested, the GPU executor is guaranteed to be available, making the use of unwrap() acceptable in this context.

Comment on lines +170 to +174
let contours = top.contours(|winding| winding.is_inside(boolean_operation));

append_linesweeper_contours(row.element_mut(), &contours);

table.push(row);
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

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.

Suggested change
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);
}

Comment on lines +196 to +216
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);
}
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

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
  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.

Comment on lines +218 to +226
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);
}
}
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

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.

Suggested change
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
  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.
  2. When a Vello render is requested, the GPU executor is guaranteed to be available, making the use of unwrap() acceptable in this context.

@devviktoria
Copy link
Copy Markdown
Author

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.

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