anyone using loadPCD/savePCD?

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

anyone using loadPCD/savePCD?

Radu B. Rusu
Administrator
Is there anyone explicitly using the loadPCD/savePCD methods?

We would like to hide them inside the PCDReader/PCDWriter classes, because we want to modify their definitions slightly.
Basically if you're using them, can you switch from:

loadPCD (...);

PCDReader foo;
foo.read (...);

? And similar for PCDWriter.


The thing that we want to add right now is viewpoint (sensor origin + orientation) information inside a PCD file. We
already added this to the pcl::PointCloud<T> class as follows (not committed yet):

144       /** \brief Sensor acquisition pose (origin/translation). */
145       Eigen3::Vector4f    sensor_origin_;
146       /** \brief Sensor acquisition pose (rotation). */
147       Eigen3::Quaternionf sensor_orientation_;

Without an acquisition viewpoint, it will be horribly hard to keep track of transforms and so on outside of ROS (no TF
love there).


If no one has anything to object, we'll make these changes in trunk.

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

Re: anyone using loadPCD/savePCD?

garratt
On Fri, 2010-12-03 at 18:16 -0800, Radu Bogdan Rusu wrote:
> Is there anyone explicitly using the loadPCD/savePCD methods?

are you talking about:
pcl::io::loadPCDFile() ?

because I do use that a lot.

>
> We would like to hide them inside the PCDReader/PCDWriter classes, because we want to modify their definitions slightly.
> Basically if you're using them, can you switch from:
>
> loadPCD (...);
>
> PCDReader foo;
> foo.read (...);
>
> ? And similar for PCDWriter.
>

Not to be questioning everything, but why does the reader need to be a
class?  Aren't there only the two functions?  I very much prefer a one
call interface, and I usually end up making some wrapper function that
goes:

void loadPCD(&cloud){
        PCDReader foo;
        foo.read (...);
}
 


>
> The thing that we want to add right now is viewpoint (sensor origin + orientation) information inside a PCD file. We
> already added this to the pcl::PointCloud<T> class as follows (not committed yet):
>
> 144       /** \brief Sensor acquisition pose (origin/translation). */
> 145       Eigen3::Vector4f    sensor_origin_;
> 146       /** \brief Sensor acquisition pose (rotation). */
> 147       Eigen3::Quaternionf sensor_orientation_;
>
> Without an acquisition viewpoint, it will be horribly hard to keep track of transforms and so on outside of ROS (no TF
> love there).
>

I've given a good deal of thought to this matter. It is particularly
relevant, because I often take multiple point clouds that were taken at
different viewpoints and compare them, merge them, and take their
normals.

Most uses of point clouds (at least based on my usage) don't involve
multiple viewpoints.  Many of those uses (such as finding normals, and
making occupancy grids) would really benefit from having the transform
of the point cloud stored in the cloud structure.

However, when I merge multiple clouds with different viewpoints, I find
it valuable to have the pcl::PointWithViewpoint point type, so I can
merge the clouds, downsample, remove outliers, and then find normals
using the whole cloud, but still flip the viewpoint to the correct
orientation by knowing the correct viewpoint for each point.  

The reason for writing this whole spiel is not to protest adding the
sensor origin to the point cloud structure (I welcome it). However, the
logical next step to putting the viewpoint at the cloud level is to
remove the PointWithViewpoint point type.  I wish to make a preemptive
argument against such an action (maybe we can put a comment in the
source code as to why this soon-to-be redundant feature still exists).

That said, onward with progress! Also, is there any chance
geometry_msgs::PointCloud2 could also store the sensor origin?  Since it
would probably break all bagfiles, I would be cool with this not getting
rolled in until Eturtle, or whenever we're breaking things anyway. ;)

cheers
Garratt



>
> If no one has anything to object, we'll make these changes in trunk.
>
> Thanks,
> Radu.
> --
> http://pointclouds.org
> _______________________________________________
> [hidden email] / http://pointclouds.org
> https://code.ros.org/mailman/listinfo/pcl-users


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

Re: anyone using loadPCD/savePCD?

Radu B. Rusu
Administrator

On 12/03/2010 07:40 PM, Garratt wrote:
> On Fri, 2010-12-03 at 18:16 -0800, Radu Bogdan Rusu wrote:
>> Is there anyone explicitly using the loadPCD/savePCD methods?
>
> are you talking about:
> pcl::io::loadPCDFile() ?
>
> because I do use that a lot.

Yup. The reason for either hiding it is that we need to change its signature now, and I'm wondering whether we would
need to do the same (again) in the future, thus breaking existing code :) Overloading is fine, but here's the problem
that we're tackling now...

Because sensor_msgs/PointCloud2 is a bit dumb :) -- sorry I meant to say "frozen" and we can't change the API for
D-Turtle (or can we, Tully?) -- it doesn't store any information about its acquisition origin. In ROS, we use TF for
everything, but outside of ROS, there's no TF (yet)... so we need to propagate this information. I think this issue
didn't come up until now because PCL didn't exist outside ROS.

We made loadPCDFile return a PointCloud that contains all dimensions (the binary blob PointCloud2), but since
PointCloud2 has no place to store this information, we would have to change the signature from:

loadPCDFile (Path, &PointCloud2)
to
loadPCDFile (Path, &PointCloud2, &SensorOrigin)

The other alternative would be to overload loadPCDFile, but then this will lead confusion as to why doesn't the
pcl::PointCloud<T> read from a PCD file through a PointCloud2 contain the correct sensor origin information.


If we discover in a few months that we need to change this again, that would be bad.


the loadPCDFile<T> (Path, &pcl::PointCloud<T>) uses
loadPCDFile (Path, &PointCloud2) + fromROSMsg (PointCloud2, pcl::PointCloud<T>)


> The reason for writing this whole spiel is not to protest adding the
> sensor origin to the point cloud structure (I welcome it). However, the
> logical next step to putting the viewpoint at the cloud level is to
> remove the PointWithViewpoint point type.  I wish to make a preemptive
> argument against such an action (maybe we can put a comment in the
> source code as to why this soon-to-be redundant feature still exists).

Hehe - yeah - I support your argument. We wont remove the PointWithViewpoint anytime soon. The sensor origin in
pcl::PointCloud just adds a nice way to transform datasets and render them with PCLVisualization, etc.

> That said, onward with progress! Also, is there any chance
> geometry_msgs::PointCloud2 could also store the sensor origin?  Since it
> would probably break all bagfiles, I would be cool with this not getting
> rolled in until Eturtle, or whenever we're breaking things anyway. ;)

I'll leave Tully reply this one.

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

Re: anyone using loadPCD/savePCD?

Radu B. Rusu
Administrator
In reply to this post by garratt
Garratt,

I pushed these changes to r34536 in trunk. Can you please help test them?

I kept the backwards compatible loadPCD, except it will generate a warning while attempting to read a PCD v.7 file. As
previously mentioned, PCD v.7 headers now contain

VIEWPOINT tx ty tz qw qx qy qz

Because sensor_msgs/PointCloud2 doesn't have these, I overloaded pcl::io::loadPCD with:

loadPCDFile (const std::string &file_name, sensor_msgs::PointCloud2 &cloud, Eigen3::Vector4f &origin,
Eigen3::Quaternionf &orientation);

Hopefully there aren't any API breaking changes... but just in case, let's double check. Also, PCDReader/PCDWriter now
contain the actual code, the C methods are just wrappers for them.

Cheers,
Radu.
--
http://pointclouds.org

On 12/03/2010 07:40 PM, Garratt wrote:

> On Fri, 2010-12-03 at 18:16 -0800, Radu Bogdan Rusu wrote:
>> Is there anyone explicitly using the loadPCD/savePCD methods?
>
> are you talking about:
> pcl::io::loadPCDFile() ?
>
> because I do use that a lot.
>
>>
>> We would like to hide them inside the PCDReader/PCDWriter classes, because we want to modify their definitions slightly.
>> Basically if you're using them, can you switch from:
>>
>> loadPCD (...);
>>
>> PCDReader foo;
>> foo.read (...);
>>
>> ? And similar for PCDWriter.
>>
>
> Not to be questioning everything, but why does the reader need to be a
> class?  Aren't there only the two functions?  I very much prefer a one
> call interface, and I usually end up making some wrapper function that
> goes:
>
> void loadPCD(&cloud){
> PCDReader foo;
> foo.read (...);
> }
>
>
>
>>
>> The thing that we want to add right now is viewpoint (sensor origin + orientation) information inside a PCD file. We
>> already added this to the pcl::PointCloud<T>  class as follows (not committed yet):
>>
>> 144       /** \brief Sensor acquisition pose (origin/translation). */
>> 145       Eigen3::Vector4f    sensor_origin_;
>> 146       /** \brief Sensor acquisition pose (rotation). */
>> 147       Eigen3::Quaternionf sensor_orientation_;
>>
>> Without an acquisition viewpoint, it will be horribly hard to keep track of transforms and so on outside of ROS (no TF
>> love there).
>>
>
> I've given a good deal of thought to this matter. It is particularly
> relevant, because I often take multiple point clouds that were taken at
> different viewpoints and compare them, merge them, and take their
> normals.
>
> Most uses of point clouds (at least based on my usage) don't involve
> multiple viewpoints.  Many of those uses (such as finding normals, and
> making occupancy grids) would really benefit from having the transform
> of the point cloud stored in the cloud structure.
>
> However, when I merge multiple clouds with different viewpoints, I find
> it valuable to have the pcl::PointWithViewpoint point type, so I can
> merge the clouds, downsample, remove outliers, and then find normals
> using the whole cloud, but still flip the viewpoint to the correct
> orientation by knowing the correct viewpoint for each point.
>
> The reason for writing this whole spiel is not to protest adding the
> sensor origin to the point cloud structure (I welcome it). However, the
> logical next step to putting the viewpoint at the cloud level is to
> remove the PointWithViewpoint point type.  I wish to make a preemptive
> argument against such an action (maybe we can put a comment in the
> source code as to why this soon-to-be redundant feature still exists).
>
> That said, onward with progress! Also, is there any chance
> geometry_msgs::PointCloud2 could also store the sensor origin?  Since it
> would probably break all bagfiles, I would be cool with this not getting
> rolled in until Eturtle, or whenever we're breaking things anyway. ;)
>
> cheers
> Garratt
>
>
>
>>
>> If no one has anything to object, we'll make these changes in trunk.
>>
>> Thanks,
>> Radu.
>> --
>> http://pointclouds.org
>> _______________________________________________
>> [hidden email] / http://pointclouds.org
>> https://code.ros.org/mailman/listinfo/pcl-users
>
>
_______________________________________________
[hidden email] / http://pointclouds.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: anyone using loadPCD/savePCD?

garratt-2
Sure, I'll update the machines tomorrow morning, and let you know how
things go.

Garratt



On Sun, 2010-12-05 at 15:57 -0800, Radu Bogdan Rusu wrote:

> Garratt,
>
> I pushed these changes to r34536 in trunk. Can you please help test them?
>
> I kept the backwards compatible loadPCD, except it will generate a warning while attempting to read a PCD v.7 file. As
> previously mentioned, PCD v.7 headers now contain
>
> VIEWPOINT tx ty tz qw qx qy qz
>
> Because sensor_msgs/PointCloud2 doesn't have these, I overloaded pcl::io::loadPCD with:
>
> loadPCDFile (const std::string &file_name, sensor_msgs::PointCloud2 &cloud, Eigen3::Vector4f &origin,
> Eigen3::Quaternionf &orientation);
>
> Hopefully there aren't any API breaking changes... but just in case, let's double check. Also, PCDReader/PCDWriter now
> contain the actual code, the C methods are just wrappers for them.
>
> Cheers,
> Radu.
> --
> http://pointclouds.org
>
> On 12/03/2010 07:40 PM, Garratt wrote:
> > On Fri, 2010-12-03 at 18:16 -0800, Radu Bogdan Rusu wrote:
> >> Is there anyone explicitly using the loadPCD/savePCD methods?
> >
> > are you talking about:
> > pcl::io::loadPCDFile() ?
> >
> > because I do use that a lot.
> >
> >>
> >> We would like to hide them inside the PCDReader/PCDWriter classes, because we want to modify their definitions slightly.
> >> Basically if you're using them, can you switch from:
> >>
> >> loadPCD (...);
> >>
> >> PCDReader foo;
> >> foo.read (...);
> >>
> >> ? And similar for PCDWriter.
> >>
> >
> > Not to be questioning everything, but why does the reader need to be a
> > class?  Aren't there only the two functions?  I very much prefer a one
> > call interface, and I usually end up making some wrapper function that
> > goes:
> >
> > void loadPCD(&cloud){
> > PCDReader foo;
> > foo.read (...);
> > }
> >
> >
> >
> >>
> >> The thing that we want to add right now is viewpoint (sensor origin + orientation) information inside a PCD file. We
> >> already added this to the pcl::PointCloud<T>  class as follows (not committed yet):
> >>
> >> 144       /** \brief Sensor acquisition pose (origin/translation). */
> >> 145       Eigen3::Vector4f    sensor_origin_;
> >> 146       /** \brief Sensor acquisition pose (rotation). */
> >> 147       Eigen3::Quaternionf sensor_orientation_;
> >>
> >> Without an acquisition viewpoint, it will be horribly hard to keep track of transforms and so on outside of ROS (no TF
> >> love there).
> >>
> >
> > I've given a good deal of thought to this matter. It is particularly
> > relevant, because I often take multiple point clouds that were taken at
> > different viewpoints and compare them, merge them, and take their
> > normals.
> >
> > Most uses of point clouds (at least based on my usage) don't involve
> > multiple viewpoints.  Many of those uses (such as finding normals, and
> > making occupancy grids) would really benefit from having the transform
> > of the point cloud stored in the cloud structure.
> >
> > However, when I merge multiple clouds with different viewpoints, I find
> > it valuable to have the pcl::PointWithViewpoint point type, so I can
> > merge the clouds, downsample, remove outliers, and then find normals
> > using the whole cloud, but still flip the viewpoint to the correct
> > orientation by knowing the correct viewpoint for each point.
> >
> > The reason for writing this whole spiel is not to protest adding the
> > sensor origin to the point cloud structure (I welcome it). However, the
> > logical next step to putting the viewpoint at the cloud level is to
> > remove the PointWithViewpoint point type.  I wish to make a preemptive
> > argument against such an action (maybe we can put a comment in the
> > source code as to why this soon-to-be redundant feature still exists).
> >
> > That said, onward with progress! Also, is there any chance
> > geometry_msgs::PointCloud2 could also store the sensor origin?  Since it
> > would probably break all bagfiles, I would be cool with this not getting
> > rolled in until Eturtle, or whenever we're breaking things anyway. ;)
> >
> > cheers
> > Garratt
> >
> >
> >
> >>
> >> If no one has anything to object, we'll make these changes in trunk.
> >>
> >> Thanks,
> >> Radu.
> >> --
> >> http://pointclouds.org
> >> _______________________________________________
> >> [hidden email] / http://pointclouds.org
> >> https://code.ros.org/mailman/listinfo/pcl-users
> >
> >
> _______________________________________________
> [hidden email] / http://pointclouds.org
> https://code.ros.org/mailman/listinfo/pcl-users


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

Re: anyone using loadPCD/savePCD?

Dejan Pangercic
Sorry for late reply, we@TUM also use those 2 functions heavily but
are however in favor of pushing them into classes in order to get
viewpoint info.
I'll then test tomorrow too.
D.

On Mon, Dec 6, 2010 at 5:08 AM, Garratt Gallagher <[hidden email]> wrote:

> Sure, I'll update the machines tomorrow morning, and let you know how
> things go.
>
> Garratt
>
>
>
> On Sun, 2010-12-05 at 15:57 -0800, Radu Bogdan Rusu wrote:
>> Garratt,
>>
>> I pushed these changes to r34536 in trunk. Can you please help test them?
>>
>> I kept the backwards compatible loadPCD, except it will generate a warning while attempting to read a PCD v.7 file. As
>> previously mentioned, PCD v.7 headers now contain
>>
>> VIEWPOINT tx ty tz qw qx qy qz
>>
>> Because sensor_msgs/PointCloud2 doesn't have these, I overloaded pcl::io::loadPCD with:
>>
>> loadPCDFile (const std::string &file_name, sensor_msgs::PointCloud2 &cloud, Eigen3::Vector4f &origin,
>> Eigen3::Quaternionf &orientation);
>>
>> Hopefully there aren't any API breaking changes... but just in case, let's double check. Also, PCDReader/PCDWriter now
>> contain the actual code, the C methods are just wrappers for them.
>>
>> Cheers,
>> Radu.
>> --
>> http://pointclouds.org
>>
>> On 12/03/2010 07:40 PM, Garratt wrote:
>> > On Fri, 2010-12-03 at 18:16 -0800, Radu Bogdan Rusu wrote:
>> >> Is there anyone explicitly using the loadPCD/savePCD methods?
>> >
>> > are you talking about:
>> > pcl::io::loadPCDFile() ?
>> >
>> > because I do use that a lot.
>> >
>> >>
>> >> We would like to hide them inside the PCDReader/PCDWriter classes, because we want to modify their definitions slightly.
>> >> Basically if you're using them, can you switch from:
>> >>
>> >> loadPCD (...);
>> >>
>> >> PCDReader foo;
>> >> foo.read (...);
>> >>
>> >> ? And similar for PCDWriter.
>> >>
>> >
>> > Not to be questioning everything, but why does the reader need to be a
>> > class?  Aren't there only the two functions?  I very much prefer a one
>> > call interface, and I usually end up making some wrapper function that
>> > goes:
>> >
>> > void loadPCD(&cloud){
>> >     PCDReader foo;
>> >     foo.read (...);
>> > }
>> >
>> >
>> >
>> >>
>> >> The thing that we want to add right now is viewpoint (sensor origin + orientation) information inside a PCD file. We
>> >> already added this to the pcl::PointCloud<T>  class as follows (not committed yet):
>> >>
>> >> 144       /** \brief Sensor acquisition pose (origin/translation). */
>> >> 145       Eigen3::Vector4f    sensor_origin_;
>> >> 146       /** \brief Sensor acquisition pose (rotation). */
>> >> 147       Eigen3::Quaternionf sensor_orientation_;
>> >>
>> >> Without an acquisition viewpoint, it will be horribly hard to keep track of transforms and so on outside of ROS (no TF
>> >> love there).
>> >>
>> >
>> > I've given a good deal of thought to this matter. It is particularly
>> > relevant, because I often take multiple point clouds that were taken at
>> > different viewpoints and compare them, merge them, and take their
>> > normals.
>> >
>> > Most uses of point clouds (at least based on my usage) don't involve
>> > multiple viewpoints.  Many of those uses (such as finding normals, and
>> > making occupancy grids) would really benefit from having the transform
>> > of the point cloud stored in the cloud structure.
>> >
>> > However, when I merge multiple clouds with different viewpoints, I find
>> > it valuable to have the pcl::PointWithViewpoint point type, so I can
>> > merge the clouds, downsample, remove outliers, and then find normals
>> > using the whole cloud, but still flip the viewpoint to the correct
>> > orientation by knowing the correct viewpoint for each point.
>> >
>> > The reason for writing this whole spiel is not to protest adding the
>> > sensor origin to the point cloud structure (I welcome it). However, the
>> > logical next step to putting the viewpoint at the cloud level is to
>> > remove the PointWithViewpoint point type.  I wish to make a preemptive
>> > argument against such an action (maybe we can put a comment in the
>> > source code as to why this soon-to-be redundant feature still exists).
>> >
>> > That said, onward with progress! Also, is there any chance
>> > geometry_msgs::PointCloud2 could also store the sensor origin?  Since it
>> > would probably break all bagfiles, I would be cool with this not getting
>> > rolled in until Eturtle, or whenever we're breaking things anyway. ;)
>> >
>> > cheers
>> > Garratt
>> >
>> >
>> >
>> >>
>> >> If no one has anything to object, we'll make these changes in trunk.
>> >>
>> >> Thanks,
>> >> Radu.
>> >> --
>> >> http://pointclouds.org
>> >> _______________________________________________
>> >> [hidden email] / http://pointclouds.org
>> >> https://code.ros.org/mailman/listinfo/pcl-users
>> >
>> >
>> _______________________________________________
>> [hidden email] / http://pointclouds.org
>> https://code.ros.org/mailman/listinfo/pcl-users
>
>
> _______________________________________________
> [hidden email] / http://pointclouds.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://pointclouds.org
https://code.ros.org/mailman/listinfo/pcl-users