-
Notifications
You must be signed in to change notification settings - Fork 71
feat(lanelet2_extension): overwriteLaneletsCenterline supports "waypoints" #252
Conversation
Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com>
Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com>
Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com>
9d7d9d0
to
125faec
Compare
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.
Clang-Tidy
found issue(s) with the introduced code (1/1)
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.
Clang-Tidy
found issue(s) with the introduced code (1/1)
f1127e8
to
f8ad8ba
Compare
Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com>
6c5bd89
to
4ebd7c9
Compare
Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com>
48c94d3
to
6d19fd5
Compare
if (use_waypoints) { | ||
if (lanelet_obj.hasCustomCenterline()) { | ||
const auto & centerline = lanelet_obj.centerline(); | ||
lanelet_obj.setAttribute("waypoints", centerline.id()); | ||
} | ||
|
||
const auto fine_center_line = generateFineCenterline(lanelet_obj, resolution); | ||
lanelet_obj.setCenterline(fine_center_line); |
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 a bit concerned that the function start to exceed what it was originally designed for.
Is it necessary to move the customized center line to waypoints
at runtime?
/**
* @brief Apply a patch for centerline because the original implementation
* doesn't have enough quality
*/
@@ -501,12 +501,28 @@ lanelet::ConstLanelets getExpandedLanelets( | |||
} | |||
|
|||
void overwriteLaneletsCenterline( | |||
lanelet::LaneletMapPtr lanelet_map, const double resolution, const bool force_overwrite) | |||
lanelet::LaneletMapPtr lanelet_map, const double resolution, const bool use_waypoints, |
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 as a comment, since the third argument of the function has changed it's meaning, we should make sure that any other code that uses force_overwrite
argument should be merged at the same time as this PR.
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.
@takayuki5168 Have you checked if the codes in autoware.universe and autoware_tools are fine with the modification?
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.
Thank you for the review.
Yes, I've already checked. The following PR is the change for the universe.
https://github.com/autowarefoundation/autoware.universe/pull/7480/files
Co-authored-by: Ryohsuke Mitsudome <43976834+mitsudome-r@users.noreply.github.com>
324989a
to
c626514
Compare
c626514
to
45a656c
Compare
Description
The Autoware's usage of the centerline has several limitations.
By introducing a
waypoints
flag in theoverwriteLaneletsCenterline
function to handle the centerline, the limitations are solved.Please see the document in this PR in detail.
NOTE: This PR includes the fix for clang-tidy in the lanelet2_extension_python package.
Tests performed
Unit test
Effects on system behavior
Nothing
Pre-review checklist for the PR author
The PR author must check the checkboxes below when creating the PR.
In-review checklist for the PR reviewers
The PR reviewers must check the checkboxes below before approval.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
After all checkboxes are checked, anyone who has write access can merge the PR.