Improve Layer Merging Reusability & Adjust Segment Insertion Logic

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

@@ -1,5 +1,6 @@
+use crate::messages::portfolio::document::node_graph::document_node_definitions;
 use crate::messages::portfolio::document::utility_types::document_metadata::LayerNodeIdentifier;
-use crate::messages::portfolio::document::utility_types::network_interface::{FlowType, NodeNetworkInterface, NodeTemplate};
+use crate::messages::portfolio::document::utility_types::network_interface::{FlowType, InputConnector, NodeNetworkInterface, NodeTemplate};
 use crate::messages::prelude::*;
 use bezier_rs::Subpath;
 use graph_craft::document::{value::TaggedValue, NodeId, NodeInput};
@@ -13,6 +14,102 @@ use graphene_core::Color;
 use glam::DVec2;
 use std::collections::VecDeque;
 
+pub fn merge_layers(document: &DocumentMessageHandler, current_layer: LayerNodeIdentifier, other_layer: LayerNodeIdentifier, responses: &mut VecDeque<Message>) {
+	// Calculate the downstream transforms in order to bring the other vector data into the same layer space
+	let current_transform = document.metadata().downstream_transform_to_document(current_layer);
+	let other_transform = document.metadata().downstream_transform_to_document(other_layer);
+	// Represents the change in position that would occur if the other layer was moved below the current layer
+	let transform_delta = current_transform * other_transform.inverse();
+	let offset = transform_delta.inverse();
+	responses.add(GraphOperationMessage::TransformChange {
+		layer: other_layer,
+		transform: offset,
+		transform_in: crate::messages::portfolio::document::graph_operation::utility_types::TransformIn::Local,
+		skip_rerender: false,
+	});
+
+	// Move the other layer below the current layer for positioning purposes
+	let current_layer_parent = current_layer.parent(document.metadata()).unwrap();
+	let current_layer_index = current_layer_parent.children(document.metadata()).position(|child| child == current_layer).unwrap();
+	responses.add(NodeGraphMessage::MoveLayerToStack {
+		layer: other_layer,
+		parent: current_layer_parent,
+		insert_index: current_layer_index + 1,
+	});
+
+	// Merge the inputs of the two layers
+	let merge_node_id = NodeId::new();
+	let merge_node = document_node_definitions::resolve_document_node_type("Merge")
+		.expect("Failed to create merge node")
+		.default_node_template();
+	responses.add(NodeGraphMessage::InsertNode {
+		node_id: merge_node_id,
+		node_template: merge_node,
+	});
+	responses.add(NodeGraphMessage::SetToNodeOrLayer {
+		node_id: merge_node_id,
+		is_layer: false,
+	});
+	responses.add(NodeGraphMessage::MoveNodeToChainStart {
+		node_id: merge_node_id,
+		parent: current_layer,
+	});
+	responses.add(NodeGraphMessage::ConnectUpstreamOutputToInput {
+		downstream_input: InputConnector::node(other_layer.to_node(), 1),
+		input_connector: InputConnector::node(merge_node_id, 1),
+	});
+	responses.add(NodeGraphMessage::DeleteNodes {
+		node_ids: vec![other_layer.to_node()],
+		delete_children: false,
+	});
+
+	// Add a flatten vector elements node after the merge
+	let flatten_node_id = NodeId::new();
+	let flatten_node = document_node_definitions::resolve_document_node_type("Flatten Vector Elements")
+		.expect("Failed to create flatten node")
+		.default_node_template();
+	responses.add(NodeGraphMessage::InsertNode {
+		node_id: flatten_node_id,
+		node_template: flatten_node,
+	});
+	responses.add(NodeGraphMessage::MoveNodeToChainStart {
+		node_id: flatten_node_id,
+		parent: current_layer,
+	});
+
+	// Add a path node after the flatten node
+	let path_node_id = NodeId::new();
+	let path_node = document_node_definitions::resolve_document_node_type("Path")
+		.expect("Failed to create path node")
+		.default_node_template();
+	responses.add(NodeGraphMessage::InsertNode {
+		node_id: path_node_id,
+		node_template: path_node,
+	});
+	responses.add(NodeGraphMessage::MoveNodeToChainStart {
+		node_id: path_node_id,
+		parent: current_layer,
+	});
+
+	// Add a transform node to ensure correct tooling modifications
+	let transform_node_id = NodeId::new();
+	let transform_node = document_node_definitions::resolve_document_node_type("Transform")
+		.expect("Failed to create transform node")
+		.default_node_template();
+	responses.add(NodeGraphMessage::InsertNode {
+		node_id: transform_node_id,
+		node_template: transform_node,
+	});
+	responses.add(NodeGraphMessage::MoveNodeToChainStart {
+		node_id: transform_node_id,
+		parent: current_layer,
+	});
+
+	responses.add(NodeGraphMessage::RunDocumentGraph);
+	responses.add(Message::StartBuffer);
+	responses.add(PenToolMessage::RecalculateLatestPointsPosition);
+}
+
 /// Create a new vector layer.
 pub fn new_vector_layer(subpaths: Vec<Subpath<PointId>>, id: NodeId, parent: LayerNodeIdentifier, responses: &mut VecDeque<Message>) -> LayerNodeIdentifier {
 	let insert_index = 0;

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

@@ -1,4 +1,4 @@
-use super::graph_modification_utils;
+use super::graph_modification_utils::{self, merge_layers};
 use super::snapping::{SnapCache, SnapCandidatePoint, SnapData, SnapManager, SnappedPoint};
 use crate::messages::portfolio::document::utility_types::document_metadata::{DocumentMetadata, LayerNodeIdentifier};
 use crate::messages::portfolio::document::utility_types::misc::{PathSnapSource, SnapSource};
@@ -249,42 +249,17 @@ impl ShapeState {
 					handles: [None, None],
 				};
 				responses.add(GraphOperationMessage::Vector { layer: layer1, modification_type });
-			}
-			// TODO: Fix the implementation of this case so it actually connects the separate layers, see:
-			// TODO: <https://github.com/GraphiteEditor/Graphite/pull/2227#issuecomment-2626342475>
-			else {
-				// Points are in different layers - find the topmost layer
-				let top_layer = document.metadata().all_layers().find(|&layer| layer == layer1 || layer == layer2).unwrap_or(layer1);
-
-				let bottom_layer = if top_layer == layer1 { layer2 } else { layer1 };
-				let bottom_point = if top_layer == layer1 { end_point } else { start_point };
-
-				// Get position of point in bottom layer
-				let Some(bottom_vector_data) = document.network_interface.compute_modified_vector(bottom_layer) else {
-					return;
-				};
-				let Some(point_pos) = bottom_vector_data.point_domain.position_from_id(bottom_point) else {
-					return;
-				};
-
-				// Create new point in top layer
-				let new_point_id = PointId::generate();
-				let modification_type = VectorModificationType::InsertPoint {
-					id: new_point_id,
-					position: point_pos,
-				};
-				responses.add(GraphOperationMessage::Vector { layer: top_layer, modification_type });
-
-				// Create segment between points in top layer
+			} else {
+				// Merge the layers
+				merge_layers(document, layer1, layer2, responses);
+				// Create segmen between the two points
 				let segment_id = SegmentId::generate();
-				let points = if top_layer == layer1 { [start_point, new_point_id] } else { [new_point_id, end_point] };
-
 				let modification_type = VectorModificationType::InsertSegment {
 					id: segment_id,
-					points,
+					points: [end_point, start_point],
 					handles: [None, None],
 				};
-				responses.add(GraphOperationMessage::Vector { layer: top_layer, modification_type });
+				responses.add(GraphOperationMessage::Vector { layer: layer1, modification_type });
 			}
 			return;
 		}

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

