Skip to content
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(landmark_based_localizer): refactored landmark_tf_caster #5414

Conversation

SakodaShintaro
Copy link
Contributor

@SakodaShintaro SakodaShintaro commented Oct 26, 2023

Description

  • Removed landmark_tf_caster node, and modified the package as a library providing parsing functions.
    • Renamed landmark_tf_caster to landmark_parser
  • Landmark information is not informed via /tf_static, but is held by each landmark_based_localizer as a std::map<std::string, geometry_msgs::msg::Pose>.
  • Subscribe to autoware_auto_mapping_msgs::msg::HADMapBin is also performed by each landmark based localizer.
  • Added debug topic ~/debug/marker
    • can be watched by image
    • result
      Screenshot from 2023-10-26 14-39-16

Tests performed

The AR tag based localizer is confirmed to work properly.

Effects on system behavior

There is no change in the behavior on the system as a whole.

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.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

SakodaShintaro and others added 3 commits October 26, 2023 10:05
Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>
Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>
@github-actions github-actions bot added type:documentation Creating or refining documentation. (auto-assigned) component:localization Vehicle's position determination in its environment. (auto-assigned) component:launch Launch files, scripts and initialization tools. (auto-assigned) labels Oct 26, 2023
SakodaShintaro and others added 3 commits October 26, 2023 10:22
Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>
Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>
@SakodaShintaro SakodaShintaro added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Oct 26, 2023
@SakodaShintaro SakodaShintaro marked this pull request as ready for review October 26, 2023 01:34
@SakodaShintaro SakodaShintaro marked this pull request as draft October 26, 2023 02:23
Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>
Signed-off-by: Shintaro Sakoda <shintaro.sakoda@tier4.jp>
@SakodaShintaro SakodaShintaro marked this pull request as ready for review October 26, 2023 05:51
<license>Apache License 2.0</license>

<buildtool_depend>ament_cmake</buildtool_depend>
<buildtool_depend>autoware_cmake</buildtool_depend>

<depend>autoware_auto_mapping_msgs</depend>
<depend>eigen</depend>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<depend>eigen</depend>
<build_depend>eigen</build_depend>

Signed-off-by: Shintaro Sakoda <shintaro.sakoda@tier4.jp>
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Attention: 90 lines in your changes are missing coverage. Please review.

Comparison is base (82a6c0c) 14.75% compared to head (acbd19e) 14.74%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5414      +/-   ##
==========================================
- Coverage   14.75%   14.74%   -0.02%     
==========================================
  Files        1661     1662       +1     
  Lines      115525   115612      +87     
  Branches    35670    35670              
==========================================
  Hits        17050    17050              
- Misses      79231    79318      +87     
  Partials    19244    19244              
Flag Coverage Δ *Carryforward flag
differential 0.00% <0.00%> (?)
total 14.75% <ø> (+<0.01%) ⬆️ Carriedforward from 9e3ddcd

*This pull request uses carry forward flags. Click here to find out more.

Files Coverage Δ
...ased_localizer/src/ar_tag_based_localizer_core.cpp 0.00% <0.00%> (ø)
...lizer/landmark_parser/src/landmark_parser_core.cpp 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KYabuuchi
Copy link
Contributor

@SakodaShintaro In my environment, some landmarks does not appear in rviz. Do you know what is causing it?
image

Signed-off-by: Shintaro Sakoda <shintaro.sakoda@tier4.jp>
@KYabuuchi
Copy link
Contributor

⬆️ now I understand the cause.
I think it would be better to use visualization_msgs::msg::MarkerArray and publish it as one topic.

Signed-off-by: Shintaro Sakoda <shintaro.sakoda@tier4.jp>
@SakodaShintaro
Copy link
Contributor Author

@KYabuuchi
I agree 100% that it would be better to use a single MarkerArray instead of multiple Markers. Fixed in acbd19e

Copy link
Contributor

@KYabuuchi KYabuuchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👌

@SakodaShintaro SakodaShintaro enabled auto-merge (squash) October 26, 2023 08:12
Copy link
Contributor

@Motsu-san Motsu-san left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTMeow

@SakodaShintaro SakodaShintaro merged commit 64c8ac8 into autowarefoundation:main Oct 26, 2023
@SakodaShintaro SakodaShintaro deleted the refactor/remove_landmark_tf_caster branch October 26, 2023 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:launch Launch files, scripts and initialization tools. (auto-assigned) component:localization Vehicle's position determination in its environment. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) type:documentation Creating or refining documentation. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants