From 58364166323d77d69b88b756b5670c242b85e0c2 Mon Sep 17 00:00:00 2001 From: Dennis Kobert Date: Thu, 11 Sep 2025 12:08:26 +0200 Subject: [PATCH] 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 --- editor/src/dispatcher.rs | 2 +- .../utility_types/document_metadata.rs | 16 +----- .../utility_types/network_interface.rs | 28 +++++---- .../network_interface/memo_network.rs | 57 +++++++++++++++++++ .../portfolio/document/utility_types/nodes.rs | 4 +- .../tool/common_functionality/snapping.rs | 2 +- editor/src/node_graph_executor.rs | 2 +- node-graph/gcore/src/vector/click_target.rs | 2 +- .../gcore/src/vector/vector_attributes.rs | 5 ++ node-graph/gcore/src/vector/vector_types.rs | 5 ++ node-graph/graph-craft/src/document.rs | 7 +-- node-graph/gsvg-renderer/src/renderer.rs | 4 +- 12 files changed, 97 insertions(+), 37 deletions(-) create mode 100644 editor/src/messages/portfolio/document/utility_types/network_interface/memo_network.rs diff --git a/editor/src/dispatcher.rs b/editor/src/dispatcher.rs index 7e73a54f..94aa76f7 100644 --- a/editor/src/dispatcher.rs +++ b/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)); } } diff --git a/editor/src/messages/portfolio/document/utility_types/document_metadata.rs b/editor/src/messages/portfolio/document/utility_types/document_metadata.rs index acef9702..c2dfd988 100644 --- a/editor/src/messages/portfolio/document/utility_types/document_metadata.rs +++ b/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) } 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 a42a7c5c..53d40ee6 100644 --- a/editor/src/messages/portfolio/document/utility_types/network_interface.rs +++ b/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)) } diff --git a/editor/src/messages/portfolio/document/utility_types/network_interface/memo_network.rs b/editor/src/messages/portfolio/document/utility_types/network_interface/memo_network.rs new file mode 100644 index 00000000..e6f7b126 --- /dev/null +++ b/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>, +} + +impl<'de> serde::Deserialize<'de> for MemoNetwork { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + Ok(Self::new(NodeNetwork::deserialize(deserializer)?)) + } +} + +impl serde::Serialize for MemoNetwork { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + self.network.serialize(serializer) + } +} + +impl Hash for MemoNetwork { + fn hash(&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() + } +} diff --git a/editor/src/messages/portfolio/document/utility_types/nodes.rs b/editor/src/messages/portfolio/document/utility_types/nodes.rs index c120938a..0e70f90f 100644 --- a/editor/src/messages/portfolio/document/utility_types/nodes.rs +++ b/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) -> 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()) } } diff --git a/editor/src/messages/tool/common_functionality/snapping.rs b/editor/src/messages/tool/common_functionality/snapping.rs index 27b658e9..4be02cb5 100644 --- a/editor/src/messages/tool/common_functionality/snapping.rs +++ b/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); diff --git a/editor/src/node_graph_executor.rs b/editor/src/node_graph_executor.rs index ee639991..ff73192e 100644 --- a/editor/src/node_graph_executor.rs +++ b/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, 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(); diff --git a/node-graph/gcore/src/vector/click_target.rs b/node-graph/gcore/src/vector/click_target.rs index 6ab7c186..1b665144 100644 --- a/node-graph/gcore/src/vector/click_target.rs +++ b/node-graph/gcore/src/vector/click_target.rs @@ -40,7 +40,7 @@ pub struct ClickTarget { impl ClickTarget { pub fn new_with_subpath(subpath: Subpath, 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, diff --git a/node-graph/gcore/src/vector/vector_attributes.rs b/node-graph/gcore/src/vector/vector_attributes.rs index 215473e5..2817a756 100644 --- a/node-graph/gcore/src/vector/vector_attributes.rs +++ b/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) diff --git a/node-graph/gcore/src/vector/vector_types.rs b/node-graph/gcore/src/vector/vector_types.rs index 7f26e409..c49b12be 100644 --- a/node-graph/gcore/src/vector/vector_types.rs +++ b/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() diff --git a/node-graph/graph-craft/src/document.rs b/node-graph/graph-craft/src/document.rs index be798346..dc6265a2 100644 --- a/node-graph/graph-craft/src/document.rs +++ b/node-graph/graph-craft/src/document.rs @@ -9,7 +9,7 @@ pub use graphene_core::uuid::NodeId; pub use graphene_core::uuid::generate_uuid; use graphene_core::{Context, ContextDependencies, Cow, MemoHash, ProtoNodeIdentifier, Type}; use log::Metadata; -use rustc_hash::FxHashMap; +use rustc_hash::{FxBuildHasher, FxHashMap}; use std::collections::HashMap; use std::collections::hash_map::DefaultHasher; use std::hash::{Hash, Hasher}; @@ -551,9 +551,8 @@ impl PartialEq for NodeNetwork { /// Graph modification functions impl NodeNetwork { pub fn current_hash(&self) -> u64 { - let mut hasher = DefaultHasher::new(); - self.hash(&mut hasher); - hasher.finish() + use std::hash::BuildHasher; + FxBuildHasher.hash_one(self) } pub fn value_network(node: DocumentNode) -> Self { diff --git a/node-graph/gsvg-renderer/src/renderer.rs b/node-graph/gsvg-renderer/src/renderer.rs index d67b3442..f7b6015f 100644 --- a/node-graph/gsvg-renderer/src/renderer.rs +++ b/node-graph/gsvg-renderer/src/renderer.rs @@ -1117,7 +1117,7 @@ impl Render for Table { // For free-floating anchors, we need to add a click target for each let single_anchors_targets = vector.point_domain.ids().iter().filter_map(|&point_id| { - if vector.connected_count(point_id) == 0 { + if !vector.any_connected(point_id) { let anchor = vector.point_domain.position_from_id(point_id).unwrap_or_default(); let point = FreePoint::new(point_id, anchor); @@ -1162,7 +1162,7 @@ impl Render for Table { // For free-floating anchors, we need to add a click target for each let single_anchors_targets = row.element.point_domain.ids().iter().filter_map(|&point_id| { - if row.element.connected_count(point_id) > 0 { + if row.element.any_connected(point_id) { return None; }