Improve resolved types

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

@@ -7,6 +7,7 @@ pub struct Compiler {}
 impl Compiler {
 	pub fn compile(&self, mut network: NodeNetwork) -> impl Iterator<Item = Result<ProtoNetwork, String>> {
 		let node_ids = network.nodes.keys().copied().collect::<Vec<_>>();
+		network.generate_node_paths(&[]);
 		network.populate_dependants();
 		for id in node_ids {
 			network.flatten(id);
@@ -19,6 +20,9 @@ impl Compiler {
 		proto_networks.map(move |mut proto_network| {
 			proto_network.resolve_inputs()?;
 			proto_network.generate_stable_node_ids();
+			// Deduplication of identical protonodes is more efficient, but also means the type information is lost for the corresponding document node
+			// let mut seen = std::collections::HashSet::new();
+			// proto_network.nodes.retain(|(protonode_id, _)| seen.insert(*protonode_id));
 			Ok(proto_network)
 		})
 	}

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

@@ -193,6 +193,11 @@ impl ProtoNode {
 
 		self.identifier.name.hash(&mut hasher);
 		self.construction_args.hash(&mut hasher);
+		// TODO: This is necessary since a mapping of path to type has to be stored for document node
+		// If this is removed, the identical node would be removed and the mapping wouldn't be added
+		// This can be improved by storing a list of paths for each proto node.
+		self.original_location.path.hash(&mut hasher);
+
 		if self.skip_deduplication {
 			self.original_location.path.hash(&mut hasher);
 		}
@@ -210,25 +215,6 @@ impl ProtoNode {
 		Some(NodeId(hasher.finish()))
 	}
 
-	/// Construct a new [`ProtoNode`] with the specified construction args and a `ClonedNode` implementation.
-	pub fn value(value: ConstructionArgs, path: Vec<NodeId>) -> Self {
-		let inputs_exposed = match &value {
-			ConstructionArgs::Nodes(nodes) => nodes.len() + 1,
-			_ => 2,
-		};
-		Self {
-			identifier: ProtoNodeIdentifier::new("graphene_core::value::ClonedNode"),
-			construction_args: value,
-			input: ProtoNodeInput::ManualComposition(concrete!(Context)),
-			original_location: OriginalLocation {
-				path: Some(path),
-				inputs_exposed: vec![false; inputs_exposed],
-				..Default::default()
-			},
-			skip_deduplication: false,
-		}
-	}
-
 	/// Converts all references to other node IDs into new IDs by running the specified function on them.
 	/// This can be used when changing the IDs of the nodes, for example in the case of generating stable IDs.
 	pub fn map_ids(&mut self, f: impl Fn(NodeId) -> NodeId, skip_lambdas: bool) {
@@ -319,29 +305,6 @@ impl ProtoNetwork {
 		}
 	}
 
-	// TODO: Remove
-	/// Create a hashmap with the list of nodes this proto network depends on/uses as inputs.
-	pub fn collect_inwards_edges(&self) -> HashMap<NodeId, Vec<NodeId>> {
-		let mut edges: HashMap<NodeId, Vec<NodeId>> = HashMap::new();
-		for (id, node) in &self.nodes {
-			match &node.input {
-				ProtoNodeInput::Node(ref_id) | ProtoNodeInput::NodeLambda(ref_id) => {
-					self.check_ref(ref_id, id);
-					edges.entry(*id).or_default().push(*ref_id)
-				}
-				_ => (),
-			}
-
-			if let ConstructionArgs::Nodes(ref_nodes) = &node.construction_args {
-				for (ref_id, _) in ref_nodes {
-					self.check_ref(ref_id, id);
-					edges.entry(*id).or_default().push(*ref_id)
-				}
-			}
-		}
-		edges
-	}
-
 	fn collect_inwards_edges_with_mapping(&self) -> (Vec<Vec<usize>>, FxHashMap<NodeId, usize>) {
 		let id_map: FxHashMap<_, _> = self.nodes.iter().enumerate().map(|(idx, (id, _))| (*id, idx)).collect();
 
@@ -477,9 +440,22 @@ impl ProtoNetwork {
 	fn is_topologically_sorted(&self) -> bool {
 		let mut visited = HashSet::new();
 
-		let inwards_edges = self.collect_inwards_edges();
-		for (id, _) in &self.nodes {
-			for &dependency in inwards_edges.get(id).unwrap_or(&Vec::new()) {
+		for (id, node) in &self.nodes {
+			let mut upstream_nodes = Vec::new();
+			match &node.input {
+				ProtoNodeInput::Node(ref_id) | ProtoNodeInput::NodeLambda(ref_id) => {
+					upstream_nodes.push(*ref_id);
+				}
+				_ => (),
+			}
+
+			if let ConstructionArgs::Nodes(ref_nodes) = &node.construction_args {
+				for (ref_id, _) in ref_nodes {
+					upstream_nodes.push(*ref_id);
+				}
+			}
+
+			for dependency in upstream_nodes {
 				if !visited.contains(&dependency) {
 					dbg!(id, dependency);
 					dbg!(&visited);

node-graph/interpreted-executor/src/dynamic_executor.rs

@@ -18,8 +18,6 @@ pub struct DynamicExecutor {
 	tree: BorrowTree,
 	/// Stores the types of the proto nodes.
 	typing_context: TypingContext,
-	// This allows us to keep the nodes around for one more frame which is used for introspection
-	orphaned_nodes: HashSet<NodeId>,
 }
 
 impl Default for DynamicExecutor {
@@ -28,7 +26,6 @@ impl Default for DynamicExecutor {
 			output: Default::default(),
 			tree: Default::default(),
 			typing_context: TypingContext::new(&node_registry::NODE_REGISTRY),
-			orphaned_nodes: HashSet::new(),
 		}
 	}
 }
@@ -59,12 +56,7 @@ impl DynamicExecutor {
 		let output = proto_network.output;
 		let tree = BorrowTree::new(proto_network, &typing_context).await?;
 
-		Ok(Self {
-			tree,
-			output,
-			typing_context,
-			orphaned_nodes: HashSet::new(),
-		})
+		Ok(Self { tree, output, typing_context })
 	}
 
 	/// Updates the existing [`BorrowTree`] to reflect the new [`ProtoNetwork`], reusing nodes where possible.
@@ -72,16 +64,13 @@ impl DynamicExecutor {
 	pub async fn update(&mut self, proto_network: ProtoNetwork) -> Result<ResolvedDocumentNodeTypesDelta, GraphErrors> {
 		self.output = proto_network.output;
 		self.typing_context.update(&proto_network)?;
-		let (add, orphaned) = self.tree.update(proto_network, &self.typing_context).await?;
-		let old_to_remove = core::mem::replace(&mut self.orphaned_nodes, orphaned);
-		let mut remove = Vec::with_capacity(old_to_remove.len() - self.orphaned_nodes.len().min(old_to_remove.len()));
-		for node_id in old_to_remove {
-			if self.orphaned_nodes.contains(&node_id) {
-				let path = self.tree.free_node(node_id);
-				self.typing_context.remove_inference(node_id);
-				if let Some(path) = path {
-					remove.push(path);
-				}
+		let (add, orphaned_proto_nodes) = self.tree.update(proto_network, &self.typing_context).await?;
+		let mut remove = Vec::new();
+		for node_id in orphaned_proto_nodes {
+			let path = self.tree.free_node(node_id);
+			self.typing_context.remove_inference(node_id);
+			if let Some(path) = path {
+				remove.push(path);
 			}
 		}
 		let add = self.document_node_types(add.into_iter()).collect();
@@ -111,8 +100,6 @@ impl DynamicExecutor {
 
 	pub fn document_node_types<'a>(&'a self, nodes: impl Iterator<Item = Path> + 'a) -> impl Iterator<Item = (Path, NodeTypes)> + 'a {
 		nodes.flat_map(|id| self.tree.source_map().get(&id).map(|(_, b)| (id, b.clone())))
-		// TODO: https://github.com/GraphiteEditor/Graphite/issues/1767
-		// TODO: Non exposed inputs are not added to the inputs_source_map, so they are not included in the resolved_document_node_types. The type is still available in the typing_context. This only affects the UI-only "Import" node.
 	}
 }
 
@@ -193,21 +180,28 @@ impl BorrowTree {
 		Ok(nodes)
 	}
 
-	/// Pushes new nodes into the tree and return orphaned nodes
+	/// Pushes new nodes into the tree and returns a vec of nodes that had their types changed, and a vec of nodes that were removed
+	/// Does not return cloned nodes in the vec of added nodes, since their types are not needed by the editor
 	pub async fn update(&mut self, proto_network: ProtoNetwork, typing_context: &TypingContext) -> Result<(Vec<Path>, HashSet<NodeId>), GraphErrors> {
-		let mut old_nodes: HashSet<_> = self.nodes.keys().copied().collect();
-		let mut new_nodes: Vec<_> = Vec::new();
-		// TODO: Problem: When an identity node is connected directly to an export the first input to identity node is not added to the proto network, while the second input is. This means the primary input does not have a type.
+		let mut old_nodes = self.nodes.keys().copied().collect::<HashSet<_>>();
+		let mut nodes_with_new_type: Vec<_> = Vec::new();
 		for (id, node) in proto_network.nodes {
+			let is_cloned_node = node.identifier == "graphene_core::value::ClonedNode".into();
+			let node_path = node.original_location.path.clone().unwrap_or_default().into();
+			// A document node is either added, or changed so that a new stable node id was regenerated
 			if !self.nodes.contains_key(&id) {
-				new_nodes.push(node.original_location.path.clone().unwrap_or_default().into());
+				// If the path changes, then the id must change as well, which means this will update the mapping
+				// Returns whether the type changed for the document node path
+				if !is_cloned_node && self.update_source_map(id, typing_context, &node) {
+					nodes_with_new_type.push(node_path);
+				}
 				self.push_node(id, node, typing_context).await?;
-			} else if self.update_source_map(id, typing_context, &node) {
-				new_nodes.push(node.original_location.path.clone().unwrap_or_default().into());
 			}
+
 			old_nodes.remove(&id);
 		}
-		Ok((new_nodes, old_nodes))
+
+		Ok((nodes_with_new_type, old_nodes))
 	}
 
 	fn node_deps(&self, nodes: &[NodeId]) -> Vec<SharedNodeContainer> {
@@ -308,9 +302,13 @@ impl BorrowTree {
 	pub fn free_node(&mut self, id: NodeId) -> Option<Path> {
 		let (_, path) = self.nodes.remove(&id)?;
 		if self.source_map.get(&path)?.0 == id {
+			// This occurs if the protonode got removed, and a new protonode with the same path was not created.
+			// This occurs when the document node got deleted.
 			self.source_map.remove(&path);
 			return Some(path);
 		}
+		// The proto node id changed for a document node, but the source map was already updated
+		// This occurs when an input changes for example
 		None
 	}
 
@@ -335,7 +333,7 @@ impl BorrowTree {
 	/// - Updates or inserts an entry in the `source_map` HashMap.
 	/// - Uses the `ProtoNode`'s original location path as the key for the source map.
 	/// - Collects input types from both the main input and parameters.
-	/// - Returns `false` and logs a warning if the node's type information is not found in the typing context.
+	/// - Returns `false` if the old type is the same as the new type
 	fn update_source_map(&mut self, id: NodeId, typing_context: &TypingContext, proto_node: &ProtoNode) -> bool {
 		let Some(node_io) = typing_context.type_of(id) else {
 			log::warn!("did not find type");
@@ -354,9 +352,10 @@ impl BorrowTree {
 				output: node_io.return_value.clone(),
 			},
 		);
-		let modified = *entry != update;
+
+		let type_changed = entry.1 != update.1;
 		*entry = update;
-		modified
+		return type_changed;
 	}
 
 	/// Inserts a new node into the [`BorrowTree`], calling the constructor function from `node_registry.rs`.
@@ -418,7 +417,12 @@ mod test {
 	#[test]
 	fn push_node_sync() {
 		let mut tree = BorrowTree::default();
-		let val_1_protonode = ProtoNode::value(ConstructionArgs::Value(TaggedValue::U32(2u32).into()), vec![]);
+		let val_1_protonode = ProtoNode {
+			identifier: ProtoNodeIdentifier::new("graphene_core::value::ClonedNode"),
+			construction_args: ConstructionArgs::Value(TaggedValue::U32(2u32).into()),
+			input: ProtoNodeInput::ManualComposition(concrete!(Context)),
+			..Default::default()
+		};
 		let context = TypingContext::default();
 		let future = tree.push_node(NodeId(0), val_1_protonode, &context);
 		futures::executor::block_on(future).unwrap();