Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions editor/src/node_graph_executor/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -345,13 +345,13 @@ impl NodeRuntime {
None
}

async fn update_network(&mut self, mut graph: NodeNetwork) -> Result<ResolvedDocumentNodeTypesDelta, (ResolvedDocumentNodeTypesDelta, String)> {
if let Err(e) = self.preprocessor.expand_network(&mut graph, &self.resources) {
async fn update_network(&mut self, graph: NodeNetwork) -> Result<ResolvedDocumentNodeTypesDelta, (ResolvedDocumentNodeTypesDelta, String)> {
let mut scoped_network = wrap_network_in_scope(graph, self.editor_api.clone());

if let Err(e) = self.preprocessor.preprocess(&mut scoped_network, &self.resources) {
return Err((ResolvedDocumentNodeTypesDelta::default(), e.to_string()));
}

let scoped_network = wrap_network_in_scope(graph, self.editor_api.clone());

// We assume only one output
assert_eq!(scoped_network.exports.len(), 1, "Graph with multiple outputs not yet handled");

Expand Down
100 changes: 69 additions & 31 deletions node-graph/graph-craft/src/document.rs
Original file line number Diff line number Diff line change
Expand Up @@ -655,6 +655,73 @@ impl NodeNetwork {
}
}

// Functions for resolving scope inputs
impl NodeNetwork {
pub fn resolve_scope_inputs(&mut self) {
let mut leftover = Vec::new();
self.resolve_scope_inputs_impl(None, &mut leftover);
assert!(leftover.is_empty(), "Unresolved scope keys at top level: {leftover:?}");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: The leftover vector and the subsequent assertion are dead code. At the top level, if a scope key is not found in self.scope_injections, chain.get() will also fail (since parent is None), causing the unwrap_or_else to panic before anything is pushed to leftover. The vector is guaranteed to always be empty.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/graph-craft/src/document.rs, line 663:

<comment>The `leftover` vector and the subsequent assertion are dead code. At the top level, if a scope key is not found in `self.scope_injections`, `chain.get()` will also fail (since `parent` is `None`), causing the `unwrap_or_else` to panic before anything is pushed to `leftover`. The vector is guaranteed to always be empty.</comment>

<file context>
@@ -655,6 +655,59 @@ impl NodeNetwork {
+	pub fn resolve_scope_inputs(&mut self) {
+		let mut leftover = Vec::new();
+		self.resolve_scope_inputs_with(None, &mut leftover);
+		assert!(leftover.is_empty(), "Unresolved scope keys at top level: {leftover:?}");
+	}
+
</file context>

}
Comment on lines +660 to +664

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The leftover vector and the subsequent assertion are dead code. Because resolve_scope_inputs_with panics immediately on line 688 if a scope key is not found in any ancestor, an unresolved key at the top level will never be pushed to leftover. If a key is successfully resolved at the top level, it is handled by the scope_injections check and also not pushed to leftover. Thus, leftover is guaranteed to always be empty, making the assertion unreachable. We can simplify this by removing the assertion and passing a temporary vector.

	pub fn resolve_scope_inputs(&mut self) {
		self.resolve_scope_inputs_with(None, &mut Vec::new());
	}


fn resolve_scope_inputs_impl(&mut self, parent: Option<&ScopeChain<'_>>, network_inputs: &mut Vec<NodeInput>) {
let scope_chain = ScopeChain {
scopes: &self.scope_injections,
parent,
};

for node in self.nodes.values_mut() {
let DocumentNodeImplementation::Network(network) = &mut node.implementation else { continue };
network.resolve_scope_inputs_impl(Some(&scope_chain), &mut node.inputs);
}

let mut key_to_idx: FxHashMap<Cow<'static, str>, usize> = FxHashMap::default();
for node in self.nodes.values_mut() {
for input in node.inputs.iter_mut() {
let NodeInput::Scope(key) = input else { continue };
*input = match scope_chain.lookup(key) {
ScopeChainLookup::Current(node_id, _ty) => NodeInput::node(*node_id, 0),
ScopeChainLookup::Parent(_node_id, ty) => {
let import_index = *key_to_idx.entry(key.clone()).or_insert_with(|| {
let index = network_inputs.len();
network_inputs.push(NodeInput::Scope(key.clone()));
index
});
NodeInput::Import {
import_type: ty.clone(),
import_index,
}
}
ScopeChainLookup::None => panic!("Scope key `{key}` not found in any ancestor scope_injections"),
};
}
}
}
}

struct ScopeChain<'a> {
scopes: &'a FxHashMap<String, (NodeId, Type)>,
parent: Option<&'a ScopeChain<'a>>,
}
enum ScopeChainLookup<'a> {
Current(&'a NodeId, &'a Type),
Parent(&'a NodeId, &'a Type),
None,
}
impl ScopeChain<'_> {
fn lookup(&'_ self, key: &str) -> ScopeChainLookup<'_> {
self.scopes
.get(key)
.map(|(id, ty)| ScopeChainLookup::Current(id, ty))
.or_else(|| {
self.parent.and_then(|parent| match parent.lookup(key) {
ScopeChainLookup::Current(id, ty) | ScopeChainLookup::Parent(id, ty) => Some(ScopeChainLookup::Parent(id, ty)),
ScopeChainLookup::None => None,
})
})
.unwrap_or(ScopeChainLookup::None)
}
}

/// Functions for compiling the network
impl NodeNetwork {
/// Replace all references in the graph of a node ID with a new node ID defined by the function `f`.
Expand Down Expand Up @@ -784,18 +851,6 @@ impl NodeNetwork {
are_inputs_used
}

pub fn resolve_scope_inputs(&mut self) {
for node in self.nodes.values_mut() {
for input in node.inputs.iter_mut() {
if let NodeInput::Scope(key) = input {
let (import_id, _ty) = self.scope_injections.get(key.as_ref()).expect("Tried to import a non existent key from scope");
// TODO use correct output index
*input = NodeInput::node(*import_id, 0);
}
}
}
}

/// Remove all nodes that contain [`DocumentNodeImplementation::Network`] by moving the nested nodes into the parent network.
pub fn flatten(&mut self, node_id: NodeId) {
self.flatten_with_fns(node_id, merge_ids, NodeId::new)
Expand Down Expand Up @@ -886,11 +941,7 @@ impl NodeNetwork {
}
NodeInput::Value { .. } => unreachable!("Value inputs should have been replaced with value nodes"),
NodeInput::Inline(_) => (),
NodeInput::Scope(ref key) => {
let (import_id, _ty) = self.scope_injections.get(key.as_ref()).expect("Tried to import a non existent key from scope");
// TODO use correct output index
nested_node.inputs[nested_input_index] = NodeInput::node(*import_id, 0);
}
NodeInput::Scope(_) => unreachable!("Scope inputs should have been resolved by resolve_scope_inputs_recursive before flattening"),
NodeInput::Reflection(_) => unreachable!("Reflection inputs should have been replaced with value nodes"),
}
}
Expand All @@ -906,21 +957,8 @@ impl NodeNetwork {
}
}

/// Connect all nodes that were previously connected to this node to the nodes of the inner network
fn replace_node_with_its_exports(&mut self, id: NodeId, original_location: &OriginalLocation, exports: &[NodeInput]) {
// Connect scope injections to the inner network export
self.scope_injections.values_mut().for_each(|(node_id, _ty)| {
if node_id == &id {
let Some(export) = exports.first() else {
log::error!("Inner network should have at least one export");
return;
};
if let NodeInput::Node { node_id: export_id, output_index: _ } = export {
*node_id = *export_id;
}
}
});

// Connect all nodes that were previously connected to this node to the nodes of the inner network
for (i, export) in exports.iter().enumerate() {
if let NodeInput::Node { node_id, output_index, .. } = &export {
for deps in &original_location.dependants {
Expand Down
3 changes: 2 additions & 1 deletion node-graph/graph-craft/src/graphene_compiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,13 @@ pub struct Compiler {}

impl Compiler {
pub fn compile(&self, mut network: NodeNetwork) -> impl Iterator<Item = Result<ProtoNetwork, String>> {
network.resolve_scope_inputs();
network.generate_node_paths(&[]);
let node_ids = network.nodes.keys().copied().collect::<Vec<_>>();
network.populate_dependants();
for id in node_ids {
network.flatten(id);
}
network.resolve_scope_inputs();
network.remove_redundant_passthrough_nodes();
// network.remove_dead_nodes(0);
let proto_networks = network.into_proto_networks();
Expand Down
6 changes: 3 additions & 3 deletions node-graph/graphene-cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,10 +242,10 @@ fn compile_graph(document_string: String, editor_api: Arc<PlatformEditorApi>) ->
let mut network = load_network(&document_string);
fix_nodes(&mut network);

let preprocessor = preprocessor::Preprocessor::new();
preprocessor.expand_network(&mut network, &ResourceRegistry::default()).expect("Failed to expand network"); // TODO: actually load the resources from the document
let mut wrapped_network = wrap_network_in_scope(network, editor_api);

let wrapped_network = wrap_network_in_scope(network.clone(), editor_api);
let preprocessor = preprocessor::Preprocessor::new();
preprocessor.preprocess(&mut wrapped_network, &ResourceRegistry::default()).expect("Failed to expand network"); // TODO: actually load the resources from the document

let compiler = Compiler {};
compiler.compile_single(wrapped_network).map_err(|x| x.into())
Comment on lines 247 to 251

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The variable wrapped_network is used on lines 246 and 249 but is never declared in this scope, which will cause a compilation error. You need to wrap network in the scope using wrap_network_in_scope before expanding it, similar to the changes made in runtime.rs and benchmark_util.rs.

	let mut wrapped_network = wrap_network_in_scope(network, editor_api);
	let preprocessor = preprocessor::Preprocessor::new();
	preprocessor.expand_network(&mut wrapped_network, &ResourceRegistry::default()).expect("Failed to expand network"); // TODO: actually load the resources from the document

	let compiler = Compiler {};
	compiler.compile_single(wrapped_network).map_err(|x| x.into())

Expand Down
6 changes: 3 additions & 3 deletions node-graph/interpreted-executor/benches/benchmark_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ use interpreted_executor::dynamic_executor::DynamicExecutor;
use interpreted_executor::util::wrap_network_in_scope;

pub fn setup_network(name: &str) -> (DynamicExecutor, ProtoNetwork) {
let mut network = load_from_name(name);
let network = load_from_name(name);
let editor_api = std::sync::Arc::new(EditorApi::default());
let mut network = wrap_network_in_scope(network, editor_api);
let preprocessor = preprocessor::Preprocessor::new();
preprocessor.expand_network(&mut network, &ResourceRegistry::default()).unwrap();
let network = wrap_network_in_scope(network, editor_api);
preprocessor.preprocess(&mut network, &ResourceRegistry::default()).unwrap();
let proto_network = compile(network);
let executor = block_on(DynamicExecutor::new(proto_network.clone())).unwrap();
(executor, proto_network)
Expand Down
7 changes: 2 additions & 5 deletions node-graph/interpreted-executor/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ use graphene_std::uuid::NodeId;
use std::sync::Arc;
use wgpu_executor::WgpuExecutor;

pub fn wrap_network_in_scope(mut network: NodeNetwork, editor_api: Arc<PlatformEditorApi>) -> NodeNetwork {
network.generate_node_paths(&[]);

pub fn wrap_network_in_scope(network: NodeNetwork, editor_api: Arc<PlatformEditorApi>) -> NodeNetwork {
let inner_network = DocumentNode {
implementation: DocumentNodeImplementation::Network(network),
inputs: vec![],
Expand Down Expand Up @@ -126,7 +124,6 @@ pub fn wrap_network_in_scope(mut network: NodeNetwork, editor_api: Arc<PlatformE
exports: vec![NodeInput::node(NodeId(1), 0)],
nodes: nodes.into_iter().enumerate().map(|(id, node)| (NodeId(id as u64), node)).collect(),
scope_injections: scope_injections.into_iter().collect(),
// TODO(TrueDoctor): check if it makes sense to set `generated` to `true`
generated: false,
generated: true,
}
}
10 changes: 0 additions & 10 deletions node-graph/nodes/gcore/src/debug.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,3 @@ fn unwrap_option<T: Default>(_: impl Ctx, #[implementations(Option<f64>, Option<
fn clone<'i, T: Clone + 'i>(_: impl Ctx, #[implementations(&List<Raster<CPU>>)] value: &'i T) -> T {
value.clone()
}

#[node_macro::node(category("Debug"), inject_scope)]
fn inject_scope_test_producer(_: impl Ctx) -> bool {
true
}

#[node_macro::node(category("Debug"))]
fn inject_scope_test_consumer(_: impl Ctx, _primary: (), #[scope(inject_scope_test_producer::IDENTIFIER)] injected: bool) -> bool {
injected
}
100 changes: 48 additions & 52 deletions node-graph/preprocessor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,59 +19,14 @@ pub struct Preprocessor {
}

impl Preprocessor {
pub fn expand_network(&self, network: &mut NodeNetwork, resources: &ResourceRegistry) -> Result<(), PreprocessorError> {
pub fn preprocess(&self, network: &mut NodeNetwork, resources: &ResourceRegistry) -> Result<(), PreprocessorError> {
self.insert_inject_scopes(network);
replace_resource_inputs(network, resources)?;
self.expand_network_inner(network);
self.replace_resource_inputs(network, resources)?;
self.expand_network(network);
Ok(())
}
}

/// Replace every `TaggedValue::Resource(hash)` input with a reference to a freshly inserted `resource` proto node.
fn replace_resource_inputs(network: &mut NodeNetwork, resources: &ResourceRegistry) -> Result<(), PreprocessorError> {
let mut hash_to_node_id: HashMap<graph_craft::application_io::resource::ResourceHash, NodeId> = HashMap::new();
let mut new_resource_nodes: Vec<(NodeId, DocumentNode)> = Vec::new();

for node in network.nodes.values_mut() {
if let DocumentNodeImplementation::Network(nested) = &mut node.implementation {
replace_resource_inputs(nested, resources)?;
continue;
}

if matches!(&node.implementation, DocumentNodeImplementation::ProtoNode(identifier) if *identifier == platform_application_io::resource::IDENTIFIER) {
continue;
}

for input in node.inputs.iter_mut() {
let NodeInput::Value { tagged_value, .. } = input else { continue };
let TaggedValue::Resource(resource_id) = **tagged_value else { continue };

let Some(hash) = resources.hash(&resource_id) else {
return Err(PreprocessorError::ResourceNotFound(resource_id));
};

let resource_id = *hash_to_node_id.entry(hash).or_insert_with(|| {
let id = NodeId::new();
let resource_node = DocumentNode {
inputs: vec![NodeInput::value(TaggedValue::ResourceHash(hash), false), NodeInput::scope("editor-api")],
implementation: DocumentNodeImplementation::ProtoNode(platform_application_io::resource::IDENTIFIER),
..Default::default()
};
new_resource_nodes.push((id, resource_node));
id
});

*input = NodeInput::node(resource_id, 0);
}
}

for (id, node) in new_resource_nodes {
network.nodes.insert(id, node);
}

Ok(())
}

impl Preprocessor {
fn insert_inject_scopes(&self, network: &mut NodeNetwork) {
for (identifier, (template, ty)) in self.inject_scopes.iter() {
Expand All @@ -85,14 +40,55 @@ impl Preprocessor {
}
}

fn expand_network_inner(&self, network: &mut NodeNetwork) {
if network.generated {
return;
/// Replace every `TaggedValue::Resource(hash)` input with a reference to a freshly inserted `resource` proto node.
fn replace_resource_inputs(&self, network: &mut NodeNetwork, resources: &ResourceRegistry) -> Result<(), PreprocessorError> {
let mut hash_to_node_id: HashMap<graph_craft::application_io::resource::ResourceHash, NodeId> = HashMap::new();
let mut new_resource_nodes: Vec<(NodeId, DocumentNode)> = Vec::new();

for node in network.nodes.values_mut() {
if let DocumentNodeImplementation::Network(nested) = &mut node.implementation {
self.replace_resource_inputs(nested, resources)?;
continue;
}

if matches!(&node.implementation, DocumentNodeImplementation::ProtoNode(identifier) if *identifier == platform_application_io::resource::IDENTIFIER) {
continue;
}

for input in node.inputs.iter_mut() {
let NodeInput::Value { tagged_value, .. } = input else { continue };
let TaggedValue::Resource(resource_id) = **tagged_value else { continue };

let Some(hash) = resources.hash(&resource_id) else {
return Err(PreprocessorError::ResourceNotFound(resource_id));
};

let resource_id = *hash_to_node_id.entry(hash).or_insert_with(|| {
let id = NodeId::new();
let resource_node = DocumentNode {
inputs: vec![NodeInput::value(TaggedValue::ResourceHash(hash), false), NodeInput::scope("editor-api")],
implementation: DocumentNodeImplementation::ProtoNode(platform_application_io::resource::IDENTIFIER),
..Default::default()
};
new_resource_nodes.push((id, resource_node));
id
});

*input = NodeInput::node(resource_id, 0);
}
}

for (id, node) in new_resource_nodes {
network.nodes.insert(id, node);
}

Ok(())
}

fn expand_network(&self, network: &mut NodeNetwork) {
for node in network.nodes.values_mut() {
match &mut node.implementation {
DocumentNodeImplementation::Network(node_network) => self.expand_network_inner(node_network),
DocumentNodeImplementation::Network(node_network) => self.expand_network(node_network),
DocumentNodeImplementation::ProtoNode(proto_node_identifier) => {
if let Some(new_node) = self.substitutions.get(proto_node_identifier) {
// Reconcile the document node's inputs with what the current node definition expects,
Expand Down
Loading