Skip to content

Fix error parsing subtree when loading from file path#5950

Merged
SteveMacenski merged 20 commits intoros-navigation:mainfrom
mini-1235:fix/subtree
Feb 20, 2026
Merged

Fix error parsing subtree when loading from file path#5950
SteveMacenski merged 20 commits intoros-navigation:mainfrom
mini-1235:fix/subtree

Conversation

@mini-1235
Copy link
Collaborator

@mini-1235 mini-1235 commented Feb 11, 2026


Basic Info

Info Please fill out this column
Ticket(s) this addresses Fixes #5923
Primary OS tested on Ubuntu
Robotic platform tested on (Steve's Robot, gazebo simulation of Tally, hardware turtlebot)
Does this PR contain AI generated software? (No; Yes and it is marked inline in the code)
Was this PR description generated by AI software? Out of respect for maintainers, AI for human-to-human communications are banned

Description of contribution in a few bullet points

Description of documentation updates required from your changes

Description of how this change was tested


Future work that may be required in bullet points

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-*.

…5938)"

This reverts commit 517ebf4.

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
This reverts commit 00dfd67.

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
@mini-1235
Copy link
Collaborator Author

This PR reverts #5938 and #5594.

I believe the correct logic should be:

@adivardi
Copy link
Contributor

Thanks @mini-1235

Tested:

  • With all BTs having unique IDs
    • requesting both filepath & ID work, with subTree both before and after the main tree
  • With some duplicated IDs
    • requesting filepath work
    • requesting ID: Actually also work, but I am getting the confusing warning again Duplicate BT IDs found. Make sure all BT IDs are unique! ID: MainTree File:...
      - maybe that's intended? I don't have multiple BTs for the same action to test though, only multiple ones for different navigators.

@mini-1235
Copy link
Collaborator Author

requesting ID: Actually also work, but I am getting the confusing warning again Duplicate BT IDs found. Make sure all BT IDs are unique! ID: MainTree File:...

  • maybe that's intended? I don't have multiple BTs for the same action to test though, only multiple ones for different navigators.

Yeah, I think this is expected since it can be confusing which BT is being referenced if there are duplicate ids and the tree is requested by id. In that case, I would expect the user to see this warning and update their BTs accordingly. But I am open to suggestions on how to make this clearer, perhaps doing something similar to what was proposed in #5938?

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
@mini-1235
Copy link
Collaborator Author

The other failing test seems to be related to 797a171

@adivardi
Copy link
Contributor

Yeah, I think this is expected since it can be confusing which BT is being referenced if there are duplicate ids and the tree is requested by id. In that case, I would expect the user to see this warning and update their BTs accordingly. But I am open to suggestions on how to make this clearer, perhaps doing something similar to what was proposed in #5938?

I think that would be nice. Thought it is already better since it doesn't have the skipping BT file while not actually skipping anything 🤭

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.

Seems like this may drop a feature or two accidentally

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
@codecov
Copy link

codecov bot commented Feb 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
...nclude/nav2_behavior_tree/behavior_tree_engine.hpp 100.00% <100.00%> (ø)
...ee/include/nav2_behavior_tree/bt_action_server.hpp 83.33% <ø> (ø)
...clude/nav2_behavior_tree/bt_action_server_impl.hpp 87.98% <100.00%> (+0.29%) ⬆️
nav2_behavior_tree/src/behavior_tree_engine.cpp 89.28% <100.00%> (+0.39%) ⬆️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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 LGTM as long as it does what we need functionally!

auto id = bt_->extractBehaviorTreeID(entry.path().string());
if (id.empty()) {
auto tree_info = bt_->parseTreeInfo(entry.path().string());
if (tree_info.all_ids.empty()) {
Copy link
Member

Choose a reason for hiding this comment

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

main_id instead?

Copy link
Collaborator Author

@mini-1235 mini-1235 Feb 17, 2026

Choose a reason for hiding this comment

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

What about renaming all_ids to behavior_tree_ids, so in the new struct BTInfo:

main_id -> the ID provided in main_tree_to_execute
behavior_tree_ids -> all <BehaviorTree> tag IDs

Can you clarify? I don't think we are using main_id in this function

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the main_id be checked as required, not all_ids? But I think from the recent comments below, this point is much less important - semantically it will have the same effect

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are looping through all ids to check if it has been registered, so I think we should still check all instead of main here

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
@mini-1235
Copy link
Collaborator Author

Tested:
With all BTs having unique IDs
requesting both filepath & ID work, with subTree both before and after the main tree
With some duplicated IDs
requesting filepath work
requesting ID: Actually also work, but I am getting the confusing warning again Duplicate BT IDs found. Make sure all BT IDs are unique! ID: MainTree File:...

I tested these again with my latest version, and everything looks fine to me

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
@adivardi
Copy link
Contributor

I tested these again with my latest version, and everything looks fine to me

I didn't do a full test again now, just tested our main xml file, and it is working with this PR, even when requesting a filename

@mini-1235
Copy link
Collaborator Author

I tested these again with my latest version, and everything looks fine to me

I didn't do a full test again now, just tested our main xml file, and it is working with this PR, even when requesting a filename

Thank you @adivardi !

…present

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
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.

LGTM - did you update / add all the testing you wanted?

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
@mini-1235
Copy link
Collaborator Author

022477e added some test cases, hopefully this covers all the corner cases :)

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.

@adivardi are you happy with the final changes as well?

@adivardi
Copy link
Contributor

In general looks good, also tested a few scenarios and it is working fine.
The Skipping conflicting BT XML files shows up correctly when I have conflicting IDs.

@SteveMacenski SteveMacenski merged commit 9132053 into ros-navigation:main Feb 20, 2026
17 checks passed
@SteveMacenski
Copy link
Member

SteveMacenski commented Feb 20, 2026

Great, thanks for being persistent on this issue & thanks @mini-1235 for handling this!

@mini-1235 mini-1235 deleted the fix/subtree branch February 21, 2026 07:36
Pana1v pushed a commit to Arnav-panjla/navigation2 that referenced this pull request Feb 21, 2026
…#5950)

* Revert "Changing BT loading warning to be more clear (ros-navigation#5938)"

This reverts commit 517ebf4.

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Revert "Bt warning fix (ros-navigation#5594)"

This reverts commit 00dfd67.

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix subtree parsing error when using filepath

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Revert unrelated changes

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix test

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Apply suggestions

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Second prototype

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix test

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Apply suggestions

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Only require main id when using filepath

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix comment formatting in behavior_tree_engine.cpp

Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix test

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Require main_tree_to_execute to be set if multiple BehaviorTree tags present

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Add test to cover corner case

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

---------

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>
Signed-off-by: panav <panav@10xconstruction.com>
Pana1v pushed a commit to Arnav-panjla/navigation2 that referenced this pull request Feb 21, 2026
…#5950)

* Revert "Changing BT loading warning to be more clear (ros-navigation#5938)"

This reverts commit 517ebf4.

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Revert "Bt warning fix (ros-navigation#5594)"

This reverts commit 00dfd67.

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix subtree parsing error when using filepath

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Revert unrelated changes

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix test

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Apply suggestions

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Second prototype

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix test

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Apply suggestions

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Only require main id when using filepath

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix comment formatting in behavior_tree_engine.cpp

Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix test

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Require main_tree_to_execute to be set if multiple BehaviorTree tags present

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Add test to cover corner case

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

---------

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>
Signed-off-by: panav <panav@10xconstruction.com>
Pana1v pushed a commit to Arnav-panjla/navigation2 that referenced this pull request Feb 21, 2026
…#5950)

* Revert "Changing BT loading warning to be more clear (ros-navigation#5938)"

This reverts commit 517ebf4.

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Revert "Bt warning fix (ros-navigation#5594)"

This reverts commit 00dfd67.

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix subtree parsing error when using filepath

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Revert unrelated changes

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix test

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Apply suggestions

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Second prototype

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix test

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Apply suggestions

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Only require main id when using filepath

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix comment formatting in behavior_tree_engine.cpp

Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix test

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Require main_tree_to_execute to be set if multiple BehaviorTree tags present

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Add test to cover corner case

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

---------

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>
Pana1v pushed a commit to Arnav-panjla/navigation2 that referenced this pull request Feb 22, 2026
…#5950)

* Revert "Changing BT loading warning to be more clear (ros-navigation#5938)"

This reverts commit 517ebf4.

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Revert "Bt warning fix (ros-navigation#5594)"

This reverts commit 00dfd67.

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix subtree parsing error when using filepath

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Revert unrelated changes

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix test

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Apply suggestions

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Second prototype

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix test

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Apply suggestions

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Only require main id when using filepath

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix comment formatting in behavior_tree_engine.cpp

Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix test

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Require main_tree_to_execute to be set if multiple BehaviorTree tags present

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Add test to cover corner case

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

---------

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>
Signed-off-by: panav <panav@10xconstruction.com>
Pana1v pushed a commit to Arnav-panjla/navigation2 that referenced this pull request Feb 22, 2026
…#5950)

* Revert "Changing BT loading warning to be more clear (ros-navigation#5938)"

This reverts commit 517ebf4.

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Revert "Bt warning fix (ros-navigation#5594)"

This reverts commit 00dfd67.

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix subtree parsing error when using filepath

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Revert unrelated changes

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix test

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Apply suggestions

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Second prototype

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix test

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Apply suggestions

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Only require main id when using filepath

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix comment formatting in behavior_tree_engine.cpp

Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix test

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Require main_tree_to_execute to be set if multiple BehaviorTree tags present

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Add test to cover corner case

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

---------

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>
Signed-off-by: panav <panav@10xconstruction.com>
Signed-off-by: Maurice <mauricepurnawan@gmail.com>
EricoMeger pushed a commit to EricoMeger/navigation2 that referenced this pull request Feb 22, 2026
…#5950)

* Revert "Changing BT loading warning to be more clear (ros-navigation#5938)"

This reverts commit 517ebf4.

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Revert "Bt warning fix (ros-navigation#5594)"

This reverts commit 00dfd67.

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix subtree parsing error when using filepath

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Revert unrelated changes

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix test

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Apply suggestions

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Second prototype

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix test

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Apply suggestions

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Only require main id when using filepath

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix comment formatting in behavior_tree_engine.cpp

Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>

* Lint

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Fix test

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Require main_tree_to_execute to be set if multiple BehaviorTree tags present

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

* Add test to cover corner case

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

---------

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: Maurice Alexander Purnawan <mauricepurnawan@gmail.com>
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.

SubTrees in the same BT xml file lead to crash when a filepath is used

3 participants