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.
Small changes to create_cloud in point_cloud2.py to support ordered point clouds with specific height and width.
Mentioned in issue #126