feat(autoware_pointcloud_preprocessor): templated version of the pointcloud concatenation #10298

knzo25 · 2025-03-19T00:37:45Z

Description

As part of #9722,
the pointcloud concatenation as accelerated using cuda.
Since we do not want to add a cuda dependency to this package, nor want to duplicate a node that has become more complex, I implemented a templated version of the node, which will enable cuda acceleration in the respective package with minimal code duplication and will easily enable to reuse this logic for radar concatenation.

In addition, this node PR changes the use of some shared pointers to unique pointers, which reduces an entire copy of the pointclouds.

The layout checks were also copied to the autoware_point_types package, yet they are still in this package. I will move them in a later PR

How was this PR tested?

Tested using my local environment an a robotaxi rosbag

Notes for reviewers

This PR should work the same as the baseline as it only refactors it into a templated form

Interface changes

None.

Effects on system behavior

None.

…lementations and extend it to radars Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

github-actions · 2025-03-19T00:37:59Z

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

You've checked our contribution guidelines.
Your PR follows our pull request guidelines.
All required CI checks pass before marking the PR ready for review.

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

codecov · 2025-03-19T01:20:42Z

Codecov Report

Attention: Patch coverage is 64.70588% with 150 lines in your changes missing coverage. Please review.

Project coverage is 25.90%. Comparing base (b70f555) to head (8b615c8).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...oncatenate_data/concatenate_and_time_sync_node.ipp	64.17%	45 Missing and 27 partials ⚠️
..._preprocessor/concatenate_data/cloud_collector.ipp	40.90%	32 Missing and 7 partials ⚠️
...r/concatenate_data/collector_matching_strategy.ipp	52.38%	17 Missing and 3 partials ⚠️
...are_pointcloud_preprocessor/src/utility/memory.cpp	37.50%	8 Missing and 2 partials ⚠️
...rc/concatenate_data/combine_cloud_handler_base.cpp	84.48%	5 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10298      +/-   ##
==========================================
+ Coverage   25.44%   25.90%   +0.46%     
==========================================
  Files        1321     1385      +64     
  Lines      102799   107044    +4245     
  Branches    39102    40990    +1888     
==========================================
+ Hits        26154    27728    +1574     
- Misses      74131    76603    +2472     
- Partials     2514     2713     +199

Flag	Coverage Δ		*Carryforward flag
differential	`21.77% <64.70%> (?)`
differential-cuda	`18.80% <64.23%> (?)`
total	`25.76% <ø> (+0.32%)`	⬆️	Carriedforward from d83e50a

*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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

vividf

Thanks for the PR!

I noticed that one or more functions might not be using the latest version from the concatenation node.
Could you please update them to reflect the most recent implementation?

vividf · 2025-03-21T07:10:07Z

...d_preprocessor/include/autoware/pointcloud_preprocessor/concatenate_data/cloud_collector.hpp

    double timeout_sec, bool debug_mode);
  bool topic_exists(const std::string & topic_name);
