Fix/prevent warning when loading bt

[Jad-ELHAJJ]

---

Basic Info

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

---

Description of contribution in a few bullet points

This PR resolves the BTCPP warning that is logged when loadBehaviorTree() calls createTreeFromFile() immediately after registerTreeFromFile()

The updated approach ensures that createTree() is called directly after registerTreeFromFile() when appropriate, thereby eliminating the warning.

To support this, a small interface change was introduced:

• The behavior_tree goal variable can now represent either a BT file path (like before) or a BT ID.
• loadBehaviorTree() will extract a BT ID if the input is an XML file.
• Otherwise, it assumes the input is already a BT ID and directly calls createTree() using that identifier.

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
• Should this be backported to current distributions? If so, tag with backport-*.

[mergify]

@Jad-ELHAJJ, 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]

@Jad-ELHAJJ, 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).

[SteveMacenski]

This causes all of the tests related to this test to fail, including system tests which are the stack functioning. Did you test your changes locally before pushing? Using CI to test changes isn't good practice 😄

Linking the issue report comment: #5494 (comment)

[Jad-ELHAJJ]

@claude review the PR

[SteveMacenski]

They all still fail, please look into that :-)

[Jad-ELHAJJ]

They all still fail, please look into that :-)

Yes, I am aware of that. I am still investigating the issue. Once its ready to review I will let you know. Thanks!

[mergify]

@Jad-ELHAJJ, 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).

[codecov]

Codecov Report

❌ Patch coverage is 86.53846% with 7 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...clude/nav2_behavior_tree/bt_action_server_impl.hpp	80.00%	4 Missing ⚠️
nav2_behavior_tree/src/behavior_tree_engine.cpp	90.90%	2 Missing ⚠️
...2_bt_navigator/src/navigators/navigate_to_pose.cpp	75.00%	1 Missing ⚠️

Files with missing lines	Coverage Δ
...nclude/nav2_behavior_tree/behavior_tree_engine.hpp	100.00% <ø> (ø)
...ee/include/nav2_behavior_tree/bt_action_server.hpp	83.33% <100.00%> (ø)
...avigator/src/navigators/navigate_through_poses.cpp	87.91% <100.00%> (ø)
...2_bt_navigator/src/navigators/navigate_to_pose.cpp	81.48% <75.00%> (ø)
nav2_behavior_tree/src/behavior_tree_engine.cpp	86.79% <90.90%> (+2.92%)	⬆️
...clude/nav2_behavior_tree/bt_action_server_impl.hpp	88.94% <80.00%> (+0.11%)	⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mini-1235]

I can actually see the warnings in the latest commit

[component_container_isolated-7] [INFO 1759424050.256294767] [bt_navigator]: Activating (on_activate() at /home/mini1235/navigation2/nav2_bt_navigator/src/bt_navigator.cpp:135)
[component_container_isolated-7] WARNING: You executed BehaviorTreeFactory::createTreeFromFile after registerBehaviorTreeFrom[File/Text].
[component_container_isolated-7] This is NOT, probably, what you want to do.
[component_container_isolated-7] You should probably use BehaviorTreeFactory::createTree, instead
[component_container_isolated-7] WARNING: You executed BehaviorTreeFactory::createTreeFromFile after registerBehaviorTreeFrom[File/Text].
[component_container_isolated-7] This is NOT, probably, what you want to do.
[component_container_isolated-7] You should probably use BehaviorTreeFactory::createTree, instead

To reproduce, launch

ros2 launch nav2_bringup tb4_loopback_simulation_launch.py

[SteveMacenski]

@Jad-ELHAJJ can you verify and let me know if you see this as well still locally too?

[Jad-ELHAJJ]

@SteveMacenski I implemented your suggestion. I also added tests that covers most of the cases. So now, the user can use the filepath and get a warning to migrate. The user can also use the BT ID. And in both cases the user will be notified if multiple behavior trees have the same ID.

@Jad-ELHAJJ can you verify and let me know if you see this as well still locally too?

As mentioned in the comment above, we settled on letting the user use either file or ID. And in the case of file, the user will get a warning.

[SteveMacenski]

I thought that was only if they had the same ID, not for all tree files. Can you clarify?

If just the same ID - @mini-1235 just needs to update the tree IDs to be unique

[mini-1235]

I am not sure what those warnings mean, but I think they shouldn't show up when launching nav2's default bringup (tb3,tb4)

[SteveMacenski]

I agree, the point of this PR was to remove that. If that's still happening for the default Nav2 bringup with the default Nav2 trees which have that ID set as unique for each tree, this feature may need to be reverted.

[Jad-ELHAJJ]

Let me clarify. This warning has showed up after adding an important feature to the BT “support subtrees”. Briefly, loading a behavior tree that includes a subtree requires registering, beforehand, all the trees that are used as subtrees in the loaded main tree, therefore the usage of registerTreeFromFile before createTreeFromFile. When these two functions get called sequently, BTCPP logs a warning, but the functionality will still be the same. But in any way, in order to skip that warning, the good approach is to use registerTreeFromFile and createTree. CreateTree function takes as input the BT ID not the file path. And in this feature we also supported ID as behavior tree in the interface not only file path. So, the best practice is to have unique BT IDs and pass to the loadBehaviorTree the ID instead of the file path. But, Steve requested to still support file path in order to not break user’s work. We also added docs to the migration guide that highlights that change.
Supporting subtrees feature is very useful, it’s being used to trigger the same peace of BT(that you create once) in multiple navigators. And adding the ability to use BT ID throughout the interface makes life easier.

[Jad-ELHAJJ]

A fix PR will be open soon to refactor that, making sure that BTCPP warning would never be logged. The final logic will be:

• If a user has conflicting IDs and specifies a file name, only the Behavior Tree (BT) from that file will be loaded. Other trees with the same ID will be ignored. Subtree functionality will fail if subtrees have conflicting IDs.
• If a user has unique IDs (no conflicts) and specifies a file name, all BTs are loaded, and the subtree feature works correctly.
• If a user has unique IDs and specifies a BT ID, all BTs are loaded, and the subtree feature works correctly.
• If a user has conflicting IDs and specifies a BT ID, an error is reported, but the system still attempts to load the BT corresponding to the specified ID.

[SteveMacenski]

So, the best practice is to have unique BT IDs and pass to the loadBehaviorTree the ID instead of the file path.

I believe I've mentioned in other reviews that I do not believe this is necessarily best practice and we will need to support both as first-class non-error-causing citizens :-) If folks want to use files, they should be able to. Complaining about non-unique names I think is OK as a nudge for migration, but there shouldn't be loading errors.

We highlighted that this is a new feature that is supported, not that you must migrate to using ID-based requests.I believe my review comments and request to remove certain error messages from previous versions reflects that intent.

I think your new PR points make sense. When can we expect that?

[Jad-ELHAJJ]

So, the best practice is to have unique BT IDs and pass to the loadBehaviorTree the ID instead of the file path.

I believe I've mentioned in other reviews that I do not believe this is necessarily best practice and we will need to support both as first-class non-error-causing citizens :-) If folks want to use files, they should be able to. Complaining about non-unique names I think is OK as a nudge for migration, but there shouldn't be loading errors.

We highlighted that this is a new feature that is supported, not that you must migrate to using ID-based requests.I believe my review comments and request to remove certain error messages from previous versions reflects that intent.

I think your new PR points make sense. When can we expect that?

There you go :-) #5594

[adivardi]

Hi,
I am trying to move to the newest main branch and having issues with this PR.
When using a SubTree, defined in the same file as the main tree, it is now not possible to use a filepath to specify the BT. Only ID works. When requesting a file (or using a file as the default BT), the subtree fails due to remapping. If I specify the ID of the main tree, it works fine, so I am sure it is not an xml issue.

I have checked all Subtrees have unique IDs, and my xml file also uses main_tree_to_execute="ID_of_the_main_tree".

I must also say that the migration guide is really unclear. I thought it is just a new option to load BTs from different directories, but actually one must rename all the BT IDs and add a new param bt_search_directories to use any custom BTs.

[SteveMacenski]

When using a SubTree, defined in the same file as the main tree, it is now not possible to use a filepath to specify the BT.

Interesting. This works correctly when they are in different files, correct? I can see that it may be due to extracting the BT IDs from the XMLs in BtActionServer<ActionT, NodeT>::loadBehaviorTree for loading. We don't try to register multiple trees from a file when multiple are present. Seems like a fixable issue to register all trees found in the file.

@Jad-ELHAJJ thoughts?

I must also say that the migration guide is really unclear. I thought it is just a new option to load BTs from different directories, but actually one must rename all the BT IDs and add a new param bt_search_directories to use any custom BTs.

I'd appreciate an update to the docs to make it more clear -- I don't have the best sense of what's not clear to you, so you're in a good position to clarify for the next reader :-)

The renaming of the IDs is a pain, but only if you want to use the IDs. I asked @Jad-ELHAJJ to make sure that you could use it with filenames instead without that and that should work... From the current version of the branch, on first glance it seems like it should work even if you gave the bt_search_directories a bogus path. It'll try to load those but your main XML provided in the action is loaded separately

[adivardi]

I'd appreciate an update to the docs to make it more clear -- I don't have the best sense of what's not clear to you, so you're in a good position to clarify for the next reader :-)

Sure, I can do that, though let's solve the loading issue first.

The renaming of the IDs is a pain, but only if you want to use the IDs. I asked @Jad-ELHAJJ to make sure that you could use it with filenames instead without that and that should work...

Oh, is it? I got a lot of warnings about duplicated IDs, so I assume it was a must (I did try with my xml but that has the subTree issue. I didn't try with another xml)
Edit: hmm, this also doesn't work for me. If I have multiple files with MainTree, trying to run one of them is executing the other one). Though I don't think renaming the IDs is a big deal, so I can leave with that.

[SteveMacenski]

I think the duplicate tree IDs should be fine even with warnings as long as you use the file path (not sure though if you include subtrees). Is what you're trying still using subtrees or no?

[adivardi]

It did have subTrees, but it was running the other file completely.
But the much bigger issue is the subtrees in the same file don't work when specifying a file.
Should I open an issue for this?

[SteveMacenski]

Yes please. Please also tag @Jad-ELHAJJ in that.

Do you see how a completely different file runs when conflicting names? It seems like from my reading of the code that should work correctly

[mini-1235]

@adivardi can you also provide a reproducible test case? If the author doesn't reply, I can go ahead and fix this in a few days

[mini-1235]

Should I open an issue for this?

@adivardi any updates? 😄

[adivardi]

Should I open an issue for this?

@adivardi any updates? 😄

Sorry, quite busy few days.

For the subTree issue, simply extract a subtree from one of the default BT xml.
For example, I did it to the nav_through_poses xml here:
subtree.patch

Then you will get errors immediately when launching nav2:

[bt_navigator] [/opt/enway/ros2_ws/install/nav2_behavior_tree/include/nav2_behavior_tree/nav2_behavior_tree/bt_action_server_impl.hpp:327] Created BT from ID: NavToPoseWithConsistentReplanningAndIfPathBecomesInvalidEnway
[bt_navigator] [/opt/enway/ros2_ws/install/nav2_behavior_tree/include/nav2_behavior_tree/nav2_behavior_tree/bt_action_server_impl.hpp:437] Setting internal error error_code:9101, error_msg:Exception when creating BT tree from ID: Blackboard::get() error. Missing key [node]
[bt_navigator] [/opt/enway/ros2_ws/install/nav2_behavior_tree/include/nav2_behavior_tree/nav2_behavior_tree/bt_action_server_impl.hpp:194] Error loading BT: /opt/enway/ros2_ws/install/nav2_bt_navigator/share/nav2_bt_navigator/behavior_trees/navigate_through_poses_w_replanning_and_recovery.xml
[bt_navigator] [/opt/enway/ros2_ws/src/enway_ros2/navigation/navigation2/nav2_bt_navigator/src/bt_navigator.cpp:152] Deactivating
[bt_navigator] [/opt/enway/ros2_ws/install/nav2_ros_common/include/nav2_ros_common/nav2_ros_common/lifecycle_node.hpp:364] Destroying bond (bt_navigator) to lifecycle manager.
[lifecycle_manager_navigation] [/opt/enway/ros2_ws/src/enway_ros2/navigation/navigation2/nav2_lifecycle_manager/src/lifecycle_manager.cpp:315] Failed to change state for node: bt_navigator
[lifecycle_manager_navigation] [/opt/enway/ros2_ws/src/enway_ros2/navigation/navigation2/nav2_lifecycle_manager/src/lifecycle_manager.cpp:382] Failed to bring up all requested nodes. Aborting bringup.

---

Regarding the second issue of having to rename all BT, I think you can very quickly test that by undoing the change in this PR (renaming all BTs from MainTree to a unique name) and then trying navigation. By default BT_navigators use filename so I think that should work.
Let me know if you can't reproduce it and I will try it again. I can only get to it tomorrow anyway.

[mini-1235]

Thanks! Please file a ticket when you have time

Just to make sure I understand correctly: does issue 2 only cause problems when a subtree is used, or does it also happen even without any subtrees?

[adivardi]

Just to make sure I understand correctly: does issue 2 only cause problems when a subtree is used, or does it also happen even without any subtrees?

That's the part I didn't have time to double-check yet. I will try it out and update (probably tomorrow)

[adivardi]

@mini-1235
I opened an issue for the subTree crash: #5923

I also opened an issue f:
Regarding the duplicate IDs, I am a quite confused about what going on and whether it is just a confusing warning message or something is actually broken. So I opened another issue so we can discuss. #5924

[mini-1235]

Thanks, I will follow up in the corresponding ticket

[SteveMacenski]

Thanks!