PCL standalone and 0.3.x+

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

PCL standalone and 0.3.x+

Radu B. Rusu
Administrator
One of our major goals right now is to "stabilize" PCL, in order to get closer to a 1.0 release in Diamondback. This
will not include the entire point_cloud_perception stack, which contains other packages (some being more experimental)
as well.

Because we want to introduce a version that won't require major API changes in Diamondback, in order to be able to
support it for some time, we made an internal API breaking change in PCL 0.3.0, switching from Eigen 2.x to Eigen 3.x.

(in case you're not familiar with Eigen, Eigen is our backend for most of the matrix-vector optimizations in PCL)


We apologize for this API change, but we felt it was necessary to bite the bullet and do it now, rather than cry later
after we release 1.0, and we realize that Eigen 3.x is better, faster, supported, etc. What does this mean?

1) we introduced a ROS package called "eigen3", which resides along "eigen" (which uses eigen2)

2) due to the way the rosbuild system works ("with great flexibility comes great ... ?" :) ), we had to patch eigen3, by
changing the namespace from Eigen:: to Eigen3:: and even the include directories from include/Eigen/ to include/Eigen3/,
otherwise a package that depends on other packages that import both "eigen" and "eigen3" would blow

3) we need to change all Eigen:: code to Eigen3:: now in all your packages, in order to be compatible with these new
changes. I talked to some of my colleagues that use "eigen" in other research fields and ROS packages, and we will try
to change those packages too for Diamondback



Now, we're also working hard on providing an easy way for PCL to be extracted from ROS and used as a standalone library.
This is the first time a library is being pulled from ROS, so we'll probably need to iterate a few times to get it
right. I won't go into details on how that's being done right now (some part of the script is already committed to the
repository, some not - we're still working on it), but the bottom line is this:

- the libraries that PCL depends on (cminpack, ANN, FLANN) can be simply downloaded and installed and PCL will function
with them

- because of the above mentioned API change in ROS, simply downloading Eigen will not work. We can easily handle the
namespace change, but not the include/Eigen -> include/Eigen3 issue, unless when we extract PCL, our script will do a
rename again from Eigen3 to Eigen, thus reverting the ROS changes

- however, there might be a few patches that we unfortunately had to do for certain 3rd party libraries in ROS (eigen3
is one example, but there are others too)


With this in mind, what should we do?

1) should we provide PCL as a .tgz/.tbz - and tell users "please download these set of libraries from their respective
homepages and deal with the installation process"?

2) should we provide PCL _AND_ all the libraries it depends on as .TGZ/.TBZ files to include our ROS patches and to
simplify the users job?

3) should we just wait until the ROS build tools are available outside ROS?


Any feedback is highly appreciated. Thanks!

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: PCL standalone and 0.3.x+

Jack O'Quin
On Thu, Sep 23, 2010 at 1:55 PM, Radu Bogdan Rusu <[hidden email]> wrote:

> Because we want to introduce a version that won't require major API changes in Diamondback, in order to be able to
> support it for some time, we made an internal API breaking change in PCL 0.3.0, switching from Eigen 2.x to Eigen 3.x.
>
> (in case you're not familiar with Eigen, Eigen is our backend for most of the matrix-vector optimizations in PCL)
>
>
> We apologize for this API change, but we felt it was necessary to bite the bullet and do it now, rather than cry later
> after we release 1.0, and we realize that Eigen 3.x is better, faster, supported, etc. What does this mean?
>
> 1) we introduced a ROS package called "eigen3", which resides along "eigen" (which uses eigen2)
>
> 2) due to the way the rosbuild system works ("with great flexibility comes great ... ?" :) ), we had to patch eigen3, by
> changing the namespace from Eigen:: to Eigen3:: and even the include directories from include/Eigen/ to include/Eigen3/,
> otherwise a package that depends on other packages that import both "eigen" and "eigen3" would blow

Do the eigen 2 and 3 libraries not coexist peacefully? That seems like
a problem than the upstream Eigen developers should be addressing.
(Maybe they should have renamed all the interfaces.)

Why does rosbuild force this change? Is it something unique to the ROS
environment?

Many of the other problems you mention seem to stem from this rather
massive interface change. If the problem exists upstream as well, they
should support it, too. If it is unique to ROS build, then fixes to
the ROS build system should be considered as an alternative.
--
 joq
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: PCL standalone and 0.3.x+

Radu B. Rusu
Administrator
Jack,

The eigen 2 and 3 libraries do _not_ coexist peacefully. Since we have a good relationship with the eigen developers we
could ask for _some_ stuff, but in general it will be hard for us to dictate the development of other 3rd party
libraries. And every now and then, they might break the API, even if in minimal ways (you can see how eigen3 affected
our code at http://www.ros.org/wiki/eigen3).

The reason of eigen2 and eigen3 not co-existing lies in their unique nature of being templated header-only libraries
with lots of macro definitions. Since in ROS, a package exports its cpp flags via a manifest.xml, both eigen and eigen3
now are exporting the same include/Eigen, which will result in a mess for another ROS package that imports both (via
upstream imports or directly).

It would be nice if we could experiment with something that would allow us to solve issues like this, and PCL seems to
have introduced this problem with 0.3.0.

Cheers,
Radu.


On 09/23/2010 01:13 PM, Jack O'Quin wrote:

> On Thu, Sep 23, 2010 at 1:55 PM, Radu Bogdan Rusu<[hidden email]>  wrote:
>
>> Because we want to introduce a version that won't require major API changes in Diamondback, in order to be able to
>> support it for some time, we made an internal API breaking change in PCL 0.3.0, switching from Eigen 2.x to Eigen 3.x.
>>
>> (in case you're not familiar with Eigen, Eigen is our backend for most of the matrix-vector optimizations in PCL)
>>
>>
>> We apologize for this API change, but we felt it was necessary to bite the bullet and do it now, rather than cry later
>> after we release 1.0, and we realize that Eigen 3.x is better, faster, supported, etc. What does this mean?
>>
>> 1) we introduced a ROS package called "eigen3", which resides along "eigen" (which uses eigen2)
>>
>> 2) due to the way the rosbuild system works ("with great flexibility comes great ... ?" :) ), we had to patch eigen3, by
>> changing the namespace from Eigen:: to Eigen3:: and even the include directories from include/Eigen/ to include/Eigen3/,
>> otherwise a package that depends on other packages that import both "eigen" and "eigen3" would blow
>
> Do the eigen 2 and 3 libraries not coexist peacefully? That seems like
> a problem than the upstream Eigen developers should be addressing.
> (Maybe they should have renamed all the interfaces.)
>
> Why does rosbuild force this change? Is it something unique to the ROS
> environment?
>
> Many of the other problems you mention seem to stem from this rather
> massive interface change. If the problem exists upstream as well, they
> should support it, too. If it is unique to ROS build, then fixes to
> the ROS build system should be considered as an alternative.
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: PCL standalone and 0.3.x+

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