-  void process_pointcloud(
-    const std::string & topic_name, sensor_msgs::msg::PointCloud2::SharedPtr cloud);
+  bool process_pointcloud(


Could you update this function to reflect the latest version?

Sorry, the rebase did not work as expected. Check at a glance all the cpp from the base with the ipp and cpp from this PR. This once is addressed in 593ddc6

mojomex

Looking good functionally!
I left some comments on factoring out unrelated classes into their own files, and on reducing templated code by introducing ..Base versions of some classes. Please have a look!

...autoware_pointcloud_preprocessor/include/autoware/pointcloud_preprocessor/utility/memory.hpp

sensing/autoware_pointcloud_preprocessor/src/concatenate_data/combine_cloud_handler.cpp

mojomex · 2025-03-21T06:59:13Z

...include/autoware/pointcloud_preprocessor/concatenate_data/concatenate_and_time_sync_node.ipp

+        if (
+          (*concatenated_cloud_result.topic_to_transformed_cloud_map).find(topic) !=
+          (*concatenated_cloud_result.topic_to_transformed_cloud_map).end()) {
+          auto transformed_cloud_output = std::move((*concatenated_cloud_result.topic_to_transformed_cloud_map).at(topic));


This leaves moved-from unique ptr in the map (which will be a nullptr).
Using .extract instead of .at is a safe alternative that removes the element.

The method you provided does not compile.
If it is for peace of mind, I used an erase afterwards in 34f8008

mojomex · 2025-03-21T07:04:18Z

...include/autoware/pointcloud_preprocessor/concatenate_data/concatenate_and_time_sync_node.hpp

    topic_to_transformed_cloud_publisher_map_;
  std::unique_ptr<autoware_utils::DebugPublisher> debug_publisher_;

  std::unique_ptr<autoware_utils::StopWatch<std::chrono::milliseconds>> stop_watch_ptr_;
  diagnostic_updater::Updater diagnostic_updater_{this};

+  void initialize();


Please choose a more descriptive name, such as initialize_pub_sub etc.

Addressed in 2e49f11

mojomex · 2025-03-21T07:14:39Z

...include/autoware/pointcloud_preprocessor/concatenate_data/concatenate_and_time_sync_node.ipp

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//#include "autoware/pointcloud_preprocessor/concatenate_data/concatenate_and_time_sync_node.hpp"


Please remove this unneeded commented out statement

Thanks. Addressed in 2c51d20

mojomex · 2025-03-21T07:31:55Z

...include/autoware/pointcloud_preprocessor/concatenate_data/concatenate_and_time_sync_node.ipp

+      this->get_clock()->now().seconds(),
+      this->get_clock()->now().seconds() - rclcpp::Time(input_ptr->header.stamp).seconds());
+  }


This should be reverted to cloud_arrival_time given that's what the log message says.

Thanks. It seem this got into due to the rebases...
Addressed in 8fe850c

mojomex · 2025-03-21T07:46:17Z

...or/include/autoware/pointcloud_preprocessor/concatenate_data/collector_matching_strategy.ipp

As far as I can tell, nothing in the CollectorMatchingStrategy directly interacts with pointclouds and thus no templating should be necessary.

It looks like templating was done because CloudCollector is also templated and appears as an argument. I would suggest creating a non-templated CloudCollectorBase to solve this.

I did try that, but ended up creating more mess than it solved. For such simple structure, I don't think that the increased binary size is a problem. Additionally, when everything is templated, it is easier to extend behavior later down the line

mojomex · 2025-03-21T07:48:09Z

...include/autoware/pointcloud_preprocessor/concatenate_data/concatenate_and_time_sync_node.ipp

+}
+
+template <typename MsgTraits>
+std::string PointCloudConcatenateDataSynchronizerComponentTemplated<MsgTraits>::format_timestamp(


Functions like these should not need to be templated. With this particular one, moving it out of the class is probably best.

If there were more than just this one or there was an helper header with these functions I would agree. But I prefer having it here repeated by each template instantiation rather than have a single dangling method

mojomex · 2025-03-21T07:56:12Z

sensing/autoware_pointcloud_preprocessor/src/concatenate_data/combine_cloud_handler.cpp

Please put CombineCloudHandlerBase and CombineCloudHandler<PointCloud2Traits> into separate files. It is difficult to quickly discern between common and PC2-specific functionality.

Addressed in 8b44c16

sensing/autoware_pointcloud_preprocessor/CMakeLists.txt

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

…autoware.universe into feat/concat_pointcloud_templated

knzo25 added 2 commits March 19, 2025 09:05

feat: refactored the concat into a templated design to allow cuda imp…

96999f0

…lementations and extend it to radars Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

fix: moved the concat cpp for consistency and component loading

89fd2b7

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

knzo25 requested review from amc-nu, badai-nguyen, drwnz, kyoichi-sugahara, mojomex, meliketanrikulu, vividf, YoshiRi and yukkysaito as code owners March 19, 2025 00:37

github-actions bot added the component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) label Mar 19, 2025

