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

Add workaround for deprecated filter functions #285

Merged
merged 2 commits into from
Jul 14, 2020

Conversation

mvieth
Copy link
Contributor

@mvieth mvieth commented Jun 26, 2020

@SteveMacenski
Copy link
Member

I'm OK with this once CI passes

@mvieth
Copy link
Contributor Author

mvieth commented Jul 9, 2020

Ping @SteveMacenski The reasons why the checks fail seem unrelated

@SteveMacenski
Copy link
Member

SteveMacenski commented Jul 9, 2020

Toggling to rerun, lets see what it says now. The noetic failure was caused by Eigen rather than Eigen3 rosdep and CMakelists entries. The Melodic looks like a fluke but lets see.

Ping me when it finishes and we can take a look again. If you're so motivated to change Eigen -> Eigen3 that would be great.

@mvieth
Copy link
Contributor Author

mvieth commented Jul 10, 2020

@SteveMacenski ok, melodic passes now. The Eigen thing comes from pcl itself.

@SteveMacenski
Copy link
Member

I think the Eigen thing is because the cmakelists / package.xml of pcl_ros points at Eigen and not Eigen3. I've experienced this issue in other packages as well, its a really easy "add a 3" situation anywhere Eigen exists. I'd appreciate if you were to add that, but if not, I can do it at a larger date.

@mvieth
Copy link
Contributor Author

mvieth commented Jul 11, 2020

I am quite sure that the Eigen issue has to be fixed in pcl itself, not here in perception_pcl. Everywhere here, Eigen is already called Eigen3, except in the package.xml, but even with that changed the cmake warning still pops up. The log says:

Call Stack (most recent call first):
/usr/lib/x86_64-linux-gnu/cmake/pcl/PCLConfig.cmake:152 (find_package)
/usr/lib/x86_64-linux-gnu/cmake/pcl/PCLConfig.cmake:316 (find_eigen)
/usr/lib/x86_64-linux-gnu/cmake/pcl/PCLConfig.cmake:551 (find_external_library)
CMakeLists.txt:10 (find_package)

PCL has a FindEigen.cmake that actually finds Eigen3, and there is a FindEigen.cmake in the ros distribution under share/cmake_modules that is deprecated. I guess these two get mixed up and that causes the problem. I might create a PR for this issue in the pcl repo.

@mvieth
Copy link
Contributor Author

mvieth commented Jul 12, 2020

After thinking a bit more about the Eigen issue, it seems there is a really simple solution after all. The warning disappears just by moving the find_package(PCL) above the find_package(cmake_modules) in the CMakeLists of pcl_ros. Maybe the dependence on cmake_modules could be even completely removed, it seems to be not needed (any more).

@SteveMacenski
Copy link
Member

@mvieth please make that change then

PCL searches for a FindEigen.cmake file. The FindEigen.cmake file delivered with PCL actually finds Eigen3, but there is a FindEigen.cmake in cmake_modules that is deprecated and generates a cmake warning. With the old order of the find_packages, there seems to be a mix-up and PCL finds the deprecated file. The new order solves that problem.
@mvieth
Copy link
Contributor Author

mvieth commented Jul 14, 2020

@SteveMacenski done. No cmake warnings anymore.

@SteveMacenski
Copy link
Member

Still warnings

05:15:46 /tmp/ws/src/perception_pcl/pcl_ros/src/pcl_ros/surface/moving_least_squares.cpp:215:48: warning: ‘void pcl::MovingLeastSquares<PointInT, PointOutT>::setPolynomialFit(bool) [with PointInT = pcl::PointXYZ; PointOutT = pcl::PointNormal]’ is deprecated: use setPolynomialOrder() instead [-Wdeprecated-declarations]
05:15:46   215 |     impl_.setPolynomialFit (use_polynomial_fit_);
05:15:46       |                                                ^
05:15:46 In file included from /tmp/ws/src/perception_pcl/pcl_ros/include/pcl_ros/surface/moving_least_squares.h:44,
05:15:46                  from /tmp/ws/src/perception_pcl/pcl_ros/src/pcl_ros/surface/moving_least_squares.cpp:39:
05:15:46 /usr/include/pcl-1.10/pcl/surface/mls.h:361:7: note: declared here
05:15:46   361 |       setPolynomialFit (bool polynomial_fit)
05:15:46       |       ^~~~~~~~~~~~~~~~

but not your problem in this PR. I'm good with this

@SteveMacenski SteveMacenski merged commit 1887ba2 into ros-perception:melodic-devel Jul 14, 2020
@SteveMacenski SteveMacenski mentioned this pull request Jul 14, 2020
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.

2 participants