-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Find FLANN using both module- and config-based methods #3202
Find FLANN using both module- and config-based methods #3202
Conversation
Seems to work. I cleaned up the finder script a bit to remove unnecessary variables. Note that this PR makes a transition of back, requiring to link against Ping @SunBlack in case you want to have a look. |
I will take a look at this later this day :) |
Had until now not the time to check what the config script of FLANN is exporting, but:
Workaround:
This adds |
Hm, this may work. @SergioRAgostinho should we go this way? One downside is that there will be multiple targets ( |
I agree that we are required to have a target to abstract which
Everything seems functional to me. |
OK, will update this PR. One correction though:
In the module route we are in charge of creating them. And since only a single one is needed, I would only create a single one. And also I will call it
|
Is the PR supposed to also solve
|
We do support all versions of FLANN now. Whether this is sufficient to build |
It depends: there's a couple of patches vcpkg applies to PCL before building that might not be valid. It is likely that the port file needs to be updated for the |
I figured out how to make |
It installed but the config file needs to be patched somehow to in order for the downstream to use it. Still unclear on how to patch it at the moment... |
@taketwo After the merge of this PR, is pcl supposed to work with the latest master of FLANN? If that's the case, for some reason when I link to pcl in my downstream project, I got the below error during CMake configuration
|
I think I tested this, but can not say with 100% certainty. I will try to reproduce tomorrow on Ubuntu 18.04 + PCL |
@taketwo after some trial and error, I figured out what version of flann works with the current pcl master. Only the 1.9.1 release works. I tried with the latest flann master, the commit prior to @jspricke's commit that used to break pcl compilation as indicated in #3194 and neither of them works with the current pcl master now. |
I can not reproduce this on my system (Ubuntu 18.04). What I did:
This sequence succeeds for me. Are your steps similar, do did I do something substantially different from your use-case? Also, please post the following files:
|
The only additional step I took is applying the patch as indicated in flann-lib/flann#369 in order to get flann to compile, but I don't think that affects anything. As for the
are redundant, so in my downstream CMakeLists I don't have them, but again I believe this shouldn't affect things. The rest of the steps looks like it is the same to me. I have the files with pcl compiled with FLANN 1.9.1 readily available now as below: PCLConfig.cmake
FindFLANN.cmake
If you need me to show you the files with pcl compiled with FLANN master let me know. |
It's been a while, but I finally had time today to investigate and found the problem. Our new As a quick fix, just add these lines to the top of if(TARGET FLANN::FLANN)
return()
endif() I'll send a PR later. |
@taketwo amazing find, you are right that I was testing with my full-on downstream project and there are multiple |
Work-in-progress implementation of my proposal from #3194.