Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow pcl to be built against a system-wide installed metslib #299

Merged
merged 2 commits into from
Dec 14, 2013

Conversation

richmattes
Copy link
Contributor

This patch allows pcl to be built against either a system-installed metslib installation, or the version bundled with the pcl source. It discovers metslib with pkg-config where available. See commit comments for more details.

This patch allows PCL to build against a system installed metslib
where available.  It uses pkgconfig to try to discover metslib.pc,
and when both are available HAVE_METSLIB is set to ON and the
include paths are modified to use metslib's location as discovered
by pkgconfig.  When pkgconfig and/or metslib is not found, cmake
will continue to use the bundled version of metslib as before.
While making modifications for metslib, I noticed that there was
some mixed use of tabs and spaces for intentation.  I normalized
them all to 4 spaces per tab and made the indentation of all of
the "set" blocks consistent.
@jspricke
Copy link
Member

jspricke commented Oct 5, 2013

+1 from me. @arbeitor what do you think?

@rbrusu
Copy link
Member

rbrusu commented Oct 13, 2013

looks good for me too

@jspricke
Copy link
Member

I would propose to remove the build in version than, so we don't have a diverging fork.

@arbeitor
Copy link
Contributor

not sure about removing the bundled version... might cause some problems and an additional dependency.
I remember did some changes to metslib so that it compiles within PCL.

However, i think the patch is good as it will allow people knowing what they are doing to use a different metlibs library.

@jpgr87 , did you run into problems compiling pcl_recognition with your system version?

@richmattes
Copy link
Contributor Author

Actually, looking back it looks like there was one issue. I was building against metslib 0.5.3, which is a little bit newer than the one being bundled for pcl_recognition, and there were some api changes (the removal of simulated_annealing::setApplyAndEvaluate(bool)) as per https://projects.coin-or.org/metslib/wiki/MigrationGuide.

I think if possible it might be worthwhile to update the metslib version in pcl_recognition to the latest upstream version, deal with any api changes, and set a minimum version requirement of 0.5 for when an external metslib is found.

@jspricke
Copy link
Member

@arbeitor can you push your patches upstream? Thanks! Once this is done, I don't see a point in maintaining our own copy and adding an extra dependency should be easy.

@arbeitor
Copy link
Contributor

the simulated_annealing::setApplyAndEvaluate was never in the official metslib api, that is something i added for efficiency reasons (to evaluate the solution i had to first apply the move). I will have a look into this to see what needs to be changed in pcl_recognition so that it compiles against system metslib.

@rbrusu
Copy link
Member

rbrusu commented Dec 14, 2013

Since this is not breaking the current functionality, I'll merge it.

rbrusu added a commit that referenced this pull request Dec 14, 2013
Allow pcl to be built against a system-wide installed metslib
@rbrusu rbrusu merged commit 2c872f5 into PointCloudLibrary:master Dec 14, 2013
@fran6co
Copy link
Contributor

fran6co commented Dec 18, 2013

I think the include_directories directive in cmake should go in a global scope. I'm getting this error:

In file included from /tmp/pcl-pm2J/apps/3d_rec_framework/tools/apps/src/local_recognition_mian_dataset.cpp:21:
/tmp/pcl-pm2J/recognition/include/pcl/recognition/hv/hv_go.h:14:10: fatal error: 'metslib/mets.hh' file not found
#include "metslib/mets.hh"
         ^
1 error generated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants