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

Cleanup of package manifests and build scripts #62

Merged

Conversation

gavanderhoorn
Copy link
Member

Major changes:

With the above changes catkin_lint -W2 errors / warnings are down from 120+ to something like 15 (most of which are about target names being not unique enough).

Tested building on both 32 and 64 bit Ubuntu Trusty - Indigo systems, with catkin_make and catkin_tools.

Remaining issues / questions:

  • haven't tested this on Hydro, not sure whether that is something you still want to support
  • I did only minimal testing with a from-source installation of Ceres et al. (used libceres-dev, see below)
  • many service, message and action names do not follow ROS naming conventions (should be CamelCased)
  • rgbd_depth_correction: launch files reference unreleased pkgs (bolles_calibration, depth_calibration)
  • industrial_extrinsic_cal: robot_trigger.action not in add_action_files (but could be on purpose)
  • industrial_extrinsic_cal: nist_cal pkg?
  • industrial_extrinsic_cal: aravis_camera_driver pkg doesn't exist?
  • document which version of camera_aravis you're using (there are many forks!)
  • commented set(CMAKE_CXX_FLAGS ${CMAKE_CXX_FLAGS} "-fPIC") (and variants) everywhere. They are probably needed, but I couldn't easily determine why, and fi in industrial_extrinsic_cal the line simply overwrote CMAKE_CXX_FLAGS instead of appending to it. Just remove the hash to uncomment them again.
  • would it be an idea to (run & build) depend on libceres-dev on Indigo and up (packaged by Vincent Rabaud (or Michael Ferguson) (rosdep here))? Or is that not recent enough (everything compiled ok on my machine)? Maybe specify version in find_package(..)? It would greatly simplify the installation of the packages (and make distribution through the buildfarm possible) (Missing dependency declaration on (lib)ceres #32)
  • target_locator, intrinsic_cal: launch files reference unreleased pkg (ind_cal_multi_camera)
  • I've opted to just run_depend on (almost) everything used in the launch files (camara_aravis, rviz, etc). You might want to check what makes sense, prune the rest
  • libspqr.so -> missing symlink when installed only by libspqr1.3.1,
    ok when libsuitesparse-dev is installed (or maybe make symlink?). Not sure whether this is only on 32 or 64 bit machines (this is probably a known issue with libceres-dev)
  • on 64 bit machines (?): receive warnings about %d not being appropriate for size_type operand (in rgbd_depth_correction)

PS: PR is a bit large, but I didn't think it would make sense to split everything up per-package. It's probably easiest to just try out building locally and see whether everything still works.
PPS: you'll probably want to check the install rules, as I've made it install just about everything there is in these packages, which may not be what you want.
PPPS: there are certainly things I overlooked / didn't touch. This project has some sizeable source & header files and I haven't checked them all to see whether usages of external entities were correctly declared everywhere (#include <..> as well as in manifests and build scripts). Might be worth it to audit files one-by-one and see whether they are ok.

@VictorLamoine
Copy link
Contributor

Builds fine on my side apart from warnings

/home/victor/catkin_ws/src/industrial_calibration/industrial_extrinsic_cal/src/ros_camera_observer.cpp: In member function ‘virtual int industrial_extrinsic_cal::ROSCameraObserver::getObservations(industrial_extrinsic_cal::CameraObservations&)’:
/home/victor/catkin_ws/src/industrial_calibration/industrial_extrinsic_cal/src/ros_camera_observer.cpp:467:1150: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 8 has type ‘std::vector<cv::KeyPoint>::size_type {aka long unsigned int}’ [-Wformat=]
  ROS_ERROR("Red keypoints: %u",keypoints.size());

/home/victor/catkin_ws/src/industrial_calibration/industrial_extrinsic_cal/src/ros_camera_observer.cpp:477:1152: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 8 has type ‘std::vector<cv::KeyPoint>::size_type {aka long unsigned int}’ [-Wformat=]
  ROS_ERROR("Green keypoints: %u",keypoints.size());

/home/victor/catkin_ws/src/industrial_calibration/industrial_extrinsic_cal/src/ros_camera_observer.cpp:485:1151: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 8 has type ‘std::vector<cv::KeyPoint>::size_type {aka long unsigned int}’ [-Wformat=]
  ROS_ERROR("Blue keypoints: %u",keypoints.size());

/home/victor/catkin_ws/src/industrial_calibration/rgbd_depth_correction/src/depth_calibration.cpp: In member function ‘bool DepthCalibrator::calibrateCameraDepth(std_srvs::Empty::Request&, std_srvs::Empty::Response&)’:
/home/victor/catkin_ws/src/industrial_calibration/rgbd_depth_correction/src/depth_calibration.cpp:126:219: warning: format ‘%d’ expects argument of type ‘int’, but argument 8 has type ‘std::vector<pcl::PointXYZ, Eigen::aligned_allocator_indirection<pcl::PointXYZ> >::size_type {aka long unsigned int}’ [-Wformat=]
       ROS_ERROR("Point cloud pixel depth correction cloud size (%d) not the same size as saved cloud size (%d). Aborting depth calibration.",
                                                                                                                                                                                                                           ^
/home/victor/catkin_ws/src/industrial_calibration/rgbd_depth_correction/src/depth_calibration.cpp:126:219: warning: format ‘%d’ expects argument of type ‘int’, but argument 9 has type ‘std::vector<pcl::PointXYZ, Eigen::aligned_allocator_indirection<pcl::PointXYZ> >::size_type {aka long unsigned int}’ [-Wformat=]
/home/victor/catkin_ws/src/industrial_calibration/rgbd_depth_correction/src/depth_calibration.cpp: In member function ‘bool DepthCalibrator::calibrateCameraPixelDepth(std_srvs::Empty::Request&, std_srvs::Empty::Response&)’:
/home/victor/catkin_ws/src/industrial_calibration/rgbd_depth_correction/src/depth_calibration.cpp:211:1183: warning: format ‘%d’ expects argument of type ‘int’, but argument 8 has type ‘std::vector<pcl::PointCloud<pcl::PointXYZ>, Eigen::aligned_allocator_indirection<pcl::PointCloud<pcl::PointXYZ> > >::size_type {aka long unsigned int}’ [-Wformat=]
           ROS_INFO("Got point cloud %d of %d", (temp_clouds.size() + 1), num_point_clouds_ );

@gavanderhoorn
Copy link
Member Author

Thanks. Yes, those warnings are what I referred to with the last bullet under Remaining issues / questions.

# DEPENDENCIES
# std_msgs # Or other packages containing msgs
# )
find_package(catkin REQUIRED COMPONENTS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case I forget to thank you for all this, Thank you very much. I don't have a good handle on CMakeList syntax.

@drchrislewis
Copy link
Member

@gavanderhoorn

  1. I was unaware of catkin_lint. I'll run in future to help reduce errors.
  2. I am no longer trying to keep hydro-devel current
  3. all references to the package nist_cal should probably be removed, I've tried to push all needed files into industrial_calibraiton_tutorials. Generally these were launch and yaml files.
  4. the package aravis_camera_driver is unique to me. We should remove all refs to it.
  5. camera_aravis is a mess, no fork works out of the box. I have a vague memory of submitting a pull request to the version I used last, but don't recall the status. I'll check
  6. CamelCase for services and messages, Yes this is an issue.
  7. FPIC peppers many cmake files in a flailing attempt to get ceres to link without really understanding the issue. There is still an issue of when and where it is needed.
  8. the package ind_cal_multi_camera should be in industrial_calibration_tutorials. My intention is to get this package cleaned up so that it contains every major example of using the cal-library.
  9. All the later comments are beyond my knowlege .... build_depend, run_depend, install I rarely use. If you want to fix stuff I'm willing to learn.
    +1

drchrislewis added a commit that referenced this pull request Nov 11, 2015
Cleanup of package manifests and build scripts
@drchrislewis drchrislewis merged commit 40f87de into ros-industrial:indigo-devel Nov 11, 2015
@gavanderhoorn gavanderhoorn deleted the build_scripts_cleanup branch November 12, 2015 10:54
@gavanderhoorn
Copy link
Member Author

@drchrislewis: no problem, I had some time to kill.

As to the the bullets about install targets, run_ and build_ depends: they really come down to two things:

  1. are there things that (after the merge of this PR) get installed that you'd rather not see installed / or feel don't need to be installed? If yes: just remove those install(..) statements from the CMakeLists.txt.
  2. is Ceres 1.8 recent enough that it can be used with your code? If yes: we could update the install instructions to just tell ppl to install libceres-dev from the ROS package repositories, which would remove the need for everyone to compile and install ceres, glog, etc etc from sources. If 1.8 isn't recent enough, we can keep the current instructions, but should probably document that somewhere then.

If libceres-dev is recent enough it would also make it possible to release industrial_calibration pkgs through the ROS buildfarm.

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.

3 participants