Fix gradient chain metadata sync for exposed Fill nodes#4315
Fix gradient chain metadata sync for exposed Fill nodes#4315YohYamasaki wants to merge 5 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces changes to keep the rendered gradient and the Gradient tool state in sync. It adds a normalization step after input connections (NormalizeAfterInputConnection) to reset gradient metadata on a Fill node when a gradient value chain is connected, and inserts a Transform node for chain-backed gradients if one is missing. The review feedback points out an inconsistency in the graph traversal flow type, suggesting the use of FlowType::HorizontalFlow instead of FlowType::PrimaryFlow to align with other parts of the codebase.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 6 files
Confidence score: 3/5
editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rsnow emitsGraphOperationMessage::NormalizeAfterInputConnectionfromNodeGraphMessageHandler, which conflicts with the documented layering invariant and risks further cross-layer coupling/regressions in message flow as this path evolves — move the normalization trigger into the graph-operation layer (or update architecture boundaries explicitly) before merging.editor/src/messages/tool/tool_messages/gradient_tool.rsapplies Transform insertion only in the fallbackPointerDownpath and may queueGradientTransformSetoutsideStartTransaction, so similar gradient interactions can behave inconsistently and leave changes outside expected undo/abort semantics — make all relevantPointerDownbranches follow the same insertion path and wrap the transform mutation inside the transaction lifecycle.editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rsdropsoutput_indexwhen checking upstream metadata initialization, so multi-output nodes can resolve from the wrong port and initialize metadata from an unintended data flow — include the output port in upstream traversal/state lookup before merging.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
YohYamasaki
force-pushed
the
sync-gradient-chain-metadata-for-exposed-fill
branch
from
2c7bcb3 to
6acf3b6
Compare
|
@cubic-dev-ai review |
@YohYamasaki I have started the AI code review. It will take a few minutes to complete. |
| // Post wire creation process | ||
| let InputConnector::Node { node_id, input_index } = input_connector else { return }; | ||
| // Fill node: When a Gradient Value chain is connected to a Fill node's paint input, reset the hidden gradient metadata. | ||
| // This prevents the rendered gradient and Gradient tool state from getting out of sync. | ||
| if network_interface.reference(&node_id, selection_network_path).as_ref() == Some(&DefinitionIdentifier::ProtoNode(graphene_std::vector::fill::IDENTIFIER)) |
There was a problem hiding this comment.
Fill or gradient specific functionality should not be inside the generic NodeGraphMessage::CreateWire. This creates a spaghetti of dependencies. Please write code related to the gradient tool inside the gradient tool.
Thanks.
There was a problem hiding this comment.
I see, thanks for the comment. I'll find the alternative way.
There was a problem hiding this comment.
Thanks for your understanding. Hopefully it is clear why I think having seemingly generic node graph changes silently mutate specific graphic nodes is undesirable for maintainability.
Looking at the fill node, it seems the current order of priority appears to be:
- Attributes on the
List<GradientStops> - Hidden inputs on the fill node
- For transform only (if value is
None) then the bounding box of the layer
The concept of having the hidden inputs to the fill node seems rather confusing. My preferred solution would be to remove these altogether. This would involve the editor tooling setting attributes on the List<GradientStops>. This simplifies the data model and reduces inconsistencies.
However thanks to @Keavon's incomplete refactor (5 months it has had 3 names) it seems that is impossible to set the attributes on a tagged value that get serialized.
An alternative solution the gradient tool read the hidden inputs where appropriate. The gradient tooling should be able to determine if any of the attributes are set (either by traversing the graph looking for known nodes or using the monitor node system). Therefore it can fall back to the fill node inputs. This removes inconsistencies with the tooling and results. However it may sometimes result in confusing behaviour (e.g. if the hidden transform input is set).
There was a problem hiding this comment.
I also think the solution you mentioned is the way to go at this moment. Thanks for the detailed explanation.
However it may sometimes result in confusing behaviour (e.g. if the hidden transform input is set).
I think so too, and it was the reason why the hidden input reset was initially implemented in NodeGraphMessage::CreateWire. But it should be possible to defer this to the interaction point of the tool.
2 participants
Closes #4314
This fix inserts a Transform node only when the Gradient tool is selected. The first try was inserting a Transform node when wiring Gradient Value -> Fill if no transform attribute exists. This kept the tool and node state synchronized best, but was not intuitive UX wise because of the sudden node insertion.
Screen.Recording.2026-07-07.at.18.02.55.mov