BTActionServer always loads behavior tree XML files from text, which breaks relative includes in the XML

[skyler237]

The XMLParser in the BehaviorTree.CPP can load a behavior tree from text or from file. When the behavior tree is loaded from a file, the parent path of the file is used as the current path for including other subtrees with relative paths. When the behavior tree is loaded from text, the current working directory is used as the parent path for relative includes, which is unreliable.

The BT Action Server always reads the xml file into a string, then loads the behavior tree from text, which breaks the ability to do reliable relative includes. Is there a good reason that behavior tree XML files are always loaded from text?

https://github.com/ros-planning/navigation2/blob/664c09f6d4dfd769592b2611a1461c9acf26b1f8/nav2_behavior_tree/include/nav2_behavior_tree/bt_action_server_impl.hpp#L179

Another workaround here is to use the "ros_pkg" attribute in the xml "include" tag, but when I use this on Galactic, I get the error saying that the behavior tree library was not compile for ROS support. In the behavior tree CMakeList.txt, it is checking if catkin/ament were used to build the package and sets the appropriate #define values accordingly. Is there a way for that library to be built with ament_cmake?

[SteveMacenski]

I defer to @facontidavide as to which he feels is best practice. It matters not to me, though sounds like this is a topic that might be worthy of a ticket in BT.CPP.

[facontidavide]

feel free to move this question to BT.CPP repo

[SteveMacenski]

I don't have those permissions on your side. @skyler237 please reopen ticket on BT.CPP and tag me so I can track the conversation / follow up as necessary 😄

[mrmitchadams]

Part of this question may be better suited for discussion in BT.CPP, but one point @skyler237 made was that bt_->createTreeFromText() is used in bt_action_server_imp.hpp rather than bt_->createTreeFromFile(). Since bt_action_server_imp.hpp belongs to nav2, I think it is still appropriate to discuss here whether there is any reason to continue using the createTreeFromText() method rather than the createTreeFromFile(), since switching to the latter may resolve this issue.

[SteveMacenski]

I simply deferred to Davide, but I have no issue w.r.t. moving to createTreeFromFile as long as it works. Perhaps the create from text is a separate issue to make that work with subtrees as well. As far as I can tell from the API, it shouldn't matter to us one way or another, but I think its important Davide knows about the issue. Since its been filed, I think we can move forward with this easy work around.

I'd be happy to field a PR along those lines if that seems reasonable to everyone 👍

[SteveMacenski]

@mradams-bastian want to open a PR?

[mrmitchadams]

Yah, I can do that here soon

[SteveMacenski]

👀

[facontidavide]

Soooo.
My suggestion is to stop using include.
There, I said it.
I specifically introduce an improvement in the API to avoid the problem you are discussing, described here: https://www.behaviortree.dev/docs/tutorial-basics/tutorial_07_multiple_xml

This gives you more control and will work with both Text and Files. If you have any problem and something doesn't work as advertized, let me know

[SteveMacenski]

@facontidavide is there any downside of us moving to using createTreeFromFile instead of createTreeFromText for the BT Navigator? It seems to me to be a parallel and perfectly acceptable option if it resolves some issue for some users. I don't think the choice of one or another was terribly well thought out in BT Navigator. We use the text one for unit testing so I think that just bled here too. I don't know if that would resolve the subtree problem, but another user mentions it could be helpful. I figure that + your suggestion here would resolve this ticket.

[facontidavide]

If it works for you, I don't have a technical preference.
I think the approach I propose in tutorial 7 has less pitfalls, but if you are happy with include, nothing stops you

[SteveMacenski]

OK - sounds like the answer is to use BT.CPP best practices + the update in #3306 to close this ticket. Waiting on CI and will close once complete.

[mrmitchadams]

Thanks @SteveMacenski for taking care of that and sorry for not having done it myself yet.

In response to using the recommended method for registering subtrees, it seems like the registerBehaviorTreeFromFile() followed by createTree() methods used in the tutorial would replace the createTreeFromFile() method we will be using once this is merged. To enable users to register their own subtrees and not have to use "include" we would have to refactor loadBehaviorTree() such that it accepts a list of "bt_xml_filename" strings rather than a single string. We could open a new thread for this, but I'd say we run with these latest changes/the "include" method of specifying subtrees for now and see if everything works out fine with it

[SteveMacenski]

This isn't a problem I've run into before, so if you open a PR to make the changes as required for your application, I'd be happy to take a look and merge them. I don't really see why you'd need multiple strings though, I don't think making the BT from file even takes multiple filepath strings as an argument.

[mrmitchadams]

In the tutorial linked by @facontidavide, registerBehaviorTreeFromFile() is called multiple times with different filepaths (each representing a different subtree) before createTree() is called. But in short, if the changes you just made resolve the issue I was experiencing, and everyone else is happy with how it is, I will not worry about any of that.