refactor(ndt_scan_matcher, map_loader): remove map_module

[SakodaShintaro]

Description

This pull request proposes removing map_module. I think this is generally a good change, but I'm open to it being rejected based on discussion.

Context

Currently, there are two modules related to pcd loading in the ndt_scan_matcher package: map_module and map_update_module. These can be switched by setting use_dynamic_map_loading to true/false in the config. (Only one is used at a time).

• map_module : Has a callback for the map point cloud and sets the incoming point cloud to the NDT InputTarget
• map_update_module : Has a client for the pcd_loader service and dynamically loads the map. It also has ekf callback and timer callback.

Functionally, map_update_module is upwards compatible, so map_module is no longer necessary.

The map_update_module was introduced in pull request #2339 about a year ago. It has been working stably since then, so I believe now is a good time to fully migrate to using it instead of map_module.

Changes

Removing map_module also requires enable_differential_load to always be true in map_loader, so this pull request also includes a fix for that.

If this pull request is acceptable, I will also modify autoware_launch to remove the parameters.

Related PR

• refactor(ndt_scan_matcher, map_loader): remove unused parameters autoware_launch#769

Tests performed

It has been confirmed that the logging_simulator runs with the same accuracy as before on AWSIM data with GT.

Effects on system behavior

map_module will no longer be available. As the function is map_update_module, I don't think there will be any impact.

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

• I've confirmed the contribution guidelines.
• The PR follows the pull request guidelines.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

• The PR follows the pull request guidelines.

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.

[mitsudome-r]

Does this mean that maps without map_config.yaml won't be supported after the removal of map_module?

[SakodaShintaro]

@mitsudome-r
Thank you for your comment.
I apologize, I was not aware of the existence of map_config.yaml. After some research, I understand that map_config.yaml is currently not implemented yet and may be used in the future.

I believe that as long as map_config.yaml is properly handled in the map loader, differential_map_loading will run normally. Depending on the implementation at the time, it will be easy to support even without map_config.yaml.
I don't think the changes in this pull request will have any impact about map_config.yaml.

[Motsu-san]

Does this mean that maps without map_config.yaml won't be supported after the removal of map_module?

@SakodaShintaro Simply you said, "No." didn't you?
@mitsudome-r Could you let us know where you worry? Is any slide needed to show that the changes in this pull request doesn't have any impact about map_config.yaml?

[SakodaShintaro]

@Motsu-san
Yes. This pull request does not affect map_config.yaml in any way, I think.

[kminoda]

Assuming that @mitsudome-r's concern was about pointcloud_map_metadata.yaml and not map_config.yaml, the answer is no. Currently, the differential_map_loading I/F from map_loader will be successfully launched when either of the conditions is met:

1. The pcd is a single file
2. The pcd file consists of multiple files, and an appropriate pointcloud_map_metadata.yaml is provided

This does not change even if we disable the ROS 2 topic based I/F between ndt_scan_matcher and map_loader.

So, to summarize, a map without pointcloud_map_metadata.yaml will not be supported only if it consists of multiple PCD files, which is also true even before this PR.

[Motsu-san]

@mitsudome-r Does Minoda-san's explanation filled your concerns? Or should we provide an evidence(capture movie?) to prove the following?

So, to summarize, a map without pointcloud_map_metadata.yaml will not be supported only if it consists of multiple PCD files, which is also true even before this PR.

[KYabuuchi]

related discussion: https://github.com/orgs/autowarefoundation/discussions/4062

[KYabuuchi]

pointcloud_map and input_ekf_odom are no longer subscribed. So, please remove them in README.

Actually, input_ekf_odom was erased in the other PR

[codecov]

Codecov Report

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

Comparison is base (6ff47cf) 15.19% compared to head (b6e377a) 15.33%.
Report is 4 commits behind head on main.

Files	Patch %	Lines
...intcloud_map_loader/pointcloud_map_loader_node.cpp	0.00%	9 Missing ⚠️
...ion/ndt_scan_matcher/src/ndt_scan_matcher_core.cpp	0.00%	1 Missing and 1 partial ⚠️
...ization/ndt_scan_matcher/src/map_update_module.cpp	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5873      +/-   ##
==========================================
+ Coverage   15.19%   15.33%   +0.14%     
==========================================
  Files        1749     1750       +1     
  Lines      120942   119937    -1005     
  Branches    36775    36316     -459     
==========================================
+ Hits        18375    18398      +23     
+ Misses      81941    80893    -1048     
- Partials    20626    20646      +20     

Flag	Coverage Δ		*Carryforward flag
differential	17.54% <0.00%> (?)
total	15.33% <ø> (+0.13%)	⬆️	Carriedforward from fc48450

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

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

[KYabuuchi]

LGTM

[mitsudome-r]

@Motsu-san Thanks. I think @kminoda's explanation answers my concern.