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

[jsk_recognition_utils] add OpenCV to catkin_depends #2823

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mqcmd196
Copy link
Member

The current CMake implementation of jsk_recognition_utils does not propagate the need for OpenCV downstream, so I fixed it.

How to check

Create the package like

package.xml

<?xml version="1.0"?>
<package format="2">
  <name>test_utils</name>
  <version>0.0.0</version>
  <description>The test_utils package</description>
  <maintainer email="obinata@todo.todo">obinata</maintainer>
  <license>TODO</license>
  <depend>jsk_recognition_utils</depend>
  <buildtool_depend>catkin</buildtool_depend>
</package>

CMakeLists.txt

cmake_minimum_required(VERSION 3.0.2)
project(test_utils)

find_package(catkin REQUIRED COMPONENTS jsk_recognition_utils)

catkin_package()

include_directories(
  include
  ${catkin_INCLUDE_DIRS}
)

link_directories(
  ${catkin_LIBRARY_DIRS}
)

message(STATUS "catkin_INCLUDE_DIRS: ${catkin_INCLUDE_DIRS}")

add_library(${PROJECT_NAME}
  src/test_utils.cpp
)

include/test_utils/test_utils.h

#include <jsk_recognition_utils/pcl_conversion_util.h>

src/test_utils.cpp

#include <test_utils/test_utils.h>

Then compare the cmake outputs before this PR and after this PR

before

-- catkin_INCLUDE_DIRS: /home/obinata/ros/hsr_ws/devel/.private/jsk_recognition_utils/include;/home/obinata/ros/hsr_ws/devel/.private/jsk_recognition_msgs/include;/home/obinata/ros/hsr_ws/src/jsk_recognition/jsk_recognition_utils/include;/opt/ros/noetic/include;/opt/ros/noetic/share/xmlrpcpp/cmake/../../../include/xmlrpcpp;/usr/include;/usr/include/eigen3;/usr/include/pcl-1.10;/usr/include/vtk-7.1;/usr/include/freetype2;/usr/include/x86_64-linux-gnu

and fails to compile with

In file included from /home/obinata/ros/hsr_ws/src/test_utils/include/test_utils/test_utils.h:4,
                 from /home/obinata/ros/hsr_ws/src/test_utils/src/test_utils.cpp:1:
/home/obinata/ros/hsr_ws/src/jsk_recognition/jsk_recognition_utils/include/jsk_recognition_utils/pcl_conversion_util.h:56:10: fatal error: opencv2/opencv.hpp: そのようなファイルやディレクトリはありません
   56 | #include <opencv2/opencv.hpp>
      |          ^~~~~~~~~~~~~~~~~~~~
compilation terminated.

after

-- catkin_INCLUDE_DIRS: /home/obinata/ros/hsr_ws/devel/.private/jsk_recognition_utils/include;/home/obinata/ros/hsr_ws/devel/.private/jsk_recognition_msgs/include;/home/obinata/ros/hsr_ws/src/jsk_recognition/jsk_recognition_utils/include;/opt/ros/noetic/include;/opt/ros/noetic/share/xmlrpcpp/cmake/../../../include/xmlrpcpp;/usr/include/opencv4;/usr/include/pcl-1.10;/usr/include/eigen3;/usr/include;/usr/include/vtk-7.1;/usr/include/freetype2;/usr/include/x86_64-linux-gnu;/usr/include/ni;/usr/include/openni2

and successfully finds OpenCV

@pazeshun
Copy link
Contributor

pazeshun commented Feb 20, 2024

I don't understand fully, but maybe you should just add cv_bridge dependency to find_package(catkin REQUIRED COMPONENTS of CMakeLists.txt and package.xml.
cf. jsk-ros-pkg/jsk_visualization#646, jsk-ros-pkg/jsk_3rdparty#179

@k-okada
Copy link
Member

k-okada commented Feb 21, 2024

humm,

$ rospack depends-why --target=cv_bridge jsk_recognition_utils
Dependency chains from jsk_recognition_utils to cv_bridge:
* jsk_recognition_utils -> image_view -> cv_bridge 

so, we already have image_view in jsk_recognition_utils/package.xml, but maybe it is not propagate to catkin_INCLUDE_DIRS.

Before applying this patch, catkin_INCLUDE_DIRS already contains /usr/include/pcl-1.10, thanks to CATKIN_DEPENDS pcl_ros, so maybe find_package(PCL REQUIRED) and PCL_LIBRARIES could be removed. If this is true, we might just remove find_package(OpenCV) and add cv_bridge to package.xml and find_package(catkin.

How do you think? Too many changes?

@k-okada k-okada closed this Feb 21, 2024
@k-okada k-okada reopened this Feb 21, 2024
@mqcmd196
Copy link
Member Author

I don't understand fully, but maybe you should just add cv_bridge dependency to find_package(catkin REQUIRED COMPONENTS of CMakeLists.txt and package.xml.
cf. jsk-ros-pkg/jsk_visualization#646, jsk-ros-pkg/jsk_3rdparty#179

Thank you for your info. At first, I thought that tThishis method was not very clean, but I agreed that it was better to use it because different versions of OpenCV handle it differently in CMake, and cv_bridge did it well.

so, we already have image_view in jsk_recognition_utils/package.xml, but maybe it is not propagate to catkin_INCLUDE_DIRS.

Yes, it is because cv_bridge is not written in CATKIN_DEPENDS in image_view https://github.com/ros-perception/image_pipeline/blob/69488e6e455d97c2deaf924234aa5f87b83cfda8/image_view/CMakeLists.txt#L7

Before applying this patch, catkin_INCLUDE_DIRS already contains /usr/include/pcl-1.10, thanks to CATKIN_DEPENDS pcl_ros, so maybe find_package(PCL REQUIRED) and PCL_LIBRARIES could be removed. If this is true, we might just remove find_package(OpenCV) and add cv_bridge to package.xml and find_package(catkin.

Yes, so I'll handle this.

find_package(PCL REQUIRED) and PCL_LIBRARIES could be removed

This is the refactor process so I'll create another PR

@mqcmd196
Copy link
Member Author

mqcmd196 commented Apr 4, 2024

@k-okada Kindly ping. I think this PR could be merged

@knorth55
Copy link
Member

i had the same issue and I opened #2842.
I confirmed both #2842 and this PR solved my issue, JFYI.

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.

None yet

4 participants