Bug fixes and code review

Author: adamgerhant

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

@@ -366,23 +366,23 @@ impl<'a> ModifyInputsContext<'a> {
 	}
 
 	pub fn blend_mode_set(&mut self, blend_mode: BlendMode) {
-		let Some(blend_node_id) = self.existing_node_id(&DefinitionIdentifier::ProtoNode(graphene_std::raster_nodes::blending_nodes::blend::IDENTIFIER), true) else {
+		let Some(blend_node_id) = self.existing_node_id(&DefinitionIdentifier::ProtoNode(graphene_std::blending_nodes::blending::IDENTIFIER), true) else {
 			return;
 		};
 		let input_connector = InputConnector::node(blend_node_id, 1);
 		self.set_input_with_refresh(input_connector, NodeInput::value(TaggedValue::BlendMode(blend_mode), false), false);
 	}
 
 	pub fn opacity_set(&mut self, opacity: f64) {
-		let Some(blend_node_id) = self.existing_node_id(&DefinitionIdentifier::ProtoNode(graphene_std::raster_nodes::blending_nodes::blend::IDENTIFIER), true) else {
+		let Some(blend_node_id) = self.existing_node_id(&DefinitionIdentifier::ProtoNode(graphene_std::blending_nodes::blending::IDENTIFIER), true) else {
 			return;
 		};
 		let input_connector = InputConnector::node(blend_node_id, 2);
 		self.set_input_with_refresh(input_connector, NodeInput::value(TaggedValue::F64(opacity * 100.), false), false);
 	}
 
 	pub fn blending_fill_set(&mut self, fill: f64) {
-		let Some(blend_node_id) = self.existing_node_id(&DefinitionIdentifier::ProtoNode(graphene_std::raster_nodes::blending_nodes::blend::IDENTIFIER), true) else {
+		let Some(blend_node_id) = self.existing_node_id(&DefinitionIdentifier::ProtoNode(graphene_std::blending_nodes::blending::IDENTIFIER), true) else {
 			return;
 		};
 		let input_connector = InputConnector::node(blend_node_id, 3);
@@ -391,7 +391,7 @@ impl<'a> ModifyInputsContext<'a> {
 
 	pub fn clip_mode_toggle(&mut self, clip_mode: Option<bool>) {
 		let clip = !clip_mode.unwrap_or(false);
-		let Some(clip_node_id) = self.existing_node_id(&DefinitionIdentifier::ProtoNode(graphene_std::raster_nodes::blending_nodes::blend::IDENTIFIER), true) else {
+		let Some(clip_node_id) = self.existing_node_id(&DefinitionIdentifier::ProtoNode(graphene_std::blending_nodes::blending::IDENTIFIER), true) else {
 			return;
 		};
 		let input_connector = InputConnector::node(clip_node_id, 4);

editor/src/messages/portfolio/document/node_graph/document_node_definitions.rs

@@ -95,7 +95,7 @@ impl From<Value> for DefinitionIdentifier {
 /// TODO: Use this to prevent storing a copy of the implementation, if the document node is unchanged from the definition.
 #[derive(Debug, Clone)]
 pub struct DocumentNodeDefinition {
-	/// Used to create the DefinitionIdentifier::Network identifer
+	/// Used to create the DefinitionIdentifier::Network identifier
 	pub identifier: &'static str,
 
 	/// All data required to construct a [`DocumentNode`] and [`DocumentNodeMetadata`]
@@ -113,12 +113,13 @@ pub struct DocumentNodeDefinition {
 	pub properties: Option<&'static str>,
 }
 
-// We use the once cell to use the document node definitions through the editor without passing a reference
+// We use the once cell to use the document node definitions throughout the editor without passing a reference
 // TODO: If dynamic node library is required, use a Mutex as well
 static DOCUMENT_NODE_TYPES: once_cell::sync::Lazy<HashMap<DefinitionIdentifier, DocumentNodeDefinition>> = once_cell::sync::Lazy::new(node_definitions);
 
 /// Defines the "signature" or "header file"-like metadata for the document nodes, but not the implementation (which is defined in the node registry).
 /// The [`DocumentNode`] is the instance while these [`DocumentNodeDefinition`]s are the "classes" or "blueprints" from which the instances are built.
+/// Only the position can be set for protonodes within a definition. The rest of the metadata comes from the node macro in NODE_METADATA
 fn node_definitions() -> HashMap<DefinitionIdentifier, DocumentNodeDefinition> {
 	let custom = vec![
 		// TODO: Auto-generate this from its proto node macro
@@ -2636,15 +2637,19 @@ pub fn resolve_document_node_type(identifier: &DefinitionIdentifier) -> Option<&
 	DOCUMENT_NODE_TYPES.get(identifier)
 }
 
-pub fn default_display_name(identifier: &DefinitionIdentifier) -> String {
+pub fn implementation_name_from_identifier(identifier: &DefinitionIdentifier) -> String {
 	match identifier {
 		DefinitionIdentifier::Network(name) => name.clone(),
 		DefinitionIdentifier::ProtoNode(proto_node_identifier) => registry::NODE_METADATA
 			.lock()
 			.unwrap()
 			.get(proto_node_identifier)
 			.map(|metadata| metadata.display_name.to_string())
-			.unwrap_or_default(),
+			.unwrap_or_else(|| {
+				let mut last_segment = proto_node_identifier.name.split("::").last().unwrap_or_default().to_string();
+				last_segment = last_segment.strip_suffix("Node").unwrap_or(&last_segment).to_string();
+				last_segment
+			}),
 	}
 }
 
@@ -2660,10 +2665,13 @@ pub fn collect_node_types() -> Vec<FrontendNodeType> {
 				.iter()
 				.map(|node_input| node_input.as_value().map(|node_value| node_value.ty().nested_type().to_string()).unwrap_or_default())
 				.collect::<Vec<String>>();
-
+			let mut name = definition.node_template.persistent_node_metadata.display_name.clone();
+			if name.is_empty() {
+				name = implementation_name_from_identifier(identifier)
+			}
 			FrontendNodeType {
 				identifier: identifier.clone(),
-				name: default_display_name(identifier),
+				name,
 				category: definition.category.to_string(),
 				input_types,
 			}

editor/src/messages/portfolio/document/node_graph/document_node_definitions/document_node_derive.rs

@@ -25,6 +25,10 @@ pub(super) fn post_process_nodes(custom: Vec<DocumentNodeDefinition>) -> HashMap
 	// Add the rest of the protonodes from the macro
 	let node_registry = NODE_REGISTRY.lock().unwrap();
 	for (id, metadata) in NODE_METADATA.lock().unwrap().iter() {
+		let identifier = DefinitionIdentifier::ProtoNode(id.clone());
+		if definitions_map.contains_key(&identifier) {
+			continue;
+		};
 		let NodeMetadata {
 			display_name,
 			category,
@@ -43,7 +47,7 @@ pub(super) fn post_process_nodes(custom: Vec<DocumentNodeDefinition>) -> HashMap
 
 		let inputs = preprocessor::node_inputs(fields, first_node_io);
 		definitions_map.insert(
-			DefinitionIdentifier::ProtoNode(id.clone()),
+			identifier,
 			DocumentNodeDefinition {
 				identifier: display_name,
 				node_template: NodeTemplate {
@@ -78,6 +82,16 @@ pub(super) fn post_process_nodes(custom: Vec<DocumentNodeDefinition>) -> HashMap
 		);
 	}
 
+	// If any protonode does not have metadata then set its display name to its identifier string
+	for definition in definitions_map.values_mut() {
+		let metadata = NODE_METADATA.lock().unwrap();
+		if let DocumentNodeImplementation::ProtoNode(id) = &definition.node_template.document_node.implementation {
+			if !metadata.contains_key(id) {
+				definition.node_template.persistent_node_metadata.display_name = definition.identifier.to_string();
+			}
+		}
+	}
+
 	// Add the rest of the network nodes to the map and add the metadata for their internal protonodes
 	for mut network_node in network_nodes {
 		traverse_node(&network_node.node_template.document_node, &mut network_node.node_template.persistent_node_metadata, &definitions_map);

editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs

@@ -2578,6 +2578,7 @@ impl NodeGraphMessageHandler {
 				can_be_layer: network_interface.is_eligible_to_be_layer(&node_id, breadcrumb_network_path),
 				reference: network_interface.reference(&node_id, breadcrumb_network_path),
 				display_name: network_interface.display_name(&node_id, breadcrumb_network_path),
+				implementation_name: network_interface.implementation_name(&node_id, breadcrumb_network_path),
 				primary_input,
 				exposed_inputs,
 				primary_output,

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

@@ -83,6 +83,8 @@ pub struct FrontendNode {
 	pub reference: Option<DefinitionIdentifier>,
 	#[serde(rename = "displayName")]
 	pub display_name: String,
+	#[serde(rename = "implementationName")]
+	pub implementation_name: String,
 	#[serde(rename = "primaryInput")]
 	pub primary_input: Option<FrontendGraphInput>,
 	#[serde(rename = "exposedInputs")]

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

@@ -7,7 +7,7 @@ use super::misc::PTZ;
 use super::nodes::SelectedNodes;
 use crate::consts::{EXPORTS_TO_RIGHT_EDGE_PIXEL_GAP, EXPORTS_TO_TOP_EDGE_PIXEL_GAP, GRID_SIZE, IMPORTS_TO_LEFT_EDGE_PIXEL_GAP, IMPORTS_TO_TOP_EDGE_PIXEL_GAP};
 use crate::messages::portfolio::document::graph_operation::utility_types::ModifyInputsContext;
-use crate::messages::portfolio::document::node_graph::document_node_definitions::{DefinitionIdentifier, DocumentNodeDefinition, default_display_name, resolve_document_node_type};
+use crate::messages::portfolio::document::node_graph::document_node_definitions::{DefinitionIdentifier, implementation_name_from_identifier, resolve_document_node_type};
 use crate::messages::portfolio::document::node_graph::utility_types::{Direction, FrontendClickTargets, FrontendGraphDataType, FrontendGraphInput, FrontendGraphOutput};
 use crate::messages::portfolio::document::utility_types::network_interface::resolved_types::ResolvedDocumentNodeTypes;
 use crate::messages::portfolio::document::utility_types::wires::{GraphWireStyle, WirePath, WirePathUpdate, build_vector_wire};
@@ -18,7 +18,6 @@ use glam::{DAffine2, DVec2, IVec2};
 use graph_craft::Type;
 use graph_craft::document::value::TaggedValue;
 use graph_craft::document::{DocumentNode, DocumentNodeImplementation, NodeId, NodeInput, NodeNetwork, OldDocumentNodeImplementation, OldNodeNetwork};
-use graph_craft::proto::NODE_METADATA;
 use graphene_std::ContextDependencies;
 use graphene_std::math::quad::Quad;
 use graphene_std::subpath::Subpath;
@@ -470,33 +469,6 @@ impl NodeNetworkInterface {
 		node_template
 	}
 
-	/// Try and get the [`DocumentNodeDefinition`] for a node
-	pub fn node_definition(&self, node_id: &NodeId, network_path: &[NodeId]) -> Option<&DocumentNodeDefinition> {
-		let implementation = self.implementation(node_id, network_path)?;
-		match implementation {
-			DocumentNodeImplementation::Network(_) => {
-				let metadata = self.node_metadata(node_id, network_path)?;
-				let metadata = metadata.persistent_metadata.network_metadata.as_ref()?;
-				let reference = metadata.persistent_metadata.reference.clone()?;
-				let identifier = DefinitionIdentifier::Network(reference.clone());
-				let Some(definition) = resolve_document_node_type(&identifier) else {
-					log::error!("Could not get definition for node id {node_id} with reference {reference}");
-					return None;
-				};
-				Some(definition)
-			}
-			DocumentNodeImplementation::ProtoNode(proto_node_identifier) => {
-				let identifier = DefinitionIdentifier::ProtoNode(proto_node_identifier.clone());
-				let Some(definition) = resolve_document_node_type(&identifier) else {
-					log::error!("Could not get definition for node id {node_id} with proto_node_identifier {proto_node_identifier}");
-					return None;
-				};
-				Some(definition)
-			}
-			DocumentNodeImplementation::Extract => None,
-		}
-	}
-
 	pub fn input_from_connector(&self, input_connector: &InputConnector, network_path: &[NodeId]) -> Option<&NodeInput> {
 		let Some(network) = self.nested_network(network_path) else {
 			log::error!("Could not get network in input_from_connector");
@@ -956,7 +928,7 @@ impl NodeNetworkInterface {
 					.persistent_metadata
 					.reference
 					.clone()
-					.map(|reference| DefinitionIdentifier::Network(reference))
+					.map(DefinitionIdentifier::Network)
 			}
 			DocumentNodeImplementation::ProtoNode(protonode_id) => Some(DefinitionIdentifier::ProtoNode(protonode_id.clone())),
 			_ => None,
@@ -1044,7 +1016,7 @@ impl NodeNetworkInterface {
 	/// The uneditable name in the properties panel which represents the function name of the node implementation
 	pub fn implementation_name(&self, node_id: &NodeId, network_path: &[NodeId]) -> String {
 		self.reference(&node_id, network_path)
-			.map(|identifier| default_display_name(&identifier))
+			.map(|identifier| implementation_name_from_identifier(&identifier))
 			.unwrap_or("Custom Node".to_string())
 	}
 
@@ -3795,7 +3767,9 @@ impl NodeNetworkInterface {
 		let Some(metadata) = self.node_metadata_mut(node_id, network_path) else { return };
 		for added_input_index in metadata.persistent_metadata.input_metadata.len()..number_of_inputs {
 			let input_metadata = self
-				.node_definition(node_id, network_path)
+				.reference(node_id, network_path)
+				.as_ref()
+				.and_then(resolve_document_node_type)
 				.and_then(|definition| definition.node_template.persistent_node_metadata.input_metadata.get(added_input_index))
 				.cloned();
 			let Some(metadata) = self.node_metadata_mut(node_id, network_path) else { return };
@@ -6277,7 +6251,8 @@ struct InputTransientMetadata {
 /// Persistent metadata for each node in the network, which must be included when creating, serializing, and deserializing saving a node.
 #[derive(Default, Debug, Clone, PartialEq, serde::Serialize, serde::Deserialize)]
 pub struct DocumentNodePersistentMetadata {
-	/// A name chosen by the user for this instance of the node. Empty indicates no given name, in which case the reference name is displayed to the user in italics.
+	/// A name chosen by the user for this instance of the node. Empty indicates no given name, in which case the implementation name is displayed to the user in italics.
+	/// This is empty for all newly created nodes, except protonodes with #[skip_impl]
 	#[serde(default)]
 	pub display_name: String,
 	/// Stores metadata to override the properties in the properties panel for each input. These can either be generated automatically based on the type, or with a custom function.