-
Notifications
You must be signed in to change notification settings - Fork 186
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
Rewrite linekicker #2163
Rewrite linekicker #2163
Conversation
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.
Good stuff Peter! I know I wrote half of this but I still have some requested changes just to clean it up before merging. Also I'm going to merge #2151 soon and then we might have to rebase this branch
@@ -13,30 +13,17 @@ using namespace rj_geometry; | |||
namespace planning { | |||
|
|||
Trajectory LineKickPathPlanner::plan(const PlanRequest& plan_request) { | |||
// TODO(?): ros param these | |||
const float approach_speed = 0.05; | |||
SPDLOG_INFO("Robot {} state {}", plan_request.shell_id, current_state_); |
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.
let's remove this
reuse_path_count_ = 0; | ||
} | ||
Trajectory LineKickPathPlanner::initial(const PlanRequest& plan_request) { | ||
// // Getting ball info |
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.
can we fix this odd double-comment formatting?
target.position -= target.velocity.normalized(kRobotRadius + kBallRadius * 2); | ||
// velocity | ||
auto goal_to_ball = (plan_request.motion_command.target.position - ball.position); | ||
auto vel = goal_to_ball.normalized() * 1.0; |
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.
let's refactor 1.0 into a class constant something like static constexpr double final_robot_speed
?
return path; | ||
} | ||
} | ||
void LineKickPathPlanner::state_transition(BallState ball, RobotInstant start_instant) { |
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.
we can eliminate these parameters note the GH Actions warnings
|
||
// ball velocity filtering vars | ||
static constexpr double IS_DONE_BALL_VEL = 1.5; |
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.
GH doesn't like this case style try something like kIsBallDoneVelocity
or something like that?
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.
and maybe we should do a little more sim testing as to what the best value is
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.
Thanks!
automated style fixes Co-authored-by: petergarud <[email protected]>
automated style fixes Co-authored-by: petergarud <[email protected]>
469a45e
to
50cab81
Compare
automated style fixes Co-authored-by: sid-parikh <[email protected]>
Description
Rewrote linekicker.