-
Notifications
You must be signed in to change notification settings - Fork 99
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
Improve EnemyBallPlacementPlay and convert it to use FSMs #3146
Improve EnemyBallPlacementPlay and convert it to use FSMs #3146
Conversation
…into raymond/enemy_free_kick
Good work on finding this bug! With regards to this
I have a work in progress branch with a couple of core changes to the trajectory planner, including integrating #3058 into the planner. This should ™️ move the robots out of all non-robot obstacles, so hopefully all robots will move out of the stadium. You may have already done this, but it might be helpful to have a sim test with this edge case that I could run on my branch later as well. If the ball moves quick, causing the stadium to move quickly, we might still receive a foul... but Im not sure if that is really avoidable. |
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 know this PR is still a draft, but quickly looked over it to give some initial feedback. Overall it looks great!
Also, if you haven't already, I would recommend testing this out with autoref as well! #2788
// Create move tactics | ||
Vector positioning_vector = (world.ball().position() - placement_point); | ||
|
||
// If the ball is nearly placed, then adjust move tactics to position between | ||
// friendly goal and the ball | ||
if (positioning_vector.length() < 0.5) | ||
{ | ||
positioning_vector = world.field().friendlyGoalCenter() - world.ball().position(); | ||
} | ||
positioning_vector = positioning_vector.normalize() * distance_to_keep; | ||
|
||
Vector left_vector = positioning_vector.rotate(Angle::fromDegrees(-30)); | ||
Vector right_vector = positioning_vector.rotate(Angle::fromDegrees(30)); | ||
|
||
Point center = world.ball().position() + positioning_vector; | ||
Point left = world.ball().position() + left_vector; | ||
Point right = world.ball().position() + right_vector; | ||
|
||
move_tactics[0]->updateControlParams( | ||
center, positioning_vector.orientation() + Angle::half(), 0); | ||
move_tactics[1]->updateControlParams(left, left_vector.orientation() + Angle::half(), | ||
0); | ||
move_tactics[2]->updateControlParams(right, | ||
right_vector.orientation() + Angle::half(), 0); | ||
tactics_to_run[0].emplace_back(move_tactics[0]); | ||
tactics_to_run[0].emplace_back(move_tactics[1]); | ||
tactics_to_run[0].emplace_back(move_tactics[2]); |
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 would be interesting to see. We just gotta becareful of not really following the enemy robot placing the ball, as it could make it very easy to accidently get a foul if we get too close.
src/software/ai/hl/stp/play/enemy_ball_placement/enemy_ball_placement_play_fsm.cpp
Outdated
Show resolved
Hide resolved
…into raymond/enemy_ball_placement
src/software/ai/hl/stp/play/enemy_ball_placement/enemy_ball_placement_play_test.py
Outdated
Show resolved
Hide resolved
void EnemyBallPlacementPlayFSM::avoid(const Update& event) | ||
{ | ||
PriorityTacticVector tactics_to_run = {{}}; | ||
|
||
WorldPtr world_ptr = event.common.world_ptr; | ||
|
||
// Create crease defenders | ||
crease_defenders[0]->updateControlParams(placement_point, | ||
TbotsProto::CreaseDefenderAlignment::LEFT); | ||
crease_defenders[1]->updateControlParams(placement_point, | ||
TbotsProto::CreaseDefenderAlignment::RIGHT); | ||
|
||
tactics_to_run[0].emplace_back(crease_defenders[0]); | ||
tactics_to_run[0].emplace_back(crease_defenders[1]); | ||
|
||
// Create move tactics | ||
Vector positioning_vector = (world_ptr->ball().position() - placement_point); | ||
|
||
// If the ball is nearly placed, then adjust move tactics to position between | ||
// friendly goal and the ball | ||
if (positioning_vector.length() < 0.5) | ||
{ | ||
positioning_vector = world_ptr->field().friendlyGoalCenter() - world_ptr->ball().position(); | ||
} | ||
positioning_vector = positioning_vector.normalize() * distance_to_keep; | ||
|
||
Vector left_vector = positioning_vector.rotate(Angle::fromDegrees(-30)); | ||
Vector right_vector = positioning_vector.rotate(Angle::fromDegrees(30)); | ||
|
||
Point center = world_ptr->ball().position() + positioning_vector; | ||
Point left = world_ptr->ball().position() + left_vector; | ||
Point right = world_ptr->ball().position() + right_vector; | ||
|
||
move_tactics[0]->updateControlParams( | ||
center, positioning_vector.orientation() + Angle::half(), 0); | ||
move_tactics[1]->updateControlParams(left, left_vector.orientation() + Angle::half(), | ||
0); | ||
move_tactics[2]->updateControlParams(right, | ||
right_vector.orientation() + Angle::half(), 0); | ||
tactics_to_run[0].emplace_back(move_tactics[0]); | ||
tactics_to_run[0].emplace_back(move_tactics[1]); | ||
tactics_to_run[0].emplace_back(move_tactics[2]); | ||
|
||
// Set tactics to run | ||
event.common.set_tactics(tactics_to_run); | ||
} |
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.
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.
That seems like an effective way of handling the edge case. However, I pulled from upstream and after some testing, I think Nima's #3152 PR fixes the issue of robots driving into the stadium obstacle, so the edge case of the goalie and crease defenders being in the stadium isn't really an issue anymore. I will work on adding tests for now, does that sound good?
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.
Incorporated into the new implementation.
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.
nice work, looking good... left some comments about an alternate implementation that addresses your edge case that you found
…into raymond/enemy_ball_placement
…into raymond/enemy_ball_placement
…ides motion constraints
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.
Just tried the new implementation with auto-ref and it looks great! I'm excited to get this in and use autoref more often now. 🟨
I just had a few nits.
src/software/ai/hl/stp/play/enemy_ball_placement/enemy_ball_placement_play_fsm.h
Outdated
Show resolved
Hide resolved
src/software/ai/hl/stp/play/enemy_ball_placement/enemy_ball_placement_play_fsm.cpp
Outdated
Show resolved
Hide resolved
src/software/ai/hl/stp/play/enemy_ball_placement/enemy_ball_placement_play_fsm.cpp
Outdated
Show resolved
Hide resolved
src/software/ai/hl/stp/play/enemy_ball_placement/enemy_ball_placement_play_fsm.cpp
Outdated
Show resolved
Hide resolved
src/software/ai/hl/stp/play/enemy_ball_placement/enemy_ball_placement_play_fsm.cpp
Outdated
Show resolved
Hide resolved
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.
Addressed Nima's comments, new changes are ready for review.
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.
LGTM! ⚽
Just had two additional non-blocking comments
@@ -37,6 +37,7 @@ message AiConfig | |||
required EnemyCapabilityConfig enemy_capability_config = 7; | |||
required ShootOrPassPlayConfig shoot_or_pass_play_config = 8; | |||
required RobotCapabilitiesConfig robot_capabilities_config = 9; | |||
required EnemyBallPlacementPlayConfig enemy_ball_placement_play_config = 10; |
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.
it's interesting that we had skipped field 10
// If either destination point is outside the field, then pick the other | ||
// point. It is impossible for both points to be outside the field |
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.
…Software into raymond/enemy_ball_placement
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.
LGTM! Fouls begone
/** | ||
* This FSM implements the enemy ball placement play. | ||
* - If the ball placement point does not exist yet, then we wait | ||
* - If the ball placement point exists, but the ball is far from the placement point, | ||
* then we focus on avoiding ball placement interference. If our robot R is in the ball | ||
* placement stadium, then it will move perpendicular to the segment connecting * and +, | ||
* to either p1 or p2, whichever one is within the field boundaries and closest to R | ||
* | ||
* /‾‾‾‾‾\ | ||
* | + | placement point | ||
* | | | ||
* | | | ||
* p1 | R | p2 | ||
* | | | ||
* | * | ball | ||
* \_____/ | ||
* | ||
* - Once the ball is near the placement point, we set up 1 goalie, 2 crease defenders and | ||
* 3 robots to stay near the ball without interfering | ||
* | ||
* + | ||
* R x enemy robot with ball | ||
* R | ||
* R | ||
* | ||
* D D | ||
* | ||
* G | ||
* +-----+ | ||
*/ |
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.
Helpful diagrams!
Description
Fixed a bug in sensor fusion where the ball placement position is not sent to non-placing team, resulting in the stadium obstacle in Stadium Geometry Obstacle #3121 not working.
Rewrote
EnemyBallPlacementPlay
to use the FSM architecture.Re-designed the play as: if the ball is far from the placement point, then focus on avoiding ball placement interference by assigning AvoidInterferenceTactics (clones of MoveTactic) to move robots out of the way. AvoidInterferenceTactics override motion constraints, because sometimes the stadium motion constraint can actually trap robots and prevent them from moving to somewhere that doesn't interfere.
Once the ball is nearly placed, then we switch to a defensive formation with 1 goalie, 2 crease defenders, and 3 robots to stay near the ball without interfering (essentially our StopPlay behaviour). By doing this, we can transition into our other plays quickly once the game has been resumed.
The edge case where the stadium goes through the defense area is also handled well.
In the first and third gifs, there is a sensor fusion bug where the ball position does not get updated, possibly because the yellow placement robot is blocking the ball. This could be something to address in a separate issue.
Works well generally. Three robots stay near the ball without interfering, so that we can quickly transition into the play that follows, while we also have defensive robots as the goalie and crease defenders.However, there are issues with some edge cases. Currently, I try to position the three robots on the other side of a line connecting the ball to the ball placement position. This could end up outside the field, which is problematic. To fix this, I think I will try to directly send these robots near the final ball placement point, while making sure to stay out of the stadium obstacle.Also, the stadium obstacle is not fully effective at keeping out the goalie and crease defenders, in the edge case that the stadium obstacle goes right through our defense zone. While unlikely, this is technically possible and allowed under robocup rules 5.2. The best solution for this situation would probably be to use move tactics to force our defensive robots out of the stadium.Testing Done
Tested in Thunderscope and
enemy_ball_placement_play_test.py
.Resolved Issues
resolves #3018, resolves #2783, resolves #2870, resolves #2599
Length Justification and Key Files to Review
enemy_ball_placement_play_fsm.h
andenemy_ball_placement_play_fsm.cpp
for FSM implementationrobot_enters_placement_region.py
for validationReview Checklist
It is the reviewers responsibility to also make sure every item here has been covered
.h
file) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom
. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.TODO
(or similar) statements should either be completed or associated with a github issue