From 5b008936d225ea4b1d225981de200ea47f390943 Mon Sep 17 00:00:00 2001 From: Keavon Chambers Date: Wed, 17 Jun 2026 22:31:06 -0700 Subject: [PATCH] Make the node graph's "Merge Selected Nodes" target the right-clicked node, and fix it crashing (#4252) --- .../document/document_message_handler.rs | 31 +++++++++++++++++++ .../node_graph/node_graph_message_handler.rs | 10 ++++-- editor/src/node_graph_executor/runtime.rs | 14 ++++++++- 3 files changed, 52 insertions(+), 3 deletions(-) diff --git a/editor/src/messages/portfolio/document/document_message_handler.rs b/editor/src/messages/portfolio/document/document_message_handler.rs index 4d94c93b18..1629232be9 100644 --- a/editor/src/messages/portfolio/document/document_message_handler.rs +++ b/editor/src/messages/portfolio/document/document_message_handler.rs @@ -3908,6 +3908,37 @@ mod document_message_handler_tests { editor.handle_message(DocumentMessage::CreateEmptyFolder).await; assert!(true, "Application didn't crash after folder move operation"); } + + // Merging nodes whose output isn't wired downstream produces an encapsulating subnetwork with no exports. + // Inspecting it via the Data panel (which splices in a monitor node) must not leave a dangling reference that crashes compilation. + #[tokio::test] + async fn merge_selected_nodes_while_inspecting_does_not_crash() { + let mut editor = EditorTestUtils::create(); + editor.new_document().await; + editor.draw_rect(0., 0., 100., 100.).await; + + let node_a = editor.create_node_by_name(DefinitionIdentifier::ProtoNode(graphene_std::transform_nodes::transform::IDENTIFIER)).await; + let node_b = editor.create_node_by_name(DefinitionIdentifier::ProtoNode(graphene_std::transform_nodes::transform::IDENTIFIER)).await; + editor.handle_message(NodeGraphMessage::SelectedNodesSet { nodes: vec![node_a, node_b] }).await; + editor.handle_message(NodeGraphMessage::MergeSelectedNodes).await; + + let merged = editor.active_document().network_interface.selected_nodes_in_nested_network(&[]).unwrap().0.clone(); + assert_eq!(merged.len(), 1, "merge should leave one encapsulating node selected"); + + // Simulate the Data panel inspecting the merged node, which compiles the graph with a monitor node spliced in + let portfolio = &mut editor.editor.dispatcher.message_handlers.portfolio_message_handler; + let document_id = portfolio.active_document_id.unwrap(); + let document = portfolio.documents.get_mut(&document_id).unwrap(); + portfolio + .executor + .submit_node_graph_evaluation(document, document_id, glam::UVec2::ONE, 1., Default::default(), merged, true, DVec2::ZERO) + .unwrap(); + editor.runtime.run().await; + + let mut messages = VecDeque::new(); + editor.editor.poll_node_graph_evaluation(&mut messages).unwrap(); + } + #[tokio::test] async fn test_moving_folder_with_children() { let mut editor = EditorTestUtils::create(); diff --git a/editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs b/editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs index 3ac4622911..da0cce8911 100644 --- a/editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs +++ b/editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs @@ -838,11 +838,17 @@ impl<'a> MessageHandler> for NodeG let currently_is_node = !network_interface.is_layer(&node_id, breadcrumb_network_path); let can_be_layer = network_interface.is_eligible_to_be_layer(&node_id, breadcrumb_network_path); + let selected_nodes = network_interface.selected_nodes_in_nested_network(selection_network_path); + let is_clicked_selected = selected_nodes.as_ref().is_some_and(|selected| selected.selected_nodes().any(|id| *id == node_id)); + + // Right-clicking an unselected node selects just it, so selection-based actions like Merge Selected Nodes target it + if !is_clicked_selected { + responses.add(NodeGraphMessage::SelectedNodesSet { nodes: vec![node_id] }); + } + // Determine which layers the Lock/Unlock action would affect: // - If the right-clicked node is in the selection, it affects all selected layers // - If the right-clicked node is not in the selection, it affects just the right-clicked node - let selected_nodes = network_interface.selected_nodes_in_nested_network(selection_network_path); - let is_clicked_selected = selected_nodes.as_ref().is_some_and(|selected| selected.selected_nodes().any(|id| *id == node_id)); let affected_layer_ids = if is_clicked_selected { selected_nodes.map(|selected| selected.selected_nodes().copied().filter(|id| network_interface.is_layer(id, selection_network_path)).collect()) } else { diff --git a/editor/src/node_graph_executor/runtime.rs b/editor/src/node_graph_executor/runtime.rs index e2712e9f59..1b8853f9af 100644 --- a/editor/src/node_graph_executor/runtime.rs +++ b/editor/src/node_graph_executor/runtime.rs @@ -609,11 +609,23 @@ impl PartialEq for InspectResult { impl InspectState { /// Insert the monitor node alongside the inspect node identified by `inspect_path` (full path from root, last element is the target). - /// Returns `None` if the path is empty or doesn't resolve to a node inside a reachable subnetwork. + /// Returns `None` if the path is empty, doesn't resolve to a node inside a reachable subnetwork, or the target has no + /// flatten-safe primary output to monitor (e.g. an empty merged subnetwork), which would otherwise leave the monitor's + /// input dangling once the subnetwork is flattened away. pub fn monitor_inspect_node(network: &mut NodeNetwork, inspect_path: &[NodeId]) -> Option { let (inspect_node, parent_path) = inspect_path.split_last()?; let inspect_node = *inspect_node; let target_network = navigate_to_network_mut(network, parent_path)?; + + // A subnetwork's primary output only survives flattening if its first export is a node + let monitorable = match &target_network.nodes.get(&inspect_node)?.implementation { + DocumentNodeImplementation::Network(inner) => matches!(inner.exports.first(), Some(NodeInput::Node { .. })), + _ => true, + }; + if !monitorable { + return None; + } + let monitor_id = NodeId::new(); // It is necessary to replace the inputs before inserting the monitor node to avoid changing the input of the new monitor node