PointCloud in YARP #1597

Nicogene · 2018-03-15T11:14:51Z

This PR introduces the PointCloud type in YARP 🎉

A nice documentation page has been added under "YARP tutorials"

Probably the most important feature is the total compatibility with the PCL PointCloud.
In this sense in the documentation has been added a section where is explained how to convert a yarp cloud in a pcl cloud and viceversa.

Please review code.

traversaro · 2018-03-15T11:25:15Z

src/libYARP_pcl/include/yarp/pcl/Pcl.h

+template< class T1, class T2 >
+inline bool toPCL(yarp::sig::PointCloud< T1 > &yarpCloud, ::pcl::PointCloud< T2 > &pclCloud)
+{
+    yAssert(sizeof(T1) == sizeof(T2));


static_assert ?
As you are directly copyng the raw memory, perhaps it is even safer to just defined the template to operate on one consistent type.

The problem here is that we can't have any control on T2 because it is a pcl's type, the only thing that I can check is that the sizeof of the yarp type and the pcl type are the same.

I used static_assert inside the yarp PointCloud for defining which T1 are allowed and supported for now, the problem is that with the static_assert we can add the corresponding list of pcl supported types but inside these conversion functions (toPcl and fromPcl) we cannot check the correspondence between the yarp type and pcl type (e.g. yarp::sig::XYZ_DATA and pcl::PointXYZ)

Note that it is possible that sizeof(T1)==sizeof(T2) but the two types can be incompatible.
Example:
sizeof(yarp::sig::XYZ_DATA) is equal to sizeof(pcl::Normal) but they have different fields.

This check is not sufficient to be 100% safe but better than nothing 😅
I hope that the user will these functions with the correct types and not try some exotic conversions like yarp XYZ to pcl NxNyNz (Normals):sweat_smile:

I see, T1 is something like struct vec3 { double x; double y; double z; } and in memcpy you are relying of the memory structure of the two types to be the same, I get it.
However, I think it is is desirable to transform this in static_assert(sizeof(T1) == sizeof(T2)).

Ah ok now I get your point, yes probably is better static_assert in this case 👍

claudiofantacci

LGTM!
Just noticed some minor (picky) typos.

claudiofantacci · 2018-03-15T13:37:47Z

src/libYARP_pcl/include/yarp/pcl/Pcl.h

+}
+
+/**
+ * Convert a  pcl::PointCloud  to a yarp::sig::PointCloud  object


There is an extra space before and after pcl::PointCloud and after yarp::sig::PointCloud.

claudiofantacci · 2018-03-15T13:39:29Z

src/libYARP_sig/include/yarp/sig/PointCloud.h

+        }
+
+
+        /** \brief Concatenate a point cloud to the current cloud.


Probably missing a newline after /**?

claudiofantacci · 2018-03-15T13:39:36Z

src/libYARP_sig/include/yarp/sig/PointCloud.h

+            return (*this);
+        }
+
+        /** \brief Concatenate a point cloud to another cloud.


Probably missing a newline after /**?

claudiofantacci · 2018-03-15T13:40:51Z

src/libYARP_sig/include/yarp/sig/PointCloudNetworkHeader.h

+class yarp::sig::PointCloudNetworkHeader
+{
+public:
+    PointCloudNetworkHeader() :    width(10),


Here and the following lines the indentation seems somwhat wrong.

It seems ok 🤔

This is how I see it.

There are extra spaces here and there after the default constructor semicolon 😄

Ah ok I see, I thought was referred to the indentation of public 😅

traversaro · 2018-03-15T13:43:48Z

Why XY_DATA and all the other data structures are named using ALL_CAPITAL_LETTERS_WITH_UNDERSCORE style (typically used for macros in C/C++) while the rest of classes in YARP are named using the CamelCaseClassName style?

Nicogene · 2018-03-15T13:47:34Z

@traversaro because they are struct and not class (?).
The names have been chosen by @barbalberto 😅

pattacini · 2018-03-19T10:08:53Z

Thanks @Nicogene and @barbalberto for this awesome integration 💖

We have the urge to merge this PR in order to start testing an online viewer of point clouds.
Any outstanding point still to discuss?

Nicogene · 2018-03-19T14:19:09Z

All tests passed, for me the PR is ready to be merged.

traversaro · 2018-03-19T14:21:04Z

doc/tutorials.dox

@@ -49,6 +49,7 @@ Using YARP devices:
 \li \subpage yarp_config_files

 More software tutorials:
+\li \subpage yarp_pointcloud


I would add this page also on the Interoperability and advanced use section in the main page of YARP.

Is it possible to do it without restarting all the tests ? 😬

@Nicogene let's be flexible and add this change after the merge 😉 (if you like to have the green lights within the PR)

Please fix this as suggested by @traversaro in next iteration

https://github.com/robotology/yarp/pull/1597/files#diff-64c78214510ead1279a75f3949512cc3
done 16 days ago 😅

traversaro · 2018-03-19T14:22:11Z

src/libYARP_pcl/CMakeLists.txt

+        DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/yarp)
+
+set_property(GLOBAL APPEND PROPERTY YARP_LIBS YARP_pcl)
+#set_property(TARGET YARP_pcl PROPERTY FOLDER "Libraries")


This line can be removed?

Yes it is a copy-paste from the CMakeLists.txt of libYARP_eigen.
(If you uncomment this line the CMake complaints, so I think it is better to remove it 🤔)

traversaro · 2018-03-19T14:22:37Z

src/libYARP_pcl/include/yarp/pcl/Pcl.h

+
+/**
+ * Convert a yarp::sig::PointCloud to a pcl::PointCloud object
+ * @param yarpCloud yarp::sig::PointCloud input


@param[in] ?

traversaro · 2018-03-19T14:23:10Z

src/libYARP_pcl/include/yarp/pcl/Pcl.h

+ * @return a pcl PointCloud filled with data contained in the yarp cloud.
+ */
+template< class T1, class T2 >
+inline bool toPCL(yarp::sig::PointCloud< T1 > &yarpCloud, ::pcl::PointCloud< T2 > &pclCloud)


If yarpCloud is just an input argument, perhaps we can have const yarp::sig::PointCloud< T1 > &yarpCloud?

traversaro · 2018-03-19T14:23:26Z

src/libYARP_pcl/include/yarp/pcl/Pcl.h

+
+/**
+ * Convert a pcl::PointCloud to a yarp::sig::PointCloud object
+ * @param pclCloud pcl::PointCloud input


@param[in]

traversaro · 2018-03-19T14:23:39Z

src/libYARP_pcl/include/yarp/pcl/Pcl.h

+/**
+ * Convert a pcl::PointCloud to a yarp::sig::PointCloud object
+ * @param pclCloud pcl::PointCloud input
+ * @return a yarp cloud filled with data contained in the pcl cloud.


Missing yarpCloud docs.

traversaro · 2018-03-19T14:32:59Z

src/libYARP_sig/include/yarp/sig/PointCloud.h

+        inline PointCloud<T>&
+        operator+=(const PointCloud<T>& rhs)
+        {
+            //TODO: take the newest timestamp


If this is an actual TODO, can you open a issue in the YARP issue tracker?

I think that I can remove it, in PCL the operator+= takes the newest timestamp because the timestamp is in the pointcloud header, but in YARP is handled on another level.

traversaro · 2018-03-19T14:34:01Z

src/libYARP_sig/include/yarp/sig/PointCloud.h

+        {
+            //TODO: take the newest timestamp
+
+            yAssert(getPointType() == rhs.getPointType());


Some gentler error, or perhaps document this behavior in the docs?

traversaro · 2018-03-19T14:34:22Z

src/libYARP_sig/include/yarp/sig/PointCloud.h

+        }
+
+        inline void push_back (const T& pt)
+           {


The indentation of this method is different.

traversaro · 2018-03-19T14:35:38Z

src/libYARP_sig/include/yarp/sig/PointCloud.h

+         */
+        virtual void fromExternalPC(const char* source, int type, size_t width, size_t height, bool isDense = true)
+        {
+            yAssert(source);


As before, are you sure that crashing is a sensible behavior (especially if not documented) for wrong parameters?

yarp::sig::Image* classes are plenty of these yAssert 😅
I think that the assert is necessary in these cases, but probably it is better to document it

I see, then it is a discussion worth having on another place.

traversaro · 2018-03-19T14:36:40Z

src/libYARP_sig/include/yarp/sig/PointCloudNetworkHeader.h

+//     yarp::sig::Vector   orientation;     // orientation wrt origin  -- could be an Eigen::Quaternion for better PCL compatibility if yarp can afford to depend from it
+
+    // YARPish fileds
+//     char                *data;           // actual pointCloud data.


If all this commented line are not necessary, can we remove them?

I think that it is better to keep it for the future improvements

if you leave them here with no comment at all, just commented out, nobody will never know what are those lines and why they are here. Hence I agree with @traversaro

traversaro · 2018-03-19T14:38:43Z

Added some comments but:

I was not able to review all the PR
Comments are quite picky, can be ignored if the mantainer is ok, or can be addressed after the merge

Nicogene · 2018-03-19T15:11:00Z

I can do the changes requested by @traversaro, and I think that tomorrow should be ready.
I know that @pattacini needs it, I don't know how much it is urgent!
@drdanz any comment?

barbalberto

👍

Nicogene · 2018-03-27T16:57:05Z

Rebased over last devel, waiting that passes all tests...

barbalberto · 2018-03-28T06:41:54Z

Tests are ok, the only one failing is
Test #48: dev::ControlBoardRemapperTest ......***Exception: SegFault 1.34
which is not related. @traversaro can you check why that test keeps failing randomly?

P.R. looks ok, but I see it introduces a lot of warnings
: conversion from 'double' to 'float', possible loss of data src\libyarp_sig\include\yarp\sigpointcloudtypes.h on windows. @Nicogene can you check?

Nicogene · 2018-03-28T07:37:38Z

I have to check on a windows machine, the lines pointed by appveyor seems wrong

The refactoring of `PointCloudBase` has been done for the future `PointCloudMap`.

The more important change is the input cloud passed to `toPcl` and `fromPcl` made const.

- remove useless return - make isOrganized const - in toString function use char[128] instead of char[100] - use snprintf instead of sprintf - remove inline if() return

…P_INT8

drdanz · 2018-04-05T10:20:01Z

Fixed copyright headers, renamed structures to "DataXxx" in order to have some kind of "namespacing", and cleanup... Now LGTM

drdanz · 2018-04-05T10:21:36Z

Merged, thanks.

Nicogene added PR Type: Feat/Enh This PR adds some new feature or enhances some part of YARP Component: Library - YARP_sig PR Status: Changelog - OK This PR has a proper changelog entry Target: YARP v3.0.0 labels Mar 15, 2018

Nicogene requested review from drdanz, barbalberto and claudiofantacci March 15, 2018 11:14

drdanz added the Status: In Progress label Mar 15, 2018

traversaro reviewed Mar 15, 2018

View reviewed changes

claudiofantacci approved these changes Mar 15, 2018

View reviewed changes

Nicogene force-pushed the pointCloud_typeTRUE branch 2 times, most recently from 1b1be65 to be5e191 Compare March 16, 2018 09:54

Nicogene changed the title ~~[WIP] PointCloud in YARP~~ PointCloud in YARP Mar 16, 2018

Nicogene force-pushed the pointCloud_typeTRUE branch from be5e191 to 62db0ce Compare March 19, 2018 09:56

traversaro reviewed Mar 19, 2018

View reviewed changes

claudiofantacci mentioned this pull request Mar 19, 2018

Consider using VTK for display purposes robotology/superquadric-model#6

Closed

Nicogene force-pushed the pointCloud_typeTRUE branch 2 times, most recently from 352bc27 to 8b7fe66 Compare March 19, 2018 16:52

barbalberto approved these changes Mar 26, 2018

View reviewed changes

Nicogene force-pushed the pointCloud_typeTRUE branch from 8b7fe66 to a4a3e97 Compare March 27, 2018 16:55

Nicogene and others added 16 commits April 5, 2018 12:04

PointCloud: add toBottle() and fromBottle() methods.

510e550

PointCloudTest: add tests for to-fromBottle methods

1352ca9

PointCloud: cleanup + refactoring of PointCloudBase.

c3c42ca

The refactoring of `PointCloudBase` has been done for the future `PointCloudMap`.

Update release notes.

1ae252f

libYARP_pcl: add static_assert.

9930fab

Documentation update and various cleanup pre-merge.

13db26c

The more important change is the input cloud passed to `toPcl` and `fromPcl` made const.

PointCloudTypes: cleanup and rename of type following YARP guidelines

8883642

PointCloudNetworkHeader: cleanup

3540f84

PointCloud: PointCloudBase moved to its own file, namespace cleanup

9eae369

PointCloud: various cleanup

3782532

- remove useless return - make isOrganized const - in toString function use char[128] instead of char[100] - use snprintf instead of sprintf - remove inline if() return

Update documentation

9cba474

PointCloud: namespace and enum cleanup

9f84a44

PointCloud: remove define TagMap

0a64e41

PointCloudNetworkHeader: removes useless fields and makes isDense YAR…

ba077cf

…P_INT8

Rename structs from XxxData to DataXxx

2b33b5b

Cleanup

6673c6f

drdanz force-pushed the pointCloud_typeTRUE branch from ee94aa7 to 6673c6f Compare April 5, 2018 10:13

drdanz assigned drdanz, traversaro, barbalberto, claudiofantacci and Nicogene Apr 5, 2018

drdanz approved these changes Apr 5, 2018

View reviewed changes

drdanz merged commit 808e674 into robotology:devel Apr 5, 2018

drdanz removed the Status: In Progress label Apr 5, 2018

drdanz added Resolution: Merged and removed PR Status: Commits - Not OK Commits in this PR needs to be squashed or re-organized labels Apr 5, 2018

Nicogene deleted the pointCloud_typeTRUE branch April 5, 2018 10:22

PointCloud in YARP #1597

PointCloud in YARP #1597

Conversation

Nicogene commented Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Nicogene Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claudiofantacci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nicogene Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

traversaro commented Mar 15, 2018

Nicogene commented Mar 15, 2018 • edited Loading

pattacini commented Mar 19, 2018

Nicogene commented Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nicogene Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

traversaro commented Mar 19, 2018

Nicogene commented Mar 19, 2018

barbalberto left a comment

Choose a reason for hiding this comment

Nicogene commented Mar 27, 2018

barbalberto commented Mar 28, 2018

Nicogene commented Mar 28, 2018

drdanz commented Apr 5, 2018

drdanz commented Apr 5, 2018

Nicogene commented Mar 15, 2018 •

edited

Loading

Nicogene Mar 15, 2018 •

edited

Loading

Nicogene Mar 15, 2018 •

edited

Loading

Nicogene commented Mar 15, 2018 •

edited

Loading

Nicogene commented Mar 19, 2018 •

edited

Loading

Nicogene Apr 4, 2018 •

edited

Loading