-
Notifications
You must be signed in to change notification settings - Fork 625
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
refactor(lane_change): add new structs (TransientData) to store commonly used values #8849
base: main
Are you sure you want to change the base?
refactor(lane_change): add new structs (TransientData) to store commonly used values #8849
Conversation
Thank you for contributing to the Autoware project! 🚧 If your pull request is in progress, switch it to draft mode. Please ensure:
|
574f9c0
to
d51d847
Compare
...g/behavior_path_planner/autoware_behavior_path_avoidance_by_lane_change_module/src/scene.cpp
Outdated
Show resolved
Hide resolved
planning/behavior_path_planner/autoware_behavior_path_lane_change_module/src/scene.cpp
Outdated
Show resolved
Hide resolved
planning/behavior_path_planner/autoware_behavior_path_lane_change_module/src/scene.cpp
Outdated
Show resolved
Hide resolved
if (lane_changing_length + prepare_length > dist_to_end_of_current_lanes) { | ||
if ( | ||
lane_changing_length + prepare_length + | ||
common_data_ptr_->transient_data.next_lc_buffer.min > |
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 curious why did you add the next LC buffer 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.
I'm making an assumption that we want to do more than 1 lane changes. This is assuming that the end point of current and target lane align with each other.
Additional note: some additional fix is needed if the end point is not aligned.
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.
but isn't this covered by the check here
d9df9e5
to
9bac424
Compare
const auto lc_length_and_dist_buffer = | ||
utils::lane_change::calculation::calc_lc_length_and_dist_buffer( | ||
common_data_ptr_, get_current_lanes()); | ||
return std::get<1>(lc_length_and_dist_buffer).min; |
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.
(minor) I think you can do something like this which is a bit easier to read.
const auto lc_length_and_dist_buffer = | |
utils::lane_change::calculation::calc_lc_length_and_dist_buffer( | |
common_data_ptr_, get_current_lanes()); | |
return std::get<1>(lc_length_and_dist_buffer).min; | |
const auto [_, dist_buffer] = | |
utils::lane_change::calculation::calc_lc_length_and_dist_buffer( | |
common_data_ptr_, get_current_lanes()); | |
return dist_buffer.min; |
const auto current_lanes = get_current_lanes(); | ||
if (current_lanes.empty()) { | ||
RCLCPP_DEBUG(logger_, "no empty lanes"); | ||
return std::numeric_limits<double>::infinity(); |
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 I understand the code correctly, the new return value in case current_lanes
is empty is 0
compared to infinity()
previously. Is this okay ?
- the
calc_lc_length_and_dist_buffer()
returns{}
whenlanes
is empty. {}
uses the default constructor ofpair
which uses the default constructor ofBoundary
which setsmin = 0.0
.
const auto acc_boundary = calcCurrentMinMaxAcceleration(); | ||
transient_data.acc.min = std::get<0>(acc_boundary); | ||
transient_data.acc.max = std::get<1>(acc_boundary); |
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 you can do something like that:
const auto acc_boundary = calcCurrentMinMaxAcceleration(); | |
transient_data.acc.min = std::get<0>(acc_boundary); | |
transient_data.acc.max = std::get<1>(acc_boundary); | |
std::tie(transient_data.acc.min, transient_data.acc.max) = calcCurrentMinMaxAcceleration(); |
or are they the same type ? In which case:
const auto acc_boundary = calcCurrentMinMaxAcceleration(); | |
transient_data.acc.min = std::get<0>(acc_boundary); | |
transient_data.acc.max = std::get<1>(acc_boundary); | |
transient_data = calcCurrentMinMaxAcceleration(); |
Signed-off-by: Muhammad Zulfaqar Azmi <[email protected]>
Signed-off-by: Muhammad Zulfaqar Azmi <[email protected]>
Signed-off-by: Muhammad Zulfaqar Azmi <[email protected]>
Signed-off-by: Muhammad Zulfaqar Azmi <[email protected]>
Signed-off-by: Muhammad Zulfaqar Azmi <[email protected]>
Signed-off-by: Muhammad Zulfaqar Azmi <[email protected]>
Signed-off-by: Muhammad Zulfaqar Azmi <[email protected]>
Signed-off-by: Muhammad Zulfaqar Azmi <[email protected]>
Signed-off-by: Muhammad Zulfaqar Azmi <[email protected]>
Signed-off-by: Zulfaqar Azmi <[email protected]>
Signed-off-by: Zulfaqar Azmi <[email protected]>
Signed-off-by: Zulfaqar Azmi <[email protected]>
Signed-off-by: Zulfaqar Azmi <[email protected]>
Signed-off-by: Zulfaqar Azmi <[email protected]>
Signed-off-by: Zulfaqar Azmi <[email protected]>
Signed-off-by: Zulfaqar Azmi <[email protected]>
Signed-off-by: Zulfaqar Azmi <[email protected]>
9bac424
to
bb4f783
Compare
Description
This PR adds
TransientData
to standardize commonly used values in the lane change module, such as the current lanes' buffer and the distance to the terminal start and end. The goal is to ensure consistency when calculating limits and lengths.Related links
Parent Issue:
How was this PR tested?
Notes for reviewers
None.
Interface changes
None.
Effects on system behavior
None.