Skip to content

Migrate remaining node graph data types from Vec to Table#4067

Merged
Keavon merged 10 commits intomasterfrom
migrate-vec-to-table
Apr 28, 2026
Merged

Migrate remaining node graph data types from Vec to Table#4067
Keavon merged 10 commits intomasterfrom
migrate-vec-to-table

Conversation

@Keavon
Copy link
Copy Markdown
Member

@Keavon Keavon commented Apr 28, 2026

Partly closes #3779.

@Keavon Keavon changed the title Migrate vec to table Migrate remaining node graph data types from Vec to Table Apr 28, 2026
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

5 issues found across 24 files

Confidence score: 3/5

  • There is meaningful regression risk: in node-graph/graph-craft/src/document/value.rs, missing serde alias/deserialization migrations for removed NodePath and VecString variants can break loading older serialized documents.
  • Behavioral compatibility may be reduced after Vec→Table changes: node-graph/nodes/gcore/src/animation.rs and node-graph/nodes/vector/src/vector_nodes.rs appear to have dropped Vec<f64>/f64/DVec2 handling, which can affect existing numeric and point graph workflows.
  • This is not necessarily merge-blocking, but with multiple medium-to-high severity migration issues, the PR carries some user-facing risk until compatibility gaps are addressed.
  • Pay close attention to node-graph/graph-craft/src/document/value.rs, node-graph/nodes/gcore/src/animation.rs, node-graph/nodes/vector/src/vector_nodes.rs, editor/src/messages/portfolio/document/data_panel/data_panel_message_handler.rs - serialization migration and type-dispatch gaps may break existing graphs and table attribute behavior.
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/nodes/gcore/src/animation.rs">

<violation number="1" location="node-graph/nodes/gcore/src/animation.rs:76">
P2: The vec-to-table migration removed numeric collection support from both quantize nodes (`Vec<f64>` was deleted but not replaced with `Table<f64>`), which likely breaks existing numeric-table use cases.</violation>
</file>

<file name="node-graph/nodes/vector/src/vector_nodes.rs">

<violation number="1" location="node-graph/nodes/vector/src/vector_nodes.rs:2867">
P2: `count_elements` dropped `f64`/`DVec2` support during Vec→Table migration, which can break existing numeric/point graphs.</violation>
</file>

<file name="editor/src/messages/portfolio/document/data_panel/data_panel_message_handler.rs">

<violation number="1" location="editor/src/messages/portfolio/document/data_panel/data_panel_message_handler.rs:175">
P2: Attribute dispatch omits `u8` and `NodeId`, so their new `TableRowLayout` behavior is never used for table attributes.</violation>
</file>

<file name="node-graph/graph-craft/src/document/value.rs">

<violation number="1" location="node-graph/graph-craft/src/document/value.rs:171">
P1: Missing serde alias and deserialization migration for the removed `VecString` variant. Other migrated types (e.g., `F64Table` with alias `"VecF64"`, `BrushStrokeTable` with alias `"BrushStrokes"`) include migration attributes. Without a `#[serde(alias = "VecString")]` and a `deserialize_with` function that handles `Vec<String>` → `Table<String>`, old documents containing `VecString` will fail to deserialize.</violation>

<violation number="2" location="node-graph/graph-craft/src/document/value.rs:175">
P1: Missing serde alias and deserialization migration for the removed `NodePath` variant. Following the pattern of other migrated types in this PR, `NodeIdTable` should include `#[serde(alias = "NodePath")]` and a `deserialize_with` function to convert `Vec<NodeId>` → `Table<NodeId>` so old documents remain deserializable.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread node-graph/graph-craft/src/document/value.rs
Comment thread node-graph/graph-craft/src/document/value.rs
Comment thread node-graph/nodes/gcore/src/animation.rs Outdated
Comment thread node-graph/nodes/vector/src/vector_nodes.rs
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request replaces various Vec<T> and array types with Table<T> across the node graph and editor to unify data structures. Key changes include updating TaggedValue variants, implementing new migration logic for backward compatibility, and adjusting node implementations to handle table-based inputs. Feedback highlights critical issues regarding the removal of the "Spline" node migration, missing serialization aliases for StringTable and NodeIdTable which will break document loading, and missing type implementations for several nodes like index_elements and count_elements that were previously supported.

Comment thread editor/src/messages/portfolio/document_migration.rs
Comment thread node-graph/graph-craft/src/document/value.rs
Comment thread node-graph/nodes/graphic/src/graphic.rs Outdated
Comment thread node-graph/nodes/vector/src/vector_nodes.rs
Co-authored-by: Copilot <copilot@github.com>
@Keavon Keavon merged commit b2389f6 into master Apr 28, 2026
10 of 11 checks passed
@Keavon Keavon deleted the migrate-vec-to-table branch April 28, 2026 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Data trees

1 participant