chore: removed unused dep

49f22be

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

knzo25 added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Mar 19, 2025

knzo25 mentioned this pull request Mar 19, 2025

feat(autoware_cuda_pointcloud_preprocessor): pointcloud concatenation #10300

Open

fix: missing virtual destructor

13ece5c

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

vividf requested changes Mar 21, 2025

View reviewed changes

mojomex requested changes Mar 21, 2025

View reviewed changes

amadeuszsz reviewed Mar 25, 2025

View reviewed changes

sensing/autoware_pointcloud_preprocessor/CMakeLists.txt Show resolved Hide resolved

knzo25 added 5 commits March 27, 2025 17:28

fix: fixed missing dep

7332652

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

chore: removed unused var

3fbb0a0

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

chore: refactored the cloud handler

8b44c16

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

chore: updated documentation

f3bedad

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

fix: fixed rebase error

8fe850c

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

knzo25 added 6 commits March 27, 2025 20:42

chore: removed commented include

2c51d20

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

chore: removed another rebase error induced print

a20fccc

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

fix: and yet another rebase induced error

229782a

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

chore: changed method name

2e49f11

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

chore: removing key from dict for peace of mind

34f8008

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

chore: reimplemented latest changes in the base branch

593ddc6

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

knzo25 requested review from vividf, mojomex and amadeuszsz March 27, 2025 12:29

knzo25 added 5 commits March 28, 2025 08:44

chore: missed dep

e89fe75

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

Merge branch 'main' into feat/concat_pointcloud_templated

50d7c08

chore: spell

7f69bce

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

Merge branch 'feat/concat_pointcloud_templated' of github.com:knzo25/…

d83e50a

…autoware.universe into feat/concat_pointcloud_templated

Merge branch 'main' into feat/concat_pointcloud_templated

8b615c8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(autoware_pointcloud_preprocessor): templated version of the pointcloud concatenation #10298

feat(autoware_pointcloud_preprocessor): templated version of the pointcloud concatenation #10298

knzo25 commented Mar 19, 2025 •

edited

Loading

github-actions bot commented Mar 19, 2025 •

edited

Loading

codecov bot commented Mar 19, 2025 •

edited

Loading

vividf left a comment •

edited

Loading

vividf Mar 21, 2025

knzo25 Mar 27, 2025

mojomex left a comment

mojomex Mar 21, 2025

knzo25 Mar 27, 2025

mojomex Mar 21, 2025

knzo25 Mar 27, 2025

mojomex Mar 21, 2025

knzo25 Mar 27, 2025

mojomex Mar 21, 2025

knzo25 Mar 27, 2025

mojomex Mar 21, 2025

knzo25 Mar 27, 2025

mojomex Mar 21, 2025

knzo25 Mar 27, 2025

mojomex Mar 21, 2025

knzo25 Mar 27, 2025

feat(autoware_pointcloud_preprocessor): templated version of the pointcloud concatenation #10298

Are you sure you want to change the base?

feat(autoware_pointcloud_preprocessor): templated version of the pointcloud concatenation #10298

Conversation

knzo25 commented Mar 19, 2025 • edited Loading

Description

Related links

How was this PR tested?

Notes for reviewers

Interface changes

Effects on system behavior

github-actions bot commented Mar 19, 2025 • edited Loading

codecov bot commented Mar 19, 2025 • edited Loading

Codecov Report

vividf left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mojomex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knzo25 commented Mar 19, 2025 •

edited

Loading

github-actions bot commented Mar 19, 2025 •

edited

Loading

codecov bot commented Mar 19, 2025 •

edited

Loading

vividf left a comment •

edited

Loading