Time to reach rework #1634
Open
Time to reach rework #1634
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
knoellle
added
the
is:Refactoring
…here the ball teleported
knoellle
force-pushed
the
time-to-reach-rework
branch
from
de2b39b to
570ca6c
Compare
schmidma
requested changes
Feb 17, 2025
| @@ -34,10 +35,17 @@ impl Default for HulkMessage { | |||
| pub struct StrikerMessage { | |||
| pub player_number: PlayerNumber, | |||
| pub pose: Pose2<Field>, | |||
| pub ball_position: Option<BallPosition<Field>>, | |||
| pub ball_position: BallPosition<Field>, | |||
| // TODO: make non-optional | |||
There was a problem hiding this comment.
What is blocking you from that? Is this something for a next iteration?
Comment on lines
+65
to
+73
| let stand_up_back_estimated_remaining_duration = { | ||
| let last_cycle_duration = context.cycle_time.last_cycle_duration; | ||
| let condition_input = context.condition_input; | ||
|
|
||
| self.interpolator | ||
| .advance_by(last_cycle_duration, condition_input); | ||
| self.interpolator | ||
| .advance_by(last_cycle_duration, condition_input); | ||
|
|
||
| Some(self.interpolator.estimated_remaining_duration()) | ||
| } else { | ||
| self.interpolator.reset(); | ||
| None | ||
| }; | ||
| self.interpolator.estimated_remaining_duration() | ||
| }; |
There was a problem hiding this comment.
This does not look right?! You somehow miss the reset now?
Comment on lines
+41
to
+42
| stand_up_back_estimated_remaining_duration: | ||
| CyclerState<Duration, "stand_up_back_estimated_remaining_duration">, |
There was a problem hiding this comment.
Did you think about this use of CyclerState twice? We should be very sure about behavior at edges. e.g. the duration being ZERO when the standup never happened yet? etc.
But there is definitely something incorrect in this module
| time_to_turn, | ||
| ] | ||
| .into_iter() | ||
| .fold(Duration::ZERO, Duration::saturating_add); |
There was a problem hiding this comment.
Why is this a fold and not a sum? Is the saturation property actually so important? Do we run into overflows?
Comment on lines
+46
to
+61
| let walk_time = Duration::from_secs_f32( | ||
| dribble_path | ||
| .iter() | ||
| .map(|segment: &PathSegment| { | ||
| let length = segment.length(); | ||
| match segment { | ||
| PathSegment::LineSegment(_) => { | ||
| length / context.configuration.path_planning.line_walking_speed | ||
| } | ||
| }) | ||
| .sum() | ||
| }) | ||
| .map(Duration::from_secs_f32); | ||
| let turning_angle = match context.motion_command { | ||
| MotionCommand::Walk { | ||
| orientation_mode: OrientationMode::Override(orientation), | ||
| .. | ||
| } => Some(orientation.angle().abs()), | ||
| _ => { | ||
| let turning_angle_towards_path = match context.dribble_path { | ||
| Some(path) => match path.first() { | ||
| Some(PathSegment::LineSegment(line_segment)) => { | ||
| Some(line_segment.1.coords().angle(&Vector2::x_axis()).abs()) | ||
| PathSegment::Arc(_) => { | ||
| length / context.configuration.path_planning.arc_walking_speed | ||
| } | ||
| _ => None, | ||
| }, | ||
| } | ||
| }) | ||
| .sum(), | ||
| ); |
There was a problem hiding this comment.
Might be a little picky, but try to reduce some levels of indentation ;)
e.g.
let secs = dribble_path
.iter()
.map(|segment: &PathSegment| {
let length = segment.length();
match segment {
PathSegment::LineSegment(_) => {
length / context.configuration.path_planning.line_walking_speed
}
PathSegment::Arc(_) => {
length / context.configuration.path_planning.arc_walking_speed
}
}
})
.sum();
let walk_time = Duration::from_secs_f32(secs);
Comment on lines
+70
to
+101
| // === Team ball === | ||
| // if let Some(game_controller_state) = filtered_game_controller_state { | ||
| // match game_controller_state.game_phase { | ||
| // GamePhase::PenaltyShootout { | ||
| // kicking_team: Team::Hulks, | ||
| // } => return (Role::Striker, false, None), | ||
| // GamePhase::PenaltyShootout { | ||
| // kicking_team: Team::Opponent, | ||
| // } => return (Role::Keeper, false, None), | ||
| // _ => {} | ||
| // }; | ||
| // if let Some(SubState::PenaltyKick) = game_controller_state.sub_state { | ||
| // return (current_role, false, None); | ||
| // } | ||
| // } | ||
|
|
||
| // if primary_state != PrimaryState::Playing { | ||
| // match detected_own_team_ball { | ||
| // None => return (current_role, false, team_ball), Some(own_team_ball) => return (current_role, false, own_team_ball), | ||
| // } | ||
|
|
||
| // let team_ball_from_spl_message = Some(BallPosition { | ||
| // position: striker_event.ball_position.position, | ||
| // velocity: Vector::zeros(), | ||
| // last_seen: cycle_start_time - striker_event.ball_position.age, | ||
| // }); | ||
|
|
||
| // === Obstacles === | ||
| // let sender_position = ground_to_field.inverse() * spl_message.pose.position(); | ||
| // if spl_message.player_number != *context.player_number { | ||
| // network_robot_obstacles.push(sender_position); | ||
| // } |
| .filter_map(|message| Some((*time, (*message)?))) | ||
| }) | ||
| .filter_map(|(time, message)| match message { | ||
| IncomingMessage::Spl(HulkMessage::Striker(message)) => Some((time, *message)), |
There was a problem hiding this comment.
We want to handle Looser messages as well, do we?
|
|
||
| let team_ball = self.get_best_received_ball( | ||
| context.cycle_time.start_time, | ||
| context.striker_trusts_team_ball.mul_f32(4.5), |
There was a problem hiding this comment.
Should this be dependent on this other parameter?
Comment on lines
+27
to
+28
| network_message_debug: | ||
| AdditionalOutput<Vec<(SystemTime, StrikerMessage)>, "network_message_debug">, |
There was a problem hiding this comment.
I think these are not scoped, are they? This floats around in the top level now
Comment on lines
+110
to
+117
| ball.filter(|ball| { | ||
| context | ||
| .cycle_time | ||
| .start_time | ||
| .duration_since(ball.last_seen) | ||
| .expect("time ran backwards") | ||
| < context.striker_trusts_team_ball.mul_f32(4.5) | ||
| }) |
There was a problem hiding this comment.
This is duplication to the condition below, maybe extract this and refactor?
12 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why? What?
This PR refactors
I still haven't been able to reproduce the exact bug related to time to reach that we found at RoboCup last year but this PR should hopefully fix it.
Fixes #
ToDo / Known Issues
There are still some todos left in the code
Ideas for Next Iterations (Not This PR)
How to Test
Best approach is probably to play around with robots, both in simulation and on real hardware.