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

feat(ndt_scan_matcher): dynamic map loading #2339

Merged
merged 56 commits into from
Feb 3, 2023

Conversation

kminoda
Copy link
Contributor

@kminoda kminoda commented Nov 21, 2022

Signed-off-by: kminoda [email protected]

Description

I would like to add a dynamic map loading functionality option for ndt_scan_matcher.

In order to pass the build CI, we first need to merge

Must be merged with

NOTE:
This PR will replace current ndt_omp with multigrid_ndt_omp, which is a customized version for dynamic map loading feature. Although most of the interfaces prepared in ndt_omp can also be used in multigrid_ndt_omp, there are one difference:

  • multigrid_ndt_omp does not have neighborhood_search_method option, since we currently do not implement search method other than kd-tree.

Thus, do NOTE that neighborhood_search_method parameter in ndt_scan_matcher is also removed in this PR.

Related links

Tests performed

I tested with a data from Autoware tutorial. The map is divided into 20m grids (sample-map-rosbag_split.zip).

  • I have confirmed that the Autoware performs the same as the current Autoware when use_dynamic_map_loading is set false.
  • I have confirmed the following when use_dynamic_map_loading is set true.
    • localization performance does not change
    • map is dynamically loaded in ndt_scan_matcher (see /localization/pose_estimator/debug/loaded_pointcloud_map)

In addition, we, TIER IV, have confirmed that the computational resource required for dynamic map loading is low enough for several TIER IV vehicles, based on a several experiments with real hardware.

Notes for reviewers

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.

  • The PR follows the pull request guidelines.
  • The PR has been properly tested.
  • The PR has been reviewed by the code owners.

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.
  • The PR is ready for merge.

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

Signed-off-by: kminoda <[email protected]>
@kminoda kminoda self-assigned this Nov 21, 2022
@github-actions github-actions bot added the component:localization Vehicle's position determination in its environment. (auto-assigned) label Nov 21, 2022
@github-actions github-actions bot added the component:launch Launch files, scripts and initialization tools. (auto-assigned) label Nov 22, 2022
Signed-off-by: kminoda <[email protected]>
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.

I have tried all combinations of parameters and verified that the node works according to this table. 👍

parameter_combination_table

@KYabuuchi
Copy link
Contributor

Since the PR about autoware_launch will change the config of map_loader, can you also match the config in this PR as follows?
image

@codecov
Copy link

codecov bot commented Jan 19, 2023

Codecov Report

Base: 11.37% // Head: 11.35% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (1503f7d) compared to base (0305906).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2339      +/-   ##
==========================================
- Coverage   11.37%   11.35%   -0.02%     
==========================================
  Files        1277     1278       +1     
  Lines       89317    89625     +308     
  Branches    23653    23681      +28     
==========================================
+ Hits        10160    10179      +19     
- Misses      68386    68665     +279     
- Partials    10771    10781      +10     
Flag Coverage Δ *Carryforward flag
differential 12.45% <0.00%> (?)
total 11.37% <0.00%> (ø) Carriedforward from 0305906

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

Impacted Files Coverage Δ
...ization/ndt_scan_matcher/src/map_update_module.cpp 0.00% <0.00%> (ø)
...ion/ndt_scan_matcher/src/ndt_scan_matcher_core.cpp 0.00% <0.00%> (ø)
planning/static_centerline_optimizer/src/utils.cpp 33.33% <0.00%> (+4.58%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions github-actions bot added the component:map Map creation, storage, and loading. (auto-assigned) label Jan 19, 2023
@kminoda
Copy link
Contributor Author

kminoda commented Jan 19, 2023

@KYabuuchi Thanks! You're right. Resolved here: de0ef18

@RyuYamamoto Since a parameter in map_loader has been modified in this PR, would you also review the change?

@github-actions github-actions bot removed the component:map Map creation, storage, and loading. (auto-assigned) label Jan 26, 2023
@kminoda
Copy link
Contributor Author

kminoda commented Jan 26, 2023

@yukkysaito @KYabuuchi @YamatoAndo
FYI: I disabled dynamic map loading by default, since there exists a case where this setting may harm Autoware system.
(Especially the case when the given map is divided into some large grids, e.g. 1000m x 1000m.)

@github-actions github-actions bot added the component:map Map creation, storage, and loading. (auto-assigned) label Jan 27, 2023
@kminoda
Copy link
Contributor Author

kminoda commented Jan 27, 2023

@yukkysaito @KYabuuchi @YamatoAndo
FYI: After a short discussion with Saito-san, I enabled dynamic map loading by default, again.

The reason is because we can continuously test this new feature by enabling it by default.
(Furthermore, we can avoid the above-mentioned risk, which is the case when the user provide largely divided maps, by properly setting the requirements)

@yukkysaito
Copy link
Contributor

@kminoda Thank you 👍 could you please note any restrictions in the readme?

@kminoda
Copy link
Contributor Author

kminoda commented Jan 30, 2023

@RyuYamamoto Friendly ping for review 🙇

Copy link
Contributor

@RyuYamamoto RyuYamamoto left a comment

Choose a reason for hiding this comment

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

LGTM

@kminoda kminoda merged commit ace34c6 into autowarefoundation:main Feb 3, 2023
asana17 pushed a commit to asana17/autoware.universe that referenced this pull request Feb 8, 2023
* first commit

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* import map update module in core

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* minor fixes. Now map update module launches!!!

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* debugged

Signed-off-by: kminoda <[email protected]>

* revert unnecessary fix

Signed-off-by: kminoda <[email protected]>

* minor fixes

Signed-off-by: kminoda <[email protected]>

* update launch file

Signed-off-by: kminoda <[email protected]>

* update comment

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* update comment

Signed-off-by: kminoda <[email protected]>

* update comment

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* update comment

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* update for ndt_omp

Signed-off-by: kminoda <[email protected]>

* changed parameter names

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* apply pre-commit-

* ci(pre-commit): autofix

* update readme

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* update readme

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* simplify client implementation

Signed-off-by: kminoda <[email protected]>

* remove unnecessary comments

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* removed unused member variables

Signed-off-by: kminoda <[email protected]>

* set default use_dynamic_map_loading to true

Signed-off-by: kminoda <[email protected]>

* changed readme

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* reflected comments

Signed-off-by: kminoda <[email protected]>

* use std::optional instead of shared_ptr

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* fix parameter description

Signed-off-by: kminoda <[email protected]>

* revert launch output config

Signed-off-by: kminoda <[email protected]>

* change default subscriber name

Signed-off-by: kminoda <[email protected]>

* remove unnecessary setInputSource

Signed-off-by: kminoda <[email protected]>

* add gif

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* minor fix

Signed-off-by: kminoda <[email protected]>

* Update localization/ndt_scan_matcher/src/map_update_module.cpp

Co-authored-by: Daisuke Nishimatsu <[email protected]>

* update literals

Signed-off-by: kminoda <[email protected]>

* update map_loader default parameters

* update readme

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

---------

Signed-off-by: kminoda <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Daisuke Nishimatsu <[email protected]>
badai-nguyen pushed a commit to badai-nguyen/autoware.universe that referenced this pull request Mar 22, 2023
* first commit

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* import map update module in core

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* minor fixes. Now map update module launches!!!

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* debugged

Signed-off-by: kminoda <[email protected]>

* revert unnecessary fix

Signed-off-by: kminoda <[email protected]>

* minor fixes

Signed-off-by: kminoda <[email protected]>

* update launch file

Signed-off-by: kminoda <[email protected]>

* update comment

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* update comment

Signed-off-by: kminoda <[email protected]>

* update comment

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* update comment

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* update for ndt_omp

Signed-off-by: kminoda <[email protected]>

* changed parameter names

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* apply pre-commit-

* ci(pre-commit): autofix

* update readme

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* update readme

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* simplify client implementation

Signed-off-by: kminoda <[email protected]>

* remove unnecessary comments

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* removed unused member variables

Signed-off-by: kminoda <[email protected]>

* set default use_dynamic_map_loading to true

Signed-off-by: kminoda <[email protected]>

* changed readme

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* reflected comments

Signed-off-by: kminoda <[email protected]>

* use std::optional instead of shared_ptr

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* fix parameter description

Signed-off-by: kminoda <[email protected]>

* revert launch output config

Signed-off-by: kminoda <[email protected]>

* change default subscriber name

Signed-off-by: kminoda <[email protected]>

* remove unnecessary setInputSource

Signed-off-by: kminoda <[email protected]>

* add gif

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* minor fix

Signed-off-by: kminoda <[email protected]>

* Update localization/ndt_scan_matcher/src/map_update_module.cpp

Co-authored-by: Daisuke Nishimatsu <[email protected]>

* update literals

Signed-off-by: kminoda <[email protected]>

* update map_loader default parameters

* update readme

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

---------

Signed-off-by: kminoda <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Daisuke Nishimatsu <[email protected]>
badai-nguyen pushed a commit to badai-nguyen/autoware.universe that referenced this pull request Apr 4, 2023
* first commit

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* import map update module in core

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* minor fixes. Now map update module launches!!!

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* debugged

Signed-off-by: kminoda <[email protected]>

* revert unnecessary fix

Signed-off-by: kminoda <[email protected]>

* minor fixes

Signed-off-by: kminoda <[email protected]>

* update launch file

Signed-off-by: kminoda <[email protected]>

* update comment

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* update comment

Signed-off-by: kminoda <[email protected]>

* update comment

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* update comment

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* update for ndt_omp

Signed-off-by: kminoda <[email protected]>

* changed parameter names

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* apply pre-commit-

* ci(pre-commit): autofix

* update readme

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* update readme

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* simplify client implementation

Signed-off-by: kminoda <[email protected]>

* remove unnecessary comments

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* removed unused member variables

Signed-off-by: kminoda <[email protected]>

* set default use_dynamic_map_loading to true

Signed-off-by: kminoda <[email protected]>

* changed readme

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* reflected comments

Signed-off-by: kminoda <[email protected]>

* use std::optional instead of shared_ptr

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* fix parameter description

Signed-off-by: kminoda <[email protected]>

* revert launch output config

Signed-off-by: kminoda <[email protected]>

* change default subscriber name

Signed-off-by: kminoda <[email protected]>

* remove unnecessary setInputSource

Signed-off-by: kminoda <[email protected]>

* add gif

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

* minor fix

Signed-off-by: kminoda <[email protected]>

* Update localization/ndt_scan_matcher/src/map_update_module.cpp

Co-authored-by: Daisuke Nishimatsu <[email protected]>

* update literals

Signed-off-by: kminoda <[email protected]>

* update map_loader default parameters

* update readme

Signed-off-by: kminoda <[email protected]>

* ci(pre-commit): autofix

---------

Signed-off-by: kminoda <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Daisuke Nishimatsu <[email protected]>
@kminoda kminoda deleted the feature/dynamic_map_loading branch August 10, 2023 08:51
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) component:map Map creation, storage, and loading. (auto-assigned) type:documentation Creating or refining documentation. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants