Optimize editor performance for node selection, click target bounds, and batched messages (#3162)

* Don't clone messages during batch processing

* Improve selected nodes perf and memoize network hash computation

* Reuse click target bounding boxes for document bounds

* Early terminate computing the connected count

* Cleanup

Author: TrueDoctor

editor/src/dispatcher.rs

@@ -236,7 +236,7 @@ impl Dispatcher {
 				}
 				Message::NoOp => {}
 				Message::Batched { messages } => {
-					messages.iter().for_each(|message| self.handle_message(message.to_owned(), false));
+					messages.into_iter().for_each(|message| self.handle_message(message, false));
 				}
 			}
 

editor/src/messages/portfolio/document/utility_types/document_metadata.rs

@@ -154,21 +154,7 @@ impl DocumentMetadata {
 	pub fn bounding_box_with_transform(&self, layer: LayerNodeIdentifier, transform: DAffine2) -> Option<[DVec2; 2]> {
 		self.click_targets(layer)?
 			.iter()
-			.filter_map(|click_target| match click_target.target_type() {
-				ClickTargetType::Subpath(subpath) => subpath.bounding_box_with_transform(transform),
-				ClickTargetType::FreePoint(_) => click_target.bounding_box_with_transform(transform),
-			})
-			.reduce(Quad::combine_bounds)
-	}
-
-	/// Get the loose bounding box of the click target of the specified layer in the specified transform space
-	pub fn loose_bounding_box_with_transform(&self, layer: LayerNodeIdentifier, transform: DAffine2) -> Option<[DVec2; 2]> {
-		self.click_targets(layer)?
-			.iter()
-			.filter_map(|click_target| match click_target.target_type() {
-				ClickTargetType::Subpath(subpath) => subpath.loose_bounding_box_with_transform(transform),
-				ClickTargetType::FreePoint(_) => click_target.bounding_box_with_transform(transform),
-			})
+			.filter_map(|click_target| click_target.bounding_box_with_transform(transform))
 			.reduce(Quad::combine_bounds)
 	}
 

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

@@ -1,4 +1,5 @@
 mod deserialization;
+mod memo_network;
 
 use super::document_metadata::{DocumentMetadata, LayerNodeIdentifier, NodeRelations};
 use super::misc::PTZ;
@@ -26,6 +27,7 @@ use graphene_std::vector::{PointId, Vector, VectorModificationType};
 use interpreted_executor::dynamic_executor::ResolvedDocumentNodeTypes;
 use interpreted_executor::node_registry::NODE_REGISTRY;
 use kurbo::BezPath;
+use memo_network::MemoNetwork;
 use serde_json::{Value, json};
 use std::collections::{HashMap, HashSet, VecDeque};
 use std::hash::{DefaultHasher, Hash, Hasher};
@@ -36,7 +38,7 @@ use std::ops::Deref;
 pub struct NodeNetworkInterface {
 	/// The node graph that generates this document's artwork. It recursively stores its sub-graphs, so this root graph is the whole snapshot of the document content.
 	/// A public mutable reference should never be created. It should only be mutated through custom setters which perform the necessary side effects to keep network_metadata in sync
-	network: NodeNetwork,
+	network: MemoNetwork,
 	/// Stores all editor information for a NodeNetwork. Should automatically kept in sync by the setter methods when changes to the document network are made.
 	network_metadata: NodeNetworkMetadata,
 	// TODO: Wrap in TransientMetadata Option
@@ -71,7 +73,7 @@ impl PartialEq for NodeNetworkInterface {
 impl NodeNetworkInterface {
 	/// Add DocumentNodePath input to the PathModifyNode protonode
 	pub fn migrate_path_modify_node(&mut self) {
-		fix_network(&mut self.network);
+		fix_network(self.document_network_mut());
 		fn fix_network(network: &mut NodeNetwork) {
 			for node in network.nodes.values_mut() {
 				if let Some(network) = node.implementation.get_network_mut() {
@@ -91,18 +93,25 @@ impl NodeNetworkInterface {
 impl NodeNetworkInterface {
 	/// Gets the network of the root document
 	pub fn document_network(&self) -> &NodeNetwork {
-		&self.network
+		self.network.network()
+	}
+	pub fn document_network_mut(&mut self) -> &mut NodeNetwork {
+		self.network.network_mut()
 	}
 
 	/// Gets the nested network based on network_path
 	pub fn nested_network(&self, network_path: &[NodeId]) -> Option<&NodeNetwork> {
-		let Some(network) = self.network.nested_network(network_path) else {
+		let Some(network) = self.document_network().nested_network(network_path) else {
 			log::error!("Could not get nested network with path {network_path:?} in NodeNetworkInterface::network");
 			return None;
 		};
 		Some(network)
 	}
 
+	pub fn network_hash(&self) -> u64 {
+		self.network.current_hash()
+	}
+
 	/// Get the specified document node in the nested network based on node_id and network_path
 	pub fn document_node(&self, node_id: &NodeId, network_path: &[NodeId]) -> Option<&DocumentNode> {
 		let network = self.nested_network(network_path)?;
@@ -161,7 +170,7 @@ impl NodeNetworkInterface {
 				.back()
 				.cloned()
 				.unwrap_or_default()
-				.filtered_selected_nodes(network_metadata.persistent_metadata.node_metadata.keys().cloned().collect()),
+				.filtered_selected_nodes(|node_id| network_metadata.persistent_metadata.node_metadata.contains_key(node_id)),
 		)
 	}
 
@@ -1556,7 +1565,7 @@ impl NodeNetworkInterface {
 			log::error!("Could not get network or network_metadata in upstream_flow_back_from_nodes");
 			return FlowIter {
 				stack: Vec::new(),
-				network: &self.network,
+				network: &self.document_network(),
 				network_metadata: &self.network_metadata,
 				flow_type: FlowType::UpstreamFlow,
 			};
@@ -1708,7 +1717,7 @@ impl NodeNetworkInterface {
 			}
 		}
 		Self {
-			network: node_network,
+			network: MemoNetwork::new(node_network),
 			network_metadata,
 			document_metadata: DocumentMetadata::default(),
 			resolved_types: ResolvedDocumentNodeTypes::default(),
@@ -1744,7 +1753,7 @@ fn random_protonode_implementation(protonode: &graph_craft::ProtoNodeIdentifier)
 // Private mutable getters for use within the network interface
 impl NodeNetworkInterface {
 	fn network_mut(&mut self, network_path: &[NodeId]) -> Option<&mut NodeNetwork> {
-		self.network.nested_network_mut(network_path)
+		self.document_network_mut().nested_network_mut(network_path)
 	}
 
 	fn network_metadata_mut(&mut self, network_path: &[NodeId]) -> Option<&mut NodeNetworkMetadata> {
@@ -3497,8 +3506,7 @@ impl NodeNetworkInterface {
 		}
 
 		self.document_metadata
-			.click_targets
-			.get(&layer)
+			.click_targets(layer)
 			.map(|click| click.iter().map(ClickTarget::target_type))
 			.map(|target_types| Vector::from_target_types(target_types, true))
 	}

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

@@ -0,0 +1,57 @@
+use graph_craft::document::NodeNetwork;
+use std::cell::Cell;
+use std::hash::{Hash, Hasher};
+
+#[derive(Debug, Default, Clone, PartialEq)]
+pub struct MemoNetwork {
+	network: NodeNetwork,
+	hash_code: Cell<Option<u64>>,
+}
+
+impl<'de> serde::Deserialize<'de> for MemoNetwork {
+	fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+	where
+		D: serde::Deserializer<'de>,
+	{
+		Ok(Self::new(NodeNetwork::deserialize(deserializer)?))
+	}
+}
+
+impl serde::Serialize for MemoNetwork {
+	fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+	where
+		S: serde::Serializer,
+	{
+		self.network.serialize(serializer)
+	}
+}
+
+impl Hash for MemoNetwork {
+	fn hash<H: Hasher>(&self, state: &mut H) {
+		self.current_hash().hash(state);
+	}
+}
+
+impl MemoNetwork {
+	pub fn network(&self) -> &NodeNetwork {
+		&self.network
+	}
+
+	pub fn network_mut(&mut self) -> &mut NodeNetwork {
+		self.hash_code.set(None);
+		&mut self.network
+	}
+
+	pub fn new(network: NodeNetwork) -> Self {
+		Self { network, hash_code: None.into() }
+	}
+
+	pub fn current_hash(&self) -> u64 {
+		let mut hash_code = self.hash_code.get();
+		if hash_code.is_none() {
+			hash_code = Some(self.network.current_hash());
+			self.hash_code.set(hash_code);
+		}
+		hash_code.unwrap()
+	}
+}

editor/src/messages/portfolio/document/utility_types/nodes.rs

@@ -167,8 +167,8 @@ impl SelectedNodes {
 		std::mem::replace(&mut self.0, new)
 	}
 
-	pub fn filtered_selected_nodes(&self, node_ids: std::collections::HashSet<NodeId>) -> SelectedNodes {
-		SelectedNodes(self.0.iter().filter(|node_id| node_ids.contains(node_id)).cloned().collect())
+	pub fn filtered_selected_nodes(&self, filter: impl Fn(&NodeId) -> bool) -> SelectedNodes {
+		SelectedNodes(self.0.iter().copied().filter(filter).collect())
 	}
 }
 

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

@@ -333,7 +333,7 @@ impl SnapManager {
 			return;
 		}
 		// We use a loose bounding box here since these are potential candidates which will be filtered later anyway
-		let Some(bounds) = document.metadata().loose_bounding_box_with_transform(layer, DAffine2::IDENTITY) else {
+		let Some(bounds) = document.metadata().bounding_box_with_transform(layer, DAffine2::IDENTITY) else {
 			return;
 		};
 		let layer_bounds = document.metadata().transform_to_document(layer) * Quad::from_box(bounds);

editor/src/node_graph_executor.rs

@@ -115,7 +115,7 @@ impl NodeGraphExecutor {
 
 	/// Update the cached network if necessary.
 	fn update_node_graph(&mut self, document: &mut DocumentMessageHandler, node_to_inspect: Option<NodeId>, ignore_hash: bool) -> Result<(), String> {
-		let network_hash = document.network_interface.document_network().current_hash();
+		let network_hash = document.network_interface.network_hash();
 		// Refresh the graph when it changes or the inspect node changes
 		if network_hash != self.node_graph_hash || self.previous_node_to_inspect != node_to_inspect || ignore_hash {
 			let network = document.network_interface.document_network().clone();

node-graph/gcore/src/vector/click_target.rs

@@ -40,7 +40,7 @@ pub struct ClickTarget {
 
 impl ClickTarget {
 	pub fn new_with_subpath(subpath: Subpath<PointId>, stroke_width: f64) -> Self {
-		let bounding_box = subpath.loose_bounding_box();
+		let bounding_box = subpath.bounding_box();
 		Self {
 			target_type: ClickTargetType::Subpath(subpath),
 			stroke_width,

node-graph/gcore/src/vector/vector_attributes.rs

@@ -426,6 +426,11 @@ impl SegmentDomain {
 		self.all_connected(point).count()
 	}
 
+	/// Enumerate the number of segments connected to a point. If a segment starts and ends at a point then it is counted twice.
+	pub(crate) fn any_connected(&self, point: usize) -> bool {
+		self.all_connected(point).next().is_some()
+	}
+
 	/// Iterates over segments in the domain.
 	///
 	/// Tuple is: (id, start point, end point, handles)

node-graph/gcore/src/vector/vector_types.rs

@@ -322,6 +322,11 @@ impl Vector {
 		self.point_domain.resolve_id(point).map_or(0, |point| self.segment_domain.connected_count(point))
 	}
 
+	/// Enumerate the number of segments connected to a point. If a segment starts and ends at a point then it is counted twice.
+	pub fn any_connected(&self, point: PointId) -> bool {
+		self.point_domain.resolve_id(point).is_some_and(|point| self.segment_domain.any_connected(point))
+	}
+
 	pub fn check_point_inside_shape(&self, transform: DAffine2, point: DVec2) -> bool {
 		let number = self
 			.stroke_bezpath_iter()