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_parser): refactored landmark manager #5511

Merged

Conversation

SakodaShintaro
Copy link
Contributor

@SakodaShintaro SakodaShintaro commented Nov 7, 2023

Description

Refactored landmark_parser.

I have made the following changes instead of the above changes.

  • Changed the package name to landmark_manager
  • Added namespace landmark_manager
  • Change the return type of parse_landmark from map to vector

Tests performed

It has been confirmed that ar_tag_based_localizer works with the same performance as before.

Effects on system behavior

There is no effect on system behavior.

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.

Signed-off-by: Shintaro Sakoda <shintaro.sakoda@tier4.jp>
Signed-off-by: Shintaro Sakoda <shintaro.sakoda@tier4.jp>
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) labels Nov 7, 2023
@SakodaShintaro SakodaShintaro changed the title Refactor/landmark manager refactor(landmark_parser): refactored landmark manager Nov 7, 2023
pre-commit-ci bot and others added 2 commits November 7, 2023 07:16
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 Nov 7, 2023
@SakodaShintaro SakodaShintaro marked this pull request as ready for review November 7, 2023 07:27
KYabuuchi
KYabuuchi previously approved these changes Nov 7, 2023
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 😃

Copy link

codecov bot commented Nov 7, 2023

Codecov Report

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

Comparison is base (2a697c2) 15.26% compared to head (f0a49c5) 15.25%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5511      +/-   ##
==========================================
- Coverage   15.26%   15.25%   -0.02%     
==========================================
  Files        1715     1717       +2     
  Lines      118232   118298      +66     
  Branches    37797    37827      +30     
==========================================
- Hits        18045    18043       -2     
- Misses      79636    79710      +74     
+ Partials    20551    20545       -6     
Flag Coverage Δ *Carryforward flag
differential 6.52% <0.00%> (?)
total 15.26% <ø> (-0.01%) ⬇️ Carriedforward from 9110793

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

Files Coverage Δ
...ulator/vehicle_model/sim_model_delay_steer_acc.hpp 100.00% <ø> (ø)
...vehicle_model/sim_model_delay_steer_acc_geared.hpp 100.00% <ø> (ø)
...nning_simulator/simple_planning_simulator_core.cpp 35.76% <ø> (ø)
...ulator/vehicle_model/sim_model_delay_steer_acc.cpp 83.82% <ø> (+3.26%) ⬆️
...vehicle_model/sim_model_delay_steer_acc_geared.cpp 83.14% <ø> (+2.50%) ⬆️
...ager/include/landmark_manager/landmark_manager.hpp 0.00% <0.00%> (ø)
...tag_based_localizer/src/ar_tag_based_localizer.cpp 3.93% <0.00%> (-0.05%) ⬇️
...ocalizer/landmark_manager/src/landmark_manager.cpp 0.00% <0.00%> (ø)

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

@SakodaShintaro
Copy link
Contributor Author

SakodaShintaro commented Nov 7, 2023

Yamato Ando and I have identified a problem. (It is not new and was not introduced by this pull request.)

There is a need for better management of multiple tags, especially when multiple tags share the same ID.

std::map<std::string, geometry_msgs::msg::Pose> landmarks_;

should be changed as follows

std::map<std::string, std::vector<geometry_msgs::msg::Pose>> landmarks_;

The convert_landmark_pose_to_ego_pose function should return the ego pose that is closest to the EKF Pose when there are multiple landmarks with the same ID. Consequently, the EKF Pose must be passed as an additional input to this function.

For instance, in the case of the LiDAR Intensity Detector, there is currently only one type of board that it can detect.

I think, even with this modification, problems may persist if multiple landmarks with the same ID are detected in a single scan or single image. I believe the current solution is to be careful in operation to prevent this from happening.

For now, I will implement the aforementioned changes.

@SakodaShintaro SakodaShintaro marked this pull request as draft November 8, 2023 08:43
@KYabuuchi KYabuuchi dismissed their stale review November 8, 2023 09:08

dismiss my review because PR turns to draft

SakodaShintaro and others added 6 commits November 10, 2023 09:34
Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>
Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>
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 November 13, 2023 08:14
@SakodaShintaro
Copy link
Contributor Author

Changes have been completed. Please review again.

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.

👍

@SakodaShintaro SakodaShintaro merged commit ea12d77 into autowarefoundation:main Nov 13, 2023
@SakodaShintaro SakodaShintaro deleted the refactor/landmark_manager branch November 13, 2023 23:49
takayuki5168 pushed a commit to tier4/autoware.universe that referenced this pull request Nov 22, 2023
…ation#5511)

* Refactored landmark_parser

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

* Renamed landmark_parser to landmark_manager

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

* Fixed tag_id

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

* Refactored ar_tag_based_localizer

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

* style(pre-commit): autofix

* Added [[nodiscard]]

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

* Refactored landmark parsing and conversion

Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>

* style(pre-commit): autofix

* Added namespace

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

* Fixed include

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

---------

Signed-off-by: Shintaro Sakoda <shintaro.sakoda@tier4.jp>
Signed-off-by: Shintaro SAKODA <shintaro.sakoda@tier4.jp>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants