-
Notifications
You must be signed in to change notification settings - Fork 113
[Gameplay] Avoid Aimless Kick/Chip by Defenders #3230
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
[Gameplay] Avoid Aimless Kick/Chip by Defenders #3230
Conversation
std::vector<Robot> enemy_robots = event.common.world_ptr->enemyTeam().getAllRobots(); | ||
for (int i = 0; i < static_cast<int>(event.common.world_ptr->enemyTeam().numRobots()); i++) { | ||
if (contains(zone, enemy_robots[i].position())) | ||
{ | ||
return true; | ||
} | ||
} | ||
return false; |
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.
nit: try refactoring to use std::any_of
: https://en.cppreference.com/w/cpp/algorithm/all_any_none_of
// DEBUG: Visualizes threat stadium | ||
LOG(VISUALIZE) << *createDebugShapes({ | ||
*createDebugShape(threat_zone, std::to_string(event.common.robot.id()), "threatzone") | ||
}); |
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.
remove to delete when you're done
|
||
if (goal_intersections.empty() | ||
&& CreaseDefenderFSM::isAnyEnemyInZone(event, threat_zone) | ||
&& robot_to_net_m <= event.common.world_ptr->field().totalYLength() / 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.
why did you use YLength() here?
also this should be a constant with the word THRESHOLD
in it
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 ensures that chips only occur when the ball is somewhat near our net. This cannot be a constant since this value must scale with field size and this is only determined in runtime
if (goal_intersections.empty() | ||
&& CreaseDefenderFSM::isAnyEnemyInZone(event, threat_zone) | ||
&& robot_to_net_m <= event.common.world_ptr->field().totalYLength() / 2) | ||
{ | ||
// Autochip only if the robot is not facing the net, there is an enemy in front, and robot is close to net | ||
auto_chip_or_kick = AutoChipOrKick{ | ||
AutoChipOrKickMode::AUTOCHIP, | ||
chip_distance | ||
}; | ||
} | ||
else | ||
{ | ||
auto_chip_or_kick = AutoChipOrKick{AutoChipOrKickMode::OFF, 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.
if (goal_intersections.empty() | |
&& CreaseDefenderFSM::isAnyEnemyInZone(event, threat_zone) | |
&& robot_to_net_m <= event.common.world_ptr->field().totalYLength() / 2) | |
{ | |
// Autochip only if the robot is not facing the net, there is an enemy in front, and robot is close to net | |
auto_chip_or_kick = AutoChipOrKick{ | |
AutoChipOrKickMode::AUTOCHIP, | |
chip_distance | |
}; | |
} | |
else | |
{ | |
auto_chip_or_kick = AutoChipOrKick{AutoChipOrKickMode::OFF, 0}; | |
} | |
auto_chip_or_kick = AutoChipOrKick{AutoChipOrKickMode::OFF, 0}; | |
if (goal_intersections.empty() | |
&& CreaseDefenderFSM::isAnyEnemyInZone(event, threat_zone) | |
&& robot_to_net_m <= event.common.world_ptr->field().totalYLength() / 2) | |
{ | |
// Autochip only if the robot is not facing the net, there is an enemy in front, and robot is close to net | |
auto_chip_or_kick = AutoChipOrKick{ | |
AutoChipOrKickMode::AUTOCHIP, | |
chip_distance | |
}; | |
} |
constexpr double GET_POSSESSION_THRESHOLD_M = 1; | ||
constexpr double THREAT_THRESHOLD_M = GET_POSSESSION_THRESHOLD_M * 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 should be in the header so that we can find these constants easily
LOG(VISUALIZE) << *createDebugShapes({ | ||
*createDebugShape( | ||
Circle(robot_position, GET_POSSESSION_THRESHOLD_M), | ||
std::to_string(event.common.robot.id()) + "1", | ||
"ballzone" | ||
) | ||
}); |
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.
remember to remove this when you're done
boost::sml::back::process<DribbleFSM::Update> processEvent) | ||
{ | ||
DribbleFSM::ControlParams control_params{ | ||
.dribble_destination = Point(), |
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.
.dribble_destination = Point(), | |
.dribble_destination = event.common.world_ptr->ball().position(), |
{ | ||
DribbleFSM::ControlParams control_params{ | ||
.dribble_destination = Point(), | ||
.final_dribble_orientation = Angle(), |
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.
should be oriented towards the enemy net
*MoveFSM_S + Update_E / blockThreat_A, MoveFSM_S = X, | ||
*MoveFSM_S + Update_E[ballNearbyWithoutThreat_G] / prepareGetPossession_A = DribbleFSM_S, | ||
MoveFSM_S + Update_E / blockThreat_A, MoveFSM_S = X, | ||
DribbleFSM_S + Update_E[!ballNearbyWithoutThreat_G] / blockThreat_A = X, |
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.
DribbleFSM_S + Update_E[!ballNearbyWithoutThreat_G] / blockThreat_A = X, | |
DribbleFSM_S + Update_E[!ballNearbyWithoutThreat_G] / blockThreat_A = MoveFSM, |
* | ||
* @param event CreaseDefenderFSM::Update event | ||
* @param zone a stadium shape that defines the zone | ||
* @param zone a stadium shape that defines the zone |
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.
* @param zone a stadium shape that defines the zone |
fsm_map.at(tactic_update.robot.id()) | ||
->process_event(DribbleFSM::Update(dribble_control_params, tactic_update)); |
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.
do we need this? I'm not 100% sure if this is necessary
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.
No idea why, but weirdly yes
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.
left some interim feedback. Looks like one of the CHECKs fail when we play AI vs AI. I think the update that I suggested to crease_defender_fsm.h
may fix this issue
src/proto/parameters.proto
Outdated
*/ | ||
required double max_get_ball_ratio_threshold = 1 | ||
[default = 0.3, (bounds).min_double_value = 0.0, (bounds).max_double_value = 1.0]; | ||
// Max distance that the crease will try and get possession of a ball |
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.
// Max distance that the crease will try and get possession of a ball | |
// Max distance that the crease defenderwill try and get possession of a ball |
@@ -93,6 +127,15 @@ struct CreaseDefenderFSM | |||
static std::optional<Point> findDefenseAreaIntersection( | |||
const Field& field, const Ray& ray, double robot_obstacle_inflation_factor); | |||
|
|||
private: | |||
/** |
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.
still should be private:
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.
Oh i see why you made it public. The class members should still be private
though
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.
Oh yeah, this part is a bit deceptive, I just moved the private up. These functions are actually private, which I suppose means theres no point making them static.
friendlyDefenseAreaFrontCenter = tbots_cpp.Point( | ||
tbots_cpp.Field.createSSLDivisionBField() | ||
.friendlyDefenseArea() | ||
.posXPosYCorner() | ||
.x(), | ||
tbots_cpp.Field.createSSLDivisionBField().friendlyDefenseArea().centre().y(), | ||
) |
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 is just tbots_cpp.Field.createSSLDivisionBField().friendlyDefenseArea().centre()
btw
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 point I want is the center of the frontal segment of the defence area, not necessarily the exact center of the defense box.
# These aren't necessary for this test, but this is just an example | ||
# of how to send commands to the simulator. | ||
# | ||
# NOTE: The gamecontroller responses are automatically handled by | ||
# the gamecontroller context manager class | ||
simulated_test_runner.gamecontroller.send_gc_command( | ||
gc_command=Command.Type.STOP, team=Team.UNKNOWN | ||
) | ||
simulated_test_runner.gamecontroller.send_gc_command( | ||
gc_command=Command.Type.FORCE_START, team=Team.BLUE | ||
) |
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.
you can delete this, I don't believe it's necessary
# These aren't necessary for this test, but this is just an example | ||
# of how to send commands to the simulator. | ||
# | ||
# NOTE: The gamecontroller responses are automatically handled by | ||
# the gamecontroller context manager class | ||
simulated_test_runner.gamecontroller.send_gc_command( | ||
gc_command=Command.Type.STOP, team=Team.UNKNOWN | ||
) | ||
simulated_test_runner.gamecontroller.send_gc_command( | ||
gc_command=Command.Type.FORCE_START, team=Team.BLUE | ||
) |
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.
you can delete these lines, it's probably not necessary
# These aren't necessary for this test, but this is just an example | ||
# of how to send commands to the simulator. | ||
# | ||
# NOTE: The gamecontroller responses are automatically handled by | ||
# the gamecontroller context manager class | ||
simulated_test_runner.gamecontroller.send_gc_command( | ||
gc_command=Command.Type.STOP, team=Team.UNKNOWN | ||
) | ||
simulated_test_runner.gamecontroller.send_gc_command( | ||
gc_command=Command.Type.FORCE_START, team=Team.BLUE | ||
) |
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.
you can delete this
params.assigned_tactics[0].crease_defender.CopyFrom( | ||
CreaseDefenderTactic( | ||
enemy_threat_origin=tbots_cpp.createPointProto(ball_initial_pos), | ||
crease_defender_alignment=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.
you can probably do CreaseDefenderAlignment.CENTRE
here
params.assigned_tactics[0].crease_defender.CopyFrom( | ||
CreaseDefenderTactic( | ||
enemy_threat_origin=tbots_cpp.createPointProto(ball_initial_pos), | ||
crease_defender_alignment=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.
you can probably do CreaseDefenderAlignment.CENTRE
here
params.assigned_tactics[0].crease_defender.CopyFrom( | ||
CreaseDefenderTactic( | ||
enemy_threat_origin=tbots_cpp.createPointProto(ball_initial_pos), | ||
crease_defender_alignment=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.
you can probably do CreaseDefenderAlignment.CENTRE
here
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.
great tests!
|
||
|
||
class BallIsOffGround(Validation): | ||
|
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.
nit:
…nto Andrewyx/AimlessChip
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.
some nits
) | ||
|
||
|
||
friendlyDefenseAreaFrontCenter = tbots_cpp.Point( |
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.
nit: should be snake_case
src/proto/parameters.proto
Outdated
Threshold = 1.0 results in crease defenders chasing the ball even ball is equidistant | ||
to nearest enemy. Threshold < 1.0 results in crease defenders chasing the ball only | ||
when it is at least (threshold) percent closer to our bot than the nearest enemy. |
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.
nit:
Threshold = 1.0 results in crease defenders chasing the ball even ball is equidistant | |
to nearest enemy. Threshold < 1.0 results in crease defenders chasing the ball only | |
when it is at least (threshold) percent closer to our bot than the nearest enemy. | |
Threshold = 1.0 results in crease defenders chasing the ball even if the ball is equidistant | |
to nearest enemy. Threshold < 1.0 results in crease defenders chasing the ball only | |
when it is at least (threshold) percent closer to our bot than the nearest enemy. |
bool ball_is_near_friendly = | ||
ball_distance < nearest_enemy_distance * | ||
crease_defender_config.max_get_ball_ratio_threshold(); | ||
ball_distance_to_friendly < | ||
ball_distance_to_enemy * | ||
(1.0 - crease_defender_config.max_get_ball_ratio_threshold()); |
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 change in the logic seems backwards to me. Shouldn't we be multiplying by crease_defender_config.max_get_ball_ratio_threshold()
directly to get the ratio?
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.
You are right that mathematically speaking, it would be a direct multiplication. However, I think the meaning of max_get_ball_ratio_threshold()
makes a lot more sense when we flip the percentage. For example, if max_get_ball_ratio_threshold() = 0.1
, this means that the ball must be at least 10% closer to our defender than the enemy for stealing to occur. Within the code though, this could be either represented by a multiplication of 0.9 to the enemy dist, or a multiplication of 1.1 to friendly dist.
Below is some rambling since I changed the meaning of the math here today. It is only if you are curious why I made a change here.
I thought of an edge case and switched the logic to use this instead. The old code directly multiplied it since it was comparing the distance of the defender->ball vs nearest_enemy_to_defender->ball. This works in most circumstances, but in the case where the world looks like this:
Ball Crease Enemy
x O O
The crease would decide not to pursue the ball when it really should. Hence, I changed the logic to instead measure the distance from the ball to the nearest enemy and compare that with the current crease's distance.
This means that I redefined the meaning of max_get_ball_ratio_threshold()
/*
Max ratio between distances (crease defender and ball) / (nearest enemy and ball) for
pass defender to chase ball.
Threshold = 1.0 results in crease defenders chasing the ball even ball is equidistant
to nearest enemy. Threshold < 1.0 results in crease defenders chasing the ball only
when it is at least (threshold) percent closer to our bot than the nearest enemy.
*/
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 followed up in a new comment. Though Im still not sure if I fully understand the logic. I think I understand the edge case you're talking about, but I think in that case it's also okay to act as a regular crease defender trying to block the ball, as opposed to trying to take a risk by attempting to steal the ball.
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, regarding the comment:
Threshold = 1.0 results in crease defenders chasing the ball even ball is equidistant to nearest enemy. Threshold < 1.0 results in crease defenders chasing the ball only when it is at least (threshold) percent closer to our bot than the nearest enemy.
If the value is set to 1, then right side of the equality becomes 0, meaning that the ball distance to friendly has to be less than 0 (impossible) for the friendly to try to steal the ball. That's why I think just multiplying makes more sense. Threshold of 1 should indeed mean that the crease defender is the most aggressive. And 0 be that the crease defender doesn't take any risks.
@@ -30,10 +34,21 @@ std::optional<Point> CreaseDefenderFSM::findBlockThreatPoint( | |||
return findDefenseAreaIntersection(field, ray, robot_obstacle_inflation_factor); |
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.
You should make findDefenseAreaIntersection
static so findBlockThreatPoint
can call it
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.
Ohhhh right, that's why I originally wrote it as static
src/software/ai/hl/stp/tactic/crease_defender/crease_defender_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.
💯
Please fill out the following before requesting review on this PR
Description
Changes Crease Defender Autochip behaviour such that robots only chip in dire circumstances.
Further adds new Chip Validation
creasechip.mp4
Crease defenders now only autochip incoming balls when:
Further adds the following:
Testing Done
Adds new chip validation test which passes when ball is threshold meters off the ground. This allows us to test if chip behaviour occurs.
Resolved Issues
Resolves #3220
Length Justification and Key Files to Review
This PR includes a new Simulated Test along with a new Validation Type.
Review 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 issueSuggests new ticket reworking the visual component of the ball_if_off_ground validation test. This test validates changes in the z axis and the current 2D shape system is not enough to visualize this. It is purely a visual change and is not urgent, but will take some time to think of a new way to visualize.