@@ -1,13 +1,12 @@
 use super::tool_prelude::*;
 use crate::consts::{DEFAULT_STROKE_WIDTH, HIDE_HANDLE_DISTANCE, LINE_ROTATE_SNAP_ANGLE};
-use crate::messages::portfolio::document::node_graph::document_node_definitions::{self, resolve_document_node_type};
+use crate::messages::portfolio::document::node_graph::document_node_definitions::resolve_document_node_type;
 use crate::messages::portfolio::document::overlays::utility_functions::path_overlays;
 use crate::messages::portfolio::document::overlays::utility_types::OverlayContext;
 use crate::messages::portfolio::document::utility_types::document_metadata::LayerNodeIdentifier;
-use crate::messages::portfolio::document::utility_types::network_interface::InputConnector;
 use crate::messages::tool::common_functionality::auto_panning::AutoPanning;
 use crate::messages::tool::common_functionality::color_selector::{ToolColorOptions, ToolColorType};
-use crate::messages::tool::common_functionality::graph_modification_utils;
+use crate::messages::tool::common_functionality::graph_modification_utils::{self, merge_layers};
 use crate::messages::tool::common_functionality::snapping::{SnapCandidatePoint, SnapConstraint, SnapData, SnapManager, SnapTypeConfiguration};
 use crate::messages::tool::common_functionality::utility_functions::should_extend;
 