the problem with eigen3 was that there are packages that not only pull
in eigen2 by depending on a package that depends on eigen2 but also
packages that include eigen2 headers and are included by files that
also require pcl. The first case (packages that depend on packages
depending on eigen2) can be solved easily by using different include
directories. The latter case requires the full renaming of the
namespace plus at least some #ifdndef lines and the corresponding
definitions.

All in all, I don't see a better solution than our patch. For a
stand-alone version of PCL outside the ROS infrastructure, we could
provide a separate wrapper package that provides namespace Eigen3 and
contains the directory include/Eigen3 that is either a symbolic link
to an existing installation of eigen or with header files that just
include the existing eigen headers.

The best solution is, of course, to change all packages in ROS to use
eigen3. But this is not possible anymore since cturtle has already
been released and I guess new PCL releases should be available for
cturtle, too.

Lorenz

> One of our major goals right now is to "stabilize" PCL, in order to get closer to a 1.0 release in Diamondback. This
> will not include the entire point_cloud_perception stack, which contains other packages (some being more experimental)
> as well.
>
> Because we want to introduce a version that won't require major API changes in Diamondback, in order to be able to
> support it for some time, we made an internal API breaking change in PCL 0.3.0, switching from Eigen 2.x to Eigen 3.x.
>
> (in case you're not familiar with Eigen, Eigen is our backend for most of the matrix-vector optimizations in PCL)
>
>
> We apologize for this API change, but we felt it was necessary to bite the bullet and do it now, rather than cry later
> after we release 1.0, and we realize that Eigen 3.x is better, faster, supported, etc. What does this mean?
>
> 1) we introduced a ROS package called "eigen3", which resides along "eigen" (which uses eigen2)
>
> 2) due to the way the rosbuild system works ("with great flexibility comes great ... ?" :) ), we had to patch eigen3, by
> changing the namespace from Eigen:: to Eigen3:: and even the include directories from include/Eigen/ to include/Eigen3/,
> otherwise a package that depends on other packages that import both "eigen" and "eigen3" would blow
>
> 3) we need to change all Eigen:: code to Eigen3:: now in all your packages, in order to be compatible with these new
> changes. I talked to some of my colleagues that use "eigen" in other research fields and ROS packages, and we will try
> to change those packages too for Diamondback
>
>
>
> Now, we're also working hard on providing an easy way for PCL to be extracted from ROS and used as a standalone library.
> This is the first time a library is being pulled from ROS, so we'll probably need to iterate a few times to get it
> right. I won't go into details on how that's being done right now (some part of the script is already committed to the
> repository, some not - we're still working on it), but the bottom line is this:
>
> - the libraries that PCL depends on (cminpack, ANN, FLANN) can be simply downloaded and installed and PCL will function
> with them
>
> - because of the above mentioned API change in ROS, simply downloading Eigen will not work. We can easily handle the
> namespace change, but not the include/Eigen -> include/Eigen3 issue, unless when we extract PCL, our script will do a
> rename again from Eigen3 to Eigen, thus reverting the ROS changes
>
> - however, there might be a few patches that we unfortunately had to do for certain 3rd party libraries in ROS (eigen3
> is one example, but there are others too)
>
>
> With this in mind, what should we do?
>
> 1) should we provide PCL as a .tgz/.tbz - and tell users "please download these set of libraries from their respective
> homepages and deal with the installation process"?
>
> 2) should we provide PCL _AND_ all the libraries it depends on as .TGZ/.TBZ files to include our ROS patches and to
> simplify the users job?
>
> 3) should we just wait until the ROS build tools are available outside ROS?
>
>
> Any feedback is highly appreciated. Thanks!
>
> Cheers,
> Radu.
>
> _______________________________________________
> [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: PCL standalone and 0.3.x+

Radu B. Rusu
Administrator
Ok, so it seems that we're converging to option number 2.

 >> 2) should we provide PCL _AND_ all the libraries it depends on as .TGZ/.TBZ files to include our ROS patches and to
 >> simplify the users job?

Geoff, is that acceptable from your POV? A list of downloads + a tutorial on how to install them? I guess that's what
most libraries out there do anyway, no?

Cheers,
Radu.


On 09/24/2010 02:04 AM, Lorenz Mösenlechner wrote:

> Hi,
>
> the problem with eigen3 was that there are packages that not only pull
> in eigen2 by depending on a package that depends on eigen2 but also
> packages that include eigen2 headers and are included by files that
> also require pcl. The first case (packages that depend on packages
> depending on eigen2) can be solved easily by using different include
> directories. The latter case requires the full renaming of the
> namespace plus at least some #ifdndef lines and the corresponding
> definitions.
>
> All in all, I don't see a better solution than our patch. For a
> stand-alone version of PCL outside the ROS infrastructure, we could
> provide a separate wrapper package that provides namespace Eigen3 and
> contains the directory include/Eigen3 that is either a symbolic link
> to an existing installation of eigen or with header files that just
> include the existing eigen headers.
>
> The best solution is, of course, to change all packages in ROS to use
> eigen3. But this is not possible anymore since cturtle has already
> been released and I guess new PCL releases should be available for
> cturtle, too.
>
> Lorenz
>
>> One of our major goals right now is to "stabilize" PCL, in order to get closer to a 1.0 release in Diamondback. This
>> will not include the entire point_cloud_perception stack, which contains other packages (some being more experimental)
>> as well.
>>
>> Because we want to introduce a version that won't require major API changes in Diamondback, in order to be able to
>> support it for some time, we made an internal API breaking change in PCL 0.3.0, switching from Eigen 2.x to Eigen 3.x.
>>
>> (in case you're not familiar with Eigen, Eigen is our backend for most of the matrix-vector optimizations in PCL)
>>
>>
>> We apologize for this API change, but we felt it was necessary to bite the bullet and do it now, rather than cry later
>> after we release 1.0, and we realize that Eigen 3.x is better, faster, supported, etc. What does this mean?
>>
>> 1) we introduced a ROS package called "eigen3", which resides along "eigen" (which uses eigen2)
>>
>> 2) due to the way the rosbuild system works ("with great flexibility comes great ... ?" :) ), we had to patch eigen3, by
>> changing the namespace from Eigen:: to Eigen3:: and even the include directories from include/Eigen/ to include/Eigen3/,
>> otherwise a package that depends on other packages that import both "eigen" and "eigen3" would blow
>>
>> 3) we need to change all Eigen:: code to Eigen3:: now in all your packages, in order to be compatible with these new
>> changes. I talked to some of my colleagues that use "eigen" in other research fields and ROS packages, and we will try
>> to change those packages too for Diamondback
>>
>>
>>
>> Now, we're also working hard on providing an easy way for PCL to be extracted from ROS and used as a standalone library.
>> This is the first time a library is being pulled from ROS, so we'll probably need to iterate a few times to get it
>> right. I won't go into details on how that's being done right now (some part of the script is already committed to the
>> repository, some not - we're still working on it), but the bottom line is this:
>>
>> - the libraries that PCL depends on (cminpack, ANN, FLANN) can be simply downloaded and installed and PCL will function
>> with them
>>
>> - because of the above mentioned API change in ROS, simply downloading Eigen will not work. We can easily handle the
>> namespace change, but not the include/Eigen ->  include/Eigen3 issue, unless when we extract PCL, our script will do a
>> rename again from Eigen3 to Eigen, thus reverting the ROS changes
>>
>> - however, there might be a few patches that we unfortunately had to do for certain 3rd party libraries in ROS (eigen3
>> is one example, but there are others too)
>>
>>
>> With this in mind, what should we do?
>>
>> 1) should we provide PCL as a .tgz/.tbz - and tell users "please download these set of libraries from their respective
>> homepages and deal with the installation process"?
>>
>> 2) should we provide PCL _AND_ all the libraries it depends on as .TGZ/.TBZ files to include our ROS patches and to
>> simplify the users job?
>>
>> 3) should we just wait until the ROS build tools are available outside ROS?
>>
>>
>> Any feedback is highly appreciated. Thanks!
>>
>> 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: PCL standalone and 0.3.x+

