feat(lidar_transfusion): intensity as uint8 and tests - Githubissues

autowarefoundation / autoware.universe

https://autowarefoundation.github.io/autoware.universe/

Apache License 2.0

964 stars 630 forks source link

feat(lidar_transfusion): intensity as uint8 and tests #7745

Closed amadeuszsz closed 2 months ago

amadeuszsz commented 3 months ago

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:

https://github.com/autowarefoundation/autoware.universe/pull/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[bot] commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

Note: This PR should only be merged after (or together) with https://github.com/autowarefoundation/autoware.universe/pull/6996

amadeuszsz commented 3 months ago

@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 commented 3 months ago

:see_no_evil: 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 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

@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 commented 2 months ago

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