@@ -925,99 +924,3 @@ impl Fsm for PenToolFsmState {
 		responses.add(FrontendMessage::UpdateMouseCursor { cursor: MouseCursorIcon::Default });
 	}
 }
-
-fn merge_layers(document: &DocumentMessageHandler, current_layer: LayerNodeIdentifier, other_layer: LayerNodeIdentifier, responses: &mut VecDeque<Message>) {
-	// Calculate the downstream transforms in order to bring the other vector data into the same layer space
-	let current_transform = document.metadata().downstream_transform_to_document(current_layer);
-	let other_transform = document.metadata().downstream_transform_to_document(other_layer);
-	// Represents the change in position that would occur if the other layer was moved below the current layer
-	let transform_delta = current_transform * other_transform.inverse();
-	let offset = transform_delta.inverse();
-	responses.add(GraphOperationMessage::TransformChange {
-		layer: other_layer,
-		transform: offset,
-		transform_in: crate::messages::portfolio::document::graph_operation::utility_types::TransformIn::Local,
-		skip_rerender: false,
-	});
-
-	// Move the other layer below the current layer for positioning purposes
-	let current_layer_parent = current_layer.parent(document.metadata()).unwrap();
-	let current_layer_index = current_layer_parent.children(document.metadata()).position(|child| child == current_layer).unwrap();
-	responses.add(NodeGraphMessage::MoveLayerToStack {
-		layer: other_layer,
-		parent: current_layer_parent,
-		insert_index: current_layer_index + 1,
-	});
-
-	// Merge the inputs of the two layers
-	let merge_node_id = NodeId::new();
-	let merge_node = document_node_definitions::resolve_document_node_type("Merge")
-		.expect("Failed to create merge node")
-		.default_node_template();
-	responses.add(NodeGraphMessage::InsertNode {
-		node_id: merge_node_id,
-		node_template: merge_node,
-	});
-	responses.add(NodeGraphMessage::SetToNodeOrLayer {
-		node_id: merge_node_id,
-		is_layer: false,
-	});
-	responses.add(NodeGraphMessage::MoveNodeToChainStart {
-		node_id: merge_node_id,
-		parent: current_layer,
-	});
-	responses.add(NodeGraphMessage::ConnectUpstreamOutputToInput {
-		downstream_input: InputConnector::node(other_layer.to_node(), 1),
-		input_connector: InputConnector::node(merge_node_id, 1),
-	});
-	responses.add(NodeGraphMessage::DeleteNodes {
-		node_ids: vec![other_layer.to_node()],
-		delete_children: false,
-	});
-
-	// Add a flatten vector elements node after the merge
-	let flatten_node_id = NodeId::new();
-	let flatten_node = document_node_definitions::resolve_document_node_type("Flatten Vector Elements")
-		.expect("Failed to create flatten node")
-		.default_node_template();
-	responses.add(NodeGraphMessage::InsertNode {
-		node_id: flatten_node_id,
-		node_template: flatten_node,
-	});
-	responses.add(NodeGraphMessage::MoveNodeToChainStart {
-		node_id: flatten_node_id,
-		parent: current_layer,
-	});
-
-	// Add a path node after the flatten node
-	let path_node_id = NodeId::new();
-	let path_node = document_node_definitions::resolve_document_node_type("Path")
-		.expect("Failed to create path node")
-		.default_node_template();
-	responses.add(NodeGraphMessage::InsertNode {
-		node_id: path_node_id,
-		node_template: path_node,
-	});
-	responses.add(NodeGraphMessage::MoveNodeToChainStart {
-		node_id: path_node_id,
-		parent: current_layer,
-	});
-
-	// Add a transform node to ensure correct tooling modifications
-	let transform_node_id = NodeId::new();
-	let transform_node = document_node_definitions::resolve_document_node_type("Transform")
-		.expect("Failed to create transform node")
-		.default_node_template();
-	responses.add(NodeGraphMessage::InsertNode {
-		node_id: transform_node_id,
-		node_template: transform_node,
-	});
-	responses.add(NodeGraphMessage::MoveNodeToChainStart {
-		node_id: transform_node_id,
-		parent: current_layer,
-	});
-
-	responses.add(NodeGraphMessage::RunDocumentGraph);
-	responses.add(Message::StartBuffer);
-	responses.add(PenToolMessage::RecalculateLatestPointsPosition);
-}