Support for dynamic PointCloud2 creation #108
Conversation
spuetz
force-pushed
the
jade-devel
branch
2 times, most recently
from
ee8400f
to
db28d4b
Compare
There was a problem hiding this comment.
Overall adding this sort of functionality makes sense.
I had some specific comments inline. Overall all of these new functions need documentation at the level of doc blocks.
I'd also like to see the matched remove field methods. And looking at the methods it looks like the add methods will corrupt existing data in the point cloud. Some protections/warnings or non-destructive transformations would be best.
And to be confident this will work. Please add some unit tests that show it working.
| @@ -80,6 +82,51 @@ namespace sensor_msgs{ | |||
| template<> struct typeAsPointFieldType<double> { static const uint8_t value = sensor_msgs::PointField::FLOAT64; }; | |||
|
|
|||
| /*! | |||
| * \Type names of the PointField data type. | |||
| */ | |||
| int getPointFieldTypeFromString(const std::string field_name){ | |||
There was a problem hiding this comment.
Since this is not templated it needs to be inline with the implementation in the header. Also please switch to a const reference instead of pass by value of the string.
| @@ -80,6 +82,51 @@ namespace sensor_msgs{ | |||
| template<> struct typeAsPointFieldType<double> { static const uint8_t value = sensor_msgs::PointField::FLOAT64; }; | |||
|
|
|||
| /*! | |||
| * \Type names of the PointField data type. | |||
There was a problem hiding this comment.
Please add docblock info for the parameter like the other functions.
| cloud_msg_.row_step = cloud_msg_.width * cloud_msg_.point_step; | ||
| cloud_msg_.data.resize(cloud_msg_.height * cloud_msg_.row_step); | ||
| } | ||
|
|
||
|
|
||
| inline void PointCloud2Modifier::addPointCloud2Fields(std::vector<PointFieldInfo> fields){ |
There was a problem hiding this comment.
Is there a reason this is void not at least bool?
| protected: | ||
| /** A reference to the original sensor_msgs::PointCloud2 that we read */ | ||
| PointCloud2& cloud_msg_; | ||
| /** The current offset for all point fields */ | ||
| int current_offset_; |
There was a problem hiding this comment.
This seems to mirror the local storage of cloud_msg_.point_step ? Can you not query, mutate and reset without adding another data field?
| std::string datatype; | ||
| }; | ||
|
|
||
| void addPointCloud2Fields(std::vector<PointFieldInfo> fields); |
There was a problem hiding this comment.
Please mirror the setPointCloud2Fields API here instead of defining a new datatype.
|
@tfoote I changed the things you commented. The Having the The next thing I want to do here is working on the |
|
Hi @tfoote we would need the pull request now for the Sick MRS1000 Driver to dynamically enable a intensity channel in the point cloud. |
|
Thanks for resolving the direct comments. However it still does not have any test coverage of the new API, they would also provide examples of how it's designed to be used which would be helpful in checking the design. We're approaching Melodic release and getting this in before then would be great. |
|
Okay I'll add one or more tests in the next days. Thank you. Would be could to have it in Melodic. |
|
Are there any news about this PR? |
|
We are planning on writing some tests soonish. @ctieben Do you need the feature? |
|
I’m interested in this PR and looking forward to using these changes in some future experiments. It would be great to have this in the default branch / repos. |
This enhancement enables a dynamic creation of sensor_msgs::PointCloud2 messages with dynamic point fields at creation time of the cloud. This means a node or driver providing sensor_msgs::PointCloud2 clouds can now publish configurable clouds. That is useful for devices which provide much extra information to each point in the cloud. With this enhancement, e.g. the user of a high resolution laser scanner, could configure which extra information should be published within the cloud. This is useful to only publish necessary information and to keep the network communication load smaller.
With the method
bool PointCloud2Modifier::addPointCloud2Field(std::string name, std::string datatype)it is now possible to dynamically add fields if a field was enabled in the config.