Skip to content

Merge devel into ros2 branch#303

Closed
ruffsl wants to merge 33 commits intoOctoMap:ros2from
ruffsl:ros2
Closed

Merge devel into ros2 branch#303
ruffsl wants to merge 33 commits intoOctoMap:ros2from
ruffsl:ros2

Conversation

@ruffsl
Copy link
Copy Markdown

@ruffsl ruffsl commented Jul 29, 2020

Looks like the ros2 branch is lagging a bit behind devel. I've just resolved the merge conflict between the cmake version bumps in ros2 and the qt5 package.xml depends from devel. It doesn't look like ament is being used, so I'm not sure why these are two different branches to begin with, given that ament_target_dependencies still doesn't seem to work with this package and headers must be manually included for downstream packages.

Related: moveit/geometric_shapes#154

cc @mikaelarguedas

Jamie Snape and others added 30 commits May 30, 2019 12:18
Do not use std::random_shuffle with C++11 or above
Improves ROS2 compatibility with updated package.xml format
Fix unused parameter warning in OccupancyOcTreeBase<NODE>::insertPointCloudRays
- Forgot libqlviewer-qt4-dev as a dependency which caused Melodic builds to fail
- This now uses build_depend/exec_depend as previously again
Workaround for RPath on macOS no longer necessary
Still defaults to PREFIX/lib but e.g. Debian or Fedora packagers
can easily set it to lib/x86_64-linux-gnu or lib64 respectively.
Thanks to kazuki0824 and anasarrak.
Copied from root CMakeFiles.txt to make this flag default ON even if
building octovis as a module.
octovis: Enable Qt5 by default, adjust travis config accordingly
Support configurable libdir to ease packaging
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Bump CMake version to avoid CMP0048
Recently, OCTOVIS_QT5 was set to ON by default. This requires updated package dependencies to work on the buildfarm.
…dencies

[octovis] Update ROS dependencies to Qt5
std::byte can't be read from a file (no overload of operator >>
available), fall back to typedef'fing it to unsigned char again.

Related: OctoMap#240
fix ifstream >> byte with c++17
octovis: use -fPIC with all compilers (if available)
octovis adapted to be compatible with libQGLViewer >= 2.7.0
@wxmerkt
Copy link
Copy Markdown
Member

wxmerkt commented Jul 29, 2020

I have a slightly different proposition I'd love your feedback on.

I'd like to propose to retire the ros2 branch entirely and amend the package.xml to support both ROS1 and ROS2 using package format 3 conditions. This is purely a CMake project and I don't see a reason why we need to maintain a separate branch. The only difference between OctoMap for ROS1 & ROS2 is the change from catkin to ament which can be made conditional.

(When we started the ros2 branch, I wasn't aware of the condition="$ROS_VERSION == 1" option for the package format so we could fix that now and only maintain devel.)

What are your thoughts @ruffsl @mikaelarguedas?

@ruffsl
Copy link
Copy Markdown
Author

ruffsl commented Jul 29, 2020

I like the idea of consolidated code base. Are there example package elsewhere in ecosystem we could mimic? I have no idea about mixing catkin with ament via logic switching.

@wxmerkt
Copy link
Copy Markdown
Member

wxmerkt commented Jul 29, 2020

I think this PR is a good example: ethz-adrl/ifopt#52

@mikaelarguedas
Copy link
Copy Markdown

Yes this sounds like a good plan 👍
It can get tricky (needing quite some conditionals) if catkin or ament macros are used in the CMakeLists. It doesnt seem to be the case here so this should work fine.

One thing for e.g. rosdep is that ROS 2 looks only for packages that are registered in the ament index and not "all places that have a package.xml". So there may be something to tweak there (this issue has some information about it ros-infrastructure/rosdep#724)

@wxmerkt
Copy link
Copy Markdown
Member

wxmerkt commented May 10, 2021

@ruffsl Can we close this PR now that devel supports ROS2 / the ros2 branch is deprecated?

@ruffsl ruffsl closed this May 10, 2021
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.