Lccp #718

mschoeler · 2014-05-30T15:15:40Z

This pull-request is part of the GSOC 2014 project of Markus Schoeler (Automated benchmark generation for object-part segmentation).

It contains an implementation of the lccp segmentation published in:
S. C. Stein, M. Schoeler, J. Papon, F. Woergoetter
Object Partitioning using Local Convexity
In Proceedings of the IEEE Conference on Computer Vision and Pattern Recognition (CVPR) 2014

In addition, it contains an example "example_lccp_segmentation.cpp" which shows how to use the lccp segmentation.

VictorLamoine · 2014-05-30T15:17:22Z

examples/segmentation/example_lccp_segmentation.cpp

+ *
+ */
+
+


Extra carriage return

VictorLamoine · 2014-05-30T15:24:43Z

I didn't read all the code but there are bunch of little "errors" to match the PCL style guide (pointcloud.org is down today!).
The Eclipse formatter does a pretty good job at formatting the code (though it's not perfect!)

mschoeler · 2014-05-30T15:28:12Z

thanks, I will fix that.

On 05/30/2014 05:24 PM, Victor Lamoine wrote:

I didn't read all the code but there are bunch of little "errors" to
match the PCL style guide (pointcloud.org is down today!).
The Eclipse formatter
https://github.com/PointCloudLibrary/pcl/blob/master/doc/advanced/content/files/PCL_eclipse_profile.xml
does a pretty good job at formatting the code (though it's not perfect!)

—
Reply to this email directly or view it on GitHub
#718 (comment).

Georg-August-Universität Göttingen
Bernstein Center for Computational Neuroscience
Department for Computational Neuroscience
III Physikalisches Institut - Biophysik
Office E01.103
Friedrich-Hund Platz 1
37077 Göttingen Germany
Tel: +49 (0)551 3910764

rbrusu · 2014-05-30T16:15:02Z

pointclouds is back up. Thanks for the heads up. We're investigating to see what happened, and also upgrading some packages in the process.

mschoeler · 2014-06-01T17:10:20Z

hm I do not understand the clang compile error.
What does it mean if clang fails in the TASK=test environment? It throws me a bunch of linker errors, which I do not get with gcc/build as well as clang/build.
Any idea?

taketwo · 2014-06-01T17:15:18Z

This means that the code can not be compiled with the -DPCL_NO_PRECOMPILE=ON flag. Just grep for this macro in our code-base to see how it is usually used.

mschoeler · 2014-06-03T13:49:09Z

so everything compiles ,but Travis throws a timeout for the unit tests. Anything I can do about it?

taketwo · 2014-06-03T14:01:08Z

Is there any particular reason to implement all the basic vector operations (norm, dot, cross) as member functions of the segmentation class? I would suggest to use the standard implementations provided by Eigen.

Regarding the timeout, it's okay.

mschoeler · 2014-06-03T14:11:11Z

oh, makes perfect sense. I will change that.

jpapon · 2014-06-03T15:05:02Z

It's being updated right now to use the Eigen functions, but do you think it makes sense to put these functions in common?

I don't know how often people take a dotp, vectornorm or crossp between a point and a normal...

taketwo · 2014-06-03T15:40:17Z

All these functions are implemented for vectors in Eigen (docs). Points and normals in PCL can be viewed as Eigen vectors (via getVector3fMap(), getNormalVector3fMap(), etc.).

For instance, if you have a point pcl::PointXYZ p and a normal pcl::Normal n, and want to take a dot product, you just write: p.getVector3fMap ().dot (n.getNormalVector3fMap ())

jpapon · 2014-06-03T15:46:52Z

Yeah, that's what we did... but we put them in helper functions... I'm not sure why really.

We'll simplify it =)

mschoeler · 2014-06-03T16:14:12Z

thanks for the suggestions. I removed the extra functions.

taketwo · 2014-06-07T13:38:38Z

I am not sure if it really makes sense to allow color+depth input in the LLCP example program. Clearly, that is not at all the point of the example, however adds much complexity. (Both in terms of the command line options and the application code). If someone really needs to run LLCP with that kind of input he can preprocess it with e.g. pcl_png2pcd to get a point cloud.

mschoeler · 2014-06-08T10:13:33Z

Right now I'm traveling, but I will take it out end of next week.

On 7. Juni 2014 15:38:46 MESZ, Sergey Alexandrov notifications@github.com wrote:

I am not sure if it really makes sense to allow color+depth input in
the LLCP example program. Clearly, that is not at all the point of the
example, however adds much complexity. (Both in terms of the command
line options and the application code). If someone really needs to run
LLCP with that kind of input he can preprocess it with e.g.
pcl_png2pcd to get a point cloud.

Reply to this email directly or view it on GitHub:
#718 (comment)

Georg-August-Universität Göttingen
Bernstein Center for Computational Neuroscience
Department for Computational Neuroscience
III Physikalisches Institut - Biophysik
Office E01.103
Friedrich-Hund Platz 1
37077 Göttingen Germany
Tel: +49 (0)551 3910764

Sent by Android

taketwo · 2014-06-08T10:15:26Z

Sure. I will probably add more comments here and there in the meanwhile.

taketwo · 2014-06-09T10:14:15Z

segmentation/include/pcl/segmentation/lccp_segmentation.h

+    public:
+
+      // Adjacency list with nodes holding labels (uint32_t) and edges holding EdgeProperties.
+      typedef typename boost::adjacency_list<boost::setS, boost::setS, boost::undirectedS, uint32_t, EdgeProperties> SupervoxelAdjacencyList;


Why using custom struct for the edge property here? Won't simple bool suffice?

I might need to add more edge properties in the future, like is_smooth or is_invalid. This way it is easier to extend without changing too much of the code.

Okay, makes sense.

taketwo · 2014-06-16T21:10:05Z

Hmm, I don't like this idea of relabelCloud() and removeNoise() returning int error code. Could you imagine a situation where someone will actually check this return code? If removeNoise() is called before segment(), then it is a deficit in program logic, not a run-time error.

I would propose that all functions that rely on segment() having been called before them to simply print a PCL_WARN and do nothing else if this didn't happen.

taketwo · 2014-06-16T21:13:02Z

segmentation/include/pcl/segmentation/lccp_segmentation.h

+      getSVAdjacencyList (SupervoxelAdjacencyList& adjacency_list) const;
+
+      /** \brief Set normal threshold
+       *  \param[out] concavity_tolerance_threshold_arg the concavity tolerance angle in [deg] to set */


I think it's \param[in].

taketwo · 2014-09-19T09:34:02Z

So specializations will go to .cpp and will be compiled into the library?

jpapon · 2014-09-19T10:39:48Z

Yes. Though if you look here the plan is to switch to using your centroid accumulator. Unfortunately as far as I can tell I still need to specialize for each point type individually, since as far as I know one can't partially specialize like this:

template<typename PointT> void
pcl::octree::OctreePointCloudAdjacencyContainer<PointT,typename pcl::SupervoxelClustering<PointT>::VoxelData>::addPoint (const PointT &new_point)
{
  ++num_points_;
  data_.point_accumulator_.add (new_point);
}

Of course, I could remove all of these specializations altogether by adding CentroidPoint to the base container...

taketwo · 2014-09-19T10:59:34Z

What is wrong with this partial specialization? As far as I remember, partial specialization is OK as long as it reduces the number of template arguments. Or do I miss something?

jpapon · 2014-09-19T11:04:14Z

AFAIK you can't partially specialize a member function template without specializing the whole class template.

Changes necessary for lccp pull request #718

…nce-estimate * gh/master: (166 commits) Fixes for issue PointCloudLibrary#924 Fix setSize () in PCL visualizer PCL formula is in the official homebrew repo CMake pkg_check_modules sets GLEW_INCLUDEDIR Fix for Mac OS X Changes necessary for lccp pull request PointCloudLibrary#718 Change to visualization to make things a bit nicer - mainly removed broken opacity Updated supervoxel examples with normals and better description of the camera transform Removed public interface for modifying neighbors of OctreePointcloudAdjacencyContainer. Neighbors can now only be added/removed from within OctreePointcloudAdjacency. This does *not* mean neighbors can't be modified, just that the list of neighbors itself can't be modified. Fix for problem where leaves could get lost, and warning would show up. Now we use leaf indices directly for seeding, instead of doing a look up Changed OctreePointcloud Adjacency to use std::list instead of std::set, added comparator to SupervoxelHelper so that the internal set of voxels owned by a helper is sorted by index instead of memory location Added brief comment explaining comparator Fix inconsistency between method declaration and implementation that resulted in build failure Remove Boost Chrono dependency Don't normalize path if function is not in Boost Disable precompilation of PPFRegistration Fix for issue PointCloudLibrary#758 Moved pcl::PPFHashMapSearch::setInputFeatureCloud() and pcl::PPFHashMapSearch::nearestNeighborSearch() implementation from ppf_registration.hpp to ppf_registration.cpp since they belong to a not templated class Bump PCL version to 1.8.0 Fixes PointCloudLibrary#896. Add a changelist for 1.7.2 Set PCL_ONLY_CORE_POINT_TYPES on Windows (Closes PointCloudLibrary#833) ... Conflicts: common/include/pcl/common/impl/centroid.hpp

…ted changes originally from pull request PointCloudLibrary#473. Now uses the CentroidPoint accumulators instead of doing it itself, uses PointXYZRGBNormal internally instead of separate fields.

, but lets be fearless

…me conflicts with PointCloudLibrary#718

taketwo · 2014-10-08T08:53:37Z

segmentation/include/pcl/segmentation/lccp_segmentation.h

+      void
+      reset ();
+
+      /** \brief Merge supervoxels using local convexity. The input parameters are generated by using the SupervoxelClustering class.


It would be nice to elaborate here how the output of the segmentation is supposed to be retrieved. Otherwise it is not clear since there are neither return value nor out params.

taketwo · 2014-10-08T08:59:32Z

@jspricke We have done several rounds of review for this already. I am not convinced that it has best possible interface yet. But I would propose to merge this and then possibly revise it based on feedback.

jspricke · 2014-10-08T14:10:22Z

examples/segmentation/example_lccp_segmentation.cpp

+        // Add the points to the dataset
+        vtkSmartPointer<vtkPolyLine> polyLine = vtkSmartPointer<vtkPolyLine>::New ();
+        polyLine->GetPointIds ()->SetNumberOfIds (2);
+        polyLine->GetPointIds ()->SetId (0,points->GetNumberOfPoints ()-2);


Missing space in front of ,

jspricke · 2014-10-08T14:18:28Z

Apart from the comments by Sergey and me and a squashing the commits, I'm fine with merging it. Thanks for going through all this Markus!

mschoeler · 2014-10-08T15:20:39Z

@taketwo @jspricke: Thank you very much for your help. I updated the code to incorporate your changes and squashed the commits. I do agree that the interface may be confusing for users. Let's see what they say.
Cheers

taketwo · 2014-10-08T15:24:06Z

Thanks Markus. Sorry for delaying this much.

Lccp

VictorLamoine reviewed May 30, 2014
View reviewed changes

examples/segmentation/example_lccp_segmentation.cpp

*

*/

Copy link

Contributor

VictorLamoine May 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra carriage return

taketwo reviewed Jun 9, 2014
View reviewed changes

taketwo reviewed Jun 16, 2014
View reviewed changes

jpapon force-pushed the lccp branch from 7be38d4 to b345e39 Compare September 19, 2014 12:28

jpapon added a commit to jpapon/pcl that referenced this pull request Sep 19, 2014

Changes necessary for lccp pull request PointCloudLibrary#718

80d90be

jpapon mentioned this pull request Sep 19, 2014

Changes necessary for lccp pull request #718 #919

Merged

taketwo added a commit that referenced this pull request Sep 21, 2014

Merge pull request #919 from jpapon/supervoxel_impl_to_cpp

89ec4f9

Changes necessary for lccp pull request #718

jpapon added a commit to jpapon/pcl that referenced this pull request Sep 24, 2014

This will probably cause three way merge hell with PointCloudLibrary#718

3de94b0

, but lets be fearless

jpapon added a commit to jpapon/pcl that referenced this pull request Sep 24, 2014

Removed Supervoxel template parameter from lccp. Now we should get so…

98a85b7

…me conflicts with PointCloudLibrary#718

jpapon mentioned this pull request Oct 1, 2014

Add a new PointCloudColorHandler for "label" field #849

Merged

mschoeler force-pushed the lccp branch 4 times, most recently from 2314157 to 29e6cf7 Compare October 2, 2014 11:34

taketwo reviewed Oct 8, 2014
View reviewed changes

jspricke reviewed Oct 8, 2014
View reviewed changes

Added LCCP algorithm as well as an example.

a3c3383

mschoeler force-pushed the lccp branch from 29e6cf7 to a3c3383 Compare October 8, 2014 15:15

taketwo added a commit that referenced this pull request Oct 8, 2014

Merge pull request #718 from mschoeler/lccp

5660ede

Lccp

taketwo merged commit 5660ede into PointCloudLibrary:master Oct 8, 2014

mschoeler deleted the lccp branch October 9, 2014 06:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lccp #718

Lccp #718

mschoeler commented May 30, 2014

VictorLamoine May 30, 2014

VictorLamoine commented May 30, 2014

mschoeler commented May 30, 2014

rbrusu commented May 30, 2014

mschoeler commented Jun 1, 2014

taketwo commented Jun 1, 2014

mschoeler commented Jun 3, 2014

taketwo commented Jun 3, 2014

mschoeler commented Jun 3, 2014

jpapon commented Jun 3, 2014

taketwo commented Jun 3, 2014

jpapon commented Jun 3, 2014

mschoeler commented Jun 3, 2014

taketwo commented Jun 7, 2014

mschoeler commented Jun 8, 2014

taketwo commented Jun 8, 2014

taketwo Jun 9, 2014

mschoeler Jun 16, 2014

taketwo Jun 16, 2014

taketwo commented Jun 16, 2014

taketwo Jun 16, 2014

taketwo commented Sep 19, 2014

jpapon commented Sep 19, 2014

taketwo commented Sep 19, 2014

jpapon commented Sep 19, 2014

taketwo Oct 8, 2014

taketwo commented Oct 8, 2014

jspricke Oct 8, 2014

jspricke commented Oct 8, 2014

mschoeler commented Oct 8, 2014

taketwo commented Oct 8, 2014

Lccp #718

Lccp #718

Conversation

mschoeler commented May 30, 2014

VictorLamoine May 30, 2014

Choose a reason for hiding this comment

VictorLamoine commented May 30, 2014

mschoeler commented May 30, 2014

rbrusu commented May 30, 2014

mschoeler commented Jun 1, 2014

taketwo commented Jun 1, 2014

mschoeler commented Jun 3, 2014

taketwo commented Jun 3, 2014

mschoeler commented Jun 3, 2014

jpapon commented Jun 3, 2014

taketwo commented Jun 3, 2014

jpapon commented Jun 3, 2014

mschoeler commented Jun 3, 2014

taketwo commented Jun 7, 2014

mschoeler commented Jun 8, 2014

taketwo commented Jun 8, 2014

taketwo Jun 9, 2014

Choose a reason for hiding this comment

mschoeler Jun 16, 2014

Choose a reason for hiding this comment

taketwo Jun 16, 2014

Choose a reason for hiding this comment

taketwo commented Jun 16, 2014

taketwo Jun 16, 2014

Choose a reason for hiding this comment

taketwo commented Sep 19, 2014

jpapon commented Sep 19, 2014

taketwo commented Sep 19, 2014

jpapon commented Sep 19, 2014

taketwo Oct 8, 2014

Choose a reason for hiding this comment

taketwo commented Oct 8, 2014

jspricke Oct 8, 2014

Choose a reason for hiding this comment

jspricke commented Oct 8, 2014

mschoeler commented Oct 8, 2014

taketwo commented Oct 8, 2014