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

nav2_waypoint_follower: Fix opencv includes #4287

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

traversaro
Copy link
Contributor


Basic Info

Info Please fill out this column
Ticket(s) this addresses No tickets.
Primary OS tested on Windows
Robotic platform tested on Not tested in any robot, it is a compilation-related change.
Does this PR contain AI generated software? No

Description of contribution in a few bullet points

The path used for opencv includes in nav2_waypoint_follower was different from the one documented by opencv itself for its header, see for example:

and from how other ROS 2 packages include opencv headers, see:

In all cases the opencv4 part of the include path is not included, and on some platforms (in my case, Windows with libopencv installed via conda-forge) including it results in compilation errors such as:

2024-04-30T19:39:27.9695139Z %SRC_DIR%\ros-humble-nav2-waypoint-follower\src\work\include\nav2_waypoint_follower/plugins/photo_at_waypoint.hpp(35,10): fatal error C1083: Cannot open include file: 'opencv4/opencv2/core.hpp': No such file or directory [%SRC_DIR%\build\photo_at_waypoint.vcxproj]

As removing the opencv4 the compilation should work fine on all supported nav2 platforms, I guess it should be ok to uniform the opencv inclusion style with official OpenCV documentation and the rest of ROS 2.

Description of documentation updates required from your changes

No documentation changes should be required.


Future work that may be required in bullet points

For Maintainers:

  • Check that any new parameters added are updated in navigation.ros.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

@SteveMacenski
Copy link
Member

Admittedly we don't have many windows users, so I'm not surprised :-) Let CI turn over but LGTM

@traversaro
Copy link
Contributor Author

Admittedly we don't have many windows users, so I'm not surprised :-) Let CI turn over but LGTM

To be honest, I was even surprised of finding it as an option in the PR template table. :D

@SteveMacenski
Copy link
Member

SteveMacenski commented Apr 30, 2024

There are Nav2 Windows users/tickets, but not many

Thanks! Looks like this builds fine on Linux as well!

@SteveMacenski SteveMacenski merged commit be4760a into ros-navigation:main Apr 30, 2024
7 of 8 checks passed
@traversaro
Copy link
Contributor Author

Thanks for the fast feedback!

enricosutera pushed a commit to enricosutera/navigation2 that referenced this pull request May 19, 2024
Marc-Morcos pushed a commit to Marc-Morcos/navigation2 that referenced this pull request Jul 4, 2024
Manos-G pushed a commit to Manos-G/navigation2 that referenced this pull request Aug 1, 2024
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