Adopt batch-scoped delta construction with shared removal and resource context, and carry visibility as a structural delta

editor/src/messages/portfolio/document/utility_types/network_interface/editor_delta.rs

@@ -23,24 +23,67 @@ pub struct ConstructedOps {
 	pub declaration_bytes: DeclarationBytes,
 }
 
-impl EditorDelta {
+pub fn construct_batch(deltas: &[EditorDelta], working: &Registry, resources: &ResourceRegistry, peer: document_graph_storage::PeerId) -> Result<ConstructedOps, ConversionError> {
-	pub fn to_registry_deltas(&self, working: &Registry, resources: &ResourceRegistry, peer: document_graph_storage::PeerId) -> Result<ConstructedOps, ConversionError> {
+	let resolver = PathResolver::new(peer);
-		let resolver = PathResolver::new(peer);
+	let mut ops = Vec::new();
-		let mut ops = Vec::new();
+	let mut declaration_bytes = DeclarationBytes::new();
-		let mut declaration_bytes = DeclarationBytes::new();
+	let mut batch_removed_nodes = Vec::new();
+	let mut batch_removed_networks = Vec::new();
+	let mut batch_added_resources = HashSet::new();
 
+	for delta in deltas {
+		if let EditorDelta::Graph(RuntimeDelta::RemoveNode { network_path, node_id } | RuntimeDelta::ReplaceNode { network_path, node_id, .. }) = delta {
+			collect_removal_closure(resolver.node_id(network_path, *node_id), working, &mut batch_removed_nodes, &mut batch_removed_networks);
+		}
+	}
+	batch_removed_nodes.sort();
+	batch_removed_nodes.dedup();
+	batch_removed_networks.sort();
+	batch_removed_networks.dedup();
+
+	for delta in deltas {
+		delta.construct(working, resources, peer, &resolver, &batch_removed_nodes, &mut batch_added_resources, &mut ops, &mut declaration_bytes)?;
+	}
+
+	construct_resource_removals(&batch_removed_nodes, working, &mut ops);
+	Ok(ConstructedOps { ops, declaration_bytes })
+}
+
+impl EditorDelta {
+	#[allow(clippy::too_many_arguments)]
+	fn construct(
+		&self,
+		working: &Registry,
+		resources: &ResourceRegistry,
+		peer: document_graph_storage::PeerId,
+		resolver: &PathResolver,
+		batch_removed_nodes: &[document_graph_storage::NodeId],
+		batch_added_resources: &mut HashSet<ResourceId>,
+		ops: &mut Vec<RegistryDelta>,
+		declaration_bytes: &mut DeclarationBytes,
+	) -> Result<(), ConversionError> {
 		match self {
 			EditorDelta::Graph(RuntimeDelta::AddNode { network_path, node_id, node }) => {
-				declaration_bytes = construct_structural_additions(network_path, *node_id, node, working, resources, peer, &mut ops)?;
+				construct_structural_additions(network_path, *node_id, node, working, resources, peer, batch_added_resources, ops, declaration_bytes)?;
 			}
 
 			EditorDelta::Graph(RuntimeDelta::ReplaceNode { network_path, node_id, node }) => {
-				construct_removals(resolver.node_id(network_path, *node_id), working, &mut ops);
+				construct_removals(resolver.node_id(network_path, *node_id), working, ops);
-				declaration_bytes = construct_structural_additions(network_path, *node_id, node, working, resources, peer, &mut ops)?;
+				construct_structural_additions(network_path, *node_id, node, working, resources, peer, batch_added_resources, ops, declaration_bytes)?;
 			}
 
 			EditorDelta::Graph(RuntimeDelta::RemoveNode { network_path, node_id }) => {
-				construct_removals(resolver.node_id(network_path, *node_id), working, &mut ops);
+				construct_removals(resolver.node_id(network_path, *node_id), working, ops);
+			}
+
+			EditorDelta::Graph(RuntimeDelta::SetVisibility { network_path, node_id, visible }) => {
+				ops.push(RegistryDelta::ChangeNodeAttribute {
+					id: resolver.node_id(network_path, *node_id),
+					delta: AttributeDelta {
+						key: node_attr::VISIBLE.to_string(),
+						value: (!visible).then_some(serde_json::Value::Bool(false)),
+					},
+				});
 			}
 
 			EditorDelta::Graph(RuntimeDelta::SetInput {
@@ -65,14 +108,16 @@ impl EditorDelta {
 			}
 
 			EditorDelta::NodeMetadata { network_path, node_id, metadata } => {
-				construct_metadata_changes(network_path, *node_id, metadata, working, &resolver, &mut ops)?;
+				construct_metadata_changes(network_path, *node_id, metadata, working, resolver, ops)?;
 			}
 		}
 
-		Ok(ConstructedOps { ops, declaration_bytes })
+		let _ = batch_removed_nodes;
+		Ok(())
 	}
 }
 
+#[allow(clippy::too_many_arguments)]
 fn construct_structural_additions(
 	network_path: &[NodeId],
 	node_id: NodeId,
@@ -80,12 +125,14 @@ fn construct_structural_additions(
 	working: &Registry,
 	resources: &ResourceRegistry,
 	peer: document_graph_storage::PeerId,
+	batch_added_resources: &mut HashSet<ResourceId>,
 	ops: &mut Vec<RegistryDelta>,
-) -> Result<DeclarationBytes, ConversionError> {
+	declaration_bytes: &mut DeclarationBytes,
+) -> Result<(), ConversionError> {
 	let mut scoped = ScopedConversion::new(&NoMetadata, peer);
 	let mut scratch = Registry::default();
 	scoped.convert_node_at(&mut scratch, network_path, node_id, node, true)?;
-	let declaration_bytes = scoped.finish();
+	declaration_bytes.extend(scoped.finish());
 
 	let mut networks: Vec<_> = scratch.networks.iter().collect();
 	networks.sort_by_key(|(id, _)| **id);
@@ -102,7 +149,9 @@ fn construct_structural_additions(
 	let mut new_resources: Vec<_> = scratch.resources.iter().filter(|(id, _)| !working.resources.contains_key(id)).collect();
 	new_resources.sort_by_key(|(id, _)| **id);
 	for (id, entry) in new_resources {
-		ops.push(RegistryDelta::AddResource { id: *id, entry: entry.clone() });
+		if batch_added_resources.insert(*id) {
+			ops.push(RegistryDelta::AddResource { id: *id, entry: entry.clone() });
+		}
 	}
 	let mut tagged: Vec<ResourceId> = scratch.node_instances.values().flat_map(node_value_resource_refs).collect();
 	tagged.sort();
@@ -110,13 +159,15 @@ fn construct_structural_additions(
 	for id in tagged {
 		if !working.resources.contains_key(&id)
 			&& !scratch.resources.contains_key(&id)
+			&& !batch_added_resources.contains(&id)
 			&& let Some(entry) = convert_resource_entry(resources, id, peer)?
 		{
+			batch_added_resources.insert(id);
 			ops.push(RegistryDelta::AddResource { id, entry });
 		}
 	}
 
-	Ok(declaration_bytes)
+	Ok(())
 }
 
 fn construct_metadata_changes(
@@ -231,9 +282,11 @@ fn construct_removals(node_id: document_graph_storage::NodeId, working: &Registr
 			snapshot: working.networks[id].clone(),
 		});
 	}
+}
 
-	let removed_node_set: HashSet<_> = removed_nodes.iter().copied().collect();
+fn construct_resource_removals(batch_removed_nodes: &[document_graph_storage::NodeId], working: &Registry, ops: &mut Vec<RegistryDelta>) {
-	let mut candidates: Vec<ResourceId> = removed_nodes
+	let removed_node_set: HashSet<_> = batch_removed_nodes.iter().copied().collect();
+	let mut candidates: Vec<ResourceId> = batch_removed_nodes
 		.iter()
 		.flat_map(|id| {
 			let node = &working.node_instances[id];

editor/src/messages/portfolio/document/utility_types/network_interface/editor_delta_tests.rs

@@ -1,5 +1,5 @@
 use super::InputConnector;
-use super::editor_delta::EditorDelta;
+use super::editor_delta::{EditorDelta, construct_batch};
 use super::storage_metadata::StorageMetadataView;
 use crate::test_utils::test_prelude::*;
 use document_graph_storage::delta::compute_deltas;
@@ -27,9 +27,8 @@ fn convert(editor: &EditorTestUtils) -> Registry {
 	.registry
 }
 
-fn construct(editor: &EditorTestUtils, delta: &EditorDelta, working: &Registry) -> Vec<RegistryDelta> {
+fn construct(editor: &EditorTestUtils, deltas: &[EditorDelta], working: &Registry) -> Vec<RegistryDelta> {
-	delta
+	construct_batch(deltas, working, &editor.active_document().resources.registry, PEER)
-		.to_registry_deltas(working, &editor.active_document().resources.registry, PEER)
 		.expect("construction should succeed")
 		.ops
 }
@@ -84,7 +83,7 @@ async fn set_input_value_constructs_the_exact_diff_op() {
 		input,
 	});
 
-	let constructed = construct(&editor, &delta, &working);
+	let constructed = construct(&editor, std::slice::from_ref(&delta), &working);
 	let diffed = compute_deltas(&working, &convert(&editor));
 	assert_eq!(constructed, diffed, "A value edit should construct exactly the diff's op");
 }
@@ -105,7 +104,7 @@ async fn wiring_and_export_edits_construct_the_exact_diff_ops() {
 		input_index: 1,
 		input: wire,
 	});
-	let constructed = construct(&editor, &delta, &working);
+	let constructed = construct(&editor, std::slice::from_ref(&delta), &working);
 	let diffed = compute_deltas(&working, &convert(&editor));
 	assert_eq!(constructed, diffed, "A wiring edit should construct exactly the diff's op");
 
@@ -117,7 +116,7 @@ async fn wiring_and_export_edits_construct_the_exact_diff_ops() {
 		export_index: 0,
 		input: export,
 	});
-	let constructed = construct(&editor, &delta, &working);
+	let constructed = construct(&editor, std::slice::from_ref(&delta), &working);
 	let diffed = compute_deltas(&working, &convert(&editor));
 	assert_eq!(constructed, diffed, "An export edit should construct exactly the diff's op");
 }
@@ -148,7 +147,7 @@ async fn adding_a_node_as_structure_plus_metadata_matches_the_diff() {
 		},
 	];
 
-	let constructed = deltas.iter().flat_map(|delta| construct(&editor, delta, &working)).collect();
+	let constructed = construct(&editor, &deltas, &working);
 	let diffed = compute_deltas(&working, &convert(&editor));
 	assert_same_stored_effect(&working, constructed, diffed, "node addition");
 }
@@ -163,12 +162,21 @@ async fn metadata_edits_construct_the_exact_diff_ops() {
 	{
 		let network_interface = &mut editor.active_document_mut().network_interface;
 		network_interface.set_display_name(&node, "Renamed".to_string(), &[]);
+		network_interface.set_visibility(&node, &[], false);
 		network_interface.set_locked(&node, &[], true);
 		network_interface.set_pinned(&node, &[], true);
 		network_interface.shift_node(&node, glam::IVec2::new(3, 5), &[]);
 	}
 
-	let constructed = construct(&editor, &node_metadata_delta(&editor, node), &working);
+	let deltas = [
+		node_metadata_delta(&editor, node),
+		EditorDelta::Graph(RuntimeDelta::SetVisibility {
+			network_path: Vec::new(),
+			node_id: node,
+			visible: false,
+		}),
+	];
+	let constructed = construct(&editor, &deltas, &working);
 	let diffed = compute_deltas(&working, &convert(&editor));
 	assert_eq!(constructed, diffed, "Metadata edits should construct exactly the diff's attribute ops");
 }
@@ -227,12 +235,7 @@ async fn removing_a_nested_network_node_matches_the_diff() {
 		}
 	}
 
-	let without_resource_ops = |ops: Vec<RegistryDelta>| {
+	let constructed = construct(&editor, &deltas, &working);
-		ops.into_iter()
+	let diffed = compute_deltas(&working, &convert(&editor));
-			.filter(|op| !matches!(op, RegistryDelta::RemoveResource { .. } | RegistryDelta::AddResource { .. }))
-			.collect::<Vec<_>>()
-	};
-	let constructed = without_resource_ops(deltas.iter().flat_map(|delta| construct(&editor, delta, &working)).collect());
-	let diffed = without_resource_ops(compute_deltas(&working, &convert(&editor)));
 	assert_same_stored_effect(&working, constructed, diffed, "nested network removal");
 }

node-graph/graph-craft/src/runtime_delta.rs

@@ -27,4 +27,9 @@ pub enum RuntimeDelta {
 		export_index: usize,
 		input: NodeInput,
 	},
+	SetVisibility {
+		network_path: Vec<NodeId>,
+		node_id: NodeId,
+		visible: bool,
+	},
 }