Jack O'Quin
On Fri, Sep 24, 2010 at 10:28 AM, Radu Bogdan Rusu
<[hidden email]> wrote:
> Ok, so it seems that we're converging to option number 2.
>
>  >> 2) should we provide PCL _AND_ all the libraries it depends on as .TGZ/.TBZ files to include our ROS patches and to
>  >> simplify the users job?
>
> Geoff, is that acceptable from your POV? A list of downloads + a tutorial on how to install them? I guess that's what
> most libraries out there do anyway, no?

Most open source libraries do option 1), plus cmake or autoconf code
to check that the required dependencies are present and meet any
version requirements.
--
 joq
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: PCL standalone and 0.3.x+

Radu B. Rusu
Administrator
You're right Jack, but how should we deal with the patches that we apply to these libraries in ROS? Take bullet for
example - there's almost a dozen patches there. :-/

Cheers,
Radu.


On 09/24/2010 11:29 AM, Jack O'Quin wrote:

> On Fri, Sep 24, 2010 at 10:28 AM, Radu Bogdan Rusu
> <[hidden email]>  wrote:
>> Ok, so it seems that we're converging to option number 2.
>>
>>   >>  2) should we provide PCL _AND_ all the libraries it depends on as .TGZ/.TBZ files to include our ROS patches and to
>>   >>  simplify the users job?
>>
>> Geoff, is that acceptable from your POV? A list of downloads + a tutorial on how to install them? I guess that's what
>> most libraries out there do anyway, no?
>
> Most open source libraries do option 1), plus cmake or autoconf code
> to check that the required dependencies are present and meet any
> version requirements.
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: PCL standalone and 0.3.x+

Geoffrey Biggs
Yay, messages are coming! I can reply at last!

Option 1 is the ideal. It reduces the potential conflict with the rest
of the system on which the software is installed.

However, it is relatively common for a library that uses a *customised*
version of another library to include that source in its own and install
a local copy of it (in accordance with the license, of course).

Because of all the changes made to dependencies, I think that for ease
of use, option 2 is the best. Until the patches in question are applied
upstream, PCL should come as a single tarball with the dependencies
included within it as sub-directories, and those dependencies are built
as part of building PCL. They should probably be subtly renamed (add
"-pcl" to the end of installed files or something) so they don't clash
with existing installations of the upstream versions.

Note that I'm suggesting the dependencies be a part of PCL's build
process. It still needs to be easy for the user to build and install
PCL, and only needing a single make command is better than needing half
a dozen in the right order.

Geoff

On 25/09/10 07:15, Radu Bogdan Rusu wrote:

> You're right Jack, but how should we deal with the patches that we apply
> to these libraries in ROS? Take bullet for example - there's almost a
> dozen patches there. :-/
>
> Cheers,
> Radu.
>
>
> On 09/24/2010 11:29 AM, Jack O'Quin wrote:
>> On Fri, Sep 24, 2010 at 10:28 AM, Radu Bogdan Rusu
>> <[hidden email]> wrote:
>>> Ok, so it seems that we're converging to option number 2.
>>>
>>> >> 2) should we provide PCL _AND_ all the libraries it depends on as
>>> .TGZ/.TBZ files to include our ROS patches and to
>>> >> simplify the users job?
>>>
>>> Geoff, is that acceptable from your POV? A list of downloads + a
>>> tutorial on how to install them? I guess that's what
>>> most libraries out there do anyway, no?
>>
>> Most open source libraries do option 1), plus cmake or autoconf code
>> to check that the required dependencies are present and meet any
>> version requirements.
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: PCL standalone and 0.3.x+

Jack O'Quin
On Fri, Sep 24, 2010 at 7:30 PM, Geoffrey Biggs
<[hidden email]> wrote:
> Yay, messages are coming! I can reply at last!
>
> Option 1 is the ideal. It reduces the potential conflict with the rest
> of the system on which the software is installed.
>
> However, it is relatively common for a library that uses a *customised*
> version of another library to include that source in its own and install
> a local copy of it (in accordance with the license, of course).

That is why libraries avoid like the plague modifying their dependencies.

> Because of all the changes made to dependencies, I think that for ease
> of use, option 2 is the best. Until the patches in question are applied
> upstream, PCL should come as a single tarball with the dependencies
> included within it as sub-directories, and those dependencies are built
> as part of building PCL. They should probably be subtly renamed (add
> "-pcl" to the end of installed files or something) so they don't clash
> with existing installations of the upstream versions.

This direction leads to maintenance nightmares. Has upstream agreed to
accept these patches?

> On 25/09/10 07:15, Radu Bogdan Rusu wrote:
>> You're right Jack, but how should we deal with the patches that we apply
>> to these libraries in ROS? Take bullet for example - there's almost a
>> dozen patches there. :-/

That is why the bullet patches should be handled upstream and not by
us. The lack of peaceful coexistence is their mistake. They should fix
it.

Patching it and a bunch of users of it is not a reasonable solution.
What about others we don't patch? Eventually, the transitive closure
of the dependency graph gets out of hand.

None of the maintainers of these libraries are going to want to
support the patched versions. All bugs will have to be reproduced on
clean copies.

