feat(lidar_transfusion): intensity as uint8 and tests

[amadeuszsz]

Description

This PR updates intensity field as it's considered in new Autoware point clouds format. New unit tests cover this change.

Related links

Parent Issue:

Parent PR:

• feat: migrating pointcloud types #6996 - PR dependency

How was this PR tested?

• Rosbag input --> no behavior changes observed
• Unit tests

Notes for reviewers

Changes from parent PR are required to build & test this PR.

Interface changes

Now the input point cloud has to contain uint8 intensity field.

Effects on system behavior

None.

[github-actions]

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.

[knzo25]

@amadeuszsz
Now that we use more pointer algebra for the preprocessing, we are very prone to make mistakes with the data layout.
I am going to do something similar in centerpoint, but can you add some static_assert to make sure that the point steps and offsets match the autoware defined ones?

I did something similar here:
https://github.com/knzo25/cuda_pointcloud_preprocessor/blob/23642f53e8136f45d0ee17ac25dbae05921f79da/include/cuda_pointcloud_preprocessor/cuda_pointcloud_preprocessor_node.hpp#L28-L55
(no need to so exactly the same)

[knzo25]

Note:
This PR should only be merged after (or together) with
#6996

[amadeuszsz]

@amadeuszsz Now that we use more pointer algebra for the preprocessing, we are very prone to make mistakes with the data layout. I am going to do something similar in centerpoint, but can you add some static_assert to make sure that the point steps and offsets match the autoware defined ones?

I did something similar here: https://github.com/knzo25/cuda_pointcloud_preprocessor/blob/23642f53e8136f45d0ee17ac25dbae05921f79da/include/cuda_pointcloud_preprocessor/cuda_pointcloud_preprocessor_node.hpp#L28-L55 (no need to so exactly the same)

FYI we already checking first input cloud and comparing with the reference.
The difference is in Transfusion we compare input cloud with structure which is assumed further by kernels. Centerpoint, as I noticed in your provided code, compares autoware point types with structure. I think we should do first to inform unaware user about wrong input and second option as well to track API changes. I will add proposed change for sure, thanks!
We are slowly going towards necessity of perception_common package 😄

[knzo25]

🙈
I understand that you are comparing it with the reference, which looks perfect. However, the reference itself has magic numbers and would be prone to errors.

For example,
https://github.com/autowarefoundation/autoware.universe/blob/36e54e0b5210aed913ae1a7dd463128c700bd1ba/perception/lidar_transfusion/include/lidar_transfusion/utils.hpp#L56C1-L72C4
Uses datatype=7. In this case sensor_msgs::msg::PointField::FLOAT32 would be better IMO

[knzo25]

@amadeuszsz
The branch is not compiling due to the use of PointXYZIRCAEDT that will be introduced in the before-mentioned PR.
Should we leave this as a draft for now? Or do you have other way you would like to proceed with?

[amadeuszsz]

@amadeuszsz The branch is not compiling due to the use of PointXYZIRCAEDT that will be introduced in the before-mentioned PR. Should we leave this as a draft for now? Or do you have other way you would like to proceed with?

Removing tag:run-build-and-test-differential should be enough for now. I suggest to continue review this PR (my near fixes) before merging parent PR. Then this PR will be already reviewed and ready to merge alongside with parent PR.

[knzo25]

@amadeuszsz The branch is not compiling due to the use of PointXYZIRCAEDT that will be introduced in the before-mentioned PR. Should we leave this as a draft for now? Or do you have other way you would like to proceed with?

Removing tag:run-build-and-test-differential should be enough for now. I suggest to continue review this PR (my near fixes) before merging parent PR. Then this PR will be already reviewed and ready to merge alongside with parent PR.

Sorry, I did not understand what you meant. Btw, what failed to compile was a local build in my PC

‘PointXYZIRADRT’?
   53 |     autoware_point_types::PointXYZIRCAEDT, autoware_point_types::PointXYZIRCAEDTGenerator>
      |                           ^~~~~~~~~~~~~~~
      |                           PointXYZIRADRT
autoware/universe/perception/lidar_transfusion/test/test_voxel_generator.cpp:53:66: error: ‘PointXYZIRCAEDTGenerator’ is not a member of ‘autoware_point_types’; did you mean ‘PointXYZIRADRTGenerator’?
   53 |     autoware_point_types::PointXYZIRCAEDT, autoware_point_types::PointXYZIRCAEDTGenerator>
      |                                                                  ^~~~~~~~~~~~~~~~~~~~~~~~
      |                                                                  PointXYZIRADRTGenerator

[amadeuszsz]

@amadeuszsz The branch is not compiling due to the use of PointXYZIRCAEDT that will be introduced in the before-mentioned PR. Should we leave this as a draft for now? Or do you have other way you would like to proceed with?

Removing tag:run-build-and-test-differential should be enough for now. I suggest to continue review this PR (my near fixes) before merging parent PR. Then this PR will be already reviewed and ready to merge alongside with parent PR.

Sorry, I did not understand what you meant.

If we switch this PR to draft, the review will take place after merge of parent PR.
If we keep this PR open and review it before merge parent PR, we will merge both PR at the same time.

Btw, what failed to compile was a local build in my PC

‘PointXYZIRADRT’?
   53 |     autoware_point_types::PointXYZIRCAEDT, autoware_point_types::PointXYZIRCAEDTGenerator>
      |                           ^~~~~~~~~~~~~~~
      |                           PointXYZIRADRT
autoware/universe/perception/lidar_transfusion/test/test_voxel_generator.cpp:53:66: error: ‘PointXYZIRCAEDTGenerator’ is not a member of ‘autoware_point_types’; did you mean ‘PointXYZIRADRTGenerator’?
   53 |     autoware_point_types::PointXYZIRCAEDT, autoware_point_types::PointXYZIRCAEDTGenerator>
      |                                                                  ^~~~~~~~~~~~~~~~~~~~~~~~
      |                                                                  PointXYZIRADRTGenerator

My fault, I should leave a note that this PR required changes from parent PR. Required changes are not included since parent PR is still under development. For time being I'm using autoware_point_types and pointcloud_preprocessor from parent PR.

[knzo25]

Everything looks awesome (bar the evil way to decode the bytes)

However, these tests would need to be disabled by default in the meantime
https://github.com/autowarefoundation/autoware.universe/blob/b43cffb3be7059640ed30b51e17e834a1ad3227d/perception/lidar_centerpoint/CMakeLists.txt#L163
#7724

🫠

[knzo25]

LGTM