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

Feature/mw/fields2cover2 #53

Open
wants to merge 70 commits into
base: main
Choose a base branch
from

Conversation

aosmw
Copy link

@aosmw aosmw commented Apr 29, 2024

Update Fields2Cover to v2 api.

Uses Fields2Cover/ortools_vendor#2 and Fields2Cover/Fields2Cover#126

This builds on behavior tree update PR 43 by @tonynajar.

tonynajjar and others added 19 commits March 5, 2024 09:33
Signed-off-by: Mike Wake <michael.wake@aosgrp.com.au>
Signed-off-by: Mike Wake <michael.wake@aosgrp.com.au>
Use tinyxml2_vendor properly.  Like in ros/pluginlib

IOW first find_package(tinyxml2_vendor REQUIRED) which sets up a module path
that provides a module for find_package(TinyXML2 REQUIRED) to work

Signed-off-by: Mike Wake <michael.wake@aosgrp.com.au>
* fields2cover now initialises memory and provides getters/setters
so previous incorrectly/partially initialised data no longer throws

* Order of args for f2c::Random::generateRandField() changed to
  (area, sides), was (sides, area).  Tests now don't spin forever

Signed-off-by: Mike Wake <michael.wake@aosgrp.com.au>
@aosmw aosmw mentioned this pull request Apr 29, 2024
Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, but you have some linting issues that CI is unhappy about that we need resolved and the build doesn't turn over :(

I have no meaningful comments on your port to v2's API - that all looks great.

.github/deps.repos Outdated Show resolved Hide resolved
opennav_coverage/src/visualizer.cpp Outdated Show resolved Hide resolved
opennav_row_coverage/test/test_server.cpp Outdated Show resolved Hide resolved
@aosmw
Copy link
Author

aosmw commented May 3, 2024

Either the lines are too long for uncrustify or have wrong style for the cpplint.

Error: /__w/opennav_coverage/opennav_coverage/opennav_coverage/include/opennav_coverage/utils.hpp:160:  If an else has a brace on one side, it should have it on both  [readability/braces] [5]
Error: /__w/opennav_coverage/opennav_coverage/opennav_coverage/include/opennav_coverage/utils.hpp:166:  If an else has a brace on one side, it should have it on both  [readability/braces] [5]
Error: /__w/opennav_coverage/opennav_coverage/opennav_coverage/include/opennav_coverage/utils.hpp:180:  If an else has a brace on one side, it should have it on both  [readability/braces] [5]

@SteveMacenski
Copy link
Member

Either the lines are too long for uncrustify or have wrong style for the cpplint.

These are lines that this PR touches, please heed the formatting requirements :-)

Use intermediate variables for PathSectionType
to keep within line lenght limit requirements.
@aosmw
Copy link
Author

aosmw commented May 14, 2024

Hi @mikolodz,

My environment is humble on ubuntu22.04. However I do build personal snapshots of navigation2, fields2cover and opennav_coverage and robot_localisation that I periodically bring close to rolling since I really want to use and understand how to tune nav2_mppi_controller, nav2_smac_planner and nav2_behavior_tree. I do worry about the improvements/fixes to the lower layers rclcpp/rcl that I may be missing out on.

This PR and work on Fields2Cover packaging is my attempt at giving back. I am dipping in and out of this PR fighting with the flux of noble/ubuntu 24.04/python3.12 removals(imp)/rosdep breakage/colon empyv4 incompatibility. Alot of my recent changes to this PR are fighting the github action build, most of which should ultimately be squashed away and probably removed once the lower layers of infrastructure are again functional.

The problems on the bleeding edge are deep but I am seeking to contribute fixes/ideas/workarounds. I thought initially I might get away with a tweak or two to. I am being drawn in and going deeper down into the stack understanding more and more how the infrastructure works. A good thing.

BTW I also would like to use some of the new awesome SLAM stuff like LIO-EKF (https://github.com/YibinWu/LIO-EKF) (Currently ROS1)

As for compatible with Humble. The policy appears to be, get it working on rolling first, and then potentially backport.

@aosmw
Copy link
Author

aosmw commented May 14, 2024

noble_rolling now hitting warning as error in behaviortree_cpp.

/opt/ros/rolling/include/behaviortree_cpp/controls/switch_node.h:91:15: error: declaration of ‘ports’ shadows a previous local [-Werror=shadow]
91 | PortsList ports;

This is fixed in BehaviorTree/BehaviorTree.CPP@97c3f6a

Which is in https://github.com/BehaviorTree/BehaviorTree.CPP/releases/tag/4.6.0

@mikolodz
Copy link

Thank you @aosmw

I truly admire your patience. I've been working with SC-LIO-SAM (ROS1) for a while and got it running with gazebo & Nav2. Wasn't as easy as I expected :) Currently, I'm stuck with the fields2cover, and this is why I'm here asking you questions and observing your efforts.

Do you think I could use your snapshots to build your fork for Humble? As I said before, I have some linking issues and felt like I'm not sure if I should invest another day or two getting through it not knowing if it will work at all. But now, when you confirm that you're using a similar setup as I use, it gives me new motivation to give it a try.

@aosmw
Copy link
Author

aosmw commented May 14, 2024

noble_rolling is not finding libyaml-cpp-dev via https://github.com/ros2/yaml_cpp_vendor.

Needs something like this ros-navigation/navigation2#4267

[ 22%] Linking CXX executable test_map_saver_publisher
/usr/bin/ld: ../../libmap_io.so: undefined reference to YAML::Emitter::StartedScalar()' /usr/bin/ld: ../../libmap_io.so: undefined reference to vtable for YAML::BadConversion'
/usr/bin/ld: ../../libmap_io.so: undefined reference to YAML::detail::node_data::insert_map_pair(YAML::detail::node&, YAML::detail::node&)' /usr/bin/ld: ../../libmap_io.so: undefined reference to YAML::ostream_wrapper::write(std::__cxx11::basic_strin

jammy_rolling - back to same pytest incompatability - https://github.com/ros2/launch/pull/766/files
/opt/ros/rolling/lib/python3.10/site-packages/launch_testing/pytest/hooks.py:178: in find_launch_test_entrypoint
module = import_path(path, root=None)
E TypeError: import_path() missing 1 required keyword-only argument: 'consider_namespace_packages'

@SteveMacenski
Copy link
Member

SteveMacenski commented May 15, 2024

What worries me is compatibility with 22.04 and with Humble.

There is a separate humble branch - so that all should be more or less fine!

are still using ROS1, so making use of a whole ROS project becomes gradually more complex or even impossible.

There is no intention for support for ROS 1. Frankly I haven't really thought about ROS 1 in years. If someone wanted to port this all to ROS 1 and have a ROS 1 branch, I wouldn't block it, but I wouldn't implement it

By any chance will this v2 API update be compatible with Humble or should I start preparing for a big upgrade?

For maintenance reasons, the humble branch will stay on V1 since that was what it was released under and v2 has breaking changes. With that said, I'd be happy to have a humble-v2 branch that folks on Humble can use with v2 if someone opens a PR once this is all wrapped up. I just don't want to push API breaking changes to everyone except those that want that in exchange for the breakage.

This PR and work on Fields2Cover packaging is my attempt at giving back.

And I genuinely thank you for this @aosmw! I spend alot of my time fighting the good fight working on things like this, so the help is really, truly appreciated!

This is fixed in BehaviorTree/BehaviorTree.CPP@97c3f6a

Ugh. Build from source until that is released too.... I guess...

Needs something like this ros-navigation/navigation2#4267

Precisely!

I think we're on the home stretch here, update the action to 24.04 which should fix rosdep & that should be the last of the large problems

@SteveMacenski SteveMacenski mentioned this pull request May 20, 2024
@SteveMacenski
Copy link
Member

Any word?

@aosmw
Copy link
Author

aosmw commented May 29, 2024

I was going to wake this up again now that ros-navigation/navigation2#4298 has landed.
Most of what is needed is just stripping out the workarounds in .github

I was also going to look into how to point the github build at the ghcr.io/ros-navigation/navigation2:main docker image.

@aosmw
Copy link
Author

aosmw commented Jun 3, 2024

The build against the source of fields2cover works, ie using the .github/deps.repos mechanism.
The build against the packaged version of fields2cover for rolling on 24.04 currently does not.

HOWEVER,
There remains a package conflict to negotiate for nav2_constrained_smoother and nav2_smac_planner that is not resolved by rosdep. libceres-dev wants libgoogle-glog-dev that wants libunwind-dev but the docker image has libunwind7-dev.

ALSO,
The packaged install location of the fields2cover dependent libraries is pretty ugly and fails to find its libabsl libraries properly. Looking for them in /opt/ros/lib/lib rather than in /opt/ros/lib/x86_64-linux-gnu where they currently reside.

IOW,
The current install of ros-rolling-fields2cover 2.0.0-13noble.20240514.144256 put the ortools/absl/protobuf/fields2cover libraries in /opt/rolling/lib/x86_64-linux-gnu rather than a more conventional/contained ros location. They should be properly vendored into /opt/ros/rolling/opt/ortools

FYI,
I now have a way to poke around in the github action build by running it using https://github.com/nektos/act. It requires that the image being used has nodejs installed. Once it is running I find its container name using docker ps, and then get a bash shell inside it using docker exec -it $(docker ps --latest --quiet) bash

CONCLUSION,
Close but still so far.

@SteveMacenski
Copy link
Member

The build against the packaged version of fields2cover for rolling on 24.04 currently does not.

Great... I thought we'd mentioned that to fix it, but I suppose still no dice.

There remains a package conflict to negotiate for nav2_constrained_smoother and nav2_smac_planner that is not resolved by rosdep. libceres-dev wants libgoogle-glog-dev that wants libunwind-dev but the docker image has libunwind7-dev.

I'm a little confused by that considering this builds fine in Nav2's CI and for me locally with a 24.04 docker image

@SteveMacenski
Copy link
Member

Perhaps it would be easier to manually setup the build like here rather than using the ROS tooling wrappers?

@aosmw
Copy link
Author

aosmw commented Jun 5, 2024

The image used in the github workflow is currently rostooling/setup-ros-docker:ubuntu-noble-ros-rolling-ros-base-latest
That is the likely cause of the libceres-dev, libgoogle-glog-dev, libunwind-dev, libunwind7-dev dance at the moment.

The main problem of using the Fields2Cover package, (2.0.0-13noble.20240514.144256) right now is the command line flags that the ros buildfarm uses and what they do to the locations of where things get installed prior to creating the debian package.

Fields2Cover does not use ament_cmake and I when I started trying to help, I tried to workaround what was there. I introduced the use of cmake include(GNUInstallDirs) and used the variable CMAKE_INSTALL_LIBDIR which is overriden by the ros packaging command in a way that is not playing nicely.

Digging down into it the command that is getting run by the ros build farm to create the package - https://build.ros2.org/job/Rbin_uN64__fields2cover__ubuntu_noble_amd64__binary/77/console and wrapping it in a script to reproduce locally

#!/bin/bash

mkdir -p build
cd build
DEB_PYTHON_INSTALL_LAYOUT=deb cmake -DCMAKE_INSTALL_PREFIX=/usr -DCMAKE_BUILD_TYPE=None -DCMAKE_INSTALL_SYSCONFDIR=/etc -DCMAKE_INSTALL_LOCALSTATEDIR=/var -DCMAKE_EXPORT_NO_PACKAGE_REGISTRY=ON -DCMAKE_FIND_USE_PACKAGE_REGISTRY=OFF -DCMAKE_FIND_PACKAGE_NO_PACKAGE_REGISTRY=ON -DFETCHCONTENT_FULLY_DISCONNECTED=ON "-GUnix Makefiles" -DCMAKE_VERBOSE_MAKEFILE=ON -DCMAKE_INSTALL_LIBDIR=lib/x86_64-linux-gnu -DBUILD_TESTING:BOOL=OFF -DCMAKE_INSTALL_PREFIX=/optmw/ros/rolling -DCMAKE_PREFIX_PATH=/optmw/ros/rolling -DBUILD_TESTING=OFF -DFETCHCONTENT_FULLY_DISCONNECTED=OFF ..

make -j8
sudo make install

Notice the -DCMAKE_INSTALL_LIBDIR=lib/x86_64-linux-gnu
Notice that i tweak the install prefix to /optmw isolate it on my local dev system.

I have some ideas to get it to play nicely (in a ros package install) that I am playing around with.

My aim is to find a way to get the ortools sfuff installed in /opt/ros/rolling/opt/f2c_ortools while still working for a non-ros/non-ament build.

THIS Fields2Cover/Fields2Cover#146 (remove libabsl-dev from Fields2Cover/package.xml) might be a simple fix for now but the way the Fields2Cover package is dumping ortools artifacts into /opt/ros/rolling should really be improved so I will keep plugging away at it.

@SteveMacenski
Copy link
Member

I have some ideas to get it to play nicely (in a ros package install) that I am playing around with.

Great!! For the meantime though would it be useful to just build F2C in CI here from a deps.repos file that we use tagged at a version we support for a given distribution?

But fixing F2C to have the binaries work well is useful. If we're having issues here, I presume anyone else trying to use the binaries would also have a similar issue.

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

5 participants