Removed normal related accessors and types from EuclideanClusterComparator #1542

SergioRAgostinho · 2016-02-18T23:13:38Z

Edit: The description below can be considered deprecated. Please jump into #1542 (comment)

Addresses #1514. If the normals are specified, it uses the planar equation for performing the comparison. If not it defaults to the old behaviour.

I'm starting the PR, but it still needs some discussion because there are things I'm not fully sure.

In the original comment for the comparison function it mentions that two checks are made, one between the direction of the normals, which cannot exceed a certain angular threshold, and a second one saying that the 'd' component of the normal cannot exceed a certain distance threshold. I think the first statement is pretty clear, and for the second one I assumed the rationale was, if the planar equation is given by a1*x1 + b1*y1 + c1*z1 + d1 = 0with (a1, b1, c1) being the normal components in point (x1, y1, z1). This meant that

| d1 - d2 | = |(a2*x2 + b2*y2 + c2*z2) - (a1*x1 + b1*y1 + c1*z1)| < thrs

Now the way the current function is implemented, it just compares if the distance between two points is below this threshold, which is wrong.

I'm still unsure about the following points:

there's a boolean variable called depth_dependent_ which I still do not fully understand, but it seems to normalize/scale the threshold. Can anyone shed some light on the rationale behind it?
Can I assume that all normals have unit norm? Because otherwise this d component residue will be a lot different, depending on the norm of the normal, and I'll need to normalize them beforehand.
There are currently only a few files making use of the euclidean cluster comparison e.g., ni_linemod.cpp, pcd_select_object_plane.cpp, organized_segmentation_demo.cpp, stereo_ground_segmentation.cpp, and none of these ever even sets the inputs norms. So what I did was to retain the old behaviour if no normals were specified. Personally this doesn't feel correct, and I think it would be best to print an error message and abort the comparison if possible. Comments on how to proceed are appreciated.
Finally, I wanted to write some unit tests for this. I don't see much tests in segmentation folder. Should I start another file just for the euclidean_cluster_comparator (and probably euclidean segmentation) or is there a more adequate filename (agregating more other tests).
What's the logical difference between the EuclideanClusterComparator and the EuclideanPlaneCoefficientComparator?

Sorry for the trouble

VictorLamoine · 2016-02-19T08:10:24Z

segmentation/include/pcl/segmentation/euclidean_cluster_comparator.h

+          float dx = input_->points[idx1].x - input_->points[idx2].x;
+          float dy = input_->points[idx1].y - input_->points[idx2].y;
+          float dz = input_->points[idx1].z - input_->points[idx2].z;
+          float dist = sqrtf (dx*dx + dy*dy + dz*dz);


float dist = (points[idx1].getVector3fMap() - points[idx2].getVector3fMap()).norm();
Should work and is more elegant / less error prone.

👍 although in my opinion this entire block should be scrapped :)

SergioRAgostinho · 2016-02-19T09:36:57Z

@atrevor Think you can help us here? :)

taketwo · 2016-02-19T16:53:45Z

Hi Sergio, thanks for looking into this. Unfortunately I'm short in time now and next week, but I'll definitely provide comments later.

…parator Fixes PointCloudLibrary#1514. The functionality intended for the EuclideanClusterComparator appears to be clustering based on Euclidean distance only, hence all normal pertaining information is misleading.

SergioRAgostinho · 2017-07-10T18:33:29Z

Looking into this more than a year later made me realize that I got things wrong the first time I addressed the underlying issue being reported.

The EuclideanClusterComparator is nothing more than a simple distance based cluster strategy. Everywhere it's actually used, the implementation seemed to suggest exactly that.

For doing normal and distance comparison, the EuclideanPlaneCoefficientComparator is the way to go.

Also, looking into the child classes from pcl::Comparator, the EuclideanClusterComparator is the only one with a name suggestive enough for it to be responsible for performing euclidean distance clustering.

jolesinski · 2017-07-10T20:21:53Z

Hi, I think that not only normals should be cleaned here. What is the point of the label check in the comparator? Whatever it is, I think that it should be optional. Now there is a crash from uninitialized labels pointer if it is not set before clustering.

SergioRAgostinho · 2017-07-11T05:46:47Z

Based on what I could tell from the files which actually use the class, it's meant to group points from the input point cloud, under a similar index. Basically it provides the id of the segment the point belongs to.

In the comparator, then you can specify if you want to use certain label for comparisons or not. I'll need to revise this again having that in mind.

jolesinski

Hi, got some minor comments, but +1 from me. Now it will be possible to use organized segmentation just as in euclidean cluster extraction tutorial, thanks.

jolesinski · 2017-07-13T18:00:22Z

segmentation/include/pcl/segmentation/euclidean_cluster_comparator.h

@@ -50,31 +50,25 @@ namespace pcl
    *
    * \author Alex Trevor
    */
-  template<typename PointT, typename PointNT, typename PointLT>
+  template<typename PointT, typename PointLT>


If labels are now optional, can the template label default to PointLT = pcl::Label?

jolesinski · 2017-07-13T18:10:26Z

segmentation/include/pcl/segmentation/euclidean_cluster_comparator.h

@@ -162,22 +124,21 @@ namespace pcl
        exclude_labels_ = boost::make_shared<std::vector<bool> >(exclude_labels);


This looks weird, I see no point for copying excludes to a shared pointer and they should be passed as const

Agreed. I didn't change originally because it's not breaking anything at this point and it would be an additional thing breaking the API.

jolesinski · 2017-07-13T18:12:35Z

segmentation/include/pcl/segmentation/euclidean_cluster_comparator.h

+          assert (labels_->size () == input_->size ());
+          const uint32_t &label1 = labels_->at (idx1).label;
+          const uint32_t &label2 = labels_->at (idx2).label;
+          if ( (*exclude_labels_)[label1] || (*exclude_labels_)[label2])


Consider exclude_labels_->at(label) to be user proof

Is it really necessary? Both labels_ and exclude_labels_ are set by the user... I would personally just let it explode and have the user debug it.

jolesinski · 2017-07-13T18:27:08Z

segmentation/include/pcl/segmentation/euclidean_cluster_comparator.h

@@ -191,16 +152,13 @@ namespace pcl
        float dy = input_->points[idx1].y - input_->points[idx2].y;
        float dz = input_->points[idx1].z - input_->points[idx2].z;
        float dist = std::sqrt (dx*dx + dy*dy + dz*dz);


This could be wrapped with pcl::geometry::distance(input_->at(idx1), input_->at(idx2))

I would actually just use eigen for it. I'll change it only if the rewrite is requested.

jolesinski · 2017-07-13T18:34:17Z

test/visualization/CMakeLists.txt

@@ -8,6 +8,8 @@ set(build TRUE)
 PCL_SUBSYS_OPTION(build "${SUBSYS_NAME}" "${SUBSYS_DESC}" ${DEFAULT} "${REASON}")
 PCL_SUBSYS_DEPEND(build "${SUBSYS_NAME}" DEPS ${SUBSYS_DEPS} OPT_DEPS ${OPT_DEPS})

+include_directories(${VTK_INCLUDE_DIRS})


This looks like some other PR

Yeah, for some reason my test was not compiling, but there's no reason to add this in this PR.

SergioRAgostinho · 2017-07-13T20:14:04Z

Before going through your comments let me just disclaim that my intention was really to change as less things as possible.

If the general opinion is that I should do a proper rewrite of the thing there's gonna be more things to probably alter.

Thank you for taking time and reviewing the thing. Is good to see people involved.

…s are not specified

SergioRAgostinho · 2017-07-14T13:00:17Z

The previous implementation of the exclude_labels_ as a vector "imposed" that all labels used had to be be sequentially numbered and starting at 0. The map lifts such a restriction.

taketwo · 2017-07-17T13:12:36Z

segmentation/include/pcl/segmentation/euclidean_cluster_comparator.h

@@ -93,14 +95,15 @@ namespace pcl
        * \param depth_dependent
        */
      inline void
-      setDistanceThreshold (float distance_threshold, bool depth_dependent)
+      setDistanceThreshold (const float &distance_threshold,


Why did these become references?

Originally I thought: You have only a single copy, inside the body of the function. But then again, when you pass a reference I think you're passing the underlying pointer type into the function. You still have two copies in the end right: 1 pointer + 1 native type. I'll just revert it to what it was.

taketwo · 2017-07-17T13:14:34Z

segmentation/include/pcl/segmentation/euclidean_cluster_comparator.h

      {
        labels_ = labels;
      }

+      const boost::shared_ptr<const std::map<uint32_t, bool> >&


Maybe a typedef for this will improve readability?

taketwo · 2017-07-17T13:16:39Z

segmentation/include/pcl/segmentation/euclidean_cluster_comparator.h

      {
        distance_threshold_ = distance_threshold;
        depth_dependent_ = depth_dependent;
      }

      /** \brief Get the distance threshold in meters (d component of plane equation) between neighboring points, to be considered part of the same plane. */
-      inline float
+      inline const float&


Again: preventing a copy if you don't desire one. But in the end I'm not sure what exactly happens under the hood, when you create a reference.

You would gain nothing as you would need to create a pointer ;).

What I feared ^^ I'll revert it.

SergioRAgostinho · 2017-07-26T01:03:49Z

apps/cloud_composer/include/pcl/apps/cloud_composer/tools/impl/organized_segmentation.hpp

-     }
+     boost::shared_ptr<std::map<uint32_t, bool> > plane_labels = boost::make_shared<std::map<uint32_t, bool> > ();
+     for (size_t i = 0; i < label_indices.size (); ++i)
+       (*plane_labels)[i] = (label_indices[i].indices.size () > min_plane_size)? true : false;


Just noticed I did something useless here (and the other repetitions of this pattern). This can be reduced to:

(*plane_labels)[i] = (label_indices[i].indices.size () > min_plane_size);

🤦‍♂️

I'll submit the change at some point.

SergioRAgostinho · 2017-10-31T08:16:02Z

Should be good for merging, once Travis get's back on track. https://www.traviscistatus.com/incidents/4vdl52d28hz3

atrevor · 2017-11-03T22:41:20Z

Sorry to comment so late on this, but thanks for the cleanup @SergioRAgostinho and @taketwo ! IIRC I had intended to optionally use normals in addition to distance for the clustering, but seems like I didn't get around to it at the time this got merged. Probably best to keep it simple anyway. Thanks again!

SergioRAgostinho · 2017-11-06T21:05:24Z

Hey

I had intended to optionally use normals in addition to distance for the clustering

How exactly? (I'm assuming it is something different than what is done in EuclideanPlaneCoefficientComparator)

taketwo · 2017-11-16T14:10:31Z

(Updating to PCL master broke our library at work without any "depreciation warning" ceremony 😆)

BTW, why did you decide to go for a map with excluded labels? Isn't a set more semantically clear?

SergioRAgostinho · 2017-11-16T15:26:50Z

Oh true... crap. Maybe I should re add the methods and set them to deprecated before the next release.

BTW, why did you decide to go for a map with excluded labels? Isn't a set more semantically clear?

True. I overlooked that detail. You just need to know if the label is the set or not. The true vs false value redundant.

taketwo · 2017-11-16T15:38:51Z

I guess my "code review" was a fail 😢

SergioRAgostinho · 2018-09-25T09:19:23Z

I'm removing both the removal and the breaks ABI labels because we actually provided proper deprecation in #2096.

VictorLamoine reviewed Feb 19, 2016
View reviewed changes

SergioRAgostinho added the status: needs decision label Aug 20, 2016

SergioRAgostinho force-pushed the eucl_clst_cmp branch 2 times, most recently from 97949ae to 8c3e03f Compare July 10, 2017 18:25

SergioRAgostinho changed the title ~~[WIP] Euclidean cluster comparator now makes use of normals~~ Removed normal related accessors and types from EuclideanClusterComparator Jul 10, 2017

SergioRAgostinho added this to the pcl-1.9.0 milestone Jul 10, 2017

SergioRAgostinho added needs: code review Specify why not closed/merged yet and removed status: needs decision labels Jul 10, 2017

SergioRAgostinho added needs: more work Specify why not closed/merged yet needs: code review Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet needs: more work Specify why not closed/merged yet labels Jul 11, 2017

jolesinski approved these changes Jul 13, 2017

View reviewed changes

Euclidean Cluster Comparator can now cope with situations where label…

4dd532e

…s are not specified

SergioRAgostinho force-pushed the eucl_clst_cmp branch 2 times, most recently from df8146e to 6b8117e Compare July 14, 2017 12:55

taketwo reviewed Jul 17, 2017

View reviewed changes

SergioRAgostinho force-pushed the eucl_clst_cmp branch from 6b8117e to 973e2b3 Compare July 17, 2017 14:53

taketwo approved these changes Jul 17, 2017

View reviewed changes

taketwo removed the needs: code review Specify why not closed/merged yet label Jul 17, 2017

taketwo added the status: ready to merge label Jul 17, 2017

SergioRAgostinho commented Jul 26, 2017

View reviewed changes

Modified the exclude_label_ container to a map. Class cleanup.

9433518

SergioRAgostinho force-pushed the eucl_clst_cmp branch from 973e2b3 to 9433518 Compare October 31, 2017 08:12

taketwo merged commit 857a197 into PointCloudLibrary:master Nov 3, 2017

taketwo removed the status: ready to merge label Nov 3, 2017

SergioRAgostinho deleted the eucl_clst_cmp branch November 6, 2017 21:10

taketwo added changelog: ABI break Meta-information for changelog generation changelog: API break Meta-information for changelog generation labels Nov 16, 2017

SergioRAgostinho mentioned this pull request Nov 16, 2017

Provide proper depreciation to EuclideanClusterComparator #2091

Closed

This was referenced Aug 25, 2018

Change Log automation #2385

Closed

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

Merged

SergioRAgostinho added the module: segmentation label Aug 25, 2018

SergioRAgostinho added changelog: removal Meta-information for changelog generation and removed changelog: ABI break Meta-information for changelog generation labels Sep 2, 2018

SergioRAgostinho removed changelog: API break Meta-information for changelog generation changelog: removal Meta-information for changelog generation labels Sep 25, 2018

		@@ -162,22 +124,21 @@ namespace pcl
		exclude_labels_ = boost::make_shared<std::vector<bool> >(exclude_labels);

Removed normal related accessors and types from EuclideanClusterComparator #1542

Removed normal related accessors and types from EuclideanClusterComparator #1542

Conversation

SergioRAgostinho commented Feb 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioRAgostinho commented Feb 19, 2016

taketwo commented Feb 19, 2016

SergioRAgostinho commented Jul 10, 2017

jolesinski commented Jul 10, 2017

SergioRAgostinho commented Jul 11, 2017 • edited Loading

jolesinski 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 Jul 13, 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

Choose a reason for hiding this comment

SergioRAgostinho commented Jul 13, 2017

SergioRAgostinho commented Jul 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioRAgostinho commented Oct 31, 2017

atrevor commented Nov 3, 2017

SergioRAgostinho commented Nov 6, 2017

taketwo commented Nov 16, 2017

SergioRAgostinho commented Nov 16, 2017

taketwo commented Nov 16, 2017

SergioRAgostinho commented Sep 25, 2018

SergioRAgostinho commented Feb 18, 2016 •

edited

Loading

SergioRAgostinho commented Jul 11, 2017 •

edited

Loading

SergioRAgostinho Jul 13, 2017 •

edited

Loading

SergioRAgostinho commented Jul 14, 2017 •

edited

Loading