Wip: refactor PointcloudColorHandlerGenericField to deal with any field type #6309
Conversation
|
I ran the formatter to put my changes in these files, but unfortunately this has resulted in a large diff, as they seemed to not have followed the clang-format style file before. |
The style file has only been added a few years ago, and most of the code is not yet formatted according to it. We are slowly formatting more and more files, but we cannot format any files that are changed in any open pull request (otherwise the pull request would have merge conflicts and basically be lost). Unfortunately, with the formatting changes mixed in, this pull request is very difficult to review, as it is very time consuming to look for the changes you wanted to make originally. Is there any way you can revert the formatting changes in this pull request?
To be honest, I am not sure, but it is probably there for a reason so I would like to keep it. However, instead of the current structure (if-condition, then two for-loops, one for each case), you could probably merge the for-loops into one, then check something like |
themightyoarfish
force-pushed
the
colorhandler-field-types
branch
from
88d0563 to
2556e89
Compare
|
I have recommitted without formatting changes outside of the affected class. also added the handling of non-xyz data as suggested, but have not tested it yet. |
…lerGenericField class
themightyoarfish
force-pushed
the
colorhandler-field-types
branch
from
b5bfae2 to
7e7f343
Compare
|
CI seems to pass, i guess i should add some kind of test? |
That would be great. My suggestion: create a small cloud with maybe 4-5 points (manually), pass to color handler, then check if the array returned by |
|
Added a test which passes for me locally, but I don't think CI builds the visualization tests. (they are also disabled on macos, because the code only checks for UNIX + DISPLAY set, which doesn't exist on macos) |
Its run on the ubuntu CI's using xvfb - so should be good 👍 |
|
Can I get a review? |
| // Get cloud data blob | ||
| template <typename CloudT> inline const std::uint8_t* getCloudData(const CloudT& cloud) | ||
| { | ||
| return reinterpret_cast<const std::uint8_t*>(cloud.points.data()); |
There was a problem hiding this comment.
| return reinterpret_cast<const std::uint8_t*>(cloud.points.data()); | |
| return reinterpret_cast<const std::uint8_t*>(cloud.data()); |
| // Get point fields from cloud. Could not get it to work with existing | ||
| // pcl::getFields | ||
| template <typename CloudT> inline std::vector<pcl::PCLPointField> | ||
| getFields(const CloudT& cloud) |
There was a problem hiding this comment.
I would move these to the other getFields functions in io.h - so they are grouped there and not scattered around in various modules/files.
There was a problem hiding this comment.
But lets wait for @mvieth to answer as well, before moving it around.
There was a problem hiding this comment.
Yes, I would also move these to io.h: https://github.com/PointCloudLibrary/pcl/blob/master/common/include/pcl/common/io.h#L96
| } | ||
|
|
||
| template <> inline const std::uint8_t* getCloudData<pcl::PCLPointCloud2>(const typename pcl::PCLPointCloud2& cloud) { | ||
| return reinterpret_cast<const std::uint8_t*>(cloud.data.data()); |
There was a problem hiding this comment.
| return reinterpret_cast<const std::uint8_t*>(cloud.data.data()); | |
| return cloud.data.data(); |
Its already of type uint8_t* ? Not that it matter much I guess.
|
|
||
|
|
||
| // Get point step. Does not directly exist in pcl::PointCloud | ||
| template <typename CloudT> inline int getPointStep(const CloudT&) |
There was a problem hiding this comment.
I would add these getters to their respective classes - even though they currently only a used here, I don't see why it shouldn't belong to the point_cloud/PCLPointCloud2 classes.
There was a problem hiding this comment.
I would rather not add these as getters, I don't think they would be used that often
|
|
||
| // copy of pcl::getFieldIndex() from impl/io.hpp, without the unused template | ||
| // parameter | ||
| static int getFieldIndex(const std::string& field_name, |
There was a problem hiding this comment.
Also move this to the io next to the others?
| } | ||
| } | ||
|
|
||
| inline const std::uint8_t* getCloudData(const typename pcl::PCLPointCloud2& cloud) |
There was a problem hiding this comment.
Dublicate of the specialized templated version at line 524?
| // Get point fields from cloud. Could not get it to work with existing | ||
| // pcl::getFields | ||
| template <typename CloudT> inline std::vector<pcl::PCLPointField> | ||
| getFields(const CloudT& cloud) |
There was a problem hiding this comment.
Yes, I would also move these to io.h: https://github.com/PointCloudLibrary/pcl/blob/master/common/include/pcl/common/io.h#L96
|
|
||
|
|
||
| // Get point step. Does not directly exist in pcl::PointCloud | ||
| template <typename CloudT> inline int getPointStep(const CloudT&) |
There was a problem hiding this comment.
I would rather not add these as getters, I don't think they would be used that often
| * | ||
| * @return field value | ||
| */ | ||
| template <typename T> T point_field_as(const std::uint8_t* data, const std::uint8_t type) |
There was a problem hiding this comment.
I think this would also be a good candidate to move to a common header, maybe common/io.h?
| /** \brief Name of the field used to create the color handler. */ | ||
| std::string field_name_; | ||
| /** \brief Get the name of the field used. */ | ||
| std::string getFieldName() const override; |
There was a problem hiding this comment.
Changed from public to protected - intentional?
Resolves #6306
Partially resolves #6186