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

Revamp nav2_behavior_tree CMakeLists.txt to use modern idioms. #4485

Conversation

clalancette
Copy link
Contributor

Basic Info

Info Please fill out this column
Ticket(s) this addresses Follow-up to #4357
Primary OS tested on Ubuntu 24.04
Robotic platform tested on N/A
Does this PR contain AI generated software? No

Description of contribution in a few bullet points

This commit does a number of things:

  1. Switches to using target_link_libraries everywhere. This gives us finer-grained control over what dependencies are exported to downstream as public, or private. In the particular case of nav2_behavior_tree, this actually doesn't matter too much, but it will help for other packages.
  2. Moves the include directory down one level to include/${PROJECT_NAME}, which is best practice in ROS 2 since Humble.
  3. Makes sure to export nav2_behavior_tree as a CMake target, so downstream users of it can use that target.
  4. Removes the use of boost. To do this, we had to introduce our own version of the "split_string" method.
  5. Moves the test_action_server.hpp file into the main include directory. This is slightly odd, but because downstream packages (opennav_docking_bt) depend on this header file to compile their own tests, it is technically part of the public interface.

Description of documentation updates required from your changes

None needed.


Future work that may be required in bullet points

The rest of the packages in this repository need a similar treatment (PRs will be upcoming).

For Maintainers:

  • Check that any new parameters added are updated in docs.nav2.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

Copy link
Contributor

mergify bot commented Jun 24, 2024

@clalancette, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@clalancette clalancette force-pushed the clalancette/main-nav2-behavior-tree branch from 70c389f to 48b1b70 Compare June 24, 2024 23:39
Copy link
Contributor

mergify bot commented Jun 24, 2024

@clalancette, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@clalancette
Copy link
Contributor Author

This one needs a release of behaviortree.cpp to be done so we can get the exported target there. I'll contact the maintainers tomorrow and see if we can get a release of that.

@SteveMacenski
Copy link
Member

Are these changes backportable to Jazzy or only rolling?

@clalancette
Copy link
Contributor Author

Are these changes backportable to Jazzy or only rolling?

They are backportable to Jazzy, assuming that BehaviorTree/BehaviorTree.CPP#826 is released into Jazzy (it has not yet been, as far as I can tell).

nav2_behavior_tree/CMakeLists.txt Show resolved Hide resolved
nav2_behavior_tree/CMakeLists.txt Show resolved Hide resolved
nav2_behavior_tree/src/generate_nav2_tree_nodes_xml.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

mergify bot commented Jun 25, 2024

This pull request is in conflict. Could you fix it @clalancette?

@SteveMacenski
Copy link
Member

@clalancette thanks for all this work, especially this package + Smac Planner + MPPI + System Tests are the largest time hogs so even this alone probably cuts off minutes. It would be great if during these PRs you gave a ballpark of time saved for that package and/or stack. Nothing formal, just something to acknowledge your efforts for.

I was curious to know what capacity you're working on this. Is this something of your own volition to help me / the community / yourself if doing stuff on the rare occasion that Nav2 needs assistance -- or is this something Intrinsic needs bc you're looking at starting to use it?

@clalancette
Copy link
Contributor Author

Is this something of your own volition to help me / the community / yourself if doing stuff on the rare occasion that Nav2 needs assistance

It is me doing it on my own volition. I've been using Navigation2 as a test bed for rmw_zenoh work, and I noticed that the compile times were high, so I decided to look into it.

It would be great if during these PRs you gave a ballpark of time saved for that package and/or stack. Nothing formal, just something to acknowledge your efforts for.

I can give some numbers as I open PRs, but this is unfortunately a case where we won't see a lot of savings until we get the bulk of it done. That is, most of savings are going to be when the downstream packages can rely on the exported targets, rather than the LIBRARIES/INCLUDE_DIRS.

One other thing I'll mention is that I started this journey on the iron branch, and there, this series made a huge difference (as stated in #4357, I went from 22 minutes to 9 minutes locally). As I've been porting to the main branch, I've been seeing less of a difference. I'm not sure if that is because main is just better to start with, or if it's because I haven't yet ported everything over yet.

@clalancette clalancette force-pushed the clalancette/main-nav2-behavior-tree branch from 48b1b70 to c285b10 Compare June 25, 2024 20:57
@SteveMacenski
Copy link
Member

Got it! Your help is much appreciated 🫡

as a test bed for rmw_zenoh work

I had some threads with Angelo on this but haven't heard back.. I need to ping them. I wanted to add into our build matrix to run Zenoh once the binaries are available like we have to run Fast/Cyclone on our nightlies. That'll show issues if they exist. Even now, Fast and Cyclone fail on different things.

One other thing I'll mention is that I started this journey on the iron branch, and there, this series made a huge difference (as stated in #4357, I went from 22 minutes to 9 minutes locally). As I've been porting to the main branch, I've been seeing less of a difference. I'm not sure if that is because main is just better to start with, or if it's because I haven't yet ported everything over yet.

Must be - Iron is pretty close or essentially the same as main. Probably just not ported yet!

Copy link
Contributor

mergify bot commented Jun 25, 2024

@clalancette, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@clalancette
Copy link
Contributor Author

It would be great if during these PRs you gave a ballpark of time saved for that package and/or stack. Nothing formal, just something to acknowledge your efforts for.

I can give some numbers as I open PRs

As a not very scientific number, locally I went from a compile time for all of navigation2 of 10 minutes 32 seconds on main to 10 minutes 19 seconds with this PR.

@SteveMacenski
Copy link
Member

I think just waiting on the BT stuff to sync (and perhaps #4485 (comment)?)? Otherwise, let me know when we can retrigger CI for this to go on!

@clalancette
Copy link
Contributor Author

I think just waiting on the BT stuff to sync (and perhaps #4485 (comment)?)? Otherwise, let me know when we can retrigger CI for this to go on!

Yep, exactly. Once those are synced out, we can rerun CI on this and it should pass.

@SteveMacenski
Copy link
Member

OK, if there's others you can open while we wait, happy to have a few of these open in parallel. Whatever works for you :-)

Copy link
Contributor

mergify bot commented Jun 27, 2024

This pull request is in conflict. Could you fix it @clalancette?

@SteveMacenski
Copy link
Member

Fixed the merge conflict 👍

Copy link
Contributor

mergify bot commented Jun 27, 2024

@clalancette, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

Copy link
Contributor

mergify bot commented Jul 5, 2024

This pull request is in conflict. Could you fix it @clalancette?

@clalancette clalancette force-pushed the clalancette/main-nav2-behavior-tree branch from f296ef5 to abf323a Compare July 8, 2024 15:48
Copy link
Contributor

mergify bot commented Jul 8, 2024

@clalancette, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

This commit does a number of things:

1.  Switches to using target_link_libraries everywhere.
This gives us finer-grained control over what dependencies
are exported to downstream as public, or private.  In the
particular case of nav2_behavior_tree, this actually doesn't matter
*too* much, but it will help for other packages.
2.  Moves the include directory down one level to
include/${PROJECT_NAME}, which is best practice in ROS 2
since Humble.
3.  Makes sure to export nav2_behavior_tree as a CMake target, so
downstream users of it can use that target.
4.  Removes the use of boost.  To do this, we had to introduce
our own version of the "split_string" method.
5.  Moves the test_action_server.hpp file into the main include
directory.  This is slightly odd, but because downstream packages
(opennav_docking_bt) depend on this header file to compile
their own tests, it is technically part of the public interface.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette clalancette force-pushed the clalancette/main-nav2-behavior-tree branch from abf323a to 7bd5722 Compare July 22, 2024 18:21
@clalancette
Copy link
Contributor Author

All right, so this now builds against the latest Rolling. Unfortunately, it looks like the test_goal_updater_node test is failing now, but that is also failing on the main branch for me locally. So I think this is blocked until that is fixed, then we can consider merging this.

@SteveMacenski
Copy link
Member

SteveMacenski commented Jul 22, 2024

Unfortunately, it looks like the test_goal_updater_node test is failing now, but that is also failing on the main branch for me locally. So I think this is blocked until that is fixed, then we can consider merging this.

Its failing in our CI too for the last ~week or so, I'm not sure why that popped up out of no where, but that's not related to this PR. We can merge this now I think and no worse for ware. Are you OK with that?

@clalancette
Copy link
Contributor Author

Its failing in our CI too for the last ~week or so, I'm not sure why that popped up out of no where, but that's not related to this PR. We can merge this now I think and no worse for ware. Are you OK with that?

Yep, that sounds good to me. Thanks!

@SteveMacenski SteveMacenski merged commit ba247b4 into ros-navigation:main Jul 23, 2024
9 of 10 checks passed
@clalancette clalancette deleted the clalancette/main-nav2-behavior-tree branch July 23, 2024 19:46
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