-
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
Check out the wall chucker #2325
base: ros2
Are you sure you want to change the base?
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.
I haven't tested this code or even read much of it. just left some basic code style thoughts for now
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.
please uncommit this. your local settings.json are not a part of the shared code. (you can later make a case for inserting this into our .gitignore file, if you please, or you can locally tell get to ignore this file using various methods )
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.
also uncommit this, please. you seem to have cloned ER-Force's framework git repo inside of this one. git repos don't nest well unfortunately, because now you've checked their entire repo into ours.
@@ -268,7 +268,7 @@ std::optional<RobotIntent> Offense::state_to_task(RobotIntent intent) { | |||
auto collect_cmd = planning::MotionCommand{"collect"}; | |||
intent.motion_command = collect_cmd; | |||
intent.dribbler_speed = 255.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.
crazy find. looks correct, but how was the previous code compiling?
|
||
// return distance_to_ball() < kStealBallRadius; | ||
} | ||
// bool Offense::can_steal_ball() const { |
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.
not a fan of commenting-out methods. either we need them or we delete them, IMO. if we temporarily don't need them, then at least leave a comment explaining. but honestly no harm in leaving them there, in that case.
field_dimensions_.their_defense_area().contains_point(ball_pos) || | ||
!field_dimensions_.field_rect().contains_point(ball_pos)); | ||
} | ||
// // Checks whether ball is out of range for stealing/receiving |
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.
same comment as above.
!field_dimensions_.field_rect().contains_point(ball_pos)); | ||
} | ||
|
||
bool Position::can_steal_ball() const { |
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.
i see now, they were elevated to superclass? then we should definitely delete from subclass.
@@ -27,6 +27,8 @@ std::optional<RobotIntent> Waller::get_task(RobotIntent intent, const WorldState | |||
auto goal_pos = rj_geometry::Point{0, 0}; | |||
auto num_wallers = walling_robots_.size(); | |||
|
|||
|
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.
uncommit this please. just makes git history noisy
case IDLING: | ||
break; | ||
case JOINING_WALL: | ||
case IDLING:{ |
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.
this seems like it breaks code style (no space before '{' and no newline before '}'). did you run a formatter? it's also possible our formatter doesn't catch this case (blocks in a case statement is a bit nonstandard but I do love them)
|
||
// note: the way this is set up makes it impossible to | ||
// shoot on time without breakbeam | ||
// TODO(Kevin): make intent hold a manip msg instead? to be cleaner? |
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.
i think it's about time to stop adding TODOs for @kfu02, lol.
i know you just copied this from somewhere but let's take this out, and maybe make a clickup card if we think this task is actually a good idea
Description
Pull request for wall steal. In this the closest bot in the wall should move towards the ball and clear the ball forward.
Expected result:
The closest waller bot should move to the ball even though the solo offense bot goes for the ball. Then the bot that cleared the ball should join the wall or the solo offense bot joins the wall.
Key Files to Review
Group 1
Defense.cpp
Defense.hpp
Position.cpp
Position.hpp