Add node macro support for multi-output nodes, with a Destructure derive on the returned struct - #4557
Add node macro support for multi-output nodes, with a Destructure derive on the returned struct#4557Keavon wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
10 issues found across 22 files
Confidence score: 2/5
node-graph/preprocessor/Cargo.tomldeclarescore-typesonly as a dev dependency even thoughPreprocessor::new()uses it in production code, which can break normal builds; move it to[dependencies].node-graph/nodes/raster/src/adjustments.rsandnode-graph/nodes/gcore/src/extract_xy.rsremove or rename registered nodes without preserving their identifiers, so saved documents can fail registry lookup; retain compatibility nodes or add direct-node migrations.editor/src/messages/portfolio/document_migration.rsshifts Split Vec2 and Split Channels fields by one because the replacement reserves hidden output 0, potentially wiring migrated documents incorrectly; adjust migration output indices for the hidden primary output.node-graph/nodes/gcore/src/extract_xy.rsno longer supportsItem<IVec2>andItem<UVec2>, while the replaced node did, creating graph resolution failures; preserve the generic conversion implementations.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="node-graph/libraries/core-types/src/registry.rs">
<violation number="1" location="node-graph/libraries/core-types/src/registry.rs:95">
P3: The `Destructure` contract contradicts its metadata: the generated graph does carry the struct through a hidden internal output before extractors split it. Update this comment to distinguish hidden internal wires from user-visible outputs.</violation>
<violation number="2" location="node-graph/libraries/core-types/src/registry.rs:127">
P2: `MULTI_OUTPUT_NODES` freezes a snapshot of `NODE_METADATA` on first access and never refreshes, so any `destructure_output` node registered after that point is permanently missing from the map — including on wasm, where `register_metadata` runs only when invoked from JS (`#[cfg(target_family = "wasm")] extern "C"` shim in codegen), rather than at startup like the native `#[ctor]` path. The editor reads this map in `network_interface/view.rs`, so a stale snapshot silently degrades multi-output node behavior instead of failing loudly. Look up `NODE_METADATA` on each access (or rebuild the map after registration) rather than caching the snapshot in a `LazyLock`.</violation>
</file>
<file name="node-graph/nodes/raster/src/adjustments.rs">
<violation number="1" location="node-graph/nodes/raster/src/adjustments.rs:142">
P1: Removing the node macro from `extract_channel` unregisters the existing Extract Channel node, so saved documents and users of that node now fail registry lookup with `NoImplementations`. Keep a compatibility node under the old identifier and give the private split helper a different name, or add an explicit document migration.</violation>
<violation number="2" location="node-graph/nodes/raster/src/adjustments.rs:177">
P3: `split_channels` calls `extract_channel(image.clone(), ...)` for red, green, and blue, and `extract_channel` then overwrites every pixel of each clone with the extracted channel value. That makes three full deep copies of the pixel buffer that are written over and discarded immediately, tripling per-evaluation copy memory traffic for large images (the alpha channel reuses the moved original, but red/green/blue each pay a full `Raster` deep-copy plus the overwrite pass). Allocate the four output rasters directly and write only the per-pixel grayscale value into each (preferably in a single pass over the source) instead of cloning the source and re-stamping every pixel.</violation>
</file>
<file name="node-graph/node-macro/src/lib.rs">
<violation number="1" location="node-graph/node-macro/src/lib.rs:34">
P3: The default-output description contradicts itself: the node does have a primary output, but it is hidden and carries the whole struct. Describe it as a hidden primary output so users understand the output ordering and UI behavior.</violation>
</file>
<file name="node-graph/nodes/gcore/src/extract_xy.rs">
<violation number="1" location="node-graph/nodes/gcore/src/extract_xy.rs:30">
P1: When opening a document containing a direct `Extract XY` node, renaming the function changes its proto identifier and leaves that node unresolved because no direct-node migration preserves it. Retain a compatibility identifier or migrate the old node to an equivalent network that preserves its axis input.</violation>
<violation number="2" location="node-graph/nodes/gcore/src/extract_xy.rs:30">
P2: When a graph supplies an `Item<IVec2>` or `Item<UVec2>`, `Split Vec2` no longer has an implementation row even though the replaced `Extract XY` node accepted both types. Preserve the generic conversion implementations or provide equivalent typed adapter rows.</violation>
</file>
<file name="editor/src/messages/portfolio/document_migration.rs">
<violation number="1" location="editor/src/messages/portfolio/document_migration.rs:1269">
P1: When opening a document containing a legacy Split Vec2 or Split Channels wrapper, replacing the network with a destructured node shifts every field output by one: the new node reserves output 0 for the hidden struct. The existing downstream wires are left at their old indices, so they receive the wrong type/channel and the final field is not connected. Capture each old export's consumers and reconnect them to the corresponding new output (and update the output metadata) before or during this replacement.</violation>
</file>
<file name="node-graph/preprocessor/Cargo.toml">
<violation number="1" location="node-graph/preprocessor/Cargo.toml:22">
P0: `core-types` is used by production code, not just tests: `Preprocessor::new()` at src/lib.rs:118-119 calls `core_types::registry::NODE_REGISTRY` outside any `#[cfg(test)]` module. Declaring it only under `[dev-dependencies]` makes `error[E0433]: failed to resolve: use of unresolved crate or module 'core_types'` whenever the preprocessor library is built normally — e.g. plain `cargo build` or when editor/graphene-cli depend on this crate (dev-dependencies are never compiled for dependency crates). Move `core-types` into `[dependencies]`; all other new dev-dependencies (dyn-any, futures, glam, node-macro) are used only in the `#[cfg(test)]` module and are correctly placed.</violation>
</file>
<file name="editor/src/messages/portfolio/document/utility_types/network_interface/view.rs">
<violation number="1" location="editor/src/messages/portfolio/document/utility_types/network_interface/view.rs:233">
P3: The multi-output layout invariant is reimplemented in three places: `output_names` in document_node_derive.rs (`(!has_primary).then(String::new).chain(fields…)`), `number_of_outputs`/`hidden_primary_output` here in view.rs, and the `field_index` computation in resolved_types.rs. Each copy encodes `fields.len() + (has_primary ? 0 : 1)` and the 0/1-based output↔field mapping separately, linked only by comments. A future change to one site (e.g., reordering the hidden primary, or changing the `#[primary]` rule) silently breaks port counts and wire-type lookup in the others. Add `number_of_outputs()`, `is_primary_hidden()`, and `field_index_for_output(output_index)` methods on `DestructureMetadata` in core-types/src/registry.rs and call them from all three sites so the layout has a single source of truth.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| [dev-dependencies] | ||
| # Workspace dependencies | ||
| core-types = { workspace = true } |
There was a problem hiding this comment.
P0: core-types is used by production code, not just tests: Preprocessor::new() at src/lib.rs:118-119 calls core_types::registry::NODE_REGISTRY outside any #[cfg(test)] module. Declaring it only under [dev-dependencies] makes error[E0433]: failed to resolve: use of unresolved crate or module 'core_types' whenever the preprocessor library is built normally — e.g. plain cargo build or when editor/graphene-cli depend on this crate (dev-dependencies are never compiled for dependency crates). Move core-types into [dependencies]; all other new dev-dependencies (dyn-any, futures, glam, node-macro) are used only in the #[cfg(test)] module and are correctly placed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/preprocessor/Cargo.toml, line 22:
<comment>`core-types` is used by production code, not just tests: `Preprocessor::new()` at src/lib.rs:118-119 calls `core_types::registry::NODE_REGISTRY` outside any `#[cfg(test)]` module. Declaring it only under `[dev-dependencies]` makes `error[E0433]: failed to resolve: use of unresolved crate or module 'core_types'` whenever the preprocessor library is built normally — e.g. plain `cargo build` or when editor/graphene-cli depend on this crate (dev-dependencies are never compiled for dependency crates). Move `core-types` into `[dependencies]`; all other new dev-dependencies (dyn-any, futures, glam, node-macro) are used only in the `#[cfg(test)]` module and are correctly placed.</comment>
<file context>
@@ -16,3 +16,11 @@ log = { workspace = true }
+
+[dev-dependencies]
+# Workspace dependencies
+core-types = { workspace = true }
+dyn-any = { workspace = true }
+futures = { workspace = true }
</file context>
| input.element_mut().adjust(|color| { | ||
| /// Extracts one color channel as a grayscale image. Used internally by the `split_channels` node. | ||
| #[cfg(feature = "std")] | ||
| fn extract_channel<T: Adjust<Color>>(mut input: T, channel: RedGreenBlueAlpha) -> T { |
There was a problem hiding this comment.
P1: Removing the node macro from extract_channel unregisters the existing Extract Channel node, so saved documents and users of that node now fail registry lookup with NoImplementations. Keep a compatibility node under the old identifier and give the private split helper a different name, or add an explicit document migration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/raster/src/adjustments.rs, line 142:
<comment>Removing the node macro from `extract_channel` unregisters the existing Extract Channel node, so saved documents and users of that node now fail registry lookup with `NoImplementations`. Keep a compatibility node under the old identifier and give the private split helper a different name, or add an explicit document migration.</comment>
<file context>
@@ -137,18 +137,10 @@ fn gamma_correction<T: Adjust<Color>>(
- input.element_mut().adjust(|color| {
+/// Extracts one color channel as a grayscale image. Used internally by the `split_channels` node.
+#[cfg(feature = "std")]
+fn extract_channel<T: Adjust<Color>>(mut input: T, channel: RedGreenBlueAlpha) -> T {
+ input.adjust(|color| {
let extracted_value = match channel {
</file context>
| /// | ||
| /// The inverse of this node is **Combine Vec2**, which composes a vec2 from its X and Y components. | ||
| #[node_macro::node(name("Split Vec2"), category("Math: Vec2"), destructure_output)] | ||
| fn split_vec2(_: impl Ctx, #[name("Vec2")] vec2: Item<DVec2>) -> Vec2Components { |
There was a problem hiding this comment.
P1: When opening a document containing a direct Extract XY node, renaming the function changes its proto identifier and leaves that node unresolved because no direct-node migration preserves it. Retain a compatibility identifier or migrate the old node to an equivalent network that preserves its axis input.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/gcore/src/extract_xy.rs, line 30:
<comment>When opening a document containing a direct `Extract XY` node, renaming the function changes its proto identifier and leaves that node unresolved because no direct-node migration preserves it. Retain a compatibility identifier or migrate the old node to an equivalent network that preserves its axis input.</comment>
<file context>
@@ -29,3 +13,25 @@ pub enum XY {
+///
+/// The inverse of this node is **Combine Vec2**, which composes a vec2 from its X and Y components.
+#[node_macro::node(name("Split Vec2"), category("Math: Vec2"), destructure_output)]
+fn split_vec2(_: impl Ctx, #[name("Vec2")] vec2: Item<DVec2>) -> Vec2Components {
+ let (vec2, attributes) = vec2.into_parts();
+
</file context>
| let new_reference = DefinitionIdentifier::ProtoNode(new_identifier.clone()); | ||
| let Some(definition) = resolve_document_node_type(&new_reference) else { continue }; | ||
| let mut node_template = definition.default_node_template(); | ||
| document.network_interface.replace_implementation(node_id, network_path, &mut node_template); |
There was a problem hiding this comment.
P1: When opening a document containing a legacy Split Vec2 or Split Channels wrapper, replacing the network with a destructured node shifts every field output by one: the new node reserves output 0 for the hidden struct. The existing downstream wires are left at their old indices, so they receive the wrong type/channel and the final field is not connected. Capture each old export's consumers and reconnect them to the corresponding new output (and update the output metadata) before or during this replacement.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document_migration.rs, line 1269:
<comment>When opening a document containing a legacy Split Vec2 or Split Channels wrapper, replacing the network with a destructured node shifts every field output by one: the new node reserves output 0 for the hidden struct. The existing downstream wires are left at their old indices, so they receive the wrong type/channel and the final field is not connected. Capture each old export's consumers and reconnect them to the corresponding new output (and update the output metadata) before or during this replacement.</comment>
<file context>
@@ -1264,6 +1244,96 @@ pub fn document_migration_upgrades(document: &mut DocumentMessageHandler, reset_
+ let new_reference = DefinitionIdentifier::ProtoNode(new_identifier.clone());
+ let Some(definition) = resolve_document_node_type(&new_reference) else { continue };
+ let mut node_template = definition.default_node_template();
+ document.network_interface.replace_implementation(node_id, network_path, &mut node_template);
+ let Some(old_inputs) = document.network_interface.replace_inputs(node_id, network_path, &mut node_template) else {
+ continue;
</file context>
| /// | ||
| /// The inverse of this node is **Combine Vec2**, which composes a vec2 from its X and Y components. | ||
| #[node_macro::node(name("Split Vec2"), category("Math: Vec2"), destructure_output)] | ||
| fn split_vec2(_: impl Ctx, #[name("Vec2")] vec2: Item<DVec2>) -> Vec2Components { |
There was a problem hiding this comment.
P2: When a graph supplies an Item<IVec2> or Item<UVec2>, Split Vec2 no longer has an implementation row even though the replaced Extract XY node accepted both types. Preserve the generic conversion implementations or provide equivalent typed adapter rows.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/gcore/src/extract_xy.rs, line 30:
<comment>When a graph supplies an `Item<IVec2>` or `Item<UVec2>`, `Split Vec2` no longer has an implementation row even though the replaced `Extract XY` node accepted both types. Preserve the generic conversion implementations or provide equivalent typed adapter rows.</comment>
<file context>
@@ -29,3 +13,25 @@ pub enum XY {
+///
+/// The inverse of this node is **Combine Vec2**, which composes a vec2 from its X and Y components.
+#[node_macro::node(name("Split Vec2"), category("Math: Vec2"), destructure_output)]
+fn split_vec2(_: impl Ctx, #[name("Vec2")] vec2: Item<DVec2>) -> Vec2Components {
+ let (vec2, attributes) = vec2.into_parts();
+
</file context>
|
|
||
| /// A struct of wires returned by a multi-output node, implemented by `#[derive(node_macro::Destructure)]`. | ||
| /// | ||
| /// Each field is an `Item<T>` or `List<T>` wire that becomes one output connector. The struct itself never travels on a wire: |
There was a problem hiding this comment.
P3: The Destructure contract contradicts its metadata: the generated graph does carry the struct through a hidden internal output before extractors split it. Update this comment to distinguish hidden internal wires from user-visible outputs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/core-types/src/registry.rs, line 95:
<comment>The `Destructure` contract contradicts its metadata: the generated graph does carry the struct through a hidden internal output before extractors split it. Update this comment to distinguish hidden internal wires from user-visible outputs.</comment>
<file context>
@@ -59,12 +61,78 @@ pub enum RegistryValueSource {
+
+/// A struct of wires returned by a multi-output node, implemented by `#[derive(node_macro::Destructure)]`.
+///
+/// Each field is an `Item<T>` or `List<T>` wire that becomes one output connector. The struct itself never travels on a wire:
+/// the Graphene preprocessor expands the node into the derive's generated extractor nodes, one per field.
+pub trait Destructure: Sized {
</file context>
| /// Each field is an `Item<T>` or `List<T>` wire that becomes one output connector. The struct itself never travels on a wire: | |
| /// Each field is an `Item<T>` or `List<T>` wire that becomes one output connector. The generated graph carries the struct through a hidden internal output before extractors split it: |
| /// Output names default to the field name converted to title case. Use `#[name("...")]` on a field to override that | ||
| /// when the automatic conversion doesn't format correctly. Doc comments on fields are recorded as connector descriptions. | ||
| /// | ||
| /// By default the node has no primary output: a hidden primary output carries the whole struct and the fields appear as |
There was a problem hiding this comment.
P3: The default-output description contradicts itself: the node does have a primary output, but it is hidden and carries the whole struct. Describe it as a hidden primary output so users understand the output ordering and UI behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/node-macro/src/lib.rs, line 34:
<comment>The default-output description contradicts itself: the node does have a primary output, but it is hidden and carries the whole struct. Describe it as a hidden primary output so users understand the output ordering and UI behavior.</comment>
<file context>
@@ -18,6 +19,61 @@ pub fn node(attr: TokenStream, item: TokenStream) -> TokenStream {
+/// Output names default to the field name converted to title case. Use `#[name("...")]` on a field to override that
+/// when the automatic conversion doesn't format correctly. Doc comments on fields are recorded as connector descriptions.
+///
+/// By default the node has no primary output: a hidden primary output carries the whole struct and the fields appear as
+/// secondary outputs. Marking at most one field with `#[primary]` makes that field the node's primary output instead.
+///
</file context>
| /// By default the node has no primary output: a hidden primary output carries the whole struct and the fields appear as | |
| /// By default, the node's primary output is hidden and carries the whole struct; the fields appear as |
|
|
||
| // Each channel image keeps the source image's attributes, such as its transform | ||
| ImageChannels { | ||
| red: Item::from_parts(extract_channel(image.clone(), RedGreenBlueAlpha::Red), attributes.clone()), |
There was a problem hiding this comment.
P3: split_channels calls extract_channel(image.clone(), ...) for red, green, and blue, and extract_channel then overwrites every pixel of each clone with the extracted channel value. That makes three full deep copies of the pixel buffer that are written over and discarded immediately, tripling per-evaluation copy memory traffic for large images (the alpha channel reuses the moved original, but red/green/blue each pay a full Raster deep-copy plus the overwrite pass). Allocate the four output rasters directly and write only the per-pixel grayscale value into each (preferably in a single pass over the source) instead of cloning the source and re-stamping every pixel.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/raster/src/adjustments.rs, line 177:
<comment>`split_channels` calls `extract_channel(image.clone(), ...)` for red, green, and blue, and `extract_channel` then overwrites every pixel of each clone with the extracted channel value. That makes three full deep copies of the pixel buffer that are written over and discarded immediately, tripling per-evaluation copy memory traffic for large images (the alpha channel reuses the moved original, but red/green/blue each pay a full `Raster` deep-copy plus the overwrite pass). Allocate the four output rasters directly and write only the per-pixel grayscale value into each (preferably in a single pass over the source) instead of cloning the source and re-stamping every pixel.</comment>
<file context>
@@ -160,6 +152,35 @@ fn extract_channel<T: Adjust<Color>>(
+
+ // Each channel image keeps the source image's attributes, such as its transform
+ ImageChannels {
+ red: Item::from_parts(extract_channel(image.clone(), RedGreenBlueAlpha::Red), attributes.clone()),
+ green: Item::from_parts(extract_channel(image.clone(), RedGreenBlueAlpha::Green), attributes.clone()),
+ blue: Item::from_parts(extract_channel(image.clone(), RedGreenBlueAlpha::Blue), attributes.clone()),
</file context>
| DocumentNodeImplementation::Network(nested_network) => nested_network.exports.len(), | ||
| // A multi-output proto node (declared `destructure_output`) has one output per field of the struct it returns, | ||
| // preceded by a hidden primary output carrying the struct itself unless one field is marked `#[primary]` | ||
| DocumentNodeImplementation::ProtoNode(identifier) => match MULTI_OUTPUT_NODES.get(identifier) { |
There was a problem hiding this comment.
P3: The multi-output layout invariant is reimplemented in three places: output_names in document_node_derive.rs ((!has_primary).then(String::new).chain(fields…)), number_of_outputs/hidden_primary_output here in view.rs, and the field_index computation in resolved_types.rs. Each copy encodes fields.len() + (has_primary ? 0 : 1) and the 0/1-based output↔field mapping separately, linked only by comments. A future change to one site (e.g., reordering the hidden primary, or changing the #[primary] rule) silently breaks port counts and wire-type lookup in the others. Add number_of_outputs(), is_primary_hidden(), and field_index_for_output(output_index) methods on DestructureMetadata in core-types/src/registry.rs and call them from all three sites so the layout has a single source of truth.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/utility_types/network_interface/view.rs, line 233:
<comment>The multi-output layout invariant is reimplemented in three places: `output_names` in document_node_derive.rs (`(!has_primary).then(String::new).chain(fields…)`), `number_of_outputs`/`hidden_primary_output` here in view.rs, and the `field_index` computation in resolved_types.rs. Each copy encodes `fields.len() + (has_primary ? 0 : 1)` and the 0/1-based output↔field mapping separately, linked only by comments. A future change to one site (e.g., reordering the hidden primary, or changing the `#[primary]` rule) silently breaks port counts and wire-type lookup in the others. Add `number_of_outputs()`, `is_primary_hidden()`, and `field_index_for_output(output_index)` methods on `DestructureMetadata` in core-types/src/registry.rs and call them from all three sites so the layout has a single source of truth.</comment>
<file context>
@@ -228,6 +228,12 @@ impl<'a, 'p> NetworkView<'a, 'p> {
DocumentNodeImplementation::Network(nested_network) => nested_network.exports.len(),
+ // A multi-output proto node (declared `destructure_output`) has one output per field of the struct it returns,
+ // preceded by a hidden primary output carrying the struct itself unless one field is marked `#[primary]`
+ DocumentNodeImplementation::ProtoNode(identifier) => match MULTI_OUTPUT_NODES.get(identifier) {
+ Some(metadata) => metadata.fields.len() + if metadata.has_primary { 0 } else { 1 },
+ None => 1,
</file context>
dad080b to
20b476d
Compare
Closes #2517
Influenced by @TrueDoctor's incomplete commit 2e97e81