Fix self-chaining of transforms; fix compass rose getting offset when rotating a layer (#2296)

* Fix self-chaining of transforms and compass rose under single layer

https://discord.com/channels/731730685944922173/931942323644928040/1340632846863175702
https://discord.com/channels/731730685944922173/931942323644928040/1340608972071243906

* When not invertible transformation, do nothing

* Fix overlays and compass control when can't be visible

* Simplify selection logic in compass states

* Show compass only if it was possible that it could be seen before dragging

* Prevent resizing line objects

* Code review

---------

Co-authored-by: Keavon Chambers <keavon@keavon.com>

editor/src/messages/portfolio/document/document_message_handler.rs

@@ -2321,7 +2321,7 @@ pub struct ClickXRayIter<'a> {
 }
 
 fn quad_to_path_lib_segments(quad: Quad) -> Vec<path_bool_lib::PathSegment> {
-	quad.edges().into_iter().map(|[start, end]| path_bool_lib::PathSegment::Line(start, end)).collect()
+	quad.all_edges().into_iter().map(|[start, end]| path_bool_lib::PathSegment::Line(start, end)).collect()
 }
 
 fn click_targets_to_path_lib_segments<'a>(click_targets: impl Iterator<Item = &'a ClickTarget>, transform: DAffine2) -> Vec<path_bool_lib::PathSegment> {

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

@@ -1,4 +1,5 @@
 use crate::consts::{COMPASS_ROSE_ARROW_CLICK_TARGET_ANGLE, COMPASS_ROSE_HOVER_RING_DIAMETER, COMPASS_ROSE_RING_INNER_DIAMETER};
+use crate::messages::portfolio::document::utility_types::document_metadata::LayerNodeIdentifier;
 use crate::messages::prelude::DocumentMessageHandler;
 
 use glam::{DAffine2, DVec2};
@@ -10,9 +11,27 @@ pub struct CompassRose {
 }
 
 impl CompassRose {
-	pub fn refresh_transform(&mut self, document: &DocumentMessageHandler) {
+	fn get_layer_pivot_transform(layer: LayerNodeIdentifier, document: &DocumentMessageHandler) -> DAffine2 {
-		let [min, max] = document.selected_visible_and_unlock_layers_bounding_box_viewport().unwrap_or([DVec2::ZERO, DVec2::ONE]);
+		let [min, max] = document.metadata().nonzero_bounding_box(layer);
-		self.compass_center = (DAffine2::from_translation(min) * DAffine2::from_scale(max - min)).transform_point2(DVec2::splat(0.5));
+
+		let bounds_transform = DAffine2::from_translation(min) * DAffine2::from_scale(max - min);
+		let layer_transform = document.metadata().transform_to_viewport(layer);
+		layer_transform * bounds_transform
+	}
+	pub fn refresh_position(&mut self, document: &DocumentMessageHandler) {
+		let selected_nodes = document.network_interface.selected_nodes(&[]).unwrap();
+		let mut layers = selected_nodes.selected_visible_and_unlocked_layers(&document.network_interface);
+
+		let Some(first) = layers.next() else { return };
+		let count = layers.count() + 1;
+		let transform = if count == 1 {
+			Self::get_layer_pivot_transform(first, document)
+		} else {
+			let [min, max] = document.selected_visible_and_unlock_layers_bounding_box_viewport().unwrap_or([DVec2::ZERO, DVec2::ONE]);
+			DAffine2::from_translation(min) * DAffine2::from_scale(max - min)
+		};
+
+		self.compass_center = transform.transform_point2(DVec2::splat(0.5));
 	}
 
 	pub fn compass_rose_position(&self) -> DVec2 {

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

@@ -111,11 +111,12 @@ impl Pivot {
 			.selected_visible_and_unlocked_layers(&document.network_interface)
 		{
 			let transform = Self::get_layer_pivot_transform(layer, document);
+			// Only update the pivot when computed position is finite.
+			if transform.matrix2.determinant().abs() <= f64::EPSILON {
+				return;
+			};
 			let pivot = transform.inverse().transform_point2(position);
-			// Only update the pivot when computed position is finite. Infinite can happen when scale is 0.
+			responses.add(GraphOperationMessage::TransformSetPivot { layer, pivot });
-			if pivot.is_finite() {
-				responses.add(GraphOperationMessage::TransformSetPivot { layer, pivot });
-			}
 		}
 	}
 

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

@@ -411,7 +411,7 @@ impl BoundingBoxManager {
 
 	fn overlay_display_category(&self, quad: Quad) -> HandleDisplayCategory {
 		// Check if the area is essentially zero because either the width or height is smaller than an epsilon
-		if (self.bounds[0] - self.bounds[1]).abs().cmple(DVec2::splat(1e-4)).any() {
+		if self.is_bounds_flat() {
 			return HandleDisplayCategory::Flat;
 		}
 
@@ -434,6 +434,10 @@ impl BoundingBoxManager {
 		HandleDisplayCategory::Narrow
 	}
 
+	fn is_bounds_flat(&self) -> bool {
+		(self.bounds[0] - self.bounds[1]).abs().cmple(DVec2::splat(1e-4)).any()
+	}
+
 	/// Compute the threshold in viewport space. This only works with affine transforms as it assumes lines remain parallel.
 	fn compute_viewport_threshold(&self, scalar: f64) -> [f64; 2] {
 		let inverse = self.transform.inverse();
@@ -521,18 +525,18 @@ impl BoundingBoxManager {
 
 	/// Gets the required mouse cursor to show resizing bounds or optionally rotation
 	pub fn get_cursor(&self, input: &InputPreprocessorMessageHandler, rotate: bool) -> MouseCursorIcon {
-		if let Some((top, bottom, left, right)) = self.check_selected_edges(input.mouse.position) {
+		let edges = self.check_selected_edges(input.mouse.position);
-			match (top, bottom, left, right) {
+
+		match edges {
+			Some((top, bottom, left, right)) if self.is_bounds_flat() => match (top, bottom, left, right) {
 				(true, _, false, false) | (_, true, false, false) => MouseCursorIcon::NSResize,
 				(false, false, true, _) | (false, false, _, true) => MouseCursorIcon::EWResize,
 				(true, _, true, _) | (_, true, _, true) => MouseCursorIcon::NWSEResize,
 				(true, _, _, true) | (_, true, true, _) => MouseCursorIcon::NESWResize,
 				_ => MouseCursorIcon::Default,
-			}
+			},
-		} else if rotate && self.check_rotate(input.mouse.position) {
+			_ if rotate && self.check_rotate(input.mouse.position) => MouseCursorIcon::Rotate,
-			MouseCursorIcon::Rotate
+			_ => MouseCursorIcon::Default,
-		} else {
-			MouseCursorIcon::Default
 		}
 	}
 }

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

@@ -1,7 +1,10 @@
 #![allow(clippy::too_many_arguments)]
 
 use super::tool_prelude::*;
-use crate::consts::{COLOR_OVERLAY_BLUE, COLOR_OVERLAY_GREEN, COLOR_OVERLAY_RED, DRAG_DIRECTION_MODE_DETERMINATION_THRESHOLD, ROTATE_INCREMENT, SELECTION_DRAG_ANGLE, SELECTION_TOLERANCE};
+use crate::consts::{
+	COLOR_OVERLAY_BLUE, COLOR_OVERLAY_GREEN, COLOR_OVERLAY_RED, COMPASS_ROSE_HOVER_RING_DIAMETER, DRAG_DIRECTION_MODE_DETERMINATION_THRESHOLD, ROTATE_INCREMENT, SELECTION_DRAG_ANGLE,
+	SELECTION_TOLERANCE,
+};
 use crate::messages::input_mapper::utility_types::input_mouse::ViewportPosition;
 use crate::messages::portfolio::document::graph_operation::utility_types::TransformIn;
 use crate::messages::portfolio::document::overlays::utility_types::OverlayContext;
@@ -577,11 +580,11 @@ impl Fsm for SelectToolFsmState {
 
 				let show_compass = !(can_get_into_other_states || is_resizing_or_rotating);
 				let show_compass_with_ring = bounds.map(|bounds| transform * Quad::from_box(bounds)).and_then(|quad| {
-					show_compass
+					(show_compass && quad.all_sides_at_least_width(COMPASS_ROSE_HOVER_RING_DIAMETER))
 						.then_some(
 							matches!(self, SelectToolFsmState::Dragging { .. })
 								.then_some(show_hover_ring)
-								.or(quad.contains(mouse_position).then_some(show_hover_ring)),
+								.or((quad.contains(mouse_position)).then_some(show_hover_ring)),
 						)
 						.flatten()
 				});
@@ -590,7 +593,7 @@ impl Fsm for SelectToolFsmState {
 				tool_data.pivot.update_pivot(document, &mut overlay_context, angle);
 
 				// Update compass rose
-				tool_data.compass_rose.refresh_transform(document);
+				tool_data.compass_rose.refresh_position(document);
 				let compass_center = tool_data.compass_rose.compass_rose_position();
 				overlay_context.compass_rose(compass_center, angle, show_compass_with_ring);
 
@@ -600,29 +603,31 @@ impl Fsm for SelectToolFsmState {
 					compass_rose_state.axis_type().and_then(|axis| axis.is_constraint().then_some((axis, true)))
 				};
 
-				if let Some((axis, hover)) = axis_state {
+				if show_compass_with_ring.is_some() {
-					if axis.is_constraint() {
+					if let Some((axis, hover)) = axis_state {
-						let e0 = tool_data
+						if axis.is_constraint() {
-							.bounding_box_manager
+							let e0 = tool_data
-							.as_ref()
+								.bounding_box_manager
-							.map(|man| man.transform * Quad::from_box(man.bounds))
+								.as_ref()
-							.map_or(DVec2::X, |quad| (quad.top_left() - quad.top_right()).normalize_or(DVec2::X));
+								.map(|man| man.transform * Quad::from_box(man.bounds))
+								.map_or(DVec2::X, |quad| (quad.top_left() - quad.top_right()).normalize_or(DVec2::X));
 
-						let (direction, color) = match axis {
+							let (direction, color) = match axis {
-							Axis::X => (e0, COLOR_OVERLAY_RED),
+								Axis::X => (e0, COLOR_OVERLAY_RED),
-							Axis::Y => (e0.perp(), COLOR_OVERLAY_GREEN),
+								Axis::Y => (e0.perp(), COLOR_OVERLAY_GREEN),
-							_ => unreachable!(),
+								_ => unreachable!(),
-						};
+							};
 
-						let viewport_diagonal = input.viewport_bounds.size().length();
+							let viewport_diagonal = input.viewport_bounds.size().length();
 
-						let color = if !hover {
+							let color = if !hover {
-							color
+								color
-						} else {
+							} else {
-							let color_string = &graphene_std::Color::from_rgb_str(color.strip_prefix('#').unwrap()).unwrap().with_alpha(0.25).rgba_hex();
+								let color_string = &graphene_std::Color::from_rgb_str(color.strip_prefix('#').unwrap()).unwrap().with_alpha(0.25).rgba_hex();
-							&format!("#{}", color_string)
+								&format!("#{}", color_string)
-						};
+							};
-						overlay_context.line(compass_center - direction * viewport_diagonal, compass_center + direction * viewport_diagonal, Some(color));
+							overlay_context.line(compass_center - direction * viewport_diagonal, compass_center + direction * viewport_diagonal, Some(color));
+						}
 					}
 				}
 
@@ -776,16 +781,24 @@ impl Fsm for SelectToolFsmState {
 				// If the user clicks on a layer that is in their current selection, go into the dragging mode.
 				// If the user clicks on new shape, make that layer their new selection.
 				// Otherwise enter the box select mode
+				let bounds = tool_data.bounding_box_manager.as_ref().map(|man| man.transform * Quad::from_box(man.bounds));
 
-				let angle = tool_data
+				let angle = bounds.map_or(0., |quad| (quad.top_left() - quad.top_right()).to_angle());
-					.bounding_box_manager
-					.as_ref()
-					.map(|man| man.transform * Quad::from_box(man.bounds))
-					.map_or(0., |quad| (quad.top_left() - quad.top_right()).to_angle());
 				let mouse_position = input.mouse.position;
 				let compass_rose_state = tool_data.compass_rose.compass_rose_state(mouse_position, angle);
 				let is_over_pivot = tool_data.pivot.is_over(mouse_position);
 
+				let show_compass = bounds.is_some_and(|quad| quad.all_sides_at_least_width(COMPASS_ROSE_HOVER_RING_DIAMETER) && quad.contains(mouse_position));
+				let can_grab_compass_rose = compass_rose_state.can_grab() && show_compass;
+				let is_flat_layer = document
+					.network_interface
+					.selected_nodes(&[])
+					.unwrap()
+					.selected_visible_and_unlocked_layers(&document.network_interface)
+					.find(|layer| !document.network_interface.is_artboard(&layer.to_node(), &[]))
+					.map(|layer| document.metadata().transform_to_viewport(layer))
+					.is_none_or(|transform| transform.matrix2.determinant().abs() <= f64::EPSILON);
+
 				let state =
 				// Dragging the pivot
 				if is_over_pivot {
@@ -796,43 +809,24 @@ impl Fsm for SelectToolFsmState {
 
 					SelectToolFsmState::DraggingPivot
 				}
-				// Dragging one (or two, forming a corner) of the transform cage bounding box edges
+				// Dragging the selected layers around to transform them
-				else if dragging_bounds.is_some() {
+				else if can_grab_compass_rose || intersection.is_some_and(|intersection| selected.iter().any(|selected_layer| intersection.starts_with(*selected_layer, document.metadata()))) {
 					responses.add(DocumentMessage::StartTransaction);
 
+					if input.keyboard.key(select_deepest) || tool_data.nested_selection_behavior == NestedSelectionBehavior::Deepest {
+						tool_data.select_single_layer = intersection;
+					} else {
+						tool_data.select_single_layer = intersection.and_then(|intersection| intersection.ancestors(document.metadata()).find(|ancestor| selected.contains(ancestor)));
+					}
+
 					tool_data.layers_dragging = selected;
 
-					if let Some(bounds) = &mut tool_data.bounding_box_manager {
-						bounds.original_bound_transform = bounds.transform;
-
-						tool_data.layers_dragging.retain(|layer| {
-							if *layer != LayerNodeIdentifier::ROOT_PARENT {
-								document.network_interface.network(&[]).unwrap().nodes.contains_key(&layer.to_node())
-							} else {
-								log::error!("ROOT_PARENT should not be part of layers_dragging");
-								false
-							}
-						});
-
-						let mut selected = Selected::new(
-							&mut bounds.original_transforms,
-							&mut bounds.center_of_transformation,
-							&tool_data.layers_dragging,
-							responses,
-							&document.network_interface,
-							None,
-							&ToolType::Select,
-							None
-						);
-						bounds.center_of_transformation = selected.mean_average_of_pivots();
-					}
 					tool_data.get_snap_candidates(document, input);
-
+					let (axis, using_compass) = {
-					if input.keyboard.key(skew) {
+						let axis_state = compass_rose_state.axis_type().filter(|_| can_grab_compass_rose);
-						SelectToolFsmState::SkewingBounds
+						(axis_state.unwrap_or_default(), axis_state.is_some())
-					}else{
+					};
-						SelectToolFsmState::ResizingBounds
+					SelectToolFsmState::Dragging { axis, using_compass }
-					}
 				}
 				// Dragging near the transform cage bounding box to rotate it
 				else if rotating_bounds {
@@ -865,23 +859,42 @@ impl Fsm for SelectToolFsmState {
 
 					SelectToolFsmState::RotatingBounds
 				}
-				// Dragging the selected layers around to transform them
+				// Dragging one (or two, forming a corner) of the transform cage bounding box edges
-				else if compass_rose_state.can_grab() || intersection.is_some_and(|intersection| selected.iter().any(|selected_layer| intersection.starts_with(*selected_layer, document.metadata()))) {
+				else if dragging_bounds.is_some() && !is_flat_layer {
 					responses.add(DocumentMessage::StartTransaction);
 
-					if input.keyboard.key(select_deepest) || tool_data.nested_selection_behavior == NestedSelectionBehavior::Deepest {
-						tool_data.select_single_layer = intersection;
-					} else {
-						tool_data.select_single_layer = intersection.and_then(|intersection| intersection.ancestors(document.metadata()).find(|ancestor| selected.contains(ancestor)));
-					}
-
 					tool_data.layers_dragging = selected;
 
+					if let Some(bounds) = &mut tool_data.bounding_box_manager {
+						bounds.original_bound_transform = bounds.transform;
+
+						tool_data.layers_dragging.retain(|layer| {
+							if *layer != LayerNodeIdentifier::ROOT_PARENT {
+								document.network_interface.network(&[]).unwrap().nodes.contains_key(&layer.to_node())
+							} else {
+								log::error!("ROOT_PARENT should not be part of layers_dragging");
+								false
+							}
+						});
+
+						let mut selected = Selected::new(
+							&mut bounds.original_transforms,
+							&mut bounds.center_of_transformation,
+							&tool_data.layers_dragging,
+							responses,
+							&document.network_interface,
+							None,
+							&ToolType::Select,
+							None
+						);
+						bounds.center_of_transformation = selected.mean_average_of_pivots();
+					}
 					tool_data.get_snap_candidates(document, input);
-					let axis = compass_rose_state.axis_type();
+
-					match axis {
+					if input.keyboard.key(skew) {
-						Some(axis) => SelectToolFsmState::Dragging { axis, using_compass: true },
+						SelectToolFsmState::SkewingBounds
-						None => SelectToolFsmState::Dragging { axis: Axis::None, using_compass: false }
+					} else {
+						SelectToolFsmState::ResizingBounds
 					}
 				}
 				// Dragging a selection box

editor/src/messages/tool/transform_layer/transform_layer_message_handler.rs

@@ -357,8 +357,6 @@ impl MessageHandler<TransformLayerMessage, TransformData<'_>> for TransformLayer
 					|| selected_layers.is_empty()
 					|| matches!(self.transform_operation, TransformOperation::Grabbing(_))
 				{
-					selected.original_transforms.clear();
-
 					return;
 				}
 
@@ -384,7 +382,6 @@ impl MessageHandler<TransformLayerMessage, TransformData<'_>> for TransformLayer
 					|| selected_layers.is_empty()
 					|| matches!(self.transform_operation, TransformOperation::Rotating(_))
 				{
-					selected.original_transforms.clear();
 					return;
 				}
 
@@ -438,7 +435,6 @@ impl MessageHandler<TransformLayerMessage, TransformData<'_>> for TransformLayer
 					|| selected_layers.is_empty()
 					|| matches!(self.transform_operation, TransformOperation::Scaling(_))
 				{
-					selected.original_transforms.clear();
 					return;
 				}
 

node-graph/gcore/src/graphic_element/renderer.rs

@@ -90,7 +90,7 @@ impl ClickTarget {
 		// This bounding box is not very accurate as it is the axis aligned version of the transformed bounding box. However it is fast.
 		if !self
 			.bounding_box
-			.is_some_and(|loose| intersects((layer_transform * Quad::from_box(loose)).bounding_box(), target_bounds))
+			.is_some_and(|loose| (loose[0] - loose[1]).abs().cmpgt(DVec2::splat(1e-4)).all() && intersects((layer_transform * Quad::from_box(loose)).bounding_box(), target_bounds))
 		{
 			return false;
 		}

node-graph/gcore/src/graphic_element/renderer/quad.rs

@@ -44,13 +44,23 @@ impl Quad {
 	}
 
 	/// Get all the edges in the quad.
-	pub fn edges(&self) -> [[DVec2; 2]; 4] {
+	pub fn all_edges(&self) -> [[DVec2; 2]; 4] {
 		[[self.0[0], self.0[1]], [self.0[1], self.0[2]], [self.0[2], self.0[3]], [self.0[3], self.0[0]]]
 	}
 
+	/// Get two edges as orthogonal bases.
+	pub fn edges(&self) -> [[DVec2; 2]; 2] {
+		[[self.0[0], self.0[1]], [self.0[1], self.0[2]]]
+	}
+
+	/// Returns true only if the width and height are both greater than or equal to the given width.
+	pub fn all_sides_at_least_width(&self, width: f64) -> bool {
+		self.edges().into_iter().all(|[a, b]| (a - b).length_squared() >= width.powi(2))
+	}
+
 	/// Get all the edges in the quad as linear bezier curves
 	pub fn bezier_lines(&self) -> impl Iterator<Item = bezier_rs::Bezier> + '_ {
-		self.edges().into_iter().map(|[start, end]| bezier_rs::Bezier::from_linear_dvec2(start, end))
+		self.all_edges().into_iter().map(|[start, end]| bezier_rs::Bezier::from_linear_dvec2(start, end))
 	}
 
 	/// Generates the axis aligned bounding box of the quad
@@ -126,9 +136,9 @@ impl Quad {
 
 	pub fn intersects(&self, other: Quad) -> bool {
 		let intersects = self
-			.edges()
+			.all_edges()
 			.into_iter()
-			.any(|[a, b]| other.edges().into_iter().any(|[c, d]| Self::intersect_lines(a, b, c, d).is_some()));
+			.any(|[a, b]| other.all_edges().into_iter().any(|[c, d]| Self::intersect_lines(a, b, c, d).is_some()));
 		self.contains(other.center()) || other.contains(self.center()) || intersects
 	}
 }