From 54097ebcb5c77d1d7a1d1a8578199aad91caf0d4 Mon Sep 17 00:00:00 2001 From: Adam Date: Mon, 14 Jul 2025 17:38:32 -0700 Subject: [PATCH] Code review, remove identity test --- .../node_graph/document_node_definitions.rs | 8 +---- .../document/node_graph/node_graph_message.rs | 4 --- .../node_graph/node_graph_message_handler.rs | 17 +++------ .../document/node_graph/node_properties.rs | 18 ++++------ .../utility_types/network_interface.rs | 2 +- .../messages/portfolio/document_migration.rs | 25 +++++++++++-- node-graph/gcore/src/ops.rs | 36 +++++++++++++------ node-graph/graph-craft/src/document.rs | 32 ++++------------- node-graph/graph-craft/src/proto.rs | 2 +- node-graph/gstd/src/any.rs | 17 --------- node-graph/interpreted-executor/src/lib.rs | 6 ---- .../interpreted-executor/src/node_registry.rs | 5 +-- node-graph/interpreted-executor/src/util.rs | 6 ---- node-graph/preprocessor/src/lib.rs | 6 ---- 14 files changed, 71 insertions(+), 113 deletions(-) diff --git a/editor/src/messages/portfolio/document/node_graph/document_node_definitions.rs b/editor/src/messages/portfolio/document/node_graph/document_node_definitions.rs index 5bed5d0faa..2ae8d8c748 100644 --- a/editor/src/messages/portfolio/document/node_graph/document_node_definitions.rs +++ b/editor/src/messages/portfolio/document/node_graph/document_node_definitions.rs @@ -89,13 +89,7 @@ fn static_nodes() -> Vec { category: "General", node_template: NodeTemplate { document_node: DocumentNode { -<<<<<<< HEAD implementation: DocumentNodeImplementation::ProtoNode(ops::identity::IDENTIFIER), -||||||| parent of 8e045313 (Migrate pass through and value node to identity implementation) - implementation: DocumentNodeImplementation::proto("graphene_core::ops::IdentityNode"), -======= - implementation: DocumentNodeImplementation::proto("graphene_std::any::IdentityNode"), ->>>>>>> 8e045313 (Migrate pass through and value node to identity implementation) inputs: vec![NodeInput::value(TaggedValue::None, true)], ..Default::default() }, @@ -113,7 +107,7 @@ fn static_nodes() -> Vec { category: "General", node_template: NodeTemplate { document_node: DocumentNode { - implementation: DocumentNodeImplementation::proto("graphene_std::any::IdentityNode"), + implementation: DocumentNodeImplementation::ProtoNode(ops::identity::IDENTIFIER), manual_composition: Some(generic!(T)), inputs: vec![NodeInput::value(TaggedValue::None, false)], ..Default::default() diff --git a/editor/src/messages/portfolio/document/node_graph/node_graph_message.rs b/editor/src/messages/portfolio/document/node_graph/node_graph_message.rs index ec76851949..bd6c77d467 100644 --- a/editor/src/messages/portfolio/document/node_graph/node_graph_message.rs +++ b/editor/src/messages/portfolio/document/node_graph/node_graph_message.rs @@ -152,10 +152,6 @@ pub enum NodeGraphMessage { node_id: NodeId, alias: String, }, - SetReference { - node_id: NodeId, - reference: Option, - }, SetToNodeOrLayer { node_id: NodeId, is_layer: bool, diff --git a/editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs b/editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs index 416b582934..2a899eed6c 100644 --- a/editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs +++ b/editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs @@ -1582,7 +1582,6 @@ impl<'a> MessageHandler> for NodeG let document_bbox: [DVec2; 2] = viewport_bbox.map(|p| network_metadata.persistent_metadata.navigation_metadata.node_graph_to_viewport.inverse().transform_point2(p)); let mut nodes = Vec::new(); - for node_id in &self.frontend_nodes { let Some(node_bbox) = network_interface.node_bounding_box(node_id, breadcrumb_network_path) else { log::error!("Could not get bbox for node: {:?}", node_id); @@ -1730,9 +1729,6 @@ impl<'a> MessageHandler> for NodeG responses.add(NodeGraphMessage::SendWires); } - NodeGraphMessage::SetReference { node_id, reference } => { - network_interface.set_reference(&node_id, breadcrumb_network_path, reference); - } NodeGraphMessage::SetToNodeOrLayer { node_id, is_layer } => { if is_layer && !network_interface.is_eligible_to_be_layer(&node_id, selection_network_path) { return; @@ -2484,21 +2480,16 @@ impl NodeGraphMessageHandler { // Offset node insertion 3 grid spaces left and 1 grid space up so the center of the node is dragged position = position - DVec2::new(GRID_SIZE as f64 * 3., GRID_SIZE as f64); - // Offset to account for division rounding error and place the selected node to the top left of the input - if position.x < 0. { - position.x = position.x - 1.; - } - if position.y < 0. { - position.y = position.y - 1.; - } - let Some(mut input) = network_interface.take_input(&disconnecting, breadcrumb_network_path) else { return; }; match &mut input { NodeInput::Value { exposed, .. } => *exposed = false, - _ => return, + _ => { + network_interface.set_input(&disconnecting, input, breadcrumb_network_path); + return; + } } let drag_start = DragStart { diff --git a/editor/src/messages/portfolio/document/node_graph/node_properties.rs b/editor/src/messages/portfolio/document/node_graph/node_properties.rs index e6ffaa6d0b..5116e65cb3 100644 --- a/editor/src/messages/portfolio/document/node_graph/node_properties.rs +++ b/editor/src/messages/portfolio/document/node_graph/node_properties.rs @@ -89,7 +89,7 @@ pub fn start_widgets(parameter_widgets_info: ParameterWidgetsInfo) -> Vec Vec return vec![]; }; let Some(input) = document_node.inputs.get(0) else { - log::warn!("Secondary value input could not be found on value properties"); + log::warn!("Value input could not be found in value properties"); return vec![]; }; let mut select_value_widgets = Vec::new(); @@ -1932,20 +1932,16 @@ pub fn value_properties(node_id: NodeId, context: &mut NodePropertiesContext) -> return Vec::new(); }; - // let committer = || {|_| { - // let messages = vec![ - // DocumentMessage::AddTransaction.into(), - // NodeGraphMessage::RunDocumentGraph.into(), - // ]; - // Message::Batched(messages.into_boxed_slice()).into() - // }; let updater = || { move |v: &TaggedValueChoice| { let value = v.to_tagged_value(); let messages = vec![NodeGraphMessage::SetInputValue { node_id, input_index: 0, value }.into(), NodeGraphMessage::SendGraph.into()]; - Message::Batched(messages.into_boxed_slice()).into() + Message::Batched { + messages: messages.into_boxed_slice(), + } + .into() } }; let value_dropdown = enum_choice::().dropdown_menu(choice, updater, || commit_value); 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 79b9dcea97..9a96be9f7f 100644 --- a/editor/src/messages/portfolio/document/utility_types/network_interface.rs +++ b/editor/src/messages/portfolio/document/utility_types/network_interface.rs @@ -503,7 +503,7 @@ impl NodeNetworkInterface { pub fn take_input(&mut self, input_connector: &InputConnector, network_path: &[NodeId]) -> Option { let Some(network) = self.network_mut(network_path) else { - log::error!("Could not get network in input_from_connector"); + log::error!("Could not get network in take_input"); return None; }; let input = match input_connector { diff --git a/editor/src/messages/portfolio/document_migration.rs b/editor/src/messages/portfolio/document_migration.rs index ba4a73a767..a55d63ff53 100644 --- a/editor/src/messages/portfolio/document_migration.rs +++ b/editor/src/messages/portfolio/document_migration.rs @@ -979,13 +979,32 @@ fn migrate_node(node_id: &NodeId, node: &DocumentNode, network_path: &[NodeId], document.network_interface.set_input(&InputConnector::node(*node_id, 3), old_inputs[4].clone(), network_path); document.network_interface.set_input(&InputConnector::node(*node_id, 4), old_inputs[5].clone(), network_path); document.network_interface.set_input(&InputConnector::node(*node_id, 5), old_inputs[3].clone(), network_path); - } else { - // Swap it back if we're not changing anything - let _ = document.network_interface.replace_inputs(node_id, network_path, &mut current_node_template); + + upgraded = true; } } + + if !upgraded { + let _ = document.network_interface.replace_inputs(node_id, network_path, &mut current_node_template); + } } + // Add the "Depth" parameter to the "Instance Index" node + if reference == "Instance Index" && inputs_count == 0 { + let mut node_template = resolve_document_node_type(reference)?.default_node_template(); + document.network_interface.replace_implementation(node_id, network_path, &mut node_template); + + let mut node_path = network_path.to_vec(); + node_path.push(*node_id); + + document.network_interface.add_import(TaggedValue::None, false, 0, "Primary", "", &node_path); + document.network_interface.add_import(TaggedValue::U32(0), false, 1, "Loop Level", "TODO", &node_path); + } + + // ================================== + // PUT ALL MIGRATIONS ABOVE THIS LINE + // ================================== + // Ensure layers are positioned as stacks if they are upstream siblings of another layer document.network_interface.load_structure(); let all_layers = LayerNodeIdentifier::ROOT_PARENT.descendants(document.network_interface.document_metadata()).collect::>(); diff --git a/node-graph/gcore/src/ops.rs b/node-graph/gcore/src/ops.rs index d5ffd57068..9af72901b9 100644 --- a/node-graph/gcore/src/ops.rs +++ b/node-graph/gcore/src/ops.rs @@ -1,6 +1,30 @@ -use crate::Node; +use crate::{ + Node, + registry::{Any, DynFuture, SharedNodeContainer}, +}; use std::marker::PhantomData; +pub struct IdentityNode { + value: SharedNodeContainer, +} + +impl<'i> Node<'i, Any<'i>> for IdentityNode { + type Output = DynFuture<'i, Any<'i>>; + fn eval(&'i self, input: Any<'i>) -> Self::Output { + Box::pin(async move { self.value.eval(input).await }) + } +} + +impl IdentityNode { + pub const fn new(value: SharedNodeContainer) -> Self { + IdentityNode { value } + } +} + +pub mod identity { + pub const IDENTIFIER: crate::ProtoNodeIdentifier = crate::ProtoNodeIdentifier::new("graphene_core::ops::IdentityNode"); +} + // Type // TODO: Document this #[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Default)] @@ -135,13 +159,3 @@ impl<'input, I: 'input + Convert<_O> + Sync + Send, _O: 'input> Node<'input, I> Box::pin(async move { input.convert() }) } } - -#[cfg(test)] -mod test { - use super::*; - - #[test] - pub fn identity_node() { - assert_eq!(identity(&4), &4); - } -} diff --git a/node-graph/graph-craft/src/document.rs b/node-graph/graph-craft/src/document.rs index ff13a83084..ea289d41a2 100644 --- a/node-graph/graph-craft/src/document.rs +++ b/node-graph/graph-craft/src/document.rs @@ -7,7 +7,7 @@ use glam::IVec2; use graphene_core::memo::MemoHashGuard; pub use graphene_core::uuid::NodeId; pub use graphene_core::uuid::generate_uuid; -use graphene_core::{Cow, MemoHash, ProtoNodeIdentifier, Type}; +use graphene_core::{Cow, MemoHash, ProtoNodeIdentifier, Type, ops}; use log::Metadata; use rustc_hash::FxHashMap; use std::collections::HashMap; @@ -460,7 +460,7 @@ pub enum DocumentNodeImplementation { impl Default for DocumentNodeImplementation { fn default() -> Self { - Self::ProtoNode(ProtoNodeIdentifier::new("graphene_std::any::IdentityNode")) + Self::ProtoNode(ops::identity::IDENTIFIER) } } @@ -916,7 +916,7 @@ impl NodeNetwork { return; }; // If the node is hidden, replace it with an identity node - let identity_node = DocumentNodeImplementation::ProtoNode("graphene_std::any::IdentityNode".into()); + let identity_node = DocumentNodeImplementation::ProtoNode(ops::identity::IDENTIFIER); if !node.visible && node.implementation != identity_node { node.implementation = identity_node; @@ -1092,7 +1092,7 @@ impl NodeNetwork { fn remove_id_node(&mut self, id: NodeId) -> Result<(), String> { let node = self.nodes.get(&id).ok_or_else(|| format!("Node with id {id} does not exist"))?.clone(); if let DocumentNodeImplementation::ProtoNode(ident) = &node.implementation { - if ident.name == "graphene_std::any::IdentityNode" { + if ident.name == ops::identity::IDENTIFIER.name { assert_eq!(node.inputs.len(), 1, "Id node has more than one input"); if let NodeInput::Node { node_id, output_index, .. } = node.inputs[0] { let node_input_output_index = output_index; @@ -1139,13 +1139,13 @@ impl NodeNetwork { Ok(()) } - /// Strips out any [`graphene_std::any::IdentityNode`]s that are unnecessary. + /// Strips out any [`graphene_std::ops::IdentityNode`]s that are unnecessary. pub fn remove_redundant_id_nodes(&mut self) { let id_nodes = self .nodes .iter() .filter(|(_, node)| { - matches!(&node.implementation, DocumentNodeImplementation::ProtoNode(ident) if ident == &ProtoNodeIdentifier::new("graphene_std::any::IdentityNode")) + matches!(&node.implementation, DocumentNodeImplementation::ProtoNode(ident) if ident == &ops::identity::IDENTIFIER) && node.inputs.len() == 1 && matches!(node.inputs[0], NodeInput::Node { .. }) }) @@ -1333,7 +1333,7 @@ mod test { fn extract_node() { let id_node = DocumentNode { inputs: vec![], - implementation: DocumentNodeImplementation::ProtoNode("graphene_std::any::IdentityNode".into()), + implementation: DocumentNodeImplementation::ProtoNode(ops::identity::IDENTIFIER), ..Default::default() }; // TODO: Extend test cases to test nested network @@ -1535,13 +1535,7 @@ mod test { NodeId(1), DocumentNode { inputs: vec![NodeInput::network(concrete!(u32), 0)], -<<<<<<< HEAD implementation: DocumentNodeImplementation::ProtoNode(graphene_core::ops::identity::IDENTIFIER), -||||||| parent of 8e045313 (Migrate pass through and value node to identity implementation) - implementation: DocumentNodeImplementation::ProtoNode(ProtoNodeIdentifier::new("graphene_core::ops::IdentityNode")), -======= - implementation: DocumentNodeImplementation::ProtoNode(ProtoNodeIdentifier::new("graphene_std::any::IdentityNode")), ->>>>>>> 8e045313 (Migrate pass through and value node to identity implementation) ..Default::default() }, ), @@ -1549,13 +1543,7 @@ mod test { NodeId(2), DocumentNode { inputs: vec![NodeInput::network(concrete!(u32), 1)], -<<<<<<< HEAD implementation: DocumentNodeImplementation::ProtoNode(graphene_core::ops::identity::IDENTIFIER), -||||||| parent of 8e045313 (Migrate pass through and value node to identity implementation) - implementation: DocumentNodeImplementation::ProtoNode(ProtoNodeIdentifier::new("graphene_core::ops::IdentityNode")), -======= - implementation: DocumentNodeImplementation::ProtoNode(ProtoNodeIdentifier::new("graphene_std::any::IdentityNode")), ->>>>>>> 8e045313 (Migrate pass through and value node to identity implementation) ..Default::default() }, ), @@ -1582,13 +1570,7 @@ mod test { NodeId(2), DocumentNode { inputs: vec![result_node_input], -<<<<<<< HEAD implementation: DocumentNodeImplementation::ProtoNode(graphene_core::ops::identity::IDENTIFIER), -||||||| parent of 8e045313 (Migrate pass through and value node to identity implementation) - implementation: DocumentNodeImplementation::ProtoNode(ProtoNodeIdentifier::new("graphene_core::ops::IdentityNode")), -======= - implementation: DocumentNodeImplementation::ProtoNode(ProtoNodeIdentifier::new("graphene_std::any::IdentityNode")), ->>>>>>> 8e045313 (Migrate pass through and value node to identity implementation) ..Default::default() }, ), diff --git a/node-graph/graph-craft/src/proto.rs b/node-graph/graph-craft/src/proto.rs index dc325da75c..fc35f7717f 100644 --- a/node-graph/graph-craft/src/proto.rs +++ b/node-graph/graph-craft/src/proto.rs @@ -143,7 +143,7 @@ pub struct ProtoNode { impl Default for ProtoNode { fn default() -> Self { Self { - identifier: ProtoNodeIdentifier::new("graphene_std::any::IdentityNode"), + identifier: ops::identity::IDENTIFIER, construction_args: ConstructionArgs::Value(value::TaggedValue::U32(0).into()), input: ProtoNodeInput::None, original_location: OriginalLocation::default(), diff --git a/node-graph/gstd/src/any.rs b/node-graph/gstd/src/any.rs index 0cab682491..1a38d65b35 100644 --- a/node-graph/gstd/src/any.rs +++ b/node-graph/gstd/src/any.rs @@ -46,20 +46,3 @@ pub fn input_node(n: SharedNodeContainer) -> DowncastBothNode<(), pub fn downcast_node(n: SharedNodeContainer) -> DowncastBothNode { DowncastBothNode::new(n) } - -pub struct IdentityNode { - value: SharedNodeContainer, -} - -impl<'i> Node<'i, Any<'i>> for IdentityNode { - type Output = DynFuture<'i, Any<'i>>; - fn eval(&'i self, input: Any<'i>) -> Self::Output { - Box::pin(async move { self.value.eval(input).await }) - } -} - -impl IdentityNode { - pub const fn new(value: SharedNodeContainer) -> Self { - IdentityNode { value } - } -} diff --git a/node-graph/interpreted-executor/src/lib.rs b/node-graph/interpreted-executor/src/lib.rs index 2fdb942fad..5c05ef62ba 100644 --- a/node-graph/interpreted-executor/src/lib.rs +++ b/node-graph/interpreted-executor/src/lib.rs @@ -20,13 +20,7 @@ mod tests { NodeId(0), DocumentNode { inputs: vec![NodeInput::network(concrete!(u32), 0)], -<<<<<<< HEAD implementation: DocumentNodeImplementation::ProtoNode(ops::identity::IDENTIFIER), -||||||| parent of 8e045313 (Migrate pass through and value node to identity implementation) - implementation: DocumentNodeImplementation::ProtoNode(ProtoNodeIdentifier::new("graphene_core::ops::IdentityNode")), -======= - implementation: DocumentNodeImplementation::ProtoNode(ProtoNodeIdentifier::new("graphene_std::any::IdentityNode")), ->>>>>>> 8e045313 (Migrate pass through and value node to identity implementation) ..Default::default() }, ), diff --git a/node-graph/interpreted-executor/src/node_registry.rs b/node-graph/interpreted-executor/src/node_registry.rs index 7dfee999f6..3ecc4aa713 100644 --- a/node-graph/interpreted-executor/src/node_registry.rs +++ b/node-graph/interpreted-executor/src/node_registry.rs @@ -14,8 +14,9 @@ use graphene_std::Context; use graphene_std::GraphicElement; #[cfg(feature = "gpu")] use graphene_std::any::DowncastBothNode; -use graphene_std::any::{ComposeTypeErased, DynAnyNode, IdentityNode, IntoTypeErasedNode}; +use graphene_std::any::{ComposeTypeErased, DynAnyNode, IntoTypeErasedNode}; use graphene_std::application_io::{ImageTexture, SurfaceFrame}; +use graphene_std::ops::IdentityNode; #[cfg(feature = "gpu")] use graphene_std::wasm_application_io::{WasmEditorApi, WasmSurfaceHandle}; use node_registry_macros::{async_node, convert_node, into_node}; @@ -115,7 +116,7 @@ fn node_registry() -> HashMap>>>>>> 8e045313 (Migrate pass through and value node to identity implementation) inputs: vec![NodeInput::value(TaggedValue::EditorApi(editor_api), false)], ..Default::default() }, diff --git a/node-graph/preprocessor/src/lib.rs b/node-graph/preprocessor/src/lib.rs index 258b27c783..e0b4a01685 100644 --- a/node-graph/preprocessor/src/lib.rs +++ b/node-graph/preprocessor/src/lib.rs @@ -49,13 +49,7 @@ pub fn generate_node_substitutions() -> HashMap>>>>>> 8e045313 (Migrate pass through and value node to identity implementation) let into_node_registry = &interpreted_executor::node_registry::NODE_REGISTRY;