Add GASD global point cloud descriptor #1652

jpsml · 2016-07-16T00:50:35Z

The Globally Aligned Spatial Distribution (GASD) descriptor for point clouds with XYZ data (and optionally RGB data) is presented in the paper:
"An Efficient Global Point Cloud Descriptor for Object Recognition and Pose Estimation", J. Lima and V. Teichrieb, SIBGRAPI 2016 - Conference on Graphics, Patterns and Images, São José dos Campos, Brazil.

The paper is available at: http://www.cin.ufpe.br/~jpsml/uploads/8/2/6/7/82675770/pid4349755.pdf

SergioRAgostinho · 2016-07-18T14:40:04Z

Thanks for you contribution. As this is new feature it might takes us a while to start reviewing it, especially considering the goals for our next release. In the meantime, there are importing things we'll need from your side.

Unit tests (mandatory)

I've noticed you haven't submitted any unit tests with your code, although I'm sure you must have used something to validade its output. Could you please submit that as well.

Tutorial (optional)

Although not required, it would be great if you could submit also a tutorial similar to this, briefly explaining the theoretical primer and showing a simple use case. It really helps to promote your contribution as users tend to explore the tutorials, not really the repository.

Once more, thanks for this 👍

jpsml · 2016-07-18T16:00:02Z

Ok, I'll work on this material and I'll let you know once I've finished. Thanks.

jpsml · 2016-08-01T16:05:00Z

I've just added some unit tests.

SergioRAgostinho · 2016-08-01T18:41:48Z

Thanks. The test is segfaulting. I'm assuming it was not happening on your environment. Which OS, compiler are you using?

jpsml · 2016-08-01T18:45:13Z

I'm using Windows and Visual C++. I will try to reproduce this error using gcc on Ubuntu in order to fix it.

jpsml · 2016-08-03T17:52:49Z

I've fixed the issue, now the test is passing.

jpsml · 2016-08-08T00:13:09Z

I've also added a tutorial about GASD.

SergioRAgostinho · 2016-08-09T23:27:30Z

Great work @jpsml ! :) Thanks for all the effort.

SergioRAgostinho · 2017-07-05T12:51:33Z

Hey @jpsml. Sorry for the super long delay to start reviewing this. Could you rebase the PR to the current master? Otherwise we won't be able to merge this.

SergioRAgostinho

Even though I have a number of comments I really appreciate your coding style @jpsml.

Thanks for contributing this.

SergioRAgostinho · 2017-07-05T12:57:48Z

doc/tutorials/content/gasd_estimation.rst

+     gasd.setColorHistsInterpMethod (gasd.INTERP_NONE);
+
+     // Output datasets
+     pcl::PointCloud<pcl::GASDSignature984>::Ptr descriptor (new pcl::PointCloud<pcl::GASDSignature984>);


Why are you creating an shared_ptr here if then you only use the dereferenced version in gasd.compute()?

I've tried to follow the same standard of the VFH, PFH, FPFH and Normal estimation tutorials. Do you think I should change this?

SergioRAgostinho · 2017-07-05T12:59:02Z

doc/tutorials/content/gasd_estimation.rst

+
+     // Get the alignment transform
+     Eigen::Matrix4f trans;
+     gasd.getTransform (trans);


(Comment to) Self: I'll probably request to revise upon definition of the actual method.

SergioRAgostinho · 2017-07-05T13:01:25Z

doc/tutorials/content/gasd_estimation.rst

+     gasd.setShapeHistsInterpMethod (gasd.INTERP_TRILINEAR);
+
+     // Output datasets
+     pcl::PointCloud<pcl::GASDSignature512>::Ptr descriptor (new pcl::PointCloud<pcl::GASDSignature512>);


Same comment as above.

I've tried to follow the same standard of the VFH, PFH, FPFH and Normal estimation tutorials. Do you think I should change this?

SergioRAgostinho · 2017-07-05T13:01:35Z

doc/tutorials/content/gasd_estimation.rst

+
+     // Get the alignment transform
+     Eigen::Matrix4f trans;
+     gasd.getTransform (trans);


Self: Same comment as before.

SergioRAgostinho · 2017-07-05T13:02:56Z

features/include/pcl/features/gasd.h

+ * Software License Agreement (BSD License)
+ *
+ *  Point Cloud Library (PCL) - www.pointclouds.org
+ *  Copyright (c) 2014-, Open Perception, Inc.


This should only be
Copyright (c) 2016-, Open Perception, Inc.

SergioRAgostinho · 2017-07-05T16:47:50Z

test/features/test_gasd_estimation.cpp

+    PointXYZRGBA p;
+    p.x = cloud.points[i].x;
+    p.y = cloud.points[i].y;
+    p.z = cloud.points[i].z;


p.getVectorMap3f () = cloud[i].getVectorMap3f()

SergioRAgostinho · 2017-07-05T16:48:54Z

test/features/test_gasd_estimation.cpp

+TEST (PCL, GASDShapeAndColorEstimationNoInterp)
+{
+  // Create fake point cloud with colors
+  PointCloud<PointXYZRGBA> cloudWithColors;


define it as shared_ptr

SergioRAgostinho · 2017-07-05T16:50:16Z

test/features/test_gasd_estimation.cpp

+
+    p.rgba = ((i % 255) << 16) + (((255 - i) % 255) << 8) + ((i * 37) % 255);
+    cloudWithColors.push_back (p);
+  }


This is the same code as before, why not define a function to do it on top of the file.

SergioRAgostinho · 2017-07-05T16:51:32Z

test/features/test_gasd_estimation.cpp

+
+using namespace pcl;
+using namespace pcl::io;
+using namespace std;


Please remove using namespace and use only the symbols you need.

SergioRAgostinho · 2017-07-05T16:51:43Z

test/features/test_gasd_estimation.cpp

+ * Software License Agreement (BSD License)
+ *
+ *  Point Cloud Library (PCL) - www.pointclouds.org
+ *  Copyright (c) 2014-, Open Perception, Inc.


SergioRAgostinho · 2017-07-05T17:01:51Z

features/include/pcl/features/impl/gasd.hpp

+
+  //////////////////////////////////////////////////////////////////////////////////////////////
+  template <typename PointInT, typename PointOutT> void
+  GASDEstimation<PointInT, PointOutT>::addSampleToHistograms (Eigen::Vector4f &p,


Forgot to add that p can be defined const.

taketwo · 2017-07-05T18:28:00Z

Before getting into implementation details, I'd like to understand what is the relation between the size of the descriptor and the principal parameters of the method. Why there are only three possible descriptor sizes? What happens if the user chose descriptor size that does not play well with the parameters? Could it be that a base class + three deriving classes each with fixed output descriptor size will better capture the structure of your algorithm?

jpsml · 2017-07-07T14:11:30Z

Thanks for the feedback, @SergioRAgostinho and @taketwo. I'll work on your comments and get back to you soon.

SergioRAgostinho · 2017-07-11T05:31:05Z

@jpsml don't forget about @taketwo 's question. It's way more important to go through that reasoning now, than fixing my implementation specific comments.

jpsml · 2017-07-12T16:45:35Z

Replying to @taketwo: if the user chooses a descriptor size that does not play well with the parameters, it will crash. Let me check if I understood your idea: the user would be able to instantiate the derived classes with suggested configurations or to instantiate the base class if he/she wants to use a different configuration. Is this what you meant?

taketwo · 2017-07-17T11:21:47Z

Sorry for the delay, I've been out of town last week.

I meant having different classes implementing the Feature interface. I have not carefully studied the code and/or the paper yet, but from what I see the use of color seems to be an optional extension. So for example there may be GASDEstimation and another class ColorGASDEstimation or something like this. But this is just a thought. In general would be nice to have a simple interface so that the user does not need to be an expert and make too many decisions with a risk that everything will explode with a segfault unless he got all the parameters fitting together.

jpsml · 2017-08-02T18:48:23Z

Replying to @taketwo: now I'm sorry for the delay, the last weeks were very busy. I think it will be better to follow your suggestion of having two different classes. I'm thinking about naming them ShapeGASDEstimation and ShapeColorGASDEstimation. Thanks.

taketwo · 2017-08-07T15:43:37Z

@jpsml Please STOP writing "Solved" for every comment! This floods our mailboxes!
As soon as you push a commit that solves the problem, GitHub will hide the corresponding comment.

…ndentation and change hue calculation to avoid division by zero

SergioRAgostinho · 2017-11-24T14:15:19Z

Thank you for all your hard work. I think this it it :)

jpsml · 2017-11-25T21:30:42Z

Should I squash the commits?

SergioRAgostinho · 2017-11-25T22:05:49Z

No need. We can do it ourselves.

taketwo

Hi, here is the last (promise) batch of comments, all are minor. Once addressed, let's merge and celebrate!

taketwo · 2017-11-26T11:29:33Z

common/include/pcl/impl/point_types.hpp

+  struct GASDSignature512
+  {
+	  float histogram[512];
+	  static int descriptorSize() { return 512; }


Missing space here and other signature structs.

taketwo · 2017-11-26T11:31:44Z

features/include/pcl/features/gasd.h

+       * \param[out] trans transformation
+       */
+      const Eigen::Matrix4f&
+      getTransform ()


taketwo · 2017-11-26T11:34:00Z

features/include/pcl/features/gasd.h

+      size_t color_hists_size_;
+
+      /** \brief Interpolation method to be used while computing the color descriptor. */
+      typename HistogramInterpolationMethod color_interp_;


I don't think typename is necessary here.

taketwo · 2017-11-26T11:37:44Z

features/include/pcl/features/impl/gasd.hpp

+
+#include <vector>
+
+namespace pcl


In most (maybe all) of PCL we don't use namespace block in implementation files. Instead, each method name is prefixed with pcl:: and no indentation is needed. I'd suggest we stick with this tradition.

taketwo · 2017-11-26T11:39:13Z

features/include/pcl/features/impl/gasd.hpp

+      return;
+    }
+
+    // Resize the output dataset


Simply output.resize (1), will take care of width and height.

taketwo · 2017-11-26T11:40:14Z

features/include/pcl/features/impl/gasd.hpp

+    const Eigen::Vector3f centroid_xyz = centroid.head<3> ();
+
+    // compute alignment transform from axes and centroid
+    transform_ << x_axis.transpose(), -x_axis.dot (centroid_xyz),


Missing spaces

taketwo · 2017-11-26T11:40:45Z

features/include/pcl/features/impl/gasd.hpp

+    }
+
+    // compute histograms bins indices
+    const Eigen::Vector4f bins (floor (coords[0]), floor (coords[1]), floor (coords[2]), floor (coords[3]));


Please prefix with std, i.e. std::floor.

taketwo · 2017-11-26T11:45:49Z

Offtopic: Sergio, what do you mean when you add label "ready to merge"? How is it different from "approved" status?

SergioRAgostinho · 2017-11-26T11:58:03Z

I think we created it before the review system with status was in place.

Before we used it to signal PRs which had to be held off from merging till the next minor release, but that were already good merge.

Now even though the review status system does the same, it's just visual sugar, making easier to see on the PR list.

taketwo · 2017-11-26T12:05:41Z

Agree to such use and visual sugar as well, it's indeed more noticeable in the PR list.

However, in this particular situation, I'm still not sure what is the message that you send by labeling as "ready to merge". If it is ready to merge, the why don't you merge? We have no release constraints.

SergioRAgostinho · 2017-11-26T12:11:19Z

It is that, as a reviewer I believe this is ready to merge, but I still want a second maintainer to do a final check and merge it. Basically I'm requesting for a second validation.

taketwo · 2017-11-26T12:14:39Z

I see... but then it's not ready to merge yet! I think "status: needs review" is more appropriate. Github also allows you to formally request a review from me.

SergioRAgostinho · 2017-11-26T12:51:50Z

👍 will do

…tput resize

SergioRAgostinho · 2017-12-01T17:27:29Z

Gjob guys :) Thank your for your contributions @jpsml and for enduring with us through the multiple stages of this thing ^^

SergioRAgostinho · 2017-12-12T21:41:22Z

@jpsml It would be good to tweak/extend the tutorial a little bit. The first reactions on the mailing list indicated some confusion about what to do exactly. More info here.

jpsml · 2017-12-21T16:28:26Z

I've just commited an extension to the tutorial.

SergioRAgostinho · 2017-12-21T16:29:57Z

Thanks. You need to open a new PR with the extension you just added. After a PR ~~closes~~ is merged, GitHub no longer tracks the branch you were working on.

jpsml · 2017-12-21T16:49:55Z

Created PR #2167.

jpsml force-pushed the gasd branch from bcc8cfa to 588863a Compare July 17, 2016 14:56

jpsml closed this Jul 17, 2016

jpsml reopened this Jul 17, 2016

jpsml force-pushed the gasd branch from 5756034 to 849a49a Compare August 7, 2016 22:46

jpsml force-pushed the gasd branch 2 times, most recently from 9ce29b8 to 43f71df Compare August 8, 2016 00:29

SergioRAgostinho added this to the pcl-1.9.0 milestone Aug 9, 2016

SergioRAgostinho added the needs: code review Specify why not closed/merged yet label Aug 9, 2016

SergioRAgostinho self-assigned this Jul 5, 2017

SergioRAgostinho requested changes Jul 5, 2017

View reviewed changes

SergioRAgostinho reviewed Jul 5, 2017

View reviewed changes

SergioRAgostinho added needs: author reply Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Jul 5, 2017

jpsml force-pushed the gasd branch from 1a92b28 to 617918b Compare August 23, 2017 21:46

SergioRAgostinho mentioned this pull request Nov 22, 2017

[RFC] Added simple kinectv2 grabber and viewer #1410

Closed

3 tasks

change HistogramInterpolationMethod use in GASDColorEstimation, fix i…

910333a

…ndentation and change hue calculation to avoid division by zero

SergioRAgostinho added status: ready to merge and removed needs: more work Specify why not closed/merged yet labels Nov 24, 2017

taketwo reviewed Nov 26, 2017

View reviewed changes

taketwo self-requested a review November 26, 2017 12:12

SergioRAgostinho added needs: author reply Specify why not closed/merged yet and removed status: ready to merge labels Nov 26, 2017

João Paulo Lima added 3 commits November 27, 2017 10:13

Fix indentation, function/variable declarations, namespace use and ou…

13c18eb

…tput resize

Remove HistogramInterpolationMethod::

a763815

Shift HistogramInterpolationMethod enum out of class GASDEstimation

0c22ec3

taketwo approved these changes Dec 1, 2017

View reviewed changes

SergioRAgostinho merged commit 7c4a475 into PointCloudLibrary:master Dec 1, 2017

taketwo removed the needs: author reply Specify why not closed/merged yet label Dec 2, 2017

SergioRAgostinho added the module: features label Aug 29, 2018

SergioRAgostinho mentioned this pull request Aug 29, 2018

Change Log generation tool. Automates change log generation. #2396

Merged

Add GASD global point cloud descriptor #1652

Add GASD global point cloud descriptor #1652

Conversation

jpsml commented Jul 16, 2016

SergioRAgostinho commented Jul 18, 2016

Unit tests (mandatory)

Tutorial (optional)

jpsml commented Jul 18, 2016

jpsml commented Aug 1, 2016

SergioRAgostinho commented Aug 1, 2016

jpsml commented Aug 1, 2016

jpsml commented Aug 3, 2016

jpsml commented Aug 8, 2016

SergioRAgostinho commented Aug 9, 2016

SergioRAgostinho commented Jul 5, 2017

SergioRAgostinho 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioRAgostinho Jul 5, 2017 • 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

SergioRAgostinho Jul 5, 2017 • 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

taketwo commented Jul 5, 2017

jpsml commented Jul 7, 2017

SergioRAgostinho commented Jul 11, 2017

jpsml commented Jul 12, 2017

taketwo commented Jul 17, 2017

jpsml commented Aug 2, 2017 • edited Loading

taketwo commented Aug 7, 2017 • edited Loading

SergioRAgostinho commented Nov 24, 2017

jpsml commented Nov 25, 2017

SergioRAgostinho commented Nov 25, 2017

taketwo 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

Choose a reason for hiding this comment

taketwo commented Nov 26, 2017

SergioRAgostinho commented Nov 26, 2017

taketwo commented Nov 26, 2017

SergioRAgostinho commented Nov 26, 2017

taketwo commented Nov 26, 2017

SergioRAgostinho commented Nov 26, 2017

SergioRAgostinho commented Dec 1, 2017

SergioRAgostinho commented Dec 12, 2017

jpsml commented Dec 21, 2017

SergioRAgostinho commented Dec 21, 2017 • edited Loading

jpsml commented Dec 21, 2017

SergioRAgostinho Jul 5, 2017 •

edited

Loading

SergioRAgostinho Jul 5, 2017 •

edited

Loading

jpsml commented Aug 2, 2017 •

edited

Loading

taketwo commented Aug 7, 2017 •

edited

Loading

SergioRAgostinho commented Dec 21, 2017 •

edited

Loading