navigation2
navigation2 copied to clipboard
ros-navigation
Revamp nav2_behavior_tree CMakeLists.txt to use modern idioms.
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:
- 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.
- Moves the include directory down one level to include/${PROJECT_NAME}, which is best practice in ROS 2 since Humble.
- Makes sure to export nav2_behavior_tree as a CMake target, so downstream users of it can use that target.
- Removes the use of boost. To do this, we had to introduce our own version of the "split_string" method.
- 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
@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).
![mergify[bot] avatar](https://avatars.githubusercontent.com/in/10562?v=4 "Published by a pub.dev verified publisher"){kind=small}
@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 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.
Are these changes backportable to Jazzy or only rolling?
They are backportable to Jazzy, assuming that https://github.com/BehaviorTree/BehaviorTree.CPP/pull/826 is released into Jazzy (it has not yet been, as far as I can tell).
@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?
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 https://github.com/ros-navigation/navigation2/pull/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.
Got it! Your help is much appreciated :saluting_face:
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 https://github.com/ros-navigation/navigation2/pull/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!
@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).
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.
I think just waiting on the BT stuff to sync (and perhaps https://github.com/ros-navigation/navigation2/pull/4485#discussion_r1655183962?)? Otherwise, let me know when we can retrigger CI for this to go on!
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.
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 :-)
@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, 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).
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.
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?
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!