Code review

Author: Keavon

editor/src/messages/tool/common_functionality/shape_editor.rs

@@ -1,5 +1,3 @@
-use std::f64::consts::PI;
-
 use super::graph_modification_utils::merge_layers;
 use super::snapping::{SnapCache, SnapCandidatePoint, SnapData, SnapManager, SnappedPoint};
 use super::utility_functions::{adjust_handle_colinearity, calculate_bezier_bbox, calculate_segment_angle, restore_g1_continuity, restore_previous_handle_position};
@@ -17,6 +15,7 @@ use bezier_rs::{Bezier, BezierHandles, Subpath, TValue};
 use glam::{DAffine2, DVec2};
 use graphene_std::vector::{HandleExt, HandleId, SegmentId};
 use graphene_std::vector::{ManipulatorPointId, PointId, VectorData, VectorModificationType};
+use std::f64::consts::TAU;
 
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
 pub enum SelectionChange {
@@ -1034,8 +1033,8 @@ impl ShapeState {
 	/// If both or neither handles are selected, the angle of both handles will be averaged from their current angles, weighted by their lengths.
 	/// Assumes all selected manipulators have handles that are already not colinear.
 	///
-	/// For vector meshes, non colinear handle which is nearest in the direction of 180° angle separation, becomes colinear with current handle,
-	/// if there is no such handle, nothing happens
+	/// For vector meshes, the non-colinear handle which is nearest in the direction of 180° angle separation becomes colinear with current handle.
+	/// If there is no such handle, nothing happens.
 	pub fn convert_selected_manipulators_to_colinear_handles(&self, responses: &mut VecDeque<Message>, document: &DocumentMessageHandler) {
 		let mut skip_set = HashSet::new();
 
@@ -1055,43 +1054,42 @@ impl ShapeState {
 
 				// Here we take handles as the current handle and the most opposite non-colinear-handle
 
-				let is_handle_non_colinear = |handle: HandleId| -> bool { !(vector_data.colinear_manipulators.iter().any(|&handles| handles[0] == handle || handles[1] == handle)) };
+				let is_handle_colinear = |handle: HandleId| -> bool { vector_data.colinear_manipulators.iter().any(|&handles| handles[0] == handle || handles[1] == handle) };
 
-				let other_handles = match point {
-					ManipulatorPointId::Anchor(_) => point.get_handle_pair(&vector_data),
-					_ => {
-						point.get_all_connected_handles(&vector_data).and_then(|handles| {
-							let mut non_colinear_handles = handles.iter().filter(|&handle| is_handle_non_colinear(*handle)).clone().collect::<Vec<_>>();
+				let other_handles = if matches!(point, ManipulatorPointId::Anchor(_)) {
+					point.get_handle_pair(&vector_data)
+				} else {
+					point.get_all_connected_handles(&vector_data).and_then(|handles| {
+						let mut non_colinear_handles = handles.iter().filter(|&handle| !is_handle_colinear(*handle)).clone().collect::<Vec<_>>();
 
-							// Sort these by angle from the current handle
-							non_colinear_handles.sort_by(|&handle_a, &handle_b| {
-								let anchor = point.get_anchor_position(&vector_data).expect("No anchor position for handle");
-								let orig_handle_pos = point.get_position(&vector_data).expect("No handle position");
+						// Sort these by angle from the current handle
+						non_colinear_handles.sort_by(|&handle_a, &handle_b| {
+							let anchor = point.get_anchor_position(&vector_data).expect("No anchor position for handle");
+							let orig_handle_pos = point.get_position(&vector_data).expect("No handle position");
 
-								let a_pos = handle_a.to_manipulator_point().get_position(&vector_data).expect("No handle position");
-								let b_pos = handle_b.to_manipulator_point().get_position(&vector_data).expect("No handle position");
+							let a_pos = handle_a.to_manipulator_point().get_position(&vector_data).expect("No handle position");
+							let b_pos = handle_b.to_manipulator_point().get_position(&vector_data).expect("No handle position");
 
-								let v_orig = (orig_handle_pos - anchor).normalize_or_zero();
+							let v_orig = (orig_handle_pos - anchor).normalize_or_zero();
 
-								let v_a = (a_pos - anchor).normalize_or_zero();
-								let v_b = (b_pos - anchor).normalize_or_zero();
+							let v_a = (a_pos - anchor).normalize_or_zero();
+							let v_b = (b_pos - anchor).normalize_or_zero();
 
-								let angle_a = v_orig.angle_to(v_a).abs();
-								let angle_b = v_orig.angle_to(v_b).abs();
+							let angle_a = v_orig.angle_to(v_a).abs();
+							let angle_b = v_orig.angle_to(v_b).abs();
 
-								// Sort by descending angle (180° is farthest)
-								angle_b.partial_cmp(&angle_a).unwrap_or(std::cmp::Ordering::Equal)
-							});
+							// Sort by descending angle (180° is furthest)
+							angle_b.partial_cmp(&angle_a).unwrap_or(std::cmp::Ordering::Equal)
+						});
 
-							let current = match point {
-								ManipulatorPointId::EndHandle(segment) => HandleId::end(segment),
-								ManipulatorPointId::PrimaryHandle(segment) => HandleId::primary(segment),
-								ManipulatorPointId::Anchor(_) => unreachable!(),
-							};
+						let current = match point {
+							ManipulatorPointId::EndHandle(segment) => HandleId::end(segment),
+							ManipulatorPointId::PrimaryHandle(segment) => HandleId::primary(segment),
+							ManipulatorPointId::Anchor(_) => unreachable!(),
+						};
 
-							if let Some(other) = non_colinear_handles.iter().next() { Some([current, **other]) } else { None }
-						})
-					}
+						non_colinear_handles.first().map(|other| [current, **other])
+					})
 				};
 
 				let Some(handles) = other_handles else { continue };
@@ -1554,14 +1552,12 @@ impl ShapeState {
 	/// Disable colinear handles colinear.
 	pub fn disable_colinear_handles_state_on_selected(&self, network_interface: &NodeNetworkInterface, responses: &mut VecDeque<Message>) {
 		for (&layer, state) in &self.selected_shape_state {
-			let Some(vector_data) = network_interface.compute_modified_vector(layer) else {
-				continue;
-			};
+			let Some(vector_data) = network_interface.compute_modified_vector(layer) else { continue };
 
 			for &point in &state.selected_points {
 				if let ManipulatorPointId::Anchor(point) = point {
 					for connected in vector_data.all_connected(point) {
-						if let Some(&handles) = vector_data.colinear_manipulators.iter().find(|target| target.iter().any(|&target| target == connected)) {
+						if let Some(&handles) = vector_data.colinear_manipulators.iter().find(|target| target.contains(&connected)) {
 							let modification_type = VectorModificationType::SetG1Continuous { handles, enabled: false };
 							responses.add(GraphOperationMessage::Vector { layer, modification_type });
 						}
@@ -1800,7 +1796,7 @@ impl ShapeState {
 							let angle = base.angle_to(to);
 							let cross = base.perp_dot(to);
 
-							if cross < 0.0 { 2.0 * PI - angle } else { angle }
+							if cross < 0. { TAU - angle } else { angle }
 						};
 
 						let angle_a = signed_angle(v_orig, v_a);

editor/src/messages/tool/tool_messages/path_tool.rs

@@ -585,7 +585,7 @@ impl PathToolData {
 				let vector_data = document.network_interface.compute_modified_vector(single_selected_point.layer).unwrap();
 				if single_selected_point.id.get_handle_pair(&vector_data).is_some() {
 					let anchor = single_selected_point.id.get_anchor(&vector_data).expect("Cannot find connected anchor");
-					if vector_data.all_connected(anchor).count() > 2 { false } else { true }
+					vector_data.all_connected(anchor).count() <= 2
 				} else {
 					false
 				}
@@ -605,7 +605,7 @@ impl PathToolData {
 	}
 
 	fn next_drill_through_cycle(&mut self, position: DVec2) -> usize {
-		if self.last_drill_through_click_position.map_or(true, |last_pos| last_pos.distance(position) > DRILL_THROUGH_THRESHOLD) {
+		if self.last_drill_through_click_position.is_none_or(|last_pos| last_pos.distance(position) > DRILL_THROUGH_THRESHOLD) {
 			// New position, reset cycle
 			self.drill_through_cycle_index = 0;
 		} else {
@@ -625,7 +625,7 @@ impl PathToolData {
 	}
 
 	fn has_drill_through_mouse_moved(&self, position: DVec2) -> bool {
-		self.last_drill_through_click_position.map_or(true, |last_pos| last_pos.distance(position) > DRILL_THROUGH_THRESHOLD)
+		self.last_drill_through_click_position.is_none_or(|last_pos| last_pos.distance(position) > DRILL_THROUGH_THRESHOLD)
 	}
 
 	fn set_ghost_outline(&mut self, shape_editor: &ShapeState, document: &DocumentMessageHandler) {
@@ -2411,15 +2411,15 @@ impl Fsm for PathToolFsmState {
 
 					let compatible_type = first_layer.and_then(|layer| {
 						let graph_layer = graph_modification_utils::NodeGraphLayer::new(layer, &document.network_interface);
-						graph_layer.horizontal_layer_flow().nth(1).and_then(|node_id| {
+						graph_layer.horizontal_layer_flow().nth(1).map(|node_id| {
 							let (output_type, _) = document.network_interface.output_type(&node_id, 0, &[]);
-							Some(format!("type:{}", output_type.nested_type()))
+							format!("type:{}", output_type.nested_type())
 						})
 					});
 
 					let is_compatible = compatible_type.as_deref() == Some("type:Instances<VectorData>");
 
-					let is_modifiable = first_layer.map_or(false, |layer| {
+					let is_modifiable = first_layer.is_some_and(|layer| {
 						let graph_layer = graph_modification_utils::NodeGraphLayer::new(layer, &document.network_interface);
 						matches!(graph_layer.find_input("Path", 1), Some(TaggedValue::VectorModification(_)))
 					});

node-graph/gcore/src/vector/vector_data.rs

@@ -561,19 +561,26 @@ impl ManipulatorPointId {
 		}
 	}
 
-	/// Finds all the connected handles of a point. For an anchor it is all the connected handles. For a handle it is all the handles connected to its corresponding anchor other than current handle
+	/// Finds all the connected handles of a point.
+	/// For an anchor it is all the connected handles.
+	/// For a handle it is all the handles connected to its corresponding anchor other than the current handle.
 	pub fn get_all_connected_handles(self, vector_data: &VectorData) -> Option<Vec<HandleId>> {
 		match self {
-			ManipulatorPointId::Anchor(point) => Some(vector_data.all_connected(point).collect::<Vec<_>>()),
+			ManipulatorPointId::Anchor(point) => {
+				let connected = vector_data.all_connected(point).collect::<Vec<_>>();
+				Some(connected)
+			}
 			ManipulatorPointId::PrimaryHandle(segment) => {
 				let point = vector_data.segment_domain.segment_start_from_id(segment)?;
 				let current = HandleId::primary(segment);
-				Some(vector_data.segment_domain.all_connected(point).filter(|&value| value != current).collect::<Vec<_>>())
+				let connected = vector_data.segment_domain.all_connected(point).filter(|&value| value != current).collect::<Vec<_>>();
+				Some(connected)
 			}
 			ManipulatorPointId::EndHandle(segment) => {
 				let point = vector_data.segment_domain.segment_end_from_id(segment)?;
 				let current = HandleId::end(segment);
-				Some(vector_data.segment_domain.all_connected(point).filter(|&value| value != current).collect::<Vec<_>>())
+				let connected = vector_data.segment_domain.all_connected(point).filter(|&value| value != current).collect::<Vec<_>>();
+				Some(connected)
 			}
 		}
 	}