If I downloaded a package with heavily patched versions of other
libraries that were incompatible with the standard versions of those
libraries, I would not spend more than an hour cursing and swearing at
it before deciding to look elsewhere for a different package without
all that baggage. I would consider that hour as time wasted out of my
life.

This is not the way to achieve code reuse.
--
 joq
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: PCL standalone and 0.3.x+

Geoffrey Biggs
On 25/09/10 10:02, Jack O'Quin wrote:
> On Fri, Sep 24, 2010 at 7:30 PM, Geoffrey Biggs
> <[hidden email]>  wrote:
>> However, it is relatively common for a library that uses a *customised*
>> version of another library to include that source in its own and install
>> a local copy of it (in accordance with the license, of course).
>
> That is why libraries avoid like the plague modifying their dependencies.

Absolutely. If the dependency gets so bad that it becomes unusable, you
replace it with something else (or write your own if you have the time),
but it's best to work with the devs of the dependency to resolve issues
as much as possible before that point...

> This direction leads to maintenance nightmares. Has upstream agreed to
> accept these patches?

I agree it makes maintenance awful. I was working on the assumption that
the patches are necessary for PCL to function. Are they/

> If I downloaded a package with heavily patched versions of other
> libraries that were incompatible with the standard versions of those
> libraries, I would not spend more than an hour cursing and swearing at
> it before deciding to look elsewhere for a different package without
> all that baggage. I would consider that hour as time wasted out of my
> life.
>
> This is not the way to achieve code reuse.

I 100% agree with this statement. I think we need to resolve a related
issue first: are the patches necessary for PCL to function? Will
performance be completely lost without them, or just merely degraded in
certain situations?

ROS can apply patches like that because ROS is a distribution. It's a
meta-distribution, but still a distribution. In Gentoo, it is very
common for patches to be applied to the source during package
installation. They do things like changing the install directory of a
file to meet Gentoo's requirements, patching critical security problems
that upstream has not fixed in that particular version, or just fixing
bugs that the package maintainer wants fixed right now. These patches
may or may not be accepted by upstream at a later date, but applying
them now is the *distribution's* responsibility - not the package's own
internal build system.

Is it possible for PCL to be released as a standalone library using the
released versions of the dependencies, while the ROS version uses the
patched versions? Users wanting the improvements offered by the patches
would have the choice of using ROS, or patching the dependencies
manually using the (supplied) patch files - playing the part of
distribution manually.

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

Re: PCL standalone and 0.3.x+

Radu B. Rusu
Administrator
Jack, Geoffrey,

Thanks for the input given so far. All excellent and valid points.

Taken one by one, PCL currently depends on:
* TBB, QHull, Boost (all system dependencies)
* Eigen(3), CMinPack, ANN, FLANN (all from source - none of this can be installed as a package in any Linux distro)

The patches are:

1) Eigen(3)
- gcc43.patch: gcc 4.3 on ubuntu 9.04 fix (eigen fails to compile without it) -- could be patched upstream
- tridiag3x3.patch: SVD/eigenvalue decomposition fix for float matrices (eigen gives large errors for float matrices
while computing the correct solution for doubles by default) -- could be patched upstream
- warning.patch: nothing fancy, removing some compilation warnings -- could be patched upstream

- eigen3_namespace.patch: Eigen:: -> Eigen3:: rename as eigen->eigen3 is not compatible -- _CANNOT_ be patched upstream

2) CMinPack
- fPIC.patch: adds -fPIC -- could be patched upstream though Frederic wants a better solution than just adding that to
the library build, as -fPIC doesn't work in MacOS, etc ... so perhaps an OS check?
- shared.patch: builds the library as shared instead of static by default -- could potentially be patched upstream if we
could clean it up

3) ANN -- not sure if anything can be patched upstream as the library seems to be unmaintained, though I haven't checked
with the authors (will do that today)
- fPIC.patch: same story as CMinPack
- gcc43_shared.patch: same story as CMinPack
- distance.patch: changes the library's internals to operate on floats rather than doubles to speed up computation by a
large factor -- _CANNOT_ be patched upstream unless the authors make changes to their library

4) FLANN -- everything should be patcheable upstream as Marius works with us


So overall it seems that we have 2 patches that at first glance are hard to apply upstream. If we could find solutions
for these and for the rest, then we might be able to use option 1). Any volunteers willing to help?


Overall this might be a ROS problem as we don't seem to be doing a good job in sending our patches upstream. Bullet is a
horrible example with no less than 9 patches applied to the stock 2.76 version.

Cheers,
Radu.


On 09/24/2010 09:22 PM, Geoffrey Biggs wrote:

> On 25/09/10 10:02, Jack O'Quin wrote:
>> On Fri, Sep 24, 2010 at 7:30 PM, Geoffrey Biggs
>> <[hidden email]>   wrote:
>>> However, it is relatively common for a library that uses a *customised*
>>> version of another library to include that source in its own and install
>>> a local copy of it (in accordance with the license, of course).
>>
>> That is why libraries avoid like the plague modifying their dependencies.
>
> Absolutely. If the dependency gets so bad that it becomes unusable, you
> replace it with something else (or write your own if you have the time),
> but it's best to work with the devs of the dependency to resolve issues
> as much as possible before that point...
>
>> This direction leads to maintenance nightmares. Has upstream agreed to
>> accept these patches?
>
> I agree it makes maintenance awful. I was working on the assumption that
> the patches are necessary for PCL to function. Are they/
>
>> If I downloaded a package with heavily patched versions of other
>> libraries that were incompatible with the standard versions of those
>> libraries, I would not spend more than an hour cursing and swearing at
>> it before deciding to look elsewhere for a different package without
>> all that baggage. I would consider that hour as time wasted out of my
>> life.
>>
>> This is not the way to achieve code reuse.
>
> I 100% agree with this statement. I think we need to resolve a related
> issue first: are the patches necessary for PCL to function? Will
> performance be completely lost without them, or just merely degraded in
> certain situations?
>
> ROS can apply patches like that because ROS is a distribution. It's a
> meta-distribution, but still a distribution. In Gentoo, it is very
> common for patches to be applied to the source during package
> installation. They do things like changing the install directory of a
> file to meet Gentoo's requirements, patching critical security problems
> that upstream has not fixed in that particular version, or just fixing
> bugs that the package maintainer wants fixed right now. These patches
> may or may not be accepted by upstream at a later date, but applying
> them now is the *distribution's* responsibility - not the package's own
> internal build system.
>
> Is it possible for PCL to be released as a standalone library using the
> released versions of the dependencies, while the ROS version uses the
> patched versions? Users wanting the improvements offered by the patches
> would have the choice of using ROS, or patching the dependencies
> manually using the (supplied) patch files - playing the part of
> distribution manually.
>
> Geoff
> _______________________________________________
> [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: PCL standalone and 0.3.x+

Jack O'Quin
On Sat, Sep 25, 2010 at 12:16 PM, Radu Bogdan Rusu
<[hidden email]> wrote:

> The patches are:
>
> 1) Eigen(3)
> - gcc43.patch: gcc 4.3 on ubuntu 9.04 fix (eigen fails to compile without it) -- could be patched upstream
> - tridiag3x3.patch: SVD/eigenvalue decomposition fix for float matrices (eigen gives large errors for float matrices
> while computing the correct solution for doubles by default) -- could be patched upstream
> - warning.patch: nothing fancy, removing some compilation warnings -- could be patched upstream

I'd think upstream would want all these.

> - eigen3_namespace.patch: Eigen:: -> Eigen3:: rename as eigen->eigen3 is not compatible -- _CANNOT_ be patched upstream

This seems to be the sicking point. I don't know enough about Eigen to
understand the issue. What I've heard so far suggests that somehow the
Eigen developers made an incompatible API change that cannot be linked
with users of their own version 2 library.

That sounds like a mistake to me. However, we must deal with it. If
they are unwilling to fix this problem with either a namespace change
or some other trick, we need to make some hard choices.

Can we just say "use Eigen 3" and let the link-edit step fail if
someone tries to combine Eigen 2 libraries in the same executable?
Upstream has implicitly said "yes" to this by not fixing the problem.

If so, I'd suggest backing out the namespace patch and letting the
chips fall where they may. If it becomes enough of an issue, maybe
they'll fix it some day.

> 2) CMinPack
> - fPIC.patch: adds -fPIC -- could be patched upstream though Frederic wants a better solution than just adding that to
> the library build, as -fPIC doesn't work in MacOS, etc ... so perhaps an OS check?
> - shared.patch: builds the library as shared instead of static by default -- could potentially be patched upstream if we
> could clean it up

Or, we could ask them to fix it, and carry our patch in ROS until they do.

> 3) ANN -- not sure if anything can be patched upstream as the library seems to be unmaintained, though I haven't checked
> with the authors (will do that today)
> - fPIC.patch: same story as CMinPack
> - gcc43_shared.patch: same story as CMinPack
> - distance.patch: changes the library's internals to operate on floats rather than doubles to speed up computation by a
> large factor -- _CANNOT_ be patched upstream unless the authors make changes to their library

If the project is an orphan, we have two choices:

 (1) use something else (is FLANN an alternative?)
 (2) maintain it ourselves in the ROS repository (current status quo)

> 4) FLANN -- everything should be patcheable upstream as Marius works with us

Good!

> So overall it seems that we have 2 patches that at first glance are hard to apply upstream. If we could find solutions
> for these and for the rest, then we might be able to use option 1). Any volunteers willing to help?

I'd like to, but realistically I don't have time for any meaningful assistance.

> Overall this might be a ROS problem as we don't seem to be doing a good job in sending our patches upstream. Bullet is a
> horrible example with no less than 9 patches applied to the stock 2.76 version.

These things usually have to be handled on a case-by-case basis,
informed by judgement and experience. "Good judgement comes from
experience, and experience comes from bad judgement."  :-)

Maintaining good working relations with upstream developers is helpful
whenever possible. The ROS stage package, for example, has only two
small patches against the latest stage release, and one was copied
from the stage post-release SVN trunk.

In other cases, a close relationship may not be possible.
--
 joq
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: PCL standalone and 0.3.x+

Geoffrey Biggs
All the ones that can be applied upstream should be. A standalone
release of PCL should not care about them; it's up to the user to ensure
they provide correct dependencies if they're building from source.

As for the other two, see inline.

On 26/09/10 03:25, Jack O'Quin wrote:

>> - eigen3_namespace.patch: Eigen:: ->  Eigen3:: rename as eigen->eigen3 is not compatible -- _CANNOT_ be patched upstream
>
> This seems to be the sicking point. I don't know enough about Eigen to
> understand the issue. What I've heard so far suggests that somehow the
> Eigen developers made an incompatible API change that cannot be linked
> with users of their own version 2 library.
>
> That sounds like a mistake to me. However, we must deal with it. If
> they are unwilling to fix this problem with either a namespace change
> or some other trick, we need to make some hard choices.
>
> Can we just say "use Eigen 3" and let the link-edit step fail if
> someone tries to combine Eigen 2 libraries in the same executable?
> Upstream has implicitly said "yes" to this by not fixing the problem.
>
> If so, I'd suggest backing out the namespace patch and letting the
> chips fall where they may. If it becomes enough of an issue, maybe
> they'll fix it some day.

The reasons for this change are all ROS:

http://www.ros.org/wiki/eigen3

Making API-breaking changes between major versions is standard practice,
changing the namespace is not. Even though it makes things nicer for
users that use both versions of the library, those that only use one
version have to change all their code to use the new namespace. Either
way, someone loses.

I think that this is a distribution issue. ROS should apply a patch to
the PCL source that changes it to use the Eigen3 namespace in order to
comply with ROS's convention of Eigen usage, not the other way around.

>> 2) CMinPack
>> - fPIC.patch: adds -fPIC -- could be patched upstream though Frederic wants a better solution than just adding that to
>> the library build, as -fPIC doesn't work in MacOS, etc ... so perhaps an OS check?
>> - shared.patch: builds the library as shared instead of static by default -- could potentially be patched upstream if we
>> could clean it up
>
> Or, we could ask them to fix it, and carry our patch in ROS until they do.
>
>> 3) ANN -- not sure if anything can be patched upstream as the library seems to be unmaintained, though I haven't checked
>> with the authors (will do that today)
>> - fPIC.patch: same story as CMinPack
>> - gcc43_shared.patch: same story as CMinPack
>> - distance.patch: changes the library's internals to operate on floats rather than doubles to speed up computation by a
>> large factor -- _CANNOT_ be patched upstream unless the authors make changes to their library

If the library is no longer maintained, then we have to apply the
patches ourselves and provide our own release, either separately or as
part of PCL. This would solve the distance.patch problem because, by
becoming the maintainer, you can choose to make the necessary changes to
the library.

> If the project is an orphan, we have two choices:
>
>   (1) use something else (is FLANN an alternative?)
>   (2) maintain it ourselves in the ROS repository (current status quo)
>
>> 4) FLANN -- everything should be patcheable upstream as Marius works with us
>
> Good!
>
>> So overall it seems that we have 2 patches that at first glance are hard to apply upstream. If we could find solutions
>> for these and for the rest, then we might be able to use option 1). Any volunteers willing to help?

I can do a little. The CMake stuff is probably a good place for me to
work, to begin with.

>
> I'd like to, but realistically I don't have time for any meaningful assistance.
>
>> Overall this might be a ROS problem as we don't seem to be doing a good job in sending our patches upstream. Bullet is a
>> horrible example with no less than 9 patches applied to the stock 2.76 version.
>
> These things usually have to be handled on a case-by-case basis,
> informed by judgement and experience. "Good judgement comes from
> experience, and experience comes from bad judgement."  :-)
>
> Maintaining good working relations with upstream developers is helpful
> whenever possible. The ROS stage package, for example, has only two
> small patches against the latest stage release, and one was copied
> from the stage post-release SVN trunk.
>
> In other cases, a close relationship may not be possible.


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

Re: PCL standalone and 0.3.x+

Eric Perko


On Sat, Sep 25, 2010 at 8:11 PM, Geoffrey Biggs <[hidden email]> wrote:
All the ones that can be applied upstream should be. A standalone
release of PCL should not care about them; it's up to the user to ensure
they provide correct dependencies if they're building from source.

As for the other two, see inline.

On 26/09/10 03:25, Jack O'Quin wrote:
>> - eigen3_namespace.patch: Eigen:: ->  Eigen3:: rename as eigen->eigen3 is not compatible -- _CANNOT_ be patched upstream
>
> This seems to be the sicking point. I don't know enough about Eigen to
> understand the issue. What I've heard so far suggests that somehow the
> Eigen developers made an incompatible API change that cannot be linked
> with users of their own version 2 library.
>
> That sounds like a mistake to me. However, we must deal with it. If
> they are unwilling to fix this problem with either a namespace change
> or some other trick, we need to make some hard choices.
>
> Can we just say "use Eigen 3" and let the link-edit step fail if
> someone tries to combine Eigen 2 libraries in the same executable?
> Upstream has implicitly said "yes" to this by not fixing the problem.
>
> If so, I'd suggest backing out the namespace patch and letting the
> chips fall where they may. If it becomes enough of an issue, maybe
> they'll fix it some day.

The reasons for this change are all ROS:
Making API-breaking changes between major versions is standard practice,
changing the namespace is not. Even though it makes things nicer for
users that use both versions of the library, those that only use one
version have to change all their code to use the new namespace. Either
way, someone loses.

I think that this is a distribution issue. ROS should apply a patch to
the PCL source that changes it to use the Eigen3 namespace in order to
comply with ROS's convention of Eigen usage, not the other way around.

It wasn't really clear to me from reading that wiki page what the reasons for using both Eigen 2 and Eigen 3 at the same time are. Is it just that code written for Eigen 2 needs updated before being compatible with Eigen 3 or are there performance regressions or important features missing in Eigen 3? 

It seems to me that trying to use two different incompatible versions of a library at the same time is the real problem, not so much whether ROS should apply a patch to Eigen to support that.

- Eric
 

>> 2) CMinPack
>> - fPIC.patch: adds -fPIC -- could be patched upstream though Frederic wants a better solution than just adding that to
>> the library build, as -fPIC doesn't work in MacOS, etc ... so perhaps an OS check?
>> - shared.patch: builds the library as shared instead of static by default -- could potentially be patched upstream if we
>> could clean it up
>
> Or, we could ask them to fix it, and carry our patch in ROS until they do.
>
>> 3) ANN -- not sure if anything can be patched upstream as the library seems to be unmaintained, though I haven't checked
>> with the authors (will do that today)
>> - fPIC.patch: same story as CMinPack
>> - gcc43_shared.patch: same story as CMinPack
>> - distance.patch: changes the library's internals to operate on floats rather than doubles to speed up computation by a
>> large factor -- _CANNOT_ be patched upstream unless the authors make changes to their library

If the library is no longer maintained, then we have to apply the
patches ourselves and provide our own release, either separately or as
part of PCL. This would solve the distance.patch problem because, by
becoming the maintainer, you can choose to make the necessary changes to
the library.

> If the project is an orphan, we have two choices:
>
>   (1) use something else (is FLANN an alternative?)
>   (2) maintain it ourselves in the ROS repository (current status quo)
>
>> 4) FLANN -- everything should be patcheable upstream as Marius works with us
>
> Good!
>
>> So overall it seems that we have 2 patches that at first glance are hard to apply upstream. If we could find solutions
>> for these and for the rest, then we might be able to use option 1). Any volunteers willing to help?

I can do a little. The CMake stuff is probably a good place for me to
work, to begin with.

>
> I'd like to, but realistically I don't have time for any meaningful assistance.
>
>> Overall this might be a ROS problem as we don't seem to be doing a good job in sending our patches upstream. Bullet is a
>> horrible example with no less than 9 patches applied to the stock 2.76 version.
>
> These things usually have to be handled on a case-by-case basis,
> informed by judgement and experience. "Good judgement comes from
> experience, and experience comes from bad judgement."  :-)
>
> Maintaining good working relations with upstream developers is helpful
> whenever possible. The ROS stage package, for example, has only two
> small patches against the latest stage release, and one was copied
> from the stage post-release SVN trunk.
>
> In other cases, a close relationship may not be possible.


Geoff
_______________________________________________
[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: PCL standalone and 0.3.x+

Lorenz Mösenlechner
Hi,

the problem is that there are quite some packages in ROS that use
Eigen2. Eigen2 is part of the ROS CTurtle distribution, contained in
the geometry stack. Being part of CTurtle means that it cannot be
removed since CTurtle is already stable and big changes such as an
uprade to Eigen3 will not go into it.

That means we need to find a way to co-exist with Eigen2 while using
Eigen3.

Just depending on Eigen3 via the manifest file does not solve the
problem. As an example, consider you want to use KDL together with
PCL. Without our patch to Eigen3, you will run into compile problems
because KDL depends on Eigen2 and the include paths to both, Eigen2
and Eigen3 are added to the build by rospack.

Lorenz

> On Sat, Sep 25, 2010 at 8:11 PM, Geoffrey Biggs
> <[hidden email]>wrote:
>
> > All the ones that can be applied upstream should be. A standalone
> > release of PCL should not care about them; it's up to the user to ensure
> > they provide correct dependencies if they're building from source.
> >
> > As for the other two, see inline.
> >
> > On 26/09/10 03:25, Jack O'Quin wrote:
> > >> - eigen3_namespace.patch: Eigen:: ->  Eigen3:: rename as eigen->eigen3
> > is not compatible -- _CANNOT_ be patched upstream
> > >
> > > This seems to be the sicking point. I don't know enough about Eigen to
> > > understand the issue. What I've heard so far suggests that somehow the
> > > Eigen developers made an incompatible API change that cannot be linked
> > > with users of their own version 2 library.
> > >
> > > That sounds like a mistake to me. However, we must deal with it. If
> > > they are unwilling to fix this problem with either a namespace change
> > > or some other trick, we need to make some hard choices.
> > >
> > > Can we just say "use Eigen 3" and let the link-edit step fail if
> > > someone tries to combine Eigen 2 libraries in the same executable?
> > > Upstream has implicitly said "yes" to this by not fixing the problem.
> > >
> > > If so, I'd suggest backing out the namespace patch and letting the
> > > chips fall where they may. If it becomes enough of an issue, maybe
> > > they'll fix it some day.
> >
> > The reasons for this change are all ROS:
> >
> > http://www.ros.org/wiki/eigen3
> >
> > Making API-breaking changes between major versions is standard practice,
> > changing the namespace is not. Even though it makes things nicer for
> > users that use both versions of the library, those that only use one
> > version have to change all their code to use the new namespace. Either
> > way, someone loses.
> >
> > I think that this is a distribution issue. ROS should apply a patch to
> > the PCL source that changes it to use the Eigen3 namespace in order to
> > comply with ROS's convention of Eigen usage, not the other way around.
> >
>
> It wasn't really clear to me from reading that wiki page what the reasons
> for using both Eigen 2 and Eigen 3 at the same time are. Is it just that
> code written for Eigen 2 needs updated before being compatible with Eigen 3
> or are there performance regressions or important features missing in Eigen
> 3?
>
> It seems to me that trying to use two different incompatible versions of a
> library at the same time is the real problem, not so much whether ROS should
> apply a patch to Eigen to support that.
>
> - Eric
>
>
> >
> > >> 2) CMinPack
> > >> - fPIC.patch: adds -fPIC -- could be patched upstream though Frederic
> > wants a better solution than just adding that to
> > >> the library build, as -fPIC doesn't work in MacOS, etc ... so perhaps an
> > OS check?
> > >> - shared.patch: builds the library as shared instead of static by
> > default -- could potentially be patched upstream if we
> > >> could clean it up
> > >
> > > Or, we could ask them to fix it, and carry our patch in ROS until they
> > do.
> > >
> > >> 3) ANN -- not sure if anything can be patched upstream as the library
> > seems to be unmaintained, though I haven't checked
> > >> with the authors (will do that today)
> > >> - fPIC.patch: same story as CMinPack
> > >> - gcc43_shared.patch: same story as CMinPack
> > >> - distance.patch: changes the library's internals to operate on floats
> > rather than doubles to speed up computation by a
> > >> large factor -- _CANNOT_ be patched upstream unless the authors make
> > changes to their library
> >
> > If the library is no longer maintained, then we have to apply the
> > patches ourselves and provide our own release, either separately or as
> > part of PCL. This would solve the distance.patch problem because, by
> > becoming the maintainer, you can choose to make the necessary changes to
> > the library.
> >
> > > If the project is an orphan, we have two choices:
> > >
> > >   (1) use something else (is FLANN an alternative?)
> > >   (2) maintain it ourselves in the ROS repository (current status quo)
> > >
> > >> 4) FLANN -- everything should be patcheable upstream as Marius works
> > with us
> > >
> > > Good!
> > >
> > >> So overall it seems that we have 2 patches that at first glance are hard
> > to apply upstream. If we could find solutions
> > >> for these and for the rest, then we might be able to use option 1). Any
> > volunteers willing to help?
> >
> > I can do a little. The CMake stuff is probably a good place for me to
> > work, to begin with.
> >
> > >
> > > I'd like to, but realistically I don't have time for any meaningful
> > assistance.
> > >
> > >> Overall this might be a ROS problem as we don't seem to be doing a good
> > job in sending our patches upstream. Bullet is a
> > >> horrible example with no less than 9 patches applied to the stock 2.76
> > version.
> > >
> > > These things usually have to be handled on a case-by-case basis,
> > > informed by judgement and experience. "Good judgement comes from
> > > experience, and experience comes from bad judgement."  :-)
> > >
> > > Maintaining good working relations with upstream developers is helpful
> > > whenever possible. The ROS stage package, for example, has only two
> > > small patches against the latest stage release, and one was copied
> > > from the stage post-release SVN trunk.
> > >
> > > In other cases, a close relationship may not be possible.
> >
> >
> > Geoff
> > _______________________________________________
> > [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


--
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: PCL standalone and 0.3.x+

Radu B. Rusu
Administrator
In reply to this post by Eric Perko
Eric,

The problem occurs with the way ROS imports libraries. Here's a fictitious example:

eigen exports include/Eigen, namespace Eigen
eigen3 (without our patch) exports include Eigen, namespace Eigen

package A, depends on eigen
package B, depends on eigen2

package C, depends on package A and B
[boom! :)]

with the patch, package C can depend on package A and B, and even more:
Eigen3::Foo bar = Eigen::Foo (...);

will result in an error, which is precisely what we want. The only solution (let's take a Vector2d for example), is:
Eigen3::Vector2d foo;
Eigen::Vector2d bar;

foo[0] = bar[0];
foo[1] = bar[1];


I agree with you 100%, a package should never include both eigen and eigen3 directly.


Now, all these changes do not mean much in ROS, as we always patch 3rd party libraries heavily. We discussed this
already... take Gazebo (17 patches) and Bullet (9 patches) for example. ROS is a distribution, so as any other
distributions (e.g., Ubuntu) we can patch stuff so that it works better inside. One thing that we are currently
addressing with PCL at least, is to merge most of these changes upstream (we're working on doing that on 3/4
dependencies: FLANN, CMinpack and Eigen, and I sent an e-mail to the ANN people to see if they are still maintaining the
library).

The problem occurs when we want to decouple a software package like PCL, so that it can be used outside of ROS. If the
maintainers of the libraries that we depend on are not willing to accept our patches, then it might be that we need to
provide copies of these libraries along PCL, no?


Cheers,
Radu.


On 09/26/2010 01:30 PM, Eric Perko wrote:

>
> It wasn't really clear to me from reading that wiki page what the
> reasons for using both Eigen 2 and Eigen 3 at the same time are. Is it
> just that code written for Eigen 2 needs updated before being compatible
> with Eigen 3 or are there performance regressions or important features
> missing in Eigen 3?
>
> It seems to me that trying to use two different incompatible versions of
> a library at the same time is the real problem, not so much whether ROS
> should apply a patch to Eigen to support that.
>
> - Eric
_______________________________________________
[hidden email] / http://pcl.ros.org
https://code.ros.org/mailman/listinfo/pcl-users
Reply | Threaded
Open this post in threaded view
|

Re: PCL standalone and 0.3.x+

Radu B. Rusu
Administrator
In reply to this post by Geoffrey Biggs
Cool. Current status:

* Marius is working on patching FLANN upstream and providing a newer version today

* we contacted the ANN developers to see if they are still maintaining the library and whether they are willing to
accept our patches

* we'll contact Benoit and the Eigen developers today in an attempt to migrate some of the patches, and ask for a new
release. However this will not solve the Eigen3 renaming issue.

Geoff, one solution is to revert these Eigen3/Eigen changes when we build the PCL.tar.gz file. This might just work.
What do you think?


To do:
- need to clean CMinPack's patches and integrate the fPIC.patch and the shared.patch in their CMakeLists.txt on a per OS
basis. Any volunteers?

Cheers,
Radu.


On 09/25/2010 05:11 PM, Geoffrey Biggs wrote:

> All the ones that can be applied upstream should be. A standalone
> release of PCL should not care about them; it's up to the user to ensure
> they provide correct dependencies if they're building from source.
>
> As for the other two, see inline.
>
> On 26/09/10 03:25, Jack O'Quin wrote:
>>> - eigen3_namespace.patch: Eigen:: ->   Eigen3:: rename as eigen->eigen3 is not compatible -- _CANNOT_ be patched upstream
>>
>> This seems to be the sicking point. I don't know enough about Eigen to
>> understand the issue. What I've heard so far suggests that somehow the
>> Eigen developers made an incompatible API change that cannot be linked
>> with users of their own version 2 library.
>>
>> That sounds like a mistake to me. However, we must deal with it. If
>> they are unwilling to fix this problem with either a namespace change
>> or some other trick, we need to make some hard choices.
>>
>> Can we just say "use Eigen 3" and let the link-edit step fail if
>> someone tries to combine Eigen 2 libraries in the same executable?
>> Upstream has implicitly said "yes" to this by not fixing the problem.
>>
>> If so, I'd suggest backing out the namespace patch and letting the
>> chips fall where they may. If it becomes enough of an issue, maybe
>> they'll fix it some day.
>
> The reasons for this change are all ROS:
>
> http://www.ros.org/wiki/eigen3
>
> Making API-breaking changes between major versions is standard practice,
> changing the namespace is not. Even though it makes things nicer for
> users that use both versions of the library, those that only use one
> version have to change all their code to use the new namespace. Either
> way, someone loses.
>
> I think that this is a distribution issue. ROS should apply a patch to
> the PCL source that changes it to use the Eigen3 namespace in order to
> comply with ROS's convention of Eigen usage, not the other way around.
>
>>> 2) CMinPack
>>> - fPIC.patch: adds -fPIC -- could be patched upstream though Frederic wants a better solution than just adding that to
>>> the library build, as -fPIC doesn't work in MacOS, etc ... so perhaps an OS check?
>>> - shared.patch: builds the library as shared instead of static by default -- could potentially be patched upstream if we
>>> could clean it up
>>
>> Or, we could ask them to fix it, and carry our patch in ROS until they do.
>>
>>> 3) ANN -- not sure if anything can be patched upstream as the library seems to be unmaintained, though I haven't checked
>>> with the authors (will do that today)
>>> - fPIC.patch: same story as CMinPack
>>> - gcc43_shared.patch: same story as CMinPack
>>> - distance.patch: changes the library's internals to operate on floats rather than doubles to speed up computation by a
>>> large factor -- _CANNOT_ be patched upstream unless the authors make changes to their library
>
> If the library is no longer maintained, then we have to apply the
> patches ourselves and provide our own release, either separately or as
> part of PCL. This would solve the distance.patch problem because, by
> becoming the maintainer, you can choose to make the necessary changes to
> the library.
>
>> If the project is an orphan, we have two choices:
>>
>>    (1) use something else (is FLANN an alternative?)
>>    (2) maintain it ourselves in the ROS repository (current status quo)
>>
>>> 4) FLANN -- everything should be patcheable upstream as Marius works with us
>>
>> Good!
>>
>>> So overall it seems that we have 2 patches that at first glance are hard to apply upstream. If we could find solutions
>>> for these and for the rest, then we might be able to use option 1). Any volunteers willing to help?
>
> I can do a little. The CMake stuff is probably a good place for me to
> work, to begin with.
>
>>
>> I'd like to, but realistically I don't have time for any meaningful assistance.
>>
>>> Overall this might be a ROS problem as we don't seem to be doing a good job in sending our patches upstream. Bullet is a
>>> horrible example with no less than 9 patches applied to the stock 2.76 version.
>>
>> These things usually have to be handled on a case-by-case basis,
>> informed by judgement and experience. "Good judgement comes from
>> experience, and experience comes from bad judgement."  :-)
>>
>> Maintaining good working relations with upstream developers is helpful
>> whenever possible. The ROS stage package, for example, has only two
>> small patches against the latest stage release, and one was copied
>> from the stage post-release SVN trunk.
>>
>> In other cases, a close relationship may not be possible.
>
>
> Geoff
> _______________________________________________
> [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: PCL standalone and 0.3.x+

Radu B. Rusu
Administrator
Some news on this...


On 09/26/2010 02:04 PM, Radu Bogdan Rusu wrote:
> * Marius is working on patching FLANN upstream and providing a newer
> version today

Done! Marius, can you please update http://www.ros.org/wiki/point_cloud_perception/ChangeList ? :)


> * we contacted the ANN developers to see if they are still maintaining
> the library and whether they are willing to accept our patches

no reply yet.

> * we'll contact Benoit and the Eigen developers today in an attempt to
> migrate some of the patches, and ask for a new release. However this
> will not solve the Eigen3 renaming issue.

3/4 patches (all except the namespace rename) have been applied in trunk. Waiting for a new release...


To do:
- need to clean CMinPack's patches and integrate the fPIC.patch and the shared.patch in their CMakeLists.txt on a per OS
basis. Any volunteers? The URL is https://code.ros.org/svn/ros-pkg/stacks/point_cloud_perception/trunk/cminpack/


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: PCL standalone and 0.3.x+

Marius Muja


On Mon, Sep 27, 2010 at 9:28 AM, Radu Bogdan Rusu <[hidden email]> wrote:
Some news on this...


On 09/26/2010 02:04 PM, Radu Bogdan Rusu wrote:
> * Marius is working on patching FLANN upstream and providing a newer
> version today

Done! Marius, can you please update http://www.ros.org/wiki/point_cloud_perception/ChangeList ? :)

Done.


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

Re: PCL standalone and 0.3.x+

Geoffrey Biggs
In reply to this post by Radu B. Rusu
On 27/09/10 06:04, Radu Bogdan Rusu wrote:
> * we'll contact Benoit and the Eigen developers today in an attempt to
> migrate some of the patches, and ask for a new release. However this
> will not solve the Eigen3 renaming issue.
>
> Geoff, one solution is to revert these Eigen3/Eigen changes when we
> build the PCL.tar.gz file. This might just work. What do you think?

It will work fine, and users shouldn't notice it, but I still think it's
backwards. :) Libraries should be written to released dependencies, then
patched to meet individual distribution needs by distributions.

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