Girls gen updates #189
Girls gen updates #189
Conversation
markpete
left a comment
markpete
left a comment
There was a problem hiding this comment.
Let's see if we can get rid of the redundant code and make this more maintainable.
| blueBranchI, | ||
| blueBranchJ, | ||
| blueBranchK, | ||
| blueBranchL) |
There was a problem hiding this comment.
List.of returns an immutable object, so each of these lists could just be assigned to a static final class variable and then used instead of calling List.of() and recreating these fixed lists all the time. Not blocking - just an area for improvement.
| spinnyClaw.stop()) | ||
| .withTimeout(0.5), | ||
| repeatPrescoreScoreSwing( | ||
| Commands.repeatingSequence( |
There was a problem hiding this comment.
Seems odd to have a repeating sequence passed in to a function that repeats the passed in command. Is that intentional?
There was a problem hiding this comment.
Other than that, this is identical to coralLevelTwo() but with different constants for a few things. If they should match other than constants, it'd be useful to rework this post-comp to remove the duplicated code (via a template function or some other method)
| .until(ipScore) | ||
| .until(score)), | ||
| spinnyClaw.stop()) | ||
| .withTimeout(0.5), |
There was a problem hiding this comment.
The .withTimeout() values (e.g., 0.5, 1.5) are hardcoded. Consider defining these as constants or configurable parameters to make the code easier to maintain and tune.
| armPivot | ||
| .moveToPosition(ArmPivot.CORAL_PRESET_DOWN) | ||
| .withTimeout(1.5) | ||
| .until(armPivot.atAngle(ArmPivot.CORAL_POST_SCORE))), |
There was a problem hiding this comment.
There is no explicit error handling in case the subsystems fail to reach their target positions. Consider adding fallback logic or logging to handle such cases. Once physical simulation of the subsystems are implemented, tweaking parameters there (to make them move slower or faster) would allow testing of the error handling here.
This comment was marked as resolved.
This comment was marked as resolved.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactoring of the auto-alignment and scoring logic, notably by adding AllianceUtils, ScoringType, and AlignType to improve code structure and reduce duplication. The Shuffleboard layout in elastic-layout.json has also been completely reorganized. My review focuses on improving performance by avoiding repeated object creation in AutoAlign, enhancing maintainability by removing magic numbers, and fixing a potential regression in the controller rumble feedback for scoring alignment. I also suggest a small improvement to the new AllianceUtils class to follow best practices.
No description provided.