more helper methods in pcl::PointCloud

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

more helper methods in pcl::PointCloud

Radu B. Rusu
Administrator
Talking to Bastian a few days ago, it occurred to us that maybe the pcl::PointCloud<T> class could contain more helper
methods, such as computeCentroid, or getMinMax, etc.

Right now, most of these methods reside either in specialized classes/files (computeCentroid is part of
libpcl_features), or dumped in various headers in pcl/common/*.h

What do people prefer? Is:

* cloud.getCentroid (centroid)

better/easier/more convenient than

* #include <blabla>
* computeCentroid (cloud, centroid)

?

--
Cheers,
Radu.

_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: more helper methods in pcl::PointCloud

Adam Stambler
Hi,

Just to put in my 2 cents, I prefer the helper method technique.  If the methods are a part of that class, your IDE can help you to use and remember those methods.  Often I will use autocomplete on a class to find out what methods are available for operating on that class.  This will show me methods I may not have known about or simply have forgotten about.  

With the separate header/library, it is not as easy to discover these functions.  You need to know that they exist before hand in order to access and use them.

Cheers,
Adam


On Thu, Oct 7, 2010 at 12:30 PM, Radu Bogdan Rusu <[hidden email]> wrote:
Talking to Bastian a few days ago, it occurred to us that maybe the pcl::PointCloud<T> class could contain more helper
methods, such as computeCentroid, or getMinMax, etc.

Right now, most of these methods reside either in specialized classes/files (computeCentroid is part of
libpcl_features), or dumped in various headers in pcl/common/*.h

What do people prefer? Is:

* cloud.getCentroid (centroid)

better/easier/more convenient than

* #include <blabla>
* computeCentroid (cloud, centroid)

?

--
Cheers,
Radu.

_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users


_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: more helper methods in pcl::PointCloud

Kai M. Wurm
In reply to this post by Radu B. Rusu
Hi Radu,

the functionality is obviously the same. However, if you do not know all
the tricks one can do with a PointCloud, those convenience methods would
make them more visible. The same could probably be achieved by
documentation/tutorials.

Best, Kai

On 10/07/2010 09:30 AM, Radu Bogdan Rusu wrote:

> Talking to Bastian a few days ago, it occurred to us that maybe the pcl::PointCloud<T>  class could contain more helper
> methods, such as computeCentroid, or getMinMax, etc.
>
> Right now, most of these methods reside either in specialized classes/files (computeCentroid is part of
> libpcl_features), or dumped in various headers in pcl/common/*.h
>
> What do people prefer? Is:
>
> * cloud.getCentroid (centroid)
>
> better/easier/more convenient than
>
> * #include<blabla>
> * computeCentroid (cloud, centroid)
>
> ?
>

_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: more helper methods in pcl::PointCloud

Radu B. Rusu
Administrator
Adam, Kai,

Good points. I'll start porting most of this stuff into the base class then, and remove the older methods, unless anyone
thinks this is a bad idea.

When we do the release, we'll have to test all our code in the repository, and since not all of it is tracked by Hudson,
I might need some help there.

We also have to be very careful and work with our fellow colleagues in helping them port the code over, as the methods
will be removed rather than deprecated, in an attempt to consolidate the API and provide a good base for when 1.0 comes
out. So I guess when we make the release announcement for 0.3.4 (if these changes do go in there), we might need to be
more explicit in what changed.

Lorenz, what about some scripting magic to do a search/replace for something like this? :) (once your CoTeSys demo is done)

Cheers,
Radu.


On 10/07/2010 09:38 AM, Kai M. Wurm wrote:

> Hi Radu,
>
> the functionality is obviously the same. However, if you do not know all
> the tricks one can do with a PointCloud, those convenience methods would
> make them more visible. The same could probably be achieved by
> documentation/tutorials.
>
> Best, Kai
>
> On 10/07/2010 09:30 AM, Radu Bogdan Rusu wrote:
>> Talking to Bastian a few days ago, it occurred to us that maybe the pcl::PointCloud<T>   class could contain more helper
>> methods, such as computeCentroid, or getMinMax, etc.
>>
>> Right now, most of these methods reside either in specialized classes/files (computeCentroid is part of
>> libpcl_features), or dumped in various headers in pcl/common/*.h
>>
>> What do people prefer? Is:
>>
>> * cloud.getCentroid (centroid)
>>
>> better/easier/more convenient than
>>
>> * #include<blabla>
>> * computeCentroid (cloud, centroid)
>>
>> ?
>>
>
> _______________________________________________
> [hidden email] / http://pcl.ros.org
> https://code.ros.org/mailman/listinfo/pcl-users
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: more helper methods in pcl::PointCloud

Dejan Pangercic
In reply to this post by Kai M. Wurm
+1 for * cloud.getCentroid (centroid) and friends.
D.

On Thu, Oct 7, 2010 at 6:38 PM, Kai M. Wurm <[hidden email]> wrote:

> Hi Radu,
>
> the functionality is obviously the same. However, if you do not know all
> the tricks one can do with a PointCloud, those convenience methods would
> make them more visible. The same could probably be achieved by
> documentation/tutorials.
>
> Best, Kai
>
> On 10/07/2010 09:30 AM, Radu Bogdan Rusu wrote:
>> Talking to Bastian a few days ago, it occurred to us that maybe the pcl::PointCloud<T>  class could contain more helper
>> methods, such as computeCentroid, or getMinMax, etc.
>>
>> Right now, most of these methods reside either in specialized classes/files (computeCentroid is part of
>> libpcl_features), or dumped in various headers in pcl/common/*.h
>>
>> What do people prefer? Is:
>>
>> * cloud.getCentroid (centroid)
>>
>> better/easier/more convenient than
>>
>> * #include<blabla>
>> * computeCentroid (cloud, centroid)
>>
>> ?
>>
>
> _______________________________________________
> [hidden email] / http://pcl.ros.org
> https://code.ros.org/mailman/listinfo/pcl-users
>



--
MSc. Dejan Pangercic
PhD Student/Researcher
Intelligent Autonomous Systems Group
Technische Universität München
Telephone: +49 (89) 289-26908
E-Mail: [hidden email]
WWW: http://ias.cs.tum.edu/people/pangercic
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: more helper methods in pcl::PointCloud

Radu B. Rusu
Administrator
Okay, I started doing so, but Michael and I have mixed feelings now again, as in...

where do we draw the line?

compute3DCentroid = makes sense, you get it for 1 big point cloud, or for 1 patch, etc
computeCovarianceMatrix = ? makes sense ? you use it for 1 big point cloud to find its principal directions, or for 1
patch to find a normal
computePointNormal = ?? makes sense ?? you use it for a patch to compute the normal at the patch


etc

:) What is the 'rule' that we should follow here? What makes sense to be in cloud.XXX and what doesn't?

Cheers,
Radu.


On 10/07/2010 01:22 PM, Dejan Pangercic wrote:

> +1 for * cloud.getCentroid (centroid) and friends.
> D.
>
> On Thu, Oct 7, 2010 at 6:38 PM, Kai M. Wurm<[hidden email]>  wrote:
>> Hi Radu,
>>
>> the functionality is obviously the same. However, if you do not know all
>> the tricks one can do with a PointCloud, those convenience methods would
>> make them more visible. The same could probably be achieved by
>> documentation/tutorials.
>>
>> Best, Kai
>>
>> On 10/07/2010 09:30 AM, Radu Bogdan Rusu wrote:
>>> Talking to Bastian a few days ago, it occurred to us that maybe the pcl::PointCloud<T>    class could contain more helper
>>> methods, such as computeCentroid, or getMinMax, etc.
>>>
>>> Right now, most of these methods reside either in specialized classes/files (computeCentroid is part of
>>> libpcl_features), or dumped in various headers in pcl/common/*.h
>>>
>>> What do people prefer? Is:
>>>
>>> * cloud.getCentroid (centroid)
>>>
>>> better/easier/more convenient than
>>>
>>> * #include<blabla>
>>> * computeCentroid (cloud, centroid)
>>>
>>> ?
>>>
>>
>> _______________________________________________
>> [hidden email] / http://pcl.ros.org
>> https://code.ros.org/mailman/listinfo/pcl-users
>>
>
>
>
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: more helper methods in pcl::PointCloud

Lorenz Mösenlechner
In reply to this post by Kai M. Wurm
Hi,

actually, I don't like the idea of helper methods in the point cloud
class too much. From the point of view of clean design, I belief that
the helper methods are semantically unrelated to the point cloud class
itself.

And why do I want to compile all that functionality when all I want to
do is just publish a point cloud or so?

How would you specialize such methods for specific point types? When
having it in a method, it might be possible to just specialize the
helper method on specific types, in the proposed class member way I
don't see a chance to do that.

Please think over the change again. It might not be as good as it
seems at the beginning although, I admit, it might be convenient in
some situations.

Lorenz

> Hi Radu,
>
> the functionality is obviously the same. However, if you do not know all
> the tricks one can do with a PointCloud, those convenience methods would
> make them more visible. The same could probably be achieved by
> documentation/tutorials.
>
> Best, Kai
>
> On 10/07/2010 09:30 AM, Radu Bogdan Rusu wrote:
> > Talking to Bastian a few days ago, it occurred to us that maybe the pcl::PointCloud<T>  class could contain more helper
> > methods, such as computeCentroid, or getMinMax, etc.
> >
> > Right now, most of these methods reside either in specialized classes/files (computeCentroid is part of
> > libpcl_features), or dumped in various headers in pcl/common/*.h
> >
> > What do people prefer? Is:
> >
> > * cloud.getCentroid (centroid)
> >
> > better/easier/more convenient than
> >
> > * #include<blabla>
> > * computeCentroid (cloud, centroid)
> >
> > ?
> >
>
> _______________________________________________
> [hidden email] / http://pcl.ros.org
> https://code.ros.org/mailman/listinfo/pcl-users
>

--
Lorenz Mösenlechner            | [hidden email]
Technische Universität München | Boltzmannstr. 3
85748 Garching bei München     | Germany
http://ias.cs.tum.edu/         | Tel: +49 (89) 289-26910
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: more helper methods in pcl::PointCloud

Radu B. Rusu
Administrator
Agreed, we need to think this through before changing most of PCL and the code that depends on it.

I would be in favor for tiny general purpose methods to go into PointCloud like getMinMax, computeCentroid, etc... but
where do you draw the line?

Should we argue that no additional library/header can be used in any such methods (besides Eigen which is already deep
embedded into our PointT structures)?

Cheers,
Radu.


On 10/07/2010 01:38 PM, Lorenz Mösenlechner wrote:

> Hi,
>
> actually, I don't like the idea of helper methods in the point cloud
> class too much. From the point of view of clean design, I belief that
> the helper methods are semantically unrelated to the point cloud class
> itself.
>
> And why do I want to compile all that functionality when all I want to
> do is just publish a point cloud or so?
>
> How would you specialize such methods for specific point types? When
> having it in a method, it might be possible to just specialize the
> helper method on specific types, in the proposed class member way I
> don't see a chance to do that.
>
> Please think over the change again. It might not be as good as it
> seems at the beginning although, I admit, it might be convenient in
> some situations.
>
> Lorenz
>
>> Hi Radu,
>>
>> the functionality is obviously the same. However, if you do not know all
>> the tricks one can do with a PointCloud, those convenience methods would
>> make them more visible. The same could probably be achieved by
>> documentation/tutorials.
>>
>> Best, Kai
>>
>> On 10/07/2010 09:30 AM, Radu Bogdan Rusu wrote:
>>> Talking to Bastian a few days ago, it occurred to us that maybe the pcl::PointCloud<T>   class could contain more helper
>>> methods, such as computeCentroid, or getMinMax, etc.
>>>
>>> Right now, most of these methods reside either in specialized classes/files (computeCentroid is part of
>>> libpcl_features), or dumped in various headers in pcl/common/*.h
>>>
>>> What do people prefer? Is:
>>>
>>> * cloud.getCentroid (centroid)
>>>
>>> better/easier/more convenient than
>>>
>>> * #include<blabla>
>>> * computeCentroid (cloud, centroid)
>>>
>>> ?
>>>
>>
>> _______________________________________________
>> [hidden email] / http://pcl.ros.org
>> https://code.ros.org/mailman/listinfo/pcl-users
>>
>
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: more helper methods in pcl::PointCloud

Marius Muja-2
In reply to this post by Lorenz Mösenlechner
I agree with Lorenz here, from a clean design point of view it's better not to have the helper methods in the point cloud class.

What would be the centroid of a point cloud where the PointT datatype does not have x,y,z ?

Marius

2010/10/7 Lorenz Mösenlechner <[hidden email]>
Hi,

actually, I don't like the idea of helper methods in the point cloud
class too much. From the point of view of clean design, I belief that
the helper methods are semantically unrelated to the point cloud class
itself.

And why do I want to compile all that functionality when all I want to
do is just publish a point cloud or so?

How would you specialize such methods for specific point types? When
having it in a method, it might be possible to just specialize the
helper method on specific types, in the proposed class member way I
don't see a chance to do that.

Please think over the change again. It might not be as good as it
seems at the beginning although, I admit, it might be convenient in
some situations.

Lorenz

> Hi Radu,
>
> the functionality is obviously the same. However, if you do not know all
> the tricks one can do with a PointCloud, those convenience methods would
> make them more visible. The same could probably be achieved by
> documentation/tutorials.
>
> Best, Kai
>
> On 10/07/2010 09:30 AM, Radu Bogdan Rusu wrote:
> > Talking to Bastian a few days ago, it occurred to us that maybe the pcl::PointCloud<T>  class could contain more helper
> > methods, such as computeCentroid, or getMinMax, etc.
> >
> > Right now, most of these methods reside either in specialized classes/files (computeCentroid is part of
> > libpcl_features), or dumped in various headers in pcl/common/*.h
> >
> > What do people prefer? Is:
> >
> > * cloud.getCentroid (centroid)
> >
> > better/easier/more convenient than
> >
> > * #include<blabla>
> > * computeCentroid (cloud, centroid)
> >
> > ?
> >
>
> _______________________________________________
> [hidden email] / http://pcl.ros.org
> https://code.ros.org/mailman/listinfo/pcl-users
>

--
Lorenz Mösenlechner            | [hidden email]
Technische Universität München | Boltzmannstr. 3
85748 Garching bei München     | Germany
http://ias.cs.tum.edu/         | Tel: +49 (89) 289-26910
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users


_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: more helper methods in pcl::PointCloud

Radu B. Rusu
Administrator
Hehe, if we could only decouple the methods defined in the class into two parts:

#include <point_cloud.h>
#include <point_cloud_statistics.h>

;)

so cloud.computeCentroid () will only work if you include the latter. Ah, C++.

Right, so it seems that we can't find a compromise to get these things in the main class. How do we make sure that they
are easily accessible? Having them in 20 different header files is confusing too, as now people have to import all these.

OpenCV's way of including EVERYTHING into one <cv.h> file is also not desirable. In fact we want to avoid the OpenCV
mistakes, which is hurting a lot these days due to its backwards compatibility problems. This is also slow if you use
templates.

What we want is:
  * a way for people to easily access these convenience methods
  * stop them in reinventing the wheel, especially since our methods might get more iterations from several developers
and might be more optimal/optimized.


So is better documentation the answer? If so, what are the suggested changes?

Cheers,
Radu.


On 10/07/2010 02:06 PM, Marius Muja wrote:

> I agree with Lorenz here, from a clean design point of view it's better
> not to have the helper methods in the point cloud class.
>
> What would be the centroid of a point cloud where the PointT datatype
> does not have x,y,z ?
>
> Marius
>
> 2010/10/7 Lorenz Mösenlechner <[hidden email]
> <mailto:[hidden email]>>
>
>     Hi,
>
>     actually, I don't like the idea of helper methods in the point cloud
>     class too much. From the point of view of clean design, I belief that
>     the helper methods are semantically unrelated to the point cloud class
>     itself.
>
>     And why do I want to compile all that functionality when all I want to
>     do is just publish a point cloud or so?
>
>     How would you specialize such methods for specific point types? When
>     having it in a method, it might be possible to just specialize the
>     helper method on specific types, in the proposed class member way I
>     don't see a chance to do that.
>
>     Please think over the change again. It might not be as good as it
>     seems at the beginning although, I admit, it might be convenient in
>     some situations.
>
>     Lorenz
>
>      > Hi Radu,
>      >
>      > the functionality is obviously the same. However, if you do not
>     know all
>      > the tricks one can do with a PointCloud, those convenience
>     methods would
>      > make them more visible. The same could probably be achieved by
>      > documentation/tutorials.
>      >
>      > Best, Kai
>      >
>      > On 10/07/2010 09:30 AM, Radu Bogdan Rusu wrote:
>      > > Talking to Bastian a few days ago, it occurred to us that maybe
>     the pcl::PointCloud<T>  class could contain more helper
>      > > methods, such as computeCentroid, or getMinMax, etc.
>      > >
>      > > Right now, most of these methods reside either in specialized
>     classes/files (computeCentroid is part of
>      > > libpcl_features), or dumped in various headers in pcl/common/*.h
>      > >
>      > > What do people prefer? Is:
>      > >
>      > > * cloud.getCentroid (centroid)
>      > >
>      > > better/easier/more convenient than
>      > >
>      > > * #include<blabla>
>      > > * computeCentroid (cloud, centroid)
>      > >
>      > > ?
>      > >
>      >
>      > _______________________________________________
>      > [hidden email] <mailto:[hidden email]> /
>     http://pcl.ros.org
>      > https://code.ros.org/mailman/listinfo/pcl-users
>      >
>
>     --
>     Lorenz Mösenlechner            | [hidden email]
>     <mailto:[hidden email]>
>     Technische Universität München | Boltzmannstr. 3
>     85748 Garching bei München     | Germany
>     http://ias.cs.tum.edu/         | Tel: +49 (89) 289-26910
>     _______________________________________________
>     [hidden email] <mailto:[hidden email]> /
>     http://pcl.ros.org
>     https://code.ros.org/mailman/listinfo/pcl-users
>
>
>
>
> _______________________________________________
> [hidden email] / http://pcl.ros.org
> https://code.ros.org/mailman/listinfo/pcl-users
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: more helper methods in pcl::PointCloud

garratt
I would also like to have the helper functions in the class.
It would greatly add to visibility (I too, develop in eclipse, and like
to explore a class using auto-complete)

>
>  From the point of view of clean design, I belief that
> the helper methods are semantically unrelated to the point cloud class
> itself.
Actually, I think cloud.getCentroid is very semantically related.

>
> And why do I want to compile all that functionality when all I want to
> do is just publish a point cloud or so?
A) I think the point of PCL (and of ROS as a whole) is that we are NOT just publishing point clouds.
PCL and ROS make the tradeoff of being bulky in exchange for including a very diverse set of utilities, which I for one appreciate.
Also, I fail to see 'added compile time' as a bigger factor that ease of interface.

> How would you specialize such methods for specific point types? When
> having it in a method, it might be possible to just specialize the
> helper method on specific types, in the proposed class member way I
> don't see a chance to do that.
Most of the functions I've seen kicked around as examples work on all the current point types. (except maybe normals, but I would propose doing away with the normals point type)
I'll start a new thread for that discussion - I've been meaning to anyway...


> I would be in favor for tiny general purpose methods to go into PointCloud like getMinMax, computeCentroid, etc... but
> where do you draw the line?

Radu, could you list the functions that you are considering making part
of the main class?  That would help put things in perspective, and allow everyone to discuss where the line needs to be drawn.

cheers
Garratt




On Thu, 2010-10-07 at 14:56 -0700, Radu Bogdan Rusu wrote:

> Hehe, if we could only decouple the methods defined in the class into two parts:
>
> #include <point_cloud.h>
> #include <point_cloud_statistics.h>
>
> ;)
>
> so cloud.computeCentroid () will only work if you include the latter. Ah, C++.
>
> Right, so it seems that we can't find a compromise to get these things in the main class. How do we make sure that they
> are easily accessible? Having them in 20 different header files is confusing too, as now people have to import all these.
>
> OpenCV's way of including EVERYTHING into one <cv.h> file is also not desirable. In fact we want to avoid the OpenCV
> mistakes, which is hurting a lot these days due to its backwards compatibility problems. This is also slow if you use
> templates.
>
> What we want is:
>   * a way for people to easily access these convenience methods
>   * stop them in reinventing the wheel, especially since our methods might get more iterations from several developers
> and might be more optimal/optimized.
>
>
> So is better documentation the answer? If so, what are the suggested changes?
>
> Cheers,
> Radu.
>
>
> On 10/07/2010 02:06 PM, Marius Muja wrote:
> > I agree with Lorenz here, from a clean design point of view it's better
> > not to have the helper methods in the point cloud class.
> >
> > What would be the centroid of a point cloud where the PointT datatype
> > does not have x,y,z ?
> >
> > Marius
> >
> > 2010/10/7 Lorenz Mösenlechner <[hidden email]
> > <mailto:[hidden email]>>
> >
> >     Hi,
> >
> >     actually, I don't like the idea of helper methods in the point cloud
> >     class too much. From the point of view of clean design, I belief that
> >     the helper methods are semantically unrelated to the point cloud class
> >     itself.
> >
> >     And why do I want to compile all that functionality when all I want to
> >     do is just publish a point cloud or so?
> >
> >     How would you specialize such methods for specific point types? When
> >     having it in a method, it might be possible to just specialize the
> >     helper method on specific types, in the proposed class member way I
> >     don't see a chance to do that.
> >
> >     Please think over the change again. It might not be as good as it
> >     seems at the beginning although, I admit, it might be convenient in
> >     some situations.
> >
> >     Lorenz
> >
> >      > Hi Radu,
> >      >
> >      > the functionality is obviously the same. However, if you do not
> >     know all
> >      > the tricks one can do with a PointCloud, those convenience
> >     methods would
> >      > make them more visible. The same could probably be achieved by
> >      > documentation/tutorials.
> >      >
> >      > Best, Kai
> >      >
> >      > On 10/07/2010 09:30 AM, Radu Bogdan Rusu wrote:
> >      > > Talking to Bastian a few days ago, it occurred to us that maybe
> >     the pcl::PointCloud<T>  class could contain more helper
> >      > > methods, such as computeCentroid, or getMinMax, etc.
> >      > >
> >      > > Right now, most of these methods reside either in specialized
> >     classes/files (computeCentroid is part of
> >      > > libpcl_features), or dumped in various headers in pcl/common/*.h
> >      > >
> >      > > What do people prefer? Is:
> >      > >
> >      > > * cloud.getCentroid (centroid)
> >      > >
> >      > > better/easier/more convenient than
> >      > >
> >      > > * #include<blabla>
> >      > > * computeCentroid (cloud, centroid)
> >      > >
> >      > > ?
> >      > >
> >      >
> >      > _______________________________________________
> >      > [hidden email] <mailto:[hidden email]> /
> >     http://pcl.ros.org
> >      > https://code.ros.org/mailman/listinfo/pcl-users
> >      >
> >
> >     --
> >     Lorenz Mösenlechner            | [hidden email]
> >     <mailto:[hidden email]>
> >     Technische Universität München | Boltzmannstr. 3
> >     85748 Garching bei München     | Germany
> >     http://ias.cs.tum.edu/         | Tel: +49 (89) 289-26910
> >     _______________________________________________
> >     [hidden email] <mailto:[hidden email]> /
> >     http://pcl.ros.org
> >     https://code.ros.org/mailman/listinfo/pcl-users
> >
> >
> >
> >
> > _______________________________________________
> > [hidden email] / http://pcl.ros.org
> > https://code.ros.org/mailman/listinfo/pcl-users
> _______________________________________________
> [hidden email] / http://pcl.ros.org
> https://code.ros.org/mailman/listinfo/pcl-users


_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: more helper methods in pcl::PointCloud

Jack O'Quin
In reply to this post by Radu B. Rusu
On Thu, Oct 7, 2010 at 4:56 PM, Radu Bogdan Rusu <[hidden email]> wrote:
> Hehe, if we could only decouple the methods defined in the class into two parts:
>
> #include <point_cloud.h>
> #include <point_cloud_statistics.h>
>
> ;)
>
> so cloud.computeCentroid () will only work if you include the latter. Ah, C++.

I agree that a more extensible, modular packaging is highly desirable.

I don't see any requirement to use class.method() syntax. I suppose
that could possibly be done using derived classes, but hardly seems
worthwhile, especially in the presence of all that Template<> gorp.

> Right, so it seems that we can't find a compromise to get these things in the main class. How do we make sure that they
> are easily accessible? Having them in 20 different header files is confusing too, as now people have to import all these.

How about several header files that declare some related helper
functions as a group? Things that might logically be used together.

#include <point_cloud.h>
#include <point_cloud_statistics.h>

pcl::statistics::computeCentroid(cloud, ...);

--
 joq
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: more helper methods in pcl::PointCloud

Pitzer Benjamin (CR/RTC1.1-NA)
In reply to this post by Radu B. Rusu
I second the thought of NOT including the helper methods into the pcl::PointCloud<T> class. There is a lot of helpers I can think of and it would not be very clean to include only some of them and others not. In fact, most serious CV libraries don't include helpers at all in their image classes.

My suggestion is clean structure of helper classes, e.g.:

PointCloudFunctor
        |
        --> StatisticFunctor
                |
                --> Centroid
                |
                --> Bounds
                |
                --> GaussianMixtureModel
        |
        --> FilterFunctor
        |
        --> FeatureExtractor
        |
        ...

I think, an intuitive class hierarchy along with a good documentation makes more sense than clogging the pcl::PointCloud<T> class. An example for a good documentation is the ltilib:
http://ltilib.sourceforge.net/doc/html/classHierarchy.html

Cheers,
Ben


-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Radu Bogdan Rusu
Sent: Thursday, October 07, 2010 2:57 PM
To: Point Cloud Library (PCL) mailing list
Subject: Re: [PCL-users] more helper methods in pcl::PointCloud

Hehe, if we could only decouple the methods defined in the class into two parts:

#include <point_cloud.h>
#include <point_cloud_statistics.h>

;)

so cloud.computeCentroid () will only work if you include the latter. Ah, C++.

Right, so it seems that we can't find a compromise to get these things in the main class. How do we make sure that they
are easily accessible? Having them in 20 different header files is confusing too, as now people have to import all these.

OpenCV's way of including EVERYTHING into one <cv.h> file is also not desirable. In fact we want to avoid the OpenCV
mistakes, which is hurting a lot these days due to its backwards compatibility problems. This is also slow if you use
templates.

What we want is:
  * a way for people to easily access these convenience methods
  * stop them in reinventing the wheel, especially since our methods might get more iterations from several developers
and might be more optimal/optimized.


So is better documentation the answer? If so, what are the suggested changes?

Cheers,
Radu.


On 10/07/2010 02:06 PM, Marius Muja wrote:

> I agree with Lorenz here, from a clean design point of view it's better
> not to have the helper methods in the point cloud class.
>
> What would be the centroid of a point cloud where the PointT datatype
> does not have x,y,z ?
>
> Marius
>
> 2010/10/7 Lorenz Mösenlechner <[hidden email]
> <mailto:[hidden email]>>
>
>     Hi,
>
>     actually, I don't like the idea of helper methods in the point cloud
>     class too much. From the point of view of clean design, I belief that
>     the helper methods are semantically unrelated to the point cloud class
>     itself.
>
>     And why do I want to compile all that functionality when all I want to
>     do is just publish a point cloud or so?
>
>     How would you specialize such methods for specific point types? When
>     having it in a method, it might be possible to just specialize the
>     helper method on specific types, in the proposed class member way I
>     don't see a chance to do that.
>
>     Please think over the change again. It might not be as good as it
>     seems at the beginning although, I admit, it might be convenient in
>     some situations.
>
>     Lorenz
>
>      > Hi Radu,
>      >
>      > the functionality is obviously the same. However, if you do not
>     know all
>      > the tricks one can do with a PointCloud, those convenience
>     methods would
>      > make them more visible. The same could probably be achieved by
>      > documentation/tutorials.
>      >
>      > Best, Kai
>      >
>      > On 10/07/2010 09:30 AM, Radu Bogdan Rusu wrote:
>      > > Talking to Bastian a few days ago, it occurred to us that maybe
>     the pcl::PointCloud<T>  class could contain more helper
>      > > methods, such as computeCentroid, or getMinMax, etc.
>      > >
>      > > Right now, most of these methods reside either in specialized
>     classes/files (computeCentroid is part of
>      > > libpcl_features), or dumped in various headers in pcl/common/*.h
>      > >
>      > > What do people prefer? Is:
>      > >
>      > > * cloud.getCentroid (centroid)
>      > >
>      > > better/easier/more convenient than
>      > >
>      > > * #include<blabla>
>      > > * computeCentroid (cloud, centroid)
>      > >
>      > > ?
>      > >
>      >
>      > _______________________________________________
>      > [hidden email] <mailto:[hidden email]> /
>     http://pcl.ros.org
>      > https://code.ros.org/mailman/listinfo/pcl-users
>      >
>
>     --
>     Lorenz Mösenlechner            | [hidden email]
>     <mailto:[hidden email]>
>     Technische Universität München | Boltzmannstr. 3
>     85748 Garching bei München     | Germany
>     http://ias.cs.tum.edu/         | Tel: +49 (89) 289-26910
>     _______________________________________________
>     [hidden email] <mailto:[hidden email]> /
>     http://pcl.ros.org
>     https://code.ros.org/mailman/listinfo/pcl-users
>
>
>
>
> _______________________________________________
> [hidden email] / http://pcl.ros.org
> https://code.ros.org/mailman/listinfo/pcl-users
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: more helper methods in pcl::PointCloud

Radu B. Rusu
Administrator
Ben, interesting...

So would this make the FilterFunctor a child of PointCloud? That is,

* PointCloud would include only the basic data structures (width, height, data[], ...)
* FilterFunctor would inherit all those, and add extra method helpers?


Right now, the Filter class is separate, and accepts PointCloud data as input via setInputCloud (). Both ways seem
interesting though.


Would Centroid in this case become a class by itself? Is that too much overhead or is that fine?


Bastian, Kai, and the folks at Freiburg have been using a similar PointCloud class that included a lot of functionality
in it, but it was not templated, which made the code compilation quite fast. This is what spawned the thread.

The PCL PointCloud class right now is at the other extreme, where partly due to templates, we kept the code into
different headers/modules (Features -> libpcl_features, Filters -> libpcl_filters, etc), each individually as simple as
possible.


The main problem that I see right now is not a complete feature or filter, but more these tiny little helper methods,
which so far we have not built as separate classes. Examples

* compute3DCentroid
* getMinMax
* computeCovarianceMatrix
* etc.


As Garratt suggested, we can look at them individually, and either:

* put them in the base PointCloud<T> class, though it seems that the general consensus is to avoid such practices, due to:
    a) not knowing where to draw the line - PointCloud<T> could easily become a hog;
    b) PointCloud being a container such as Images in OpenCV, etc
    c) people not wanting to wait for all these methods to compile if their code is not using them, etc

* leave them as they are in separate header files, and improve the documentation on how to find and access them

* group them together in a smarter way, as Jack suggested (is sub-namespacing the solution? - we used to have this in
the old point_cloud_mapping library - the grandfather of PCL)

* ... ?

Cheers,
Radu.


On 10/07/2010 03:56 PM, Pitzer Benjamin (CR/RTC1.1-NA) wrote:

> I second the thought of NOT including the helper methods into the pcl::PointCloud<T>  class. There is a lot of helpers I can think of and it would not be very clean to include only some of them and others not. In fact, most serious CV libraries don't include helpers at all in their image classes.
>
> My suggestion is clean structure of helper classes, e.g.:
>
> PointCloudFunctor
>          |
>          -->  StatisticFunctor
>                  |
>                  -->  Centroid
>                  |
>                  -->  Bounds
>                  |
>                  -->  GaussianMixtureModel
>          |
>          -->  FilterFunctor
>          |
>          -->  FeatureExtractor
>          |
>          ...
>
> I think, an intuitive class hierarchy along with a good documentation makes more sense than clogging the pcl::PointCloud<T>  class. An example for a good documentation is the ltilib:
> http://ltilib.sourceforge.net/doc/html/classHierarchy.html
>
> Cheers,
> Ben
>
>
> -----Original Message-----
> From: [hidden email] [mailto:[hidden email]] On Behalf Of Radu Bogdan Rusu
> Sent: Thursday, October 07, 2010 2:57 PM
> To: Point Cloud Library (PCL) mailing list
> Subject: Re: [PCL-users] more helper methods in pcl::PointCloud
>
> Hehe, if we could only decouple the methods defined in the class into two parts:
>
> #include<point_cloud.h>
> #include<point_cloud_statistics.h>
>
> ;)
>
> so cloud.computeCentroid () will only work if you include the latter. Ah, C++.
>
> Right, so it seems that we can't find a compromise to get these things in the main class. How do we make sure that they
> are easily accessible? Having them in 20 different header files is confusing too, as now people have to import all these.
>
> OpenCV's way of including EVERYTHING into one<cv.h>  file is also not desirable. In fact we want to avoid the OpenCV
> mistakes, which is hurting a lot these days due to its backwards compatibility problems. This is also slow if you use
> templates.
>
> What we want is:
>    * a way for people to easily access these convenience methods
>    * stop them in reinventing the wheel, especially since our methods might get more iterations from several developers
> and might be more optimal/optimized.
>
>
> So is better documentation the answer? If so, what are the suggested changes?
>
> Cheers,
> Radu.
>
>
> On 10/07/2010 02:06 PM, Marius Muja wrote:
>> I agree with Lorenz here, from a clean design point of view it's better
>> not to have the helper methods in the point cloud class.
>>
>> What would be the centroid of a point cloud where the PointT datatype
>> does not have x,y,z ?
>>
>> Marius
>>
>> 2010/10/7 Lorenz Mösenlechner<[hidden email]
>> <mailto:[hidden email]>>
>>
>>      Hi,
>>
>>      actually, I don't like the idea of helper methods in the point cloud
>>      class too much. From the point of view of clean design, I belief that
>>      the helper methods are semantically unrelated to the point cloud class
>>      itself.
>>
>>      And why do I want to compile all that functionality when all I want to
>>      do is just publish a point cloud or so?
>>
>>      How would you specialize such methods for specific point types? When
>>      having it in a method, it might be possible to just specialize the
>>      helper method on specific types, in the proposed class member way I
>>      don't see a chance to do that.
>>
>>      Please think over the change again. It might not be as good as it
>>      seems at the beginning although, I admit, it might be convenient in
>>      some situations.
>>
>>      Lorenz
>>
>>       >  Hi Radu,
>>       >
>>       >  the functionality is obviously the same. However, if you do not
>>      know all
>>       >  the tricks one can do with a PointCloud, those convenience
>>      methods would
>>       >  make them more visible. The same could probably be achieved by
>>       >  documentation/tutorials.
>>       >
>>       >  Best, Kai
>>       >
>>       >  On 10/07/2010 09:30 AM, Radu Bogdan Rusu wrote:
>>       >  >  Talking to Bastian a few days ago, it occurred to us that maybe
>>      the pcl::PointCloud<T>   class could contain more helper
>>       >  >  methods, such as computeCentroid, or getMinMax, etc.
>>       >  >
>>       >  >  Right now, most of these methods reside either in specialized
>>      classes/files (computeCentroid is part of
>>       >  >  libpcl_features), or dumped in various headers in pcl/common/*.h
>>       >  >
>>       >  >  What do people prefer? Is:
>>       >  >
>>       >  >  * cloud.getCentroid (centroid)
>>       >  >
>>       >  >  better/easier/more convenient than
>>       >  >
>>       >  >  * #include<blabla>
>>       >  >  * computeCentroid (cloud, centroid)
>>       >  >
>>       >  >  ?
>>       >  >
>>       >
>>       >  _______________________________________________
>>       >  [hidden email]<mailto:[hidden email]>  /
>>      http://pcl.ros.org
>>       >  https://code.ros.org/mailman/listinfo/pcl-users
>>       >
>>
>>      --
>>      Lorenz Mösenlechner            | [hidden email]
>>      <mailto:[hidden email]>
>>      Technische Universität München | Boltzmannstr. 3
>>      85748 Garching bei München     | Germany
>>      http://ias.cs.tum.edu/         | Tel: +49 (89) 289-26910
>>      _______________________________________________
>>      [hidden email]<mailto:[hidden email]>  /
>>      http://pcl.ros.org
>>      https://code.ros.org/mailman/listinfo/pcl-users
>>
>>
>>
>>
>> _______________________________________________
>> [hidden email] / http://pcl.ros.org
>> https://code.ros.org/mailman/listinfo/pcl-users
> _______________________________________________
> [hidden email] / http://pcl.ros.org
> https://code.ros.org/mailman/listinfo/pcl-users
> _______________________________________________
> [hidden email] / http://pcl.ros.org
> https://code.ros.org/mailman/listinfo/pcl-users
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: more helper methods in pcl::PointCloud

Radu B. Rusu
Administrator
In reply to this post by Lorenz Mösenlechner



On 10/07/2010 01:38 PM, Lorenz Mösenlechner wrote:
> Hi,
>
> actually, I don't like the idea of helper methods in the point cloud
> class too much. From the point of view of clean design, I belief that
> the helper methods are semantically unrelated to the point cloud class
> itself.

New thought:

pcl::fromROSMsg and pcl::toROSMsg.

They reside in a weird pcl/ros/conversions.h header right now.

* does it make sense to move them in the base pcl::PointCloud<T> class?

* should we add a constructor for pcl::PointCloud<T> (const sensor_msgs::PointCloud2::ConstPtr &) which is pretty much
equivalent with fromROSMsg ?

...any other ideas?

Cheers,
Radu.
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: more helper methods in pcl::PointCloud

Kai M. Wurm
Hi,

moving msg (ROS) related stuff to the PC class voids the idea of
pcl_ros, doesn't it? What about the stand-alone version then?

Best, Kai

On 10/10/2010 10:45 PM, Radu Bogdan Rusu wrote:

>
>
>
> On 10/07/2010 01:38 PM, Lorenz Mösenlechner wrote:
>> Hi,
>>
>> actually, I don't like the idea of helper methods in the point cloud
>> class too much. From the point of view of clean design, I belief that
>> the helper methods are semantically unrelated to the point cloud class
>> itself.
>
> New thought:
>
> pcl::fromROSMsg and pcl::toROSMsg.
>
> They reside in a weird pcl/ros/conversions.h header right now.
>
> * does it make sense to move them in the base pcl::PointCloud<T>  class?
>
> * should we add a constructor for pcl::PointCloud<T>  (const sensor_msgs::PointCloud2::ConstPtr&) which is pretty much
> equivalent with fromROSMsg ?
>
> ...any other ideas?
>
> Cheers,
> Radu.
> _______________________________________________
> [hidden email] / http://pcl.ros.org
> https://code.ros.org/mailman/listinfo/pcl-users

_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: more helper methods in pcl::PointCloud

Radu B. Rusu
Administrator
The standalone version is extracting the PointCloud2 message and keeping it as a simple struct in a header. It's basically our only way to hold any hyperdimensional point clouds.

The two methods perform the conversion to/from nD binary blob arrays (PointCloud2) and specific point cloud structures (pcl::PointCloud2). We can rename them to something else if their current names are misleading. ;)

Cheers,
Radu.

On Oct 11, 2010, at 9:37 AM, "Kai M. Wurm" <[hidden email]> wrote:

> Hi,
>
> moving msg (ROS) related stuff to the PC class voids the idea of
> pcl_ros, doesn't it? What about the stand-alone version then?
>
> Best, Kai
>
> On 10/10/2010 10:45 PM, Radu Bogdan Rusu wrote:
>>
>>
>>
>> On 10/07/2010 01:38 PM, Lorenz Mösenlechner wrote:
>>> Hi,
>>>
>>> actually, I don't like the idea of helper methods in the point cloud
>>> class too much. From the point of view of clean design, I belief that
>>> the helper methods are semantically unrelated to the point cloud class
>>> itself.
>>
>> New thought:
>>
>> pcl::fromROSMsg and pcl::toROSMsg.
>>
>> They reside in a weird pcl/ros/conversions.h header right now.
>>
>> * does it make sense to move them in the base pcl::PointCloud<T>  class?
>>
>> * should we add a constructor for pcl::PointCloud<T>  (const sensor_msgs::PointCloud2::ConstPtr&) which is pretty much
>> equivalent with fromROSMsg ?
>>
>> ...any other ideas?
>>
>> Cheers,
>> Radu.
>> _______________________________________________
>> [hidden email] / http://pcl.ros.org
>> https://code.ros.org/mailman/listinfo/pcl-users
>
> _______________________________________________
> [hidden email] / http://pcl.ros.org
> https://code.ros.org/mailman/listinfo/pcl-users
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: more helper methods in pcl::PointCloud

Radu B. Rusu
Administrator

On Oct 11, 2010, at 9:45 AM, Radu Bogdan Rusu <[hidden email]> wrote:

> The two methods perform the conversion to/from nD binary blob arrays (PointCloud2) and specific point cloud structures (pcl::PointCloud2).

I obviously meant pcl::PointCloud<T> :) need more coffee.

_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users