FLARELocalReferenceFrameEstimation class added #1571

aliosciapetrelli · 2016-03-30T14:44:41Z

Implementation of the Fast LocAl Reference framE algorithm for local reference frame estimation as described here:
A. Petrelli, L. Di Stefano,
"A repeatable and efficient canonical reference for surface matching",
3DimPVT, 2012

FLARE algorithm has been devised by the same authors of BOARD method (implemented by the BOARDLocalReferenceFrameEstimation class) and proved to be more repeatable and faster than BOARD.

List of modifications:

FLARELocalReferenceFrameEstimation class added
unit test for FLARELocalReferenceFrameEstimation class added
directedOrthogonalAxis, projectPointOnPlane, getAngleBetweenUnitVectors, randomOrthogonalAxis, normalDisambiguation and planeFitting methods of BOARDLocalReferenceFrameEstimation class
moved to lrf_utils.hpp

aliosciapetrelli · 2016-06-09T14:38:25Z

The pull request has been rebased and all the fixes has been squashed.

SergioRAgostinho · 2016-08-18T13:45:51Z

Hi @aliosciapetrelli. Thanks for contributing this to the project. Sorry for the delay in providing feedback.

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 👍

aliosciapetrelli · 2016-08-20T08:32:44Z

Hi Sérgio,
I'm very interested in promoting my algorithm to the community.
Following your advice, I'll work on a tutorial in the next days.

Regards,
Alioscia

On Thu, Aug 18, 2016 at 3:46 PM, Sérgio Agostinho notifications@github.com
wrote:

Hi @aliosciapetrelli https://github.com/aliosciapetrelli. Thanks for
contributing this to the project. Sorry for the delay in providing
feedback.

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 👍

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1571 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ARTHYaJGBDueiu1nizc9Dvmpg5wwLUa6ks5qhGIbgaJpZM4H71fB
.

SergioRAgostinho · 2017-07-05T17:10:44Z

This was superseeded by #1614, but given the request for splitting the PR this might be a logical division

taketwo · 2017-07-05T18:04:14Z

You are right, I think merging this PR would be a great starting point.

@aliosciapetrelli are you still interesting in contributing?

aliosciapetrelli · 2017-07-05T20:02:31Z

Hi,
Sorry if I have not responded yet, but these days I'm very busy.
I have planned to start the commits this weekend.

taketwo · 2017-07-06T08:27:07Z

Great to hear that you are still with us! So, as I said, let's work on merging this PR first. As far as I see, this PR is exactly the same as the first commit of the ReLOC pull request (#1614). Sergio already reviewed it there. It's somewhat inconvenient, but I would propose you to take into account those comments, but carry on the discussion here.

taketwo · 2017-07-08T10:16:01Z

I had a look at the functions that you extracted from Board into a separate utils file. As I wrote before, I think they should rather be moved into appropriate places in existing PCL modules. And in fact, some of them are just duplicating functionality that already exists. Here is a list:

getAngleBetweenUnitVectors: we have a similar function already (getAngle3D) in "common/common.h". I think this one can be renamed and moved there becoming an overload.
planeFitting: implemented in "features/normal_3d.h"
normalDisambiguation: there is a similar in spirit function (flipNormalTowardsViewpoint) in "features/normal_3d.h", perhaps can move there
projectPointOnPlane: in "common/geometry.h" there is already project function which does the same
areEquals: this can be deleted, we have equal in "common/utils.h"
directedOrthogonalAxis and randomOrthogonalAxis: can not find anything similar. Perhaps can go to "common/geometry.h"? Or any better ideas?

aliosciapetrelli · 2017-07-08T10:41:57Z

features/include/pcl/features/flare.h

+        /** \brief Get a pointer to the sampled_surface_ cloud dataset. */
+        inline PointCloudInConstPtr 
+          getSearchSampledSurface() const
+        {


If I understood correctly, I should simply replace "PointCloudInConstPtr" with "const PointCloudInConstPtr&".
But in this case, what's the difference with getSearchSurface () in Feature class? They seem equivalent to me.

You we're mimicking something you saw in the base class, which is good, but it doesn't mean the base class is doing it right. The reason I ask for the const reference is because with it you can do both

const FLARELocalReferenceFrameEstimation<PointInT, PointOutT>::PointCloudInConstPtr& sampled_surface = flare.getSampledSurface (); //No copy of anything is made, no triggering of ref count FLARELocalReferenceFrameEstimation<PointInT, PointOutT>::PointCloudInConstPtr sampled_surface = flare.getSampledSurface (); //A copy is made, ref count is triggered.

With the old signature you can only do the second one, because you can't bind a reference to an rvalue.

Basically you're giving an option to the caller to decide if he needs the copy or just the reference.

aliosciapetrelli · 2017-07-08T10:43:52Z

features/include/pcl/features/flare.h

+        /** \brief Get a pointer to the search method used for the extimation of x axis. */
+        inline KdTreePtr 
+          getSearchMethodForSampledSurface () const
+        {


Same comment as above but now related to getSearchMethod () to Feature class.

Same answer as above.

aliosciapetrelli · 2017-07-09T11:46:26Z

How should I proceed with the virtual keyword?
I agree with @taketwo that it makes the code more readable.
Moreover, in the future migration, the presence of the virtual keyword could make easier to identify where to place the override keyword.

Nevertheless, I'm ready to commit the changes.

SergioRAgostinho · 2017-07-09T13:17:44Z

Go with @taketwo 's pointers on this one.

taketwo · 2017-07-09T14:09:15Z

Moreover, in the future migration, the presence of the virtual keyword could make easier to identify where to place the override keyword.

... there is also a special warning for this, so the compiler will tell us.

SergioRAgostinho · 2017-07-12T14:47:30Z

I'm not sure if you're done already but you need to ditch that merge commit from master you just did. What you need to do is rebase your FLARE original commit to the tip of master. This is a good read for that https://www.atlassian.com/git/tutorials/rewriting-history

aliosciapetrelli · 2017-07-12T17:44:08Z

I'm not familiar with Git, specially with rebasing, so I need to study the topic.

SergioRAgostinho · 2017-07-12T18:41:29Z

No worries, take your time. Don't forget to backup everything and try it first on a copy of your repo.

Some quick pointers:

with rebase -i you can completely delete your last merge commit from master
then you'll need to invoke your rebase against upstream/master
confirm if things went as intended with git log. Your commit should be right on top of the last commit of upstream/master
if things are ok then do a git push -f <your fork>

aliosciapetrelli · 2017-07-16T10:05:37Z

In the end, is it better if the pull request will result in a unique commit (with the initial changes together with the ones after the review) or in two different commits (one for the initial changes and another for the review)?

SergioRAgostinho · 2017-07-16T11:13:34Z

Most likely it will be a single commit. We'll squash everything after all reviews and corresponding changes are committed.

- Add FLARELocalReferenceFrameEstimation class - Add unit test for FLARELocalReferenceFrameEstimation class - Add LRF_utils.h - Add LRF_utils.cpp - Add directedOrthogonalAxis, projectPointOnPlane in lrf_utils - getAngleBetweenUnitVectors, randomOrthogonalAxis, normalDisambiguation and planeFitting methods of BOARDLocalReferenceFrameEstimation class moved to lrf_utils.hpp

SergioRAgostinho · 2017-08-07T18:09:25Z

I have 12 hours of flight ahead of me. It might take me some days to get back to this :')

aliosciapetrelli · 2017-08-07T22:49:22Z

I think it's ok now.

SergioRAgostinho

Sorry for the delay. I think this is my last round. Everything are minor comments.

SergioRAgostinho · 2017-11-01T11:04:01Z

common/include/pcl/common/geometry.h

+      project (point, plane_origin, plane_normal, projection);
+      projected_as_unit_vector = projection - plane_origin;
+      projected_as_unit_vector.normalize ();
+    }


May I advise to simply return projected_as_unit_vectoras the function return value. My comment is exploiting something called Return Value Optimization and it would allow initializing Eigen::Vector3f variables from the output of this function.

SergioRAgostinho · 2017-11-01T11:22:08Z

common/include/pcl/common/geometry.h

+      }
+
+      rand_ortho_axis.normalize ();
+    }


Same comment here regarding RVO.

SergioRAgostinho · 2017-11-01T11:30:05Z

features/include/pcl/features/normal_3d.h

+      }
+    }
+
+    if(normal_mean.isZero())


You missed spacing here.

SergioRAgostinho · 2017-11-01T11:30:36Z

features/include/pcl/features/normal_3d.h

+    {
+      const PointNT& cur_pt = normal_cloud[normal_indices[i]];
+
+      if (pcl::isFinite(cur_pt))


Spacing here.

SergioRAgostinho · 2017-11-01T11:39:01Z

test/features/test_flare_estimation.cpp

+  ne.setViewPoint (1, 1, 10);
+  ne.setInputCloud (cloud);
+  ne.setSearchMethod (tree);
+  ne.setIndices (indices_ptr);


Do you need to set the indices here? The default behavior if you don't set the indices is to use all the points anyway. The indices are just used to allow you to mask specifics points of your input.

SergioRAgostinho · 2017-11-01T11:53:29Z

features/include/pcl/features/flare.h

+      typedef typename pcl::PointCloud<SignedDistanceT> PointCloudSignedDistance;
+      typedef typename PointCloudSignedDistance::Ptr PointCloudSignedDistancePtr;
+
+      typedef typename PointCloudIn::ConstPtr PointCloudInConstPtr;


This definition already exists in pcl::Feature so replace it with

using typename Feature<PointInT, PointOutT>::PointCloudInConstPtr;

SergioRAgostinho · 2017-11-01T11:54:27Z

features/include/pcl/features/flare.h

+      typedef boost::shared_ptr<FLARELocalReferenceFrameEstimation<PointInT, PointNT, PointOutT> > Ptr;
+      typedef boost::shared_ptr<const FLARELocalReferenceFrameEstimation<PointInT, PointNT, PointOutT> > ConstPtr;
+
+      typedef typename pcl::search::Search<PointInT>::Ptr KdTreePtr;


This definition also exist in pcl::Featureso apply the same method as above.

SergioRAgostinho · 2017-11-01T11:56:45Z

features/include/pcl/features/flare.h

+
+      /** \brief Empty destructor */
+      virtual
+      ~FLARELocalReferenceFrameEstimation () {}


You don't need to declare this destructor (even if it's virtual). See https://stackoverflow.com/questions/2198379/are-virtual-destructors-inherited#2198400

Please delete it.

SergioRAgostinho · 2017-11-01T12:09:00Z

features/include/pcl/features/impl/flare.hpp

+
+  Eigen::Vector3f best_margin_point;
+
+  float radius2 = tangent_radius_ * tangent_radius_;


SergioRAgostinho · 2017-11-01T12:09:10Z

features/include/pcl/features/impl/flare.hpp

+
+  float radius2 = tangent_radius_ * tangent_radius_;
+
+  float margin_distance2 = margin_thresh_ * margin_thresh_ * radius2;


aliosciapetrelli · 2017-11-03T17:22:29Z

Done. It should be ok now.

Before you start any review of #1614, it could be better if I perform a review of all the code on the basis of the suggestions you gave me in this discussion. It should speed up the future changes and discussion of #1614.

SergioRAgostinho

Some remnant code which stayed after indices stop being used.

SergioRAgostinho · 2017-11-03T17:44:54Z

test/features/test_flare_estimation.cpp

+typedef pcl::PointCloud<pcl::PointXYZ>::Ptr PointCloudPtr;
+
+PointCloudPtr cloud;
+std::vector<int> indices;


I this is now no longer needed I think.

SergioRAgostinho · 2017-11-03T21:26:05Z

test/features/test_flare_estimation.cpp

+
+  indices.resize (cloud->points.size ());
+  for (size_t i = 0; i < indices.size (); ++i)
+    indices[i] = static_cast<int> (i);


Everything pertaining to indices can be deleted I think.

SergioRAgostinho · 2017-11-03T21:29:47Z

That would be super 👍 Thanks for that.

SergioRAgostinho

Note to maintainers: squash before merge.

taketwo

Hi, I pointed some more style issues. Once those are fixed, I'm ready to squash-merge.

taketwo · 2017-11-04T17:40:47Z

features/include/pcl/features/impl/flare.hpp

+    }
+
+    //set z_axis as the normal of index point
+    fitted_normal = (*normals_)[index].getNormalVector3fMap();


Missing space

taketwo · 2017-11-04T17:40:55Z

features/include/pcl/features/impl/flare.hpp

+  else
+  {
+    float plane_curvature;
+    normal_estimation_.computePointNormal(*surface_, neighbours_indices, fitted_normal(0), fitted_normal(1), fitted_normal(2), plane_curvature);


Missing spaces

taketwo · 2017-11-04T17:41:48Z

features/include/pcl/features/impl/flare.hpp

+  int best_shape_index = -1;
+
+  Eigen::Vector3f best_margin_point;
+


Empty line here and some more below, do we need them?

taketwo · 2017-11-04T17:42:10Z

features/include/pcl/features/impl/flare.hpp

+  {
+    PCL_ERROR(
+      "[pcl::%s::computeFeature] Error! Search method set to k-neighborhood. Call setKSearch(0) and setRadiusSearch( radius ) to use this class.\n",
+      getClassName().c_str());


Missing spaces

taketwo · 2017-11-04T17:42:26Z

features/include/pcl/features/normal_3d.h

+      }
+    }
+
+    if(normal_mean.isZero ())


Missing space

taketwo · 2017-11-05T14:38:32Z

One and a half years, 83 comments, 11 commits, and we are finally done. Thanks everyone for the effort!

SergioRAgostinho · 2017-11-05T16:35:11Z

Woohoo 🙌

SergioRAgostinho · 2017-11-05T16:36:20Z

It might be worth to write a post to the mailing list to warn that there's a new Feature in town.

Add implementation of the Fast LocAl Reference framE (FLARE) algorithm

This reverts commit 1224939. As FLARELocalReferenceFrameEstimation class has been already added by pull request PointCloudLibrary#1571

aliosciapetrelli force-pushed the flare branch from 7134044 to c6121cc Compare March 31, 2016 10:15

This was referenced May 31, 2016

Add ReLOC - Initial alignment algorithm aliosciapetrelli/pcl#1

Closed

Add ReLOC - Initial alignment algorithm #1614

Open

aliosciapetrelli force-pushed the flare branch from 36e650f to 0fdfc24 Compare June 9, 2016 11:20

SergioRAgostinho added this to the pcl-1.9.0 milestone Jul 11, 2016

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

aliosciapetrelli commented Jul 8, 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 8, 2017

aliosciapetrelli force-pushed the flare branch from 1f25ee4 to 0fdfc24 Compare July 17, 2017 20:26

aliosciapetrelli and others added 2 commits July 17, 2017 22:35

Refinements from review

9f482de

aliosciapetrelli force-pushed the flare branch from 0fdfc24 to 9f482de Compare July 17, 2017 21:22

taketwo added needs: author reply Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Aug 7, 2017

Format Flare unit test

f754e45

taketwo approved these changes Aug 8, 2017

View reviewed changes

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

SergioRAgostinho requested changes Nov 1, 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 Nov 1, 2017

Alioscia Petrelli added 2 commits November 2, 2017 18:35

Changes in geometry.h and flare.h

74e29fc

Add typename

f5ad3c3

SergioRAgostinho requested changes Nov 3, 2017

View reviewed changes

Remove indices.

2dc827b

SergioRAgostinho approved these changes Nov 4, 2017

View reviewed changes

taketwo reviewed Nov 4, 2017

View reviewed changes

Remove spaces

937e9b5

taketwo merged commit 8182d25 into PointCloudLibrary:master Nov 5, 2017

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

UnaNancyOwen pushed a commit to UnaNancyOwen/pcl that referenced this pull request Nov 24, 2017

FLARELocalReferenceFrameEstimation class added (PointCloudLibrary#1571)

4c67d81

Add implementation of the Fast LocAl Reference framE (FLARE) algorithm

SergioRAgostinho mentioned this pull request Sep 2, 2018

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

Merged

taketwo added changelog: new feature Meta-information for changelog generation module: features labels Sep 2, 2018


		Eigen::Vector3f best_margin_point;

		float radius2 = tangent_radius_ * tangent_radius_;


		float radius2 = tangent_radius_ * tangent_radius_;

		float margin_distance2 = margin_thresh_ * margin_thresh_ * radius2;

		int best_shape_index = -1;

		Eigen::Vector3f best_margin_point;

FLARELocalReferenceFrameEstimation class added #1571

FLARELocalReferenceFrameEstimation class added #1571

Conversation

aliosciapetrelli commented Mar 30, 2016

aliosciapetrelli commented Jun 9, 2016

SergioRAgostinho commented Aug 18, 2016 • edited Loading

aliosciapetrelli commented Aug 20, 2016

SergioRAgostinho commented Jul 5, 2017

taketwo commented Jul 5, 2017

aliosciapetrelli commented Jul 5, 2017

taketwo commented Jul 6, 2017

taketwo commented Jul 8, 2017 • edited Loading

Choose a reason for hiding this comment

SergioRAgostinho Jul 8, 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

aliosciapetrelli commented Jul 9, 2017

SergioRAgostinho commented Jul 9, 2017

taketwo commented Jul 9, 2017

SergioRAgostinho commented Jul 12, 2017 • edited Loading

aliosciapetrelli commented Jul 12, 2017

SergioRAgostinho commented Jul 12, 2017

aliosciapetrelli commented Jul 16, 2017

SergioRAgostinho commented Jul 16, 2017

SergioRAgostinho commented Aug 7, 2017 via email

aliosciapetrelli commented Aug 7, 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

Choose a reason for hiding this comment

aliosciapetrelli commented Nov 3, 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

SergioRAgostinho commented Nov 3, 2017

SergioRAgostinho left a comment

Choose a reason for hiding this comment

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

taketwo commented Nov 5, 2017

SergioRAgostinho commented Nov 5, 2017

SergioRAgostinho commented Nov 5, 2017

SergioRAgostinho commented Aug 18, 2016 •

edited

Loading

taketwo commented Jul 8, 2017 •

edited

Loading

SergioRAgostinho Jul 8, 2017 •

edited

Loading

SergioRAgostinho commented Jul 12, 2017 •

edited

Loading