Modify actionIds to depict the shiftInterval along with the button pressed.#250
Modify actionIds to depict the shiftInterval along with the button pressed.#250DhritiKohli wants to merge 15 commits intomainfrom
Conversation
Started to add the modified data transformers for /analysis to work
Finished debugging the weightedSumAverages
There was a problem hiding this comment.
Pull request overview
This pull request modifies the match scouting system to include shift interval information in action IDs. Instead of having static action IDs like "teleopShooting", actions are now dynamically prefixed with their game phase and shift number (e.g., "activeShift1Shooting", "inactiveShift2Passing", "endgameFall"). This enables more granular tracking and analysis of robot performance across different match phases and shifts.
Changes:
- Modified action ID generation to dynamically prefix button IDs with camelCased phase/shift information from displayTextWithShift
- Removed phase prefixes from button IDs in match-scouting.json (e.g., "autoShooting" → "Shooting") since prefixes are now added dynamically
- Updated analysis pipeline and modules to work with the new shift-aware action ID format, including new sumAverage transformer for aggregating ratings across shifts
- Enhanced countActions transformer to handle dynamic action IDs when counting all actions
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 22 comments.
Show a summary per file
| File | Description |
|---|---|
| src/scouting/public/js/match-scouting.js | Implements dynamic action ID generation, shift tracking with counters, and camelCase utility function |
| src/analysis/transformers/countActions.js | Enhanced to handle dynamic action IDs by initializing counts with 0 or using defensive increment logic |
| src/analysis/transformers/sumAverage.js | New transformer for computing weighted averages across multiple rating groups (e.g., combining shift-specific ratings) |
| src/analysis/modules/Bar/index.js | Added default value of 0 to getPath call for safer data retrieval |
| config/match-scouting.json | Removed phase prefixes from button IDs, changed teleopActive/Inactive to "none" type, added climb highlighting executables |
| config/analysis-pipeline.json | Expanded rating calculations to include shift-specific paths and use sumAverage to aggregate across shifts |
| config/analysis-modules.json | Updated paths for endgame climb counts and fixed typo (averagesScores → averageScores) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // NEW: shift counters | ||
| var activeShiftCount = 0; | ||
| var inactiveShiftCount = 0; | ||
| var currentShiftNumber; |
There was a problem hiding this comment.
The currentShiftNumber variable is declared but not initialized (line 23). It will be undefined until a shift button is pressed. When it's used in template literals at lines 311-312, this will result in strings like "Active Shift undefined" if the automatic shift switch happens before any manual button press. Initialize currentShiftNumber to 0 or handle the undefined case.
| var currentShiftNumber; | |
| var currentShiftNumber = 0; |
| lastActions[lastActions.length - 1].num++; | ||
| } | ||
| } | ||
| console.log(actionQueueIds); |
There was a problem hiding this comment.
This console.log statement appears to be debug code that should be removed before merging to production.
| console.log(actionQueueIds); |
| if (shiftButtonPressed && elapsedSinceSwitch >= shiftSwitchInterval) { | ||
| if (currentShift === "active") { | ||
| currentShift = "inactive"; | ||
|
|
||
| inactiveShiftCount += 1; | ||
| currentShiftNumber = inactiveShiftCount; | ||
| } else { | ||
| currentShift = "active"; | ||
| activeShiftCount += 1; | ||
| currentShiftNumber = activeShiftCount; | ||
| } | ||
|
|
||
| lastShiftSwitchTime = time; | ||
| } |
There was a problem hiding this comment.
The automatic shift switching logic has been changed to use Math.abs for calculating elapsedSinceSwitch, but there's no time range check anymore. Previously, the code checked "time < teleopTime && time > endgameTime" to ensure shifts only happen during teleop. Now shifts can potentially happen at any time as long as shiftButtonPressed is true and 25 seconds have elapsed. This could cause unintended shifts during autonomous or endgame periods. Restore the time range check to ensure shifts only occur during the teleop period.
config/analysis-pipeline.json
Outdated
| "activeShift1stopShoot", | ||
| "activeShift2stopShoot", | ||
| "teleopTransitionstopShoot" |
There was a problem hiding this comment.
The actionId array includes "activeShift2stopShoot" but the corresponding pickup action is "activeShift2StartShoot" (capital S). This casing inconsistency will cause the cycle transformer to not properly match start and stop actions. All pickup and score action IDs should have consistent casing. The button ID in match-scouting.json is "StopShooting" which when combined with a shift prefix would create "activeShift2StopShooting", not "stopShoot". Verify that the actual button IDs match the expected action IDs in the pipeline.
| "activeShift1stopShoot", | |
| "activeShift2stopShoot", | |
| "teleopTransitionstopShoot" | |
| "activeShift1StopShoot", | |
| "activeShift2StopShoot", | |
| "teleopTransitionStopShoot" |
config/analysis-pipeline.json
Outdated
| ], | ||
| "scores": [ | ||
| "activeShift1stopShoot", | ||
| "activeShift2stopShoot", |
There was a problem hiding this comment.
Same typo issue as line 82 - "activeShift2stopShoot" should likely be "activeShift2StopShoot" to match the expected button ID casing.
| "activeShift2stopShoot", | |
| "activeShift2StopShoot", |
| "gridArea": ["3", "7", "6", "12"], | ||
| "class": "gray", | ||
| "type": "action", | ||
| "type": "none", |
There was a problem hiding this comment.
The button type has been changed from "action" to "none" for the teleopInactive button. However, the "none" button type still pushes an action to the actionQueue with temp: true (lines 138-143 in match-scouting.js). This means these actions will be filtered out before submission. If the intent is to track when shifts are manually selected, consider whether these should remain temporary actions or be permanent ones. If they should be temporary, the current implementation is correct but the naming "none" is misleading since it does create an action.
| "type": "none", | |
| "type": "action", |
config/analysis-pipeline.json
Outdated
| "scores": [ | ||
| "activeShift1stopShoot", | ||
| "activeShift2stopShoot", | ||
| "teleopTransitionstopShoot" |
There was a problem hiding this comment.
Same typo issue - "teleopTransitionstopShoot" should likely be "teleopTransitionStopShoot" to match the expected button ID casing.
| "teleopTransitionstopShoot" | |
| "teleopTransitionStopShoot" |
config/match-scouting.json
Outdated
| }, | ||
| { | ||
| "id": "endGameTowerRobot_Level2", | ||
| "id": "counts.l2", |
There was a problem hiding this comment.
The button ID was changed from "endGameTowerRobot_Level2" to "counts.l2". This appears to be setting the ID to a data path rather than an action identifier. Button IDs should be action identifiers (like "AttemptL2"), not data paths. This will cause issues when the action is recorded in the actionQueue and when trying to find this button for undo operations.
| "id": "counts.l2", | |
| "id": "endGameTowerRobot_Level2", |
config/match-scouting.json
Outdated
| }, | ||
| { | ||
| "id": "counts.teleopFuelScored", | ||
| "id": "teleopFuelScored", |
There was a problem hiding this comment.
The button ID was changed from "counts.teleopFuelScored" to "teleopFuelScored". This change is in the opposite direction of the changes at lines 1018, 1026, and 1034. This inconsistency suggests confusion about whether IDs should include the "counts." prefix or not. Based on the codebase patterns, button IDs should be simple action identifiers without the "counts." prefix. The "counts." prefix is used in data paths, not action IDs. This change appears correct, but verify it's consistent with the overall design.
| throw new Error("sumAverage requires options.groups") | ||
| } | ||
|
|
||
| for (let [teamNumber, team] of Object.entries(dataset.teams)) { |
There was a problem hiding this comment.
Unused variable teamNumber.
| for (let [teamNumber, team] of Object.entries(dataset.teams)) { | |
| for (let team of Object.values(dataset.teams)) { |
No description provided.