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. |
2 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