Migrate remaining node graph data types from Vec to Table#4067
Migrate remaining node graph data types from Vec to Table#4067
Conversation
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
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 removedNodePathandVecStringvariants can break loading older serialized documents. - Behavioral compatibility may be reduced after Vec→Table changes:
node-graph/nodes/gcore/src/animation.rsandnode-graph/nodes/vector/src/vector_nodes.rsappear to have droppedVec<f64>/f64/DVec2handling, 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.
There was a problem hiding this comment.
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.
Co-authored-by: Copilot <copilot@github.com>
Partly closes #3779.