Fix hardcoded input indices#3829
Conversation
Summary of ChangesHello @Tyooughtul, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors how node input indices are handled across the codebase. By replacing magic numbers with named constants, the changes significantly improve the clarity and robustness of the graph operations. This ensures that future modifications to node structures are less prone to errors caused by incorrect input indexing, making the system more resilient and easier to debug. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 does a great job of replacing hardcoded numeric indices with named constants, which significantly improves code readability and maintainability. I've identified a couple of areas for further improvement: one function can be simplified by using imported constants directly, and there's a recurring typo in an imported name that should ideally be fixed upstream. Additionally, it looks like some commented-out debug code was unintentionally left in a test file.
editor/src/messages/portfolio/document/graph_operation/transform_utils.rs
Show resolved
Hide resolved
| const TRANSLATION_INDEX: usize = 1; | ||
| const ROTATION_INDEX: usize = 2; | ||
| const SCALE_INDEX: usize = 3; | ||
| const SKEW_INDEX: usize = 4; | ||
|
|
||
| let translation = if let Some(&TaggedValue::DVec2(translation)) = inputs[TRANSLATION_INDEX].as_value() { | ||
| translation | ||
| } else { | ||
| DVec2::ZERO | ||
| }; | ||
| let rotation = if let Some(&TaggedValue::F64(rotation)) = inputs[2].as_value() { rotation } else { 0. }; | ||
| let scale = if let Some(&TaggedValue::DVec2(scale)) = inputs[3].as_value() { scale } else { DVec2::ONE }; | ||
| let shear = if let Some(&TaggedValue::DVec2(shear)) = inputs[4].as_value() { shear } else { DVec2::ZERO }; | ||
| let rotation = if let Some(&TaggedValue::F64(rotation)) = inputs[ROTATION_INDEX].as_value() { | ||
| rotation | ||
| } else { | ||
| 0. | ||
| }; | ||
| let scale = if let Some(&TaggedValue::DVec2(scale)) = inputs[SCALE_INDEX].as_value() { | ||
| scale | ||
| } else { | ||
| DVec2::ONE | ||
| }; | ||
| let shear = if let Some(&TaggedValue::DVec2(shear)) = inputs[SKEW_INDEX].as_value() { | ||
| shear | ||
| } else { | ||
| DVec2::ZERO | ||
| }; |
There was a problem hiding this comment.
Since graphene_std::transform_nodes::transform::* is already imported, you can use the INDEX constants from TranslationInput, RotationInput, ScaleInput, and SkewInput directly instead of defining local constants. This improves maintainability by using the single source of truth from the node definitions.
| const TRANSLATION_INDEX: usize = 1; | |
| const ROTATION_INDEX: usize = 2; | |
| const SCALE_INDEX: usize = 3; | |
| const SKEW_INDEX: usize = 4; | |
| let translation = if let Some(&TaggedValue::DVec2(translation)) = inputs[TRANSLATION_INDEX].as_value() { | |
| translation | |
| } else { | |
| DVec2::ZERO | |
| }; | |
| let rotation = if let Some(&TaggedValue::F64(rotation)) = inputs[2].as_value() { rotation } else { 0. }; | |
| let scale = if let Some(&TaggedValue::DVec2(scale)) = inputs[3].as_value() { scale } else { DVec2::ONE }; | |
| let shear = if let Some(&TaggedValue::DVec2(shear)) = inputs[4].as_value() { shear } else { DVec2::ZERO }; | |
| let rotation = if let Some(&TaggedValue::F64(rotation)) = inputs[ROTATION_INDEX].as_value() { | |
| rotation | |
| } else { | |
| 0. | |
| }; | |
| let scale = if let Some(&TaggedValue::DVec2(scale)) = inputs[SCALE_INDEX].as_value() { | |
| scale | |
| } else { | |
| DVec2::ONE | |
| }; | |
| let shear = if let Some(&TaggedValue::DVec2(shear)) = inputs[SKEW_INDEX].as_value() { | |
| shear | |
| } else { | |
| DVec2::ZERO | |
| }; | |
| let translation = if let Some(&TaggedValue::DVec2(translation)) = inputs[TranslationInput::INDEX].as_value() { | |
| translation | |
| } else { | |
| DVec2::ZERO | |
| }; | |
| let rotation = if let Some(&TaggedValue::F64(rotation)) = inputs[RotationInput::INDEX].as_value() { | |
| rotation | |
| } else { | |
| 0. | |
| }; | |
| let scale = if let Some(&TaggedValue::DVec2(scale)) = inputs[ScaleInput::INDEX].as_value() { | |
| scale | |
| } else { | |
| DVec2::ONE | |
| }; | |
| let shear = if let Some(&TaggedValue::DVec2(shear)) = inputs[SkewInput::INDEX].as_value() { | |
| shear | |
| } else { | |
| DVec2::ZERO | |
| }; |
editor/src/messages/tool/common_functionality/gizmos/shape_gizmos/circle_arc_radius_handle.rs
Show resolved
Hide resolved
editor/src/messages/tool/common_functionality/gizmos/shape_gizmos/number_of_points_dial.rs
Show resolved
Hide resolved
editor/src/messages/tool/common_functionality/gizmos/shape_gizmos/sweep_angle_gizmo.rs
Show resolved
Hide resolved
| use glam::DAffine2; | ||
| use graph_craft::document::NodeInput; | ||
| use graph_craft::document::value::TaggedValue; | ||
| use graphene_std::NodeInputDecleration; |
There was a problem hiding this comment.
There seems to be a typo in NodeInputDecleration. It should likely be NodeInputDeclaration. This might be an issue in the upstream graphene_std crate that should be corrected.
| use graphene_std::NodeInputDecleration; | |
| use graphene_std::NodeInputDeclaration; |
| use glam::DAffine2; | ||
| use graph_craft::document::NodeInput; | ||
| use graph_craft::document::value::TaggedValue; | ||
| use graphene_std::NodeInputDecleration; |
There was a problem hiding this comment.
There seems to be a typo in NodeInputDecleration. It should likely be NodeInputDeclaration. This might be an issue in the upstream graphene_std crate that should be corrected.
| use graphene_std::NodeInputDecleration; | |
| use graphene_std::NodeInputDeclaration; |
There was a problem hiding this comment.
this is a module introduced in March 2025
|
(you don't have to follow the AI review comments, they are advisory only) |
Tyooughtul
force-pushed
the
fix_hardcoded_input_indices
branch
from
afeccc9 to
8acf3db
Compare
|
This is a surprisingly small number of changes. Does this comprehensively capture every instance, or is this just progress towards that goal? |
Thanks for the review! 😊 The left hardcoded 0 and 1 indices are primary input for main data flow into any node and index 1 is the secondary input used by layer nodes for chain stacking. As for Artboard's location (2) and dimensions (3), I found that LocationInput::INDEX and DimensionsInput::INDEX simply don't exist anywhere in the codebase.The node macro apparently didn't generate these constants for the artboard node, so those two may not be converted without first adding that infrastructure. |
Tyooughtul
force-pushed
the
fix_hardcoded_input_indices
branch
from
8acf3db to
15b6238
Compare
|
@Ayush2k02 @Keavon Thanks for review! There is a precision test error involved here, and I have now fixed it all at once. Please have a look when you have time. 😊 |
|
Thanks for the updates. To partially (but not fully) respond to your previous answer to my question, I will emphasize that the original issue describes a need for all nodes (both proto nodes and document nodes) to only use consts. For the document nodes, those will have to be written as a |
Tyooughtul
force-pushed
the
fix_hardcoded_input_indices
branch
from
15b6238 to
cfb244e
Compare
Tyooughtul
force-pushed
the
fix_hardcoded_input_indices
branch
from
cfb244e to
4f24398
Compare
Sorry, I think I misunderstood you before. I thought it was only necessary to modify the hard-coding of the function signature😞. This time I made more changes, rg verification was used in different ways, the remaining ones are not parameter indexes. Also, I have a question: Thanks! |
Tyooughtul
force-pushed
the
fix_hardcoded_input_indices
branch
from
4f24398 to
c0d60b7
Compare
| 0.78353804, 0.79129815, 0.79910296, 0.8069525, 0.8148468, 0.822786, 0.8307701, 0.83879924, 0.84687346, | ||
| 0.8549928, 0.8631574, 0.87136734, 0.8796226, 0.8879232, 0.89626956, 0.90466136, 0.913099, 0.92158204, | ||
| 0.93011117, 0.9386859, 0.9473069, 0.9559735, 0.9646866, 0.9734455, 0.98225087, 0.9911022, 1., | ||
| f32::from_bits(0x00000000), f32::from_bits(0x399f22b4), f32::from_bits(0x3a1f22b4), f32::from_bits(0x3a6eb40f), |
There was a problem hiding this comment.
Uh, what's with all the changes in this file?
There was a problem hiding this comment.
Uh, what's with all the changes in this file?
This resolves the error image I posted under #1490. It offers better accuracy. If constants were used, there might be some errors due to compilation or platform issues. Drawback might be that there is an issue with readability.🤔
There was a problem hiding this comment.
I see. I'd suggest you stash those changes and maybe later we open a PR for it. But this isn't affected by your PR and I don't think I have ever seen it fail locally or in CI, so it might be something unique to your machine. But it should be unrelated to this entire PR's focus.
![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 32 files
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="editor/src/messages/portfolio/document/graph_operation/utility_types.rs">
<violation number="1" location="editor/src/messages/portfolio/document/graph_operation/utility_types.rs:74">
P2: Duplicated local `const LAYER_SECONDARY_INPUT_INDEX` defined in two separate methods. Consider defining it once at module level (or as an associated constant like `InputConnector::PRIMARY_INPUT_INDEX`) so both call sites share a single source of truth, consistent with how other index constants are handled in this PR.</violation>
</file>
<file name="editor/src/messages/tool/common_functionality/shapes/polygon_shape.rs">
<violation number="1" location="editor/src/messages/tool/common_functionality/shapes/polygon_shape.rs:179">
P2: Using `regular_polygon::SidesInput::<u32>::INDEX` to index into star node inputs is fragile. When the star node is found (via the `.or()` fallback), this uses the polygon module's index constant for the star's inputs. Consider distinguishing which node was found and using the corresponding module's `SidesInput::INDEX`, e.g. `graphene_std::vector::generator_nodes::star::SidesInput::<u32>::INDEX` for star nodes.</violation>
</file>
<file name="editor/src/messages/tool/common_functionality/shapes/rectangle_shape.rs">
<violation number="1" location="editor/src/messages/tool/common_functionality/shapes/rectangle_shape.rs:12">
P2: Typo in trait name: `NodeInputDecleration` should be `NodeInputDeclaration`. "Decleration" is a misspelling of "Declaration". If the trait definition also uses this misspelling, it should be corrected there as well.
(Based on your team's feedback about using consistent, precise terminology in names and docs.) [FEEDBACK_USED]</violation>
</file>
<file name="editor/src/messages/tool/common_functionality/gizmos/shape_gizmos/number_of_points_dial.rs">
<violation number="1" location="editor/src/messages/tool/common_functionality/gizmos/shape_gizmos/number_of_points_dial.rs:205">
P2: This uses `star::SidesInput::<u32>::INDEX` but `node_id` may refer to a polygon node (via the `.or(get_polygon_id(...))` fallback on line 199). If the polygon node's sides input index differs from the star's, this will silently set the wrong input. Consider branching on which node was found and using the corresponding node's `SidesInput::INDEX` constant.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| InputConnector::Export(0) | ||
| } else { | ||
| InputConnector::node(parent.to_node(), 1) | ||
| const LAYER_SECONDARY_INPUT_INDEX: usize = 1; |
There was a problem hiding this comment.
P2: Duplicated local const LAYER_SECONDARY_INPUT_INDEX defined in two separate methods. Consider defining it once at module level (or as an associated constant like InputConnector::PRIMARY_INPUT_INDEX) so both call sites share a single source of truth, consistent with how other index constants are handled in this PR.
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/graph_operation/utility_types.rs, line 74:
<comment>Duplicated local `const LAYER_SECONDARY_INPUT_INDEX` defined in two separate methods. Consider defining it once at module level (or as an associated constant like `InputConnector::PRIMARY_INPUT_INDEX`) so both call sites share a single source of truth, consistent with how other index constants are handled in this PR.</comment>
<file context>
@@ -71,7 +71,8 @@ impl<'a> ModifyInputsContext<'a> {
InputConnector::Export(0)
} else {
- InputConnector::node(parent.to_node(), 1)
+ const LAYER_SECONDARY_INPUT_INDEX: usize = 1;
+ InputConnector::node(parent.to_node(), LAYER_SECONDARY_INPUT_INDEX)
};
</file context>
| }; | ||
|
|
||
| let Some(&TaggedValue::U32(n)) = node_inputs.get(1).unwrap().as_value() else { | ||
| let Some(&TaggedValue::U32(n)) = node_inputs.get(SidesInput::<u32>::INDEX).unwrap().as_value() else { |
There was a problem hiding this comment.
P2: Using regular_polygon::SidesInput::<u32>::INDEX to index into star node inputs is fragile. When the star node is found (via the .or() fallback), this uses the polygon module's index constant for the star's inputs. Consider distinguishing which node was found and using the corresponding module's SidesInput::INDEX, e.g. graphene_std::vector::generator_nodes::star::SidesInput::<u32>::INDEX for star nodes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/common_functionality/shapes/polygon_shape.rs, line 179:
<comment>Using `regular_polygon::SidesInput::<u32>::INDEX` to index into star node inputs is fragile. When the star node is found (via the `.or()` fallback), this uses the polygon module's index constant for the star's inputs. Consider distinguishing which node was found and using the corresponding module's `SidesInput::INDEX`, e.g. `graphene_std::vector::generator_nodes::star::SidesInput::<u32>::INDEX` for star nodes.</comment>
<file context>
@@ -174,7 +176,7 @@ impl Polygon {
};
- let Some(&TaggedValue::U32(n)) = node_inputs.get(1).unwrap().as_value() else {
+ let Some(&TaggedValue::U32(n)) = node_inputs.get(SidesInput::<u32>::INDEX).unwrap().as_value() else {
return;
};
</file context>
| use glam::DAffine2; | ||
| use graph_craft::document::NodeInput; | ||
| use graph_craft::document::value::TaggedValue; | ||
| use graphene_std::NodeInputDecleration; |
There was a problem hiding this comment.
P2: Typo in trait name: NodeInputDecleration should be NodeInputDeclaration. "Decleration" is a misspelling of "Declaration". If the trait definition also uses this misspelling, it should be corrected there as well.
(Based on your team's feedback about using consistent, precise terminology in names and docs.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/common_functionality/shapes/rectangle_shape.rs, line 12:
<comment>Typo in trait name: `NodeInputDecleration` should be `NodeInputDeclaration`. "Decleration" is a misspelling of "Declaration". If the trait definition also uses this misspelling, it should be corrected there as well.
(Based on your team's feedback about using consistent, precise terminology in names and docs.) </comment>
<file context>
@@ -9,6 +9,8 @@ use crate::messages::tool::tool_messages::tool_prelude::*;
use glam::DAffine2;
use graph_craft::document::NodeInput;
use graph_craft::document::value::TaggedValue;
+use graphene_std::NodeInputDecleration;
+use graphene_std::vector::generator_nodes::rectangle::*;
use std::collections::VecDeque;
</file context>
|
|
||
| responses.add(NodeGraphMessage::SetInput { | ||
| input_connector: InputConnector::node(node_id, 1), | ||
| input_connector: InputConnector::node(node_id, graphene_std::vector::generator_nodes::star::SidesInput::<u32>::INDEX), |
There was a problem hiding this comment.
P2: This uses star::SidesInput::<u32>::INDEX but node_id may refer to a polygon node (via the .or(get_polygon_id(...)) fallback on line 199). If the polygon node's sides input index differs from the star's, this will silently set the wrong input. Consider branching on which node was found and using the corresponding node's SidesInput::INDEX constant.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/common_functionality/gizmos/shape_gizmos/number_of_points_dial.rs, line 205:
<comment>This uses `star::SidesInput::<u32>::INDEX` but `node_id` may refer to a polygon node (via the `.or(get_polygon_id(...))` fallback on line 199). If the polygon node's sides input index differs from the star's, this will silently set the wrong input. Consider branching on which node was found and using the corresponding node's `SidesInput::INDEX` constant.</comment>
<file context>
@@ -201,7 +202,7 @@ impl NumberOfPointsDial {
responses.add(NodeGraphMessage::SetInput {
- input_connector: InputConnector::node(node_id, 1),
+ input_connector: InputConnector::node(node_id, graphene_std::vector::generator_nodes::star::SidesInput::<u32>::INDEX),
input: NodeInput::value(TaggedValue::U32(new_point_count as u32), false),
});
</file context>
3 participants
fix: replace hardcoded input indices with ::INDEX constants
replaces hardcoded numeric indices with predefined ::INDEX constants across various node input operations, improving code maintainability and reducing the risk of index mismatches.
changes include:
for native nodes, replaced hardcoded indices like 1, 2, 3 with module-level constants such as TranslationInput::INDEX, BlendModeInput::INDEX, etc.
in contexts like array access within functions, introduced local const declarations.
during testing, some failing cases were debugged with comments, but these were reverted to keep the code clean in this pr.
Fixes #1490