From ff18b333624d00bee1d9688c7fba8bf3a28b07d7 Mon Sep 17 00:00:00 2001 From: Keavon Chambers Date: Thu, 23 Jul 2026 18:35:18 -0700 Subject: [PATCH] Fix assorted node graph correctness bugs and replace network interface panics with logged errors (#4364) * Deselect deleted layer children by pruning the expanded deletion set from the selection * Rebuild the document structure on redo to match undo * Check for cycles in set_input before applying layout side effects rather than reverting after * Correct the swapped old/new exposure binding names in set_input * Reload the document structure when an import or export is appended into the first two slots * Treat toggling preview on a disconnected export as a preview with nothing to restore * Replace panics on malformed networks with logged errors in the network interface * Make is_artboard an identity check independent of scene connectivity * Stop serializing the selection undo and redo history into saved documents * Run the set_input cycle check before the node-to-node disconnect so a rejected wire swap leaves the graph unchanged --- .../document/document_message_handler.rs | 3 + .../utility_types/network_interface.rs | 128 ++++++++++-------- 2 files changed, 74 insertions(+), 57 deletions(-) diff --git a/editor/src/messages/portfolio/document/document_message_handler.rs b/editor/src/messages/portfolio/document/document_message_handler.rs index 1c2abd7293..a4aef354d3 100644 --- a/editor/src/messages/portfolio/document/document_message_handler.rs +++ b/editor/src/messages/portfolio/document/document_message_handler.rs @@ -2464,6 +2464,9 @@ impl DocumentMessageHandler { let transform = self.navigation_handler.calculate_offset_transform(viewport.center_in_viewport_space().into(), &self.document_ptz); network_interface.set_document_to_viewport_transform(transform); + // Ensure document structure is loaded so that updating the selected nodes has the correct metadata + network_interface.load_structure(); + let previous_network = std::mem::replace(&mut self.network_interface, network_interface); // Push the UpdateOpenDocumentsList message to the bus in order to update the save status of the open documents responses.add(PortfolioMessage::UpdateOpenDocumentsList); diff --git a/editor/src/messages/portfolio/document/utility_types/network_interface.rs b/editor/src/messages/portfolio/document/utility_types/network_interface.rs index c6e827f210..9582327a2d 100644 --- a/editor/src/messages/portfolio/document/utility_types/network_interface.rs +++ b/editor/src/messages/portfolio/document/utility_types/network_interface.rs @@ -945,15 +945,11 @@ impl NodeNetworkInterface { log::error!("Could not get reference for node in reference: {node_id:?}"); return None; }; - node_metadata - .persistent_metadata - .network_metadata - .as_ref() - .expect("Network metadata must exist for network node in reference") - .persistent_metadata - .reference - .clone() - .map(DefinitionIdentifier::Network) + let Some(network_metadata) = node_metadata.persistent_metadata.network_metadata.as_ref() else { + log::error!("Network metadata must exist for network node in reference: {node_id:?}"); + return None; + }; + network_metadata.persistent_metadata.reference.clone().map(DefinitionIdentifier::Network) } DocumentNodeImplementation::ProtoNode(protonode_id) => Some(DefinitionIdentifier::ProtoNode(protonode_id.clone())), _ => None, @@ -1184,12 +1180,16 @@ impl NodeNetworkInterface { } } + /// Whether the node is an Artboard node by identity, regardless of whether it currently participates in the scene. + /// Callers that care about scene membership should source their layers from the document structure or check connectivity separately. pub fn is_artboard(&self, node_id: &NodeId, network_path: &[NodeId]) -> bool { self.reference(node_id, network_path) - .is_some_and(|reference| reference == DefinitionIdentifier::Network("Artboard".into()) && self.connected_to_output(node_id, &[])) + .is_some_and(|reference| reference == DefinitionIdentifier::Network("Artboard".into())) } + /// All artboard layers that participate in the scene, excluding disconnected Artboard nodes. pub fn all_artboards(&self) -> HashSet { + // O(n * (nodes + wires)) since connected_to_output performs a graph walk per artboard candidate self.document_network_metadata() .persistent_metadata .node_metadata @@ -1453,7 +1453,7 @@ impl NodeNetworkInterface { let mut node_metadata = DocumentNodeMetadata::default(); node.inputs = old_node.inputs; - node.call_argument = old_node.manual_composition.unwrap(); + node.call_argument = old_node.manual_composition.unwrap_or_default(); node.visible = old_node.visible; node.skip_deduplication = old_node.skip_deduplication; node.original_location = old_node.original_location; @@ -2105,39 +2105,28 @@ impl NodeNetworkInterface { outward_wires.insert(OutputConnector::Import(import_index), Vec::new()); } // Collect wires between all nodes and the Imports + // A missing entry means a wire references a node output or import that does not exist, so log it and register the connector anyway rather than crashing + let push_outward_wire = |outward_wires: &mut HashMap>, output_connector: OutputConnector, input_connector: InputConnector| { + let outward_wires_entry = outward_wires.entry(output_connector).or_insert_with(|| { + log::error!("Output connector {output_connector:?} should be initialized in load_outward_wires"); + Vec::new() + }); + outward_wires_entry.push(input_connector); + }; for (current_node_id, node) in network.nodes.iter() { for (input_index, input) in node.inputs.iter().enumerate() { if let NodeInput::Node { node_id, output_index, .. } = input { - // If this errors then there is an input to a node that does not exist - let outward_wires_entry = outward_wires.get_mut(&OutputConnector::node(*node_id, *output_index)).unwrap_or_else(|| { - panic!( - "Output connector {:?} should be initialized for each node output from a node", - OutputConnector::node(*node_id, *output_index) - ) - }); - outward_wires_entry.push(InputConnector::node(*current_node_id, input_index)); + push_outward_wire(&mut outward_wires, OutputConnector::node(*node_id, *output_index), InputConnector::node(*current_node_id, input_index)); } else if let NodeInput::Import { import_index, .. } = input { - let outward_wires_entry = outward_wires - .get_mut(&OutputConnector::Import(*import_index)) - .unwrap_or_else(|| panic!("Output connector {:?} should be initialized for each import from a node", OutputConnector::Import(*import_index))); - outward_wires_entry.push(InputConnector::node(*current_node_id, input_index)); + push_outward_wire(&mut outward_wires, OutputConnector::Import(*import_index), InputConnector::node(*current_node_id, input_index)); } } } for (export_index, export) in network.exports.iter().enumerate() { if let NodeInput::Node { node_id, output_index, .. } = export { - let outward_wires_entry = outward_wires.get_mut(&OutputConnector::node(*node_id, *output_index)).unwrap_or_else(|| { - panic!( - "Output connector {:?} should be initialized for each node input from exports", - OutputConnector::node(*node_id, *output_index) - ) - }); - outward_wires_entry.push(InputConnector::Export(export_index)); + push_outward_wire(&mut outward_wires, OutputConnector::node(*node_id, *output_index), InputConnector::Export(export_index)); } else if let NodeInput::Import { import_index, .. } = export { - let outward_wires_entry = outward_wires - .get_mut(&OutputConnector::Import(*import_index)) - .unwrap_or_else(|| panic!("Output connector {:?} should be initialized between imports and exports", OutputConnector::Import(*import_index))); - outward_wires_entry.push(InputConnector::Export(export_index)); + push_outward_wire(&mut outward_wires, OutputConnector::Import(*import_index), InputConnector::Export(export_index)); } } @@ -3551,14 +3540,15 @@ impl NodeNetworkInterface { } pub fn vector_modify(&mut self, node_id: &NodeId, modification_type: VectorModificationType) { - let Some(node) = self.network_mut(&[]).unwrap().nodes.get_mut(node_id) else { + let Some(node) = self.network_mut(&[]).and_then(|network| network.nodes.get_mut(node_id)) else { log::error!("Could not get node in vector_modification"); return; }; { let mut value = node.inputs.get_mut(1).and_then(|input| input.as_value_mut()); let Some(TaggedValue::VectorModification(modification)) = value.as_deref_mut() else { - panic!("Path node does not have modification input"); + log::error!("Path node {node_id} does not have a modification input"); + return; }; modification.modify(&modification_type); @@ -3574,6 +3564,7 @@ impl NodeNetworkInterface { }; let input = NodeInput::value(default_value, true); + let inserted_index = if insert_index == -1 { network.exports.len() } else { insert_index as usize }; if insert_index == -1 { network.exports.push(input); } else { @@ -3622,7 +3613,7 @@ impl NodeNetworkInterface { } // If the export is inserted as the first input or second input, and the parent network is the document_network, then it may have affected the document metadata structure - if network_path.len() == 1 && (insert_index == 0 || insert_index == 1) { + if network_path.len() == 1 && inserted_index <= 1 { self.load_structure(); } } @@ -3645,6 +3636,7 @@ impl NodeNetworkInterface { }; let input = NodeInput::value(default_value, exposed); + let inserted_index = if insert_index == -1 { node.inputs.len() } else { insert_index as usize }; if insert_index == -1 { node.inputs.push(input); } else { @@ -3677,7 +3669,7 @@ impl NodeNetworkInterface { // Update the metadata for the encapsulating node self.unload_node_click_targets(&node_id, &encapsulating_network_path); self.unload_all_nodes_bounding_box(&encapsulating_network_path); - if encapsulating_network_path.is_empty() && (insert_index == 0 || insert_index == 1) { + if encapsulating_network_path.is_empty() && inserted_index <= 1 { self.load_structure(); } @@ -4154,6 +4146,37 @@ impl NodeNetworkInterface { return; }; + // Reject a change that would create a cycle before any side effects run (only Node connections can create cycles). + // The new input is swapped in just for this test, then restored so the disconnect and layout logic below sees the unmodified network. + if matches!(new_input, NodeInput::Node { .. }) { + let Some(network) = self.network_mut(network_path) else { + log::error!("Could not get nested network in set_input"); + return; + }; + fn get_input<'a>(network: &'a mut NodeNetwork, input_connector: &InputConnector) -> Option<&'a mut NodeInput> { + match input_connector { + InputConnector::Node { node_id, input_index } => network.nodes.get_mut(node_id).and_then(|node| node.inputs.get_mut(*input_index)), + InputConnector::Export(export_index) => network.exports.get_mut(*export_index), + } + } + + let Some(input) = get_input(network, input_connector) else { + log::error!("Could not get input in set_input"); + return; + }; + let old_input = std::mem::replace(input, new_input.clone()); + let is_acyclic = network.is_acyclic(); + let Some(input) = get_input(network, input_connector) else { + log::error!("Could not get input in set_input"); + return; + }; + *input = old_input; + + if !is_acyclic { + return; + } + } + // When changing a NodeInput::Node to a NodeInput::Node, the input should first be disconnected to ensure proper side effects if (matches!(previous_input, NodeInput::Node { .. }) && matches!(new_input, NodeInput::Node { .. })) { self.disconnect_input(input_connector, network_path); @@ -4204,13 +4227,7 @@ impl NodeNetworkInterface { return; }; - // Ensure the network is not cyclic (only Node connections can create cycles) - if matches!(new_input, NodeInput::Node { .. }) && !network.is_acyclic() { - self.set_input(input_connector, old_input, network_path); - return; - } - - // It is necessary to ensure the grpah is acyclic before calling `self.position` as it sometimes crashes with cyclic graphs #3227 + // It is necessary to ensure the graph is acyclic before calling `self.position` as it sometimes crashes with cyclic graphs #3227 let previous_metadata = match &previous_input { NodeInput::Node { node_id, .. } => self.position(node_id, network_path).map(|position| (*node_id, position)), _ => None, @@ -4234,7 +4251,7 @@ impl NodeNetworkInterface { // Side effects match (&old_input, &new_input) { // If a node input is exposed or hidden reload the click targets and update the bounding box for all nodes - (NodeInput::Value { exposed: new_exposed, .. }, NodeInput::Value { exposed: old_exposed, .. }) => { + (NodeInput::Value { exposed: old_exposed, .. }, NodeInput::Value { exposed: new_exposed, .. }) => { if let InputConnector::Node { node_id, .. } = input_connector { if new_exposed != old_exposed { self.unload_upstream_node_click_targets(vec![*node_id], network_path); @@ -4640,7 +4657,7 @@ impl NodeNetworkInterface { log::error!("Could not get selected nodes in NodeGraphMessage::DeleteNodes"); return; }; - selected_nodes.retain_selected_nodes(|node_id| !nodes_to_delete.contains(node_id)); + selected_nodes.retain_selected_nodes(|node_id| !delete_nodes.contains(node_id)); } /// Removes all references to the node with the given id from the network, and reconnects the input to the node below. @@ -5049,11 +5066,10 @@ impl NodeNetworkInterface { } } } - // The primary export is disconnected + // The primary export is disconnected, so preview the node with nothing to restore, which disconnects the export again when the preview ends else { - // Set node as export and cancel any preview new_export = Some(OutputConnector::node(toggle_id, 0)); - self.start_previewing_without_restore(network_path); + new_previewing_state = Previewing::Yes { root_node_to_restore: None }; } } match new_export { @@ -5406,8 +5422,8 @@ impl NodeNetworkInterface { log::error!("Could not get outward wires in shift_selected_nodes"); return; }; - if let Some(upstream_node) = outward_wires.first() - && node_ids.contains(&upstream_node.node_id().expect("Stack layer should have downstream layer")) + if let Some(downstream_node_id) = outward_wires.first().and_then(|input_connector| input_connector.node_id()) + && node_ids.contains(&downstream_node_id) { continue; } @@ -6557,13 +6573,11 @@ pub struct NodeNetworkPersistentMetadata { pub previewing: Previewing, // Stores the transform and navigation state for the network pub navigation_metadata: NavigationMetadata, - /// Stack of selection snapshots for previous history states. - // TODO: Use `#[serde(skip)]` here instead? @TrueDoctor claims this isn't valid but hasn't satisfactorily explained how it differs from the situation where `#[serde(default)]` fills in the default value. From brief testing, skip seems to work without issue. - #[serde(default)] + /// Stack of selection snapshots for previous history states. Session state that is not persisted into saved documents. + #[serde(skip)] pub selection_undo_history: VecDeque, /// Stack of selection snapshots for future history states. - // TODO: Use `#[serde(skip)]` here instead? See above. - #[serde(default)] + #[serde(skip)] pub selection_redo_history: VecDeque, }