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(pointcloud_preprocessor): separate concatenate filter node and output synchronized pointcloud #3312

Merged

Conversation

YoshiRi
Copy link
Contributor

@YoshiRi YoshiRi commented Apr 7, 2023

Description

Purpose

  • Divide concatenate_data component based on its two functions:
    • synchronize pointcloud topics and compensate time stamp difference with twist information
    • concatenate pointclouds and convert its frame to base_link
  • Publish synchronized raw pointclouds which are needed for postprocessing, like occupancy grid map generation

Changes

  • separate concatenate_data_node component into time_synchronizer_node and concatenate_pointclouds_node
  • use separated process in the preprocessor.launch.py
  • temporary keep concatenate_data_node for backward compatibility

Changes in conventional node

  • appearance
    • Keep the conventional node for temporary backward compatibility
    • Changed conventional node name to avoid confusion
  • processing algorithm
    • Separate transform compensation and pointclouds merging
current after
image image

Related links

Tests performed

Test performed with sample-rosbag in the logging simulator tutorials.

method mean processing time [ms] min max
original 6.262653 2.787000 10.513000
after separated 7.122316 5.093000 12.041000
method processing time
original image
time_synchronizer only image
concatenator only image
total computation time after changed image

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.

@YoshiRi YoshiRi requested review from amc-nu, miursh, yukkysaito and a team as code owners April 7, 2023 11:00
@github-actions github-actions bot added the component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) label Apr 7, 2023
@YoshiRi
Copy link
Contributor Author

YoshiRi commented Apr 7, 2023

Next TODO:

  • Update README
  • Update Node diagram (depend on user choices)
  • Update sensor_kit launch (depend on projects)

@codecov
Copy link

codecov bot commented Apr 7, 2023

Codecov Report

Patch coverage: 0.14% and project coverage change: -0.80% ⚠️

Comparison is base (04ea73c) 15.03% compared to head (2d7986a) 14.23%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3312      +/-   ##
==========================================
- Coverage   15.03%   14.23%   -0.80%     
==========================================
  Files        1508     1580      +72     
  Lines      103879   108896    +5017     
  Branches    31730    31360     -370     
==========================================
- Hits        15614    15499     -115     
- Misses      71233    76551    +5318     
+ Partials    17032    16846     -186     
Flag Coverage Δ *Carryforward flag
differential 6.77% <0.00%> (?)
total 14.33% <0.59%> (-0.70%) ⬇️ Carriedforward from a5d1407

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

Files Changed Coverage Δ
...tion/distance_based_compare_map_filter_nodelet.hpp 0.00% <0.00%> (ø)
...l_based_approximate_compare_map_filter_nodelet.hpp 0.00% <0.00%> (ø)
...oxel_distance_based_compare_map_filter_nodelet.hpp 0.00% <0.00%> (ø)
...compare_map_segmentation/voxel_grid_map_loader.hpp 0.00% <0.00%> (ø)
...l_based_approximate_compare_map_filter_nodelet.cpp 0.00% <0.00%> (ø)
...ion/src/voxel_based_compare_map_filter_nodelet.cpp 0.00% <0.00%> (ø)
...oxel_distance_based_compare_map_filter_nodelet.cpp 0.00% <0.00%> (ø)
...are_map_segmentation/src/voxel_grid_map_loader.cpp 0.00% <0.00%> (ø)
...ule/dynamic_avoidance/dynamic_avoidance_module.hpp 0.00% <0.00%> (ø)
...th_planner/scene_module/scene_module_interface.hpp 16.53% <0.00%> (+0.14%) ⬆️
... and 11 more

... and 319 files with indirect coverage changes

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

@yukkysaito
Copy link
Contributor

yukkysaito commented Apr 10, 2023

If the nodes are separated, I think it is better to use concatenate_node and time_synchronizer_node for name? (not concatenate_and_time_sync_nodelet)

Also, If you have some TODO, can you change to Draft?

@YoshiRi YoshiRi marked this pull request as draft April 11, 2023 17:17
@YoshiRi YoshiRi force-pushed the feature/separate_concatenate_filter_node branch 2 times, most recently from 8f50cec to 18461c9 Compare May 28, 2023 17:26
@github-actions github-actions bot added the type:documentation Creating or refining documentation. (auto-assigned) label May 28, 2023
@YoshiRi YoshiRi marked this pull request as ready for review May 28, 2023 17:52
@miursh
Copy link
Contributor

miursh commented May 30, 2023

@YoshiRi
One question, the concatenate algorithm is exactly the same as current one?

@YoshiRi
Copy link
Contributor Author

YoshiRi commented May 31, 2023

@YoshiRi One question, the concatenate algorithm is exactly the same as current one?

As for the pointcloud synchronization process with buffer and sub-buffer, the answer is Yes.

Other changes in the conventional nodes is like:

  • changes about publish synched pointclouds
  • separate concat process and time sync process to get transformed pointcloud

YoshiRi added 11 commits June 12, 2023 13:54
Signed-off-by: yoshiri <yoshiyoshidetteiu@gmail.com>
Signed-off-by: yoshiri <yoshiyoshidetteiu@gmail.com>
Signed-off-by: yoshiri <yoshiyoshidetteiu@gmail.com>
Signed-off-by: yoshiri <yoshiyoshidetteiu@gmail.com>
Signed-off-by: yoshiri <yoshiyoshidetteiu@gmail.com>
Signed-off-by: yoshiri <yoshiyoshidetteiu@gmail.com>
Signed-off-by: yoshiri <yoshiyoshidetteiu@gmail.com>
Signed-off-by: yoshiri <yoshiyoshidetteiu@gmail.com>
Signed-off-by: yoshiri <yoshiyoshidetteiu@gmail.com>
Signed-off-by: yoshiri <yoshiyoshidetteiu@gmail.com>
Signed-off-by: yoshiri <yoshiyoshidetteiu@gmail.com>
@YoshiRi YoshiRi force-pushed the feature/separate_concatenate_filter_node branch from 542f837 to 3d390b5 Compare June 12, 2023 04:54
@YoshiRi YoshiRi enabled auto-merge (squash) June 21, 2023 03:02
YoshiRi added 3 commits July 14, 2023 18:21
Signed-off-by: yoshiri <yoshiyoshidetteiu@gmail.com>
Signed-off-by: yoshiri <yoshiyoshidetteiu@gmail.com>
Copy link
Contributor

@miursh miursh left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:sensing Data acquisition from sensors, drivers, preprocessing. (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
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants