Model outlier removal contribution #702

VictorLamoine · 2014-05-26T14:50:20Z

Is it the good way to update @TimoHackel's contribution #254 ?

taketwo · 2014-05-26T15:59:56Z

filters/include/pcl/filters/impl/model_outlier_removal.hpp

+
+////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+template <typename PointT> bool
+pcl::ModelOutlierRemoval<PointT>::initSACModel (int model_type)


I would rather be specific and use the SacModel enum instead of int here.

taketwo · 2014-05-26T16:05:47Z

@jspricke Should we remove PCLPointCloud2 specialization?

taketwo · 2014-05-26T16:06:06Z

doc/tutorials/content/model_outlier_removal.rst

+
+This tutorial demonstrates how to extract parametric models for example for planes or spheres 
+out of a PointCloud by using SAC_Models with known coefficients.
+If you don't know the models coefficients take a look at the Sample Consensus tutorials.


Would be nice to have a link here and in the previous line.

taketwo · 2014-05-26T16:09:40Z

Hi Victor, thanks for taking care of this abandoned pull request. I think it might be appropriate to deviate a bit from our usual rule of squashing commits and leave the original contribution of Timo as is, adding modifications in separate commit(s) on top of it.

taketwo · 2014-05-26T20:31:03Z

filters/include/pcl/filters/model_outlier_removal.h

+      /** \brief returns the models coefficients
+       */
+      pcl::ModelCoefficients
+      getModelCoefficients ()


We usually mark getter functions with const modifier.

jspricke · 2014-05-27T09:29:35Z

@taketwo Regarding PCLPointCloud2, I'm still in favor of deprecating it at some point (or rather have a new, more capable, point type in the future). My point was that this patch contains two identical (didn't check) copies of initSACModel(). I know that we have quite a lot of similar things in the codebase already, so people could be used to having both versions. Still I think it would be better for PCL to not add more of them. But I'm not a user of that part at the moment, so I let it to you to decide.

Regarding cleaning up Timos contribution, I would propose to adopt Something like the kernel guidelines [1] or [2].

[1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=4e8a2372f9255a1464ef488ed925455f53fbdaa1#n286
[2] https://bugs.eclipse.org/bugs/show_bug.cgi?id=375536

taketwo · 2014-05-27T09:45:35Z

Regarding cleaning up Timos contribution, I would propose to adopt Something like the kernel guidelines [1] or [2].

Do you mean that you would rather see a single commit with an attribution to Victor in the commit message? I think in this case quite a number of changes are being made on top of the original one, and it's easier to digest them as several commits.

But I'm not a user of that part at the moment, so I let it to you to decide.

I'm a PCL user since 2012, but I never had a necessity to use a specialization of any class for PCLPointCloud2. I also dislike code duplication, so +1 for removing it.

jspricke · 2014-05-27T09:51:39Z

Sergey Alexandrov notifications@github.com [2014-05-27 02:45]:

Do you mean that you would rather see a single commit with an attribution to Victor in the commit message? I think in this case quite a number of changes are being made on top of the original one, and it's easier to digest them as several commits.

No:
"Note that under no circumstances can you change the author's identity
(the From header), as it is the one which appears in the changelog."

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=4e8a2372f9255a1464ef488ed925455f53fbdaa1#n353

So as I understand it, you add a comment to the commit message stating
what you changed and a Signed-off-by, or Also-By, depending on what you
did.

VictorLamoine · 2014-05-27T09:53:05Z

@TimoHackel do not seem very active on GitHub and never wrote a Signed-off-by, do I sign for him? 😝
The goal is:

Not stealing his work
Not making him endorse bugs he did not created

I'll will delete PCLPointCloud2 specialization.

jspricke · 2014-05-27T10:02:31Z

@VictorLamoine I guess it should be enough to add your Signed-off-by, but I'm not an expert there.

taketwo · 2014-05-27T10:10:04Z

Still I don't get what are the cons for just leaving the original commit as is (like if it was merged), and then putting the updates on top?

jspricke · 2014-05-28T08:25:06Z

@taketwo It's the same as with all other style changes, you are changing the author of the code and it's making the commit history harder to read.

taketwo · 2014-05-28T10:00:16Z

That is true. On the other hand, in this case Victor stood up to take care of an abandoned code. Even though he is not the original author, he took the responsibility of cleaning up and integrating the contribution in PCL. Therefore, if the fabulous git blame returns his name, it makes perfect sense to me. Furthermore I suspect Timo would be surprised if he is held responsible for the code that he did not care enough to integrate and that was also modified by someone else.

jspricke · 2014-05-28T12:06:44Z

True, we are not the kernel community, so I'm ok with doing separate commits.

VictorLamoine · 2014-05-28T14:33:30Z

Ok fine.
I need some help if you want me to change the pointer, see comment #702 (comment)

Otherwise I think the code is now pretty clean and matches the PCL style guide !

taketwo · 2014-05-28T15:18:03Z

Sorry I overlooked your comment. The syntax is correct, however it would be nicer to use a typedef that is already present within the class: SampleConsensusModelFromNormals<PointT, pcl::Normal>::Ptr.

The errors you get are anticipated. Boost shared pointer is "similar" to a plain pointer, but does not have exactly the same semantics. I would strongly recommend you to get acquainted with this, since smart pointers are an essential tool in modern C++. This short article may get you started.

TimoHackel · 2014-05-29T12:39:00Z

Hi,
this pointer is a hack to work with multiple inheritence, the object behind this pointer gets deleted by an other shared pointer ... if you want to get rid of plain pointers, you can write an empty delete function for that pointer.

Anyways, it would be nicer to remove that pointer completely and try a type casts instead.

taketwo · 2014-05-29T12:51:23Z

Why do we need custom delete functions? Won't something like this work?

case SACMODEL_NORMAL_PLANE:
{
  PCL_DEBUG ("[pcl::%s::segment] Using a model of type: modelNORMAL_PLANE\n", getClassName ().c_str ());
  model_from_normals_.reset (new SampleConsensusModelNormalPlane<PointT, pcl::Normal> (input_));
  model_ = model_from_normals_;
  break;
}

TimoHackel · 2014-05-29T14:39:08Z

I don't think so. There's no inheritance between type of model_ and type of model_from_normals_ (compare: https://github.com/PointCloudLibrary/pcl/blob/master/sample_consensus/include/pcl/sample_consensus/sac_model.h#L557 ). Hence, model_ = model_from_normals_ will fail.

VictorLamoine · 2014-06-03T15:49:42Z

Anyways, it would be nicer to remove that pointer completely and try a type casts instead.

I don't understand how I could remove the model_from_normals_ pointer.

What do we do here ? I'm far from being a C++ expert and you quite lost me here :)

TimoHackel · 2014-06-04T17:06:37Z

I would remove the member model_from_normals_ by doing the following:
a) initSACModel:

template <typename PointT> bool
pcl::ModelOutlierRemoval<PointT>::initSACModel (pcl::SacModel model_type)
{
 ...
 case SACMODEL_NORMAL_PLANE:
 {
   PCL_DEBUG ("[pcl::%s::segment] Using a model of type: modelNORMAL_PLANE\n", getClassName ().c_str ());
   model_.reset (new SampleConsensusModelNormalPlane<PointT, pcl::Normal> (input_));
   break;
 }
 ...
}

b) applyFilterIndices:

template <typename PointT> void
pcl::ModelOutlierRemoval<PointT>::applyFilterIndices (std::vector<int> &indices)
{
 ...
  valid_setup &= initSACModel (model_type_);
  SampleConsensusModelFromNormals<PointT, pcl::Normal> *model_from_normals = dynamic_cast< SampleConsensusModelFromNormals<PointT, pcl::Normal> * >( &(*model_) ); //returns NULL if cast isn't possible
  if (model_from_normals)
  {
    ...
  }
 ...
}

(not tested and a typedef for SampleConsensusModelFromNormals<PointT, pcl::Normal> would be nice)

VictorLamoine · 2014-06-05T08:35:52Z

Thank you for your help!

It compiles fine by my side with gcc and the test unit is ok.
When this pull request will be merged don't forget to close #254

taketwo · 2014-06-07T17:27:49Z

filters/include/pcl/filters/model_outlier_removal.h

+      initSACModel (pcl::SacModel model_type);
+  };
+}
+


We need a conditional include of the implementation file here for the "no precompilation" option:

#ifdef PCL_NO_PRECOMPILE #include <pcl/filters/impl/model_outlier_removal.hpp> #endif

taketwo · 2014-06-07T17:30:35Z

filters/include/pcl/filters/model_outlier_removal.h

+#include <pcl/sample_consensus/sac_model_parallel_line.h>
+#include <pcl/sample_consensus/sac_model_perpendicular_plane.h>
+#include <pcl/sample_consensus/sac_model_plane.h>
+#include <pcl/sample_consensus/sac_model_sphere.h>


I would move as many of these model includes as possible to the implementation file. The user of this class might want only a single model, no need to pull in all the existing model headers.

taketwo · 2014-06-07T18:07:40Z

The reason why the compilation succeeds on your machine is because you did not enable PCL_ONLY_CORE_POINT_TYPES option in CMake, which is set to ON on Travis. Turn it on and you will get the same linker errors.

When this option is on, consensus models are instantiated only for a subset of XYZ point types. Our new class is instantiated for all XYZ point types unconditionally. Therefore to circumvent the problem we need to instantiate ModelOutlierRemoval only for the same reduced set of point types. This means in 'model_outlier_removal.cpp' we need something like:

#ifdef PCL_ONLY_CORE_POINT_TYPES
  PCL_INSTANTIATE (ModelOutlierRemoval, (pcl::PointXYZ)(pcl::PointXYZI)(pcl::PointXYZRGBA)(pcl::PointXYZRGB))
#else
  PCL_INSTANTIATE (ModelOutlierRemoval, PCL_XYZ_POINT_TYPES)
#endif

TimoHackel · 2014-06-07T19:42:49Z

Thanks for your work and help. The pull request #254 is now closed.

VictorLamoine · 2014-06-11T09:12:32Z

@taketwo: I think it is good to merge, is it ?

taketwo · 2014-06-11T10:01:29Z

filters/include/pcl/filters/impl/model_outlier_removal.hpp

+    {
+      PCL_DEBUG ("[pcl::%s::segment] Using a model of type: modelCYLINDER\n", getClassName ().c_str ());
+      SampleConsensusModelCylinder<PointT, pcl::Normal> *ptr = new SampleConsensusModelCylinder<PointT, pcl::Normal> (input_);
+      model_.reset (ptr);


I think we can merge pointer creation and reset() in a single instruction? And in the subsequent cases as well.

Yes, it was there before because of model_from_normals_ but now the ptr pointer is useless.

taketwo · 2014-06-11T10:15:33Z

@VictorLamoine I don't think so :)

taketwo · 2014-06-11T22:03:29Z

filters/include/pcl/filters/model_outlier_removal.h

+
+      /** \brief Set to true if we want to return the data outside the interval specified by setFilterLimits (min, max)
+       * Default: false.
+       * \warning This method will be removed in the future. Use setNegative() instead.


I wonder why we are introducing a brand new class with to-be-removed functions. We are committing to this interface, let's not pollute it.

Signed-off-by: Victor Lamoine <victor.lamoine@gmail.com>

taketwo · 2014-06-15T16:21:38Z

Thanks for working on this Victor!

Model outlier removal contribution

VictorLamoine · 2014-06-15T17:28:36Z

Finally 🎉

taketwo reviewed May 26, 2014
View reviewed changes

taketwo reviewed Jun 7, 2014
View reviewed changes

taketwo reviewed Jun 11, 2014
View reviewed changes

TimoHackel and others added 6 commits June 15, 2014 17:20

model outlier removal contribution

c854917

Code upate

f120675

Signed-off-by: Victor Lamoine <victor.lamoine@gmail.com>

Update tutorial

9d6efee

Remove model_from_normals_ class member

e531faf

Use pcl::isFinite instead of pcl_isfinite

c8e6181

Change cloud_normals_ into a const ptr

5206f4f

taketwo added a commit that referenced this pull request Jun 15, 2014

Merge pull request #702 from VictorLamoine/timoHackel

2e20bc8

Model outlier removal contribution

taketwo merged commit 2e20bc8 into PointCloudLibrary:master Jun 15, 2014

VictorLamoine deleted the timoHackel branch June 15, 2014 17:46

VictorLamoine mentioned this pull request Jul 8, 2014

Including GRSD a global radius-based surface descriptor #790

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Model outlier removal contribution #702

Model outlier removal contribution #702

VictorLamoine commented May 26, 2014

taketwo May 26, 2014

taketwo commented May 26, 2014

taketwo May 26, 2014

taketwo commented May 26, 2014

taketwo May 26, 2014

jspricke commented May 27, 2014

taketwo commented May 27, 2014

jspricke commented May 27, 2014

VictorLamoine commented May 27, 2014

jspricke commented May 27, 2014

taketwo commented May 27, 2014

jspricke commented May 28, 2014

taketwo commented May 28, 2014

jspricke commented May 28, 2014

VictorLamoine commented May 28, 2014

taketwo commented May 28, 2014

TimoHackel commented May 29, 2014

taketwo commented May 29, 2014

TimoHackel commented May 29, 2014

VictorLamoine commented Jun 3, 2014

TimoHackel commented Jun 4, 2014

VictorLamoine commented Jun 5, 2014

taketwo Jun 7, 2014

taketwo Jun 7, 2014

taketwo commented Jun 7, 2014

TimoHackel commented Jun 7, 2014

VictorLamoine commented Jun 11, 2014

taketwo Jun 11, 2014

VictorLamoine Jun 11, 2014

taketwo commented Jun 11, 2014

taketwo Jun 11, 2014

taketwo commented Jun 15, 2014

VictorLamoine commented Jun 15, 2014

Model outlier removal contribution #702

Model outlier removal contribution #702

Conversation

VictorLamoine commented May 26, 2014

Choose a reason for hiding this comment

taketwo commented May 26, 2014

Choose a reason for hiding this comment

taketwo commented May 26, 2014

Choose a reason for hiding this comment

jspricke commented May 27, 2014

taketwo commented May 27, 2014

jspricke commented May 27, 2014

VictorLamoine commented May 27, 2014

jspricke commented May 27, 2014

taketwo commented May 27, 2014

jspricke commented May 28, 2014

taketwo commented May 28, 2014

jspricke commented May 28, 2014

VictorLamoine commented May 28, 2014

taketwo commented May 28, 2014

TimoHackel commented May 29, 2014

taketwo commented May 29, 2014

TimoHackel commented May 29, 2014

VictorLamoine commented Jun 3, 2014

TimoHackel commented Jun 4, 2014

VictorLamoine commented Jun 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taketwo commented Jun 7, 2014

TimoHackel commented Jun 7, 2014

VictorLamoine commented Jun 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taketwo commented Jun 11, 2014

Choose a reason for hiding this comment

taketwo commented Jun 15, 2014

VictorLamoine commented Jun 15, 2014