[behavior_tree] Add generate_nav2_tree_nodes_xml

[facontidavide]

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	-
Primary OS tested on	Ubuntu 22.04
Robotic platform tested on	Not applicable
Does this PR contain AI generated software?	No

---

Description of contribution in a few bullet points

Currently the file nav2_tree_nodes.xml is updated by hand by the user. This is tedious and error-prone (current version is also outdated); furthermore, this is not what BT.CPP allows this step to be automated.

This PR introduces a tiny utility (meant to be used by the developers) to auto-generate that file.

In more details:

• Make the list of plugins available to the c++ code, using the cmake command configure_file.
• Use this feature to simplify one of the tests.
• Add generate_nav2_tree_nodes_xml

Description of documentation updates required from your changes

To be discussed. Where do you want me to add this documentation?

---

Future work that may be required in bullet points

This PR is not adding he modifications to nav2_tree_nodes.xml

This will be done in the a future PR

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]

CI says there are linting errors: https://app.circleci.com/pipelines/github/ros-planning/navigation2/10851/workflows/b171820c-fed3-4296-b3c9-4eb591fb4b0d/jobs/34087/tests

[SteveMacenski]

I assume this is automatically executed on build? The quick look at configure_file doesn't make me think i does, but I assume it does or else this workflow would add extra steps

I think this filename is misleading, its not generating the XML but just storing the index of default Nav2-included BT nodes. How about generate_nav2_node_list.cpp?

[facontidavide]

I think this filename is misleading, its not generating the XML

it is literally creating an XML with std::ofstream ... but I can change the name, of course.

I assume this is automatically executed on build?

No, it is not. The developer modifying a BT Node should run it and overwrite the previous XML with the new one.

But right now it is even worse, because we are asking to write the XML by hand and there is no guaranty of consistency (as a demonstration, right now it is WRONG in the main branch)

[facontidavide]

this workflow would add extra steps

That step ("double check if nav2_tree_node.xml is still valid") is there, but people just forget to do it.
This approach lowers the effort.

Maybe I can find a way to run it automatically, I don't know...

[SteveMacenski]

No, it is not. The developer modifying a BT Node should run it and overwrite the previous XML with the new one.

Oh, then this should be removed. You cannot expect the CI system or users to run some script just to get their systems working or unit tests compiling. This doesn't work for CI nor a reasonable developer workflow.

If this was autogenerated, then I love it! But it has to be executed somewhere.

[facontidavide]

Sorry, there is a misunderstanding from my side and maybe I answered something wrong
CI, user code and unit test will work just fine!!!
The file that is autogenerated is plugin_list.hpp. That is everything you need.

What I am talking about is the file nav2_tree_node.xml , that only Groot2 requires.

[SteveMacenski]

Oh ok. So this does generate the index that the plugins_list needs for the BT test + BT navigator. Cool then this works.

Now you got confused by this utility's name 😉 Rename suggestion to generate_nav2_node_list.cpp FTW

[facontidavide]

To put in perspective, I think you might want to have a look at the autogenerated file
nav2_tree_nodes.zip

[SteveMacenski]

But is this not the one groot wants? If not, then I don't really care how similar they are (just a purely pragmatist's standpoint)

[facontidavide]

yes, it is the file needed by Groot. This will always be correct, unlike the one that people edit by hand

[SteveMacenski]

Got it. Perhaps in a follow up PR, there is a way to have the source file update when compiled as well or as a CI job? Not required, but would round it off.

I was thinking also potentially having a nav2_behavior_tree unit test that checks the source version and the recently compiled version and fails the test if they don't match. That way there's something that pops up in CI if they're out of sync. That sounds comparatively easier since CI has the source + compiled versions available and executed in the workspace root

"No" is fine too. Just making the test / BT Navigator list update automatically is still an improvement

[facontidavide]

I was thinking also potentially having a nav2_behavior_tree unit test that checks the source version and the recently compiled version and fails the test if they don't match.

It is a good idea and should be quite easy to implement. I will see if I can add it to this PR

[SteveMacenski]

Any update :-)

[facontidavide]

You tell me :)

The idea of adding verification to CI is nice, but with your permission, I prefer to keep it for a future PR.

What do you think is missing, to merge this one?
Do you want me to also add the generated XML in this PR (kind of make sense, right?)

[SteveMacenski]

I will see if I can add it to this PR

I was waiting on this. Yes, it can be a follow up PR.

Do you want me to also add the generated XML in this PR (kind of make sense, right?)

Yes, but also update the BT Navigator's default BT plugins to use this like you do for the unit test.

Will this all work when we distribute as binaries? I haven't used configure_file with templates like this before, so I'm seeing how configure_file populates BT_PLUGIN_LIST with plugin_libs -- but how is plugin_libs set when this is executed on a build to contain all of the BT plugins? I'm not quite sure how this works even for local source builds, please explain.

[facontidavide]

Will this all work when we distribute as binaries? I haven't used configure_file with templates like this before,

yes, it will, the string with the list of plugins is compiled into the code.

Cmake will generate a header file and this header file is used when compiling the binary.

[SteveMacenski]

yes, it will, the string with the list of plugins is compiled into the code.

Please point to me where. I believe you, I just need to maintain this so I need to understand what's going on. I don't see where its compiled as part of these changes. Where does that string plugins_list come from? Is that something we set in Nav2 or BT.CPP is doing? Will this cover all BT plugins compiled in a workspace or only for this package? How would a user add in their own to this?

[facontidavide]

Let me explain 😄

So, CMAKE is aware of the list of plugins... because it is the one responsible for compiling them!

Also, conveniently, we have a list of plugins names in the variable ${plugin_libs}

https://github.com/ros-planning/navigation2/blob/main/nav2_behavior_tree/CMakeLists.txt#L219

We want to avoid repeating ourselves, by copying this string from CMake to C++ at compilation.

A potential solution could be to pass it as a definition with target_compile_definitions.

Another solution is the one presented here.
configure_file is a command that allows us to autogenerate files, using a template file.

In this case, we want to autogenerate a header file, containing an harcoded string that should be exactly the same as ${plugin_libs} in CMakeLists.txt.

Our template file contain this code:

namespace nav2::details 
{
  const char* BT_PLUGIN_LIST = "@plugin_libs@";
}

@plugin_libs@ will be substituted by the content of ${plugin_libs}

This is the code generated by CMake:
plugins_list.zip

Now, we can include this file as a regular header file!

Every time a new plugin is added to CMakeLists.txt (and the variable ${plugin_libs}), the string nav2::details::BT_PLUGIN_LIST will be updated auto-magically !!!

This is more maintanable than the code in master, where people should remember to update those lists in multiple places.

Since all this happens before compilation (header generation) and later at compilation time, there is no problem with distributing only the binaries.

I hope I helped you understand.
Simple tutorial here: https://cliutils.gitlab.io/modern-cmake/chapters/basics/comms.html

[SteveMacenski]

Thank you, sorry to make you spell it out - I was thinking you were fully setting up these library paths, not leveraging what we already have setup in Nav2. I scoured your changes so many times looking for something subtle I missed I assumed something must be going on in the BT.CPP side that required your knowledge 😆

This is sensible for defaults, but a real pain in the butt for users that need to add in their own BT.CPP nodes. So I suggest the following action:

• We continue to have the parameter for BT nodes to add https://github.com/ros-planning/navigation2/blob/main/nav2_bt_navigator/src/bt_navigator.cpp#L129
• But now, we use your utility boost::split(plugin_libs, nav2::details::BT_PLUGIN_LIST, boost::is_any_of(";")); to append into it all of Nav2's nodes
• Then, we can also change the plugin_lib_names default type to an empty vector of strings
• Then, we can tell users that this variable is only for user customized BT nodes and that all Nav2 nodes come for free when using the Nav2 BT navigator

You have some linting issues: https://app.circleci.com/pipelines/github/ros-planning/navigation2/10851/workflows/b171820c-fed3-4296-b3c9-4eb591fb4b0d/jobs/34087/tests.

Fix linting + add that widget into the BT Navigator and we can merge!

If you also pull in main, those test failures should disappear.

[facontidavide]

Correct.

This is not mutually exclusive with user's plugins, as you said. I can change the name of the variable to a more verbose BT_BUILTIN_PLUGIN_LIST

[SteveMacenski]

Sure, a new name would work, but please do the additional changes so that this is a complete solution.

[facontidavide]

Check the latest commit. I believe this is what you suggested

[mergify]

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

[SteveMacenski]

A ton of tests are failing: https://app.circleci.com/pipelines/github/ros-planning/navigation2/11003/workflows/a78174e2-dd8f-4547-b3b9-dc7f1b9843aa/jobs/34506/tests

I suspect it has to do with plugin_lib_names: [], because there are no entries, it probably can't decide what the type is to meet the static typing of parameters requirements.

TODO STEVE

• Migration guide about change
• Configuration guide parameter update description

[facontidavide]

damn! let me investigate

[codecov]

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (c59e0f3) 90.30% compared to head (a6325e5) 90.22%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
...behavior_tree/src/generate_nav2_tree_nodes_xml.cpp	0.00%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4073      +/-   ##
==========================================
- Coverage   90.30%   90.22%   -0.09%     
==========================================
  Files         431      432       +1     
  Lines       19394    19406      +12     
==========================================
- Hits        17514    17509       -5     
- Misses       1880     1897      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[SteveMacenski]

I updated the docs, that fix and we can merge