-
Notifications
You must be signed in to change notification settings - Fork 4
Show playback #221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Show playback #221
Conversation
Co-authored-by: Gabriel Burbach <[email protected]>
Needed to downgrade due to this issue I encountered when installing Framer Motion: yarnpkg/yarn#7807
Co-authored-by: Gabriel Burbach <[email protected]>
Co-authored-by: Colin Wong <[email protected]>
Co-authored-by: Jason Antwi-Appah <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new “Show playback” feature by introducing a new editor page for designing shows and by extending robot command functionality. Key changes include updates to the TCP interface and managers to support robot-based commands, additions of new API endpoints for parallel and big command execution, and several UI updates for the editor and simulation components.
Reviewed Changes
Copilot reviewed 24 out of 29 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/server/api/tcp-interface.ts | Updated constructor to accept robotManager and improved connection handling. |
src/server/api/api.ts | Added new endpoints ("/do-parallel" and "/do-big") for command group execution. |
src/common/getRobotStateAtTime.ts | Introduced detailed logic to compute robot state based on timeline events. |
src/client/editor/spline-editor.tsx | Added spline editor UI with direct DOM manipulation for scrolling to timeline events. |
src/client/editor/hooks.ts | Introduced a custom draggable hook that uses direct document event assignments. |
src/client/debug/simulator.tsx | Extracted RobotGrid component for clearer separation of simulator layout. |
Files not reviewed (5)
- .vscode/launch.json: Language not supported
- .vscode/settings.json: Language not supported
- package.json: Language not supported
- src/client/package.json: Language not supported
- src/server/api/bot-server-config.json: Language not supported
Comments suppressed due to low confidence (1)
src/client/editor/spline-editor.tsx:83
- Direct DOM manipulation using document.getElementById is used to locate timeline elements. Consider refactoring to use React refs instead for a more idiomatic and maintainable solution.
const el = document.getElementById(`timeline-event-${layer.startPoint.id}`);
document.onmouseup = closeDragElement; | ||
// call a fn whenever the cursor moves: | ||
document.onmousemove = elementDrag; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assigning document event handlers directly may override pre-existing listeners. Consider using document.addEventListener for 'mouseup' and 'mousemove' to avoid potential conflicts.
document.onmouseup = closeDragElement; | |
// call a fn whenever the cursor moves: | |
document.onmousemove = elementDrag; | |
document.addEventListener("mouseup", closeDragElement); | |
// call a fn whenever the cursor moves: | |
document.addEventListener("mousemove", elementDrag); |
Copilot uses AI. Check for mistakes.
if ( | ||
previousMidpointType === SplinePointType.QuadraticBezier && | ||
previousActualCP1 | ||
) { | ||
final_p1 = reflectPoint(previousActualCP1, _startPos_deriv); | ||
} else if ( | ||
previousMidpointType === SplinePointType.CubicBezier && | ||
previousActualCP2 | ||
) { | ||
final_p1 = reflectPoint(previousActualCP2, _startPos_deriv); | ||
} else { | ||
final_p1 = _startPos_deriv; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The state update logic for handling control points and the derivative functions is complex. Adding more inline comments or refactoring portions of this logic would improve code clarity and maintainability.
if ( | |
previousMidpointType === SplinePointType.QuadraticBezier && | |
previousActualCP1 | |
) { | |
final_p1 = reflectPoint(previousActualCP1, _startPos_deriv); | |
} else if ( | |
previousMidpointType === SplinePointType.CubicBezier && | |
previousActualCP2 | |
) { | |
final_p1 = reflectPoint(previousActualCP2, _startPos_deriv); | |
} else { | |
final_p1 = _startPos_deriv; | |
} | |
// Handle the Quadratic Bezier case | |
final_p1 = computeControlPoint( | |
previousMidpointType, | |
previousActualCP1, | |
previousActualCP2, | |
_startPos_deriv, | |
); |
Copilot uses AI. Check for mistakes.
This PR:
/editor
) to support designing showsthis code needs HEAVY refactoring which we will do in future issues because these branches have been open for too long 👍🏾