Migrate to point_cloud_msg_wrapper

[esteve]

Checklist

• I've read the contribution guidelines.
• I've searched other issues and no duplicate issues were found.
• I've agreed with the maintainers that I can plan this task.

Description

The current codebase uses the PointCloud2Iterator class for handling point clouds. However, this translates into code that's brittle and unreadable. Point Cloud Message Wrapper provides a more robust and natural API for handling point clouds.

Purpose

Make the point cloud handling code more robust and readable

Possible approaches

Use point_coud_msg_wrapper to handle point clouds

Definition of done

The point cloud iterator classes from sensor_msgs are no longer used, in favor of point_cloud_msg_wrapper

[stale]

This pull request has been automatically marked as stale because it has not had recent activity.

[miursh]

Just for your information here.
Because #1276 is merged, some issues are happening.
#2306

[stale]

This pull request has been automatically marked as stale because it has not had recent activity.

[vividf]

The error message referenced in this GitHub issue indicates that the problem is caused by this line of code. The Pointcloud message wrapper claims to offer safer handling for point cloud types and fields. Recently, @knzo25 has been working on migrating the point cloud type and providing checks for both the point cloud field and type. Therefore, from my perspective, investing additional effort in this area might not be necessary.

@knzo25 How do you think about this? Thanks

[knzo25]

Since we just did some cleaning regarding point types, it would certainly be a good time to try this.
My concerns are:

• Time to implement this (not that high)
• Time to test this (does not seem to high, but could turn out to take way more that initially estimated)
• Execution time (I have not read the implementation, but in addition to do so we must check that this does not add overhead to the current implementation of universe. several parts in universe access directly the data and use reinterpret...)
• Pointpainting uses "wrongly" the standard way. Could break things by using the proposed method

As this would develop into a bigger task, is there any opinion from an architect point of view (the dependency is in rosdep so there would not be issues https://index.ros.org/p/point_cloud_msg_wrapper/gitlab-ApexAI-point_cloud_msg_wrapper/) @xmfcx @mitsudome-r and if and how much priority would this be assigned @drwnz

[drwnz]

I've had a quick look at this, and while @knzo25 has made some big improvements to checking the field and type, this does look like an additional step to provide a better API for handling pointclouds. I would say that it is worth investing some time into this now that we have clarified point types. @xmfcx would be great to get your input on this.

[esteve]

Unassigning myself since I haven't worked on this for a long time.

[xmfcx]

• Execution time (I have not read the implementation, but in addition to do so we must check that this does not add overhead to the current implementation of universe. several parts in universe access directly the data and use reinterpret...)

@knzo25 This shouldn't add an overhead for runtime.

It's a more methodical/safe way of doing a reinterpret_cast:

• https://gitlab.com/ApexAI/point_cloud_msg_wrapper/-/blob/rolling/point_cloud_msg_wrapper/include/point_cloud_msg_wrapper/point_cloud_msg_wrapper.hpp#L326
• https://gitlab.com/ApexAI/point_cloud_msg_wrapper/-/blob/rolling/point_cloud_msg_wrapper/include/point_cloud_msg_wrapper/point_cloud_msg_wrapper.hpp#L342

I will add comparison of point_cloud_msg_wrapper, point_cloud2_iterator and raw reinterpret_cast later here.

But we can go on with implementing this.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity.