Create ordered point cloud (issue #126) #157
Conversation
|
Thanks for the patch, it looks straightforward and a good addition. I hope this can get merged! |
| @return: The point cloud. | ||
| @rtype: L{sensor_msgs.msg.PointCloud2} | ||
| """ | ||
| if height == 1: |
There was a problem hiding this comment.
This looks like it will have problems if height is non unary but width is defaulted to None. Some other checks are going to be necessary.
There was a problem hiding this comment.
I'll add a check on both height and width.
The original implementation didn't perform any check on header, fields and points, for example, if they are empty. Do you want me to add something like that?
There was a problem hiding this comment.
I was mostly worried about the default values messing things up. Though a little bit of sanitizing would be valuable. But we need to be careful about what error response behavior is caused by invalid data.
|
|
||
| cloud_struct = struct.Struct(_get_struct_fmt(False, fields)) | ||
|
|
||
| buff = ctypes.create_string_buffer(cloud_struct.size * len(points)) | ||
|
|
||
| point_step, pack_into = cloud_struct.size, cloud_struct.pack_into | ||
| point_step = cloud_struct.size |
There was a problem hiding this comment.
These intermediate values appear to only be used once so are probably not worth creating at all.
There was a problem hiding this comment.
Thanks for the update that looks better. Could you also write some basic unit tests. It would be good to add some that assert the ordering that this implements stays valid. As well as can validate the error handling. And it appears that we don't have any python unit tests for this at all.
|
Thank you for the PR! Unfortunately I don't think we should merge this one. ROS Noetic will reach end-of-life on May 31st, 2025. Every change comes with a risk of introducing regressions, and there isn't much time left to fix them. I'm closing pull requests that add features so that the remaining time is allocated towards bug fixes and compatibility with newer Ubuntu distros. |
Small changes to create_cloud in point_cloud2.py to support ordered point clouds with specific height and width.
Mentioned in issue #126