Issue with ports not being remapped with subtreeplus on v3.8

[Rid0n]

Here's my code.

I have the outermost tree passing down values to a subtree which is then passing down values to another subtree which ticks a navigation tree. Everything with the __autoremap flag set to true. Very simple.

<root main_tree_to_execute="Tree1">
<include path="./navigation_tree.xml"/>

<BehaviorTree ID="Tree1">

    <Sequence>

        <SubTreePlus ID="Tree2" __autoremap="true"/>

    </Sequence>

</BehaviorTree>

<BehaviorTree ID="Tree2">

    <Sequence>

        <SubTreePlus ID="Tree3" __autoremap="true"/>

    </Sequence>

</BehaviorTree>

<BehaviorTree ID="Tree3">

    <Sequence>

        <SubTreePlus ID="NavigateTree" __autoremap="true"/>

    </Sequence>

</BehaviorTree>

</root>

I expect there not to be any issues, however for some reason I'm getting an error

[bt_navigator]: Exception when loading BT: Blackboard::get() error. Missing key [node]

I found that the issue lies specifically in the switch from Tree2 to Tree3. Every other subtree node does successfully remap everything.

If I change one line
<SubTreePlus ID="Tree3" __autoremap="true"/>
To
<SubTreePlus ID="Tree3" goal="{goal}" node="{node}" bt_loop_duration="{bt_loop_duration}" tf_buffer="{tf_buffer}" server_timeout="{server_timeout}" />
It all works fine.

What's up here?

[facontidavide]

Version 4.X has been in development for almost 1 year now. Keep maintaining 3.8 is really a struggle for me.

Said that, would you be so kind to give me a unit test that I can use to reproduce the issue?

[facontidavide]

These are my current tests: https://github.com/BehaviorTree/BehaviorTree.CPP/blob/v3.8/tests/gtest_subtree.cpp

[facontidavide]

I was able to reproduce here: https://github.com/BehaviorTree/BehaviorTree.CPP/tree/issue563
it is indeed a bug. I will let you know once it is is fixed

[facontidavide]

Hi, problem fixed in branch issue563.

Would you be so kind to test it?

Once you confirm, I will merge and release a new version

[Rid0n]

still there for me D:
it also broke the delicate ways in which it did work haha

trying to find a test case where that'll show

[facontidavide]

oh god, don't tell me that!
Please tell me more, this issues is very important and I want to get it fixed as soon as possible.

[facontidavide]

also, if you experienced a regression, please share that with me. I hope I am not breaking anything with these changes (my unit tests pass)

[Rid0n]

So, the unit test passes fine, but when I'm trying a simple thing with the nav2 stack, it fails.

<root main_tree_to_execute="Tree">

<BehaviorTree ID="Tree">

    <Sequence>

        <SubTreePlus ID="Main" __autoremap="true"/>

    </Sequence>

</BehaviorTree>

<BehaviorTree ID="Main">

    <Wait wait_duration="1"/>

</BehaviorTree>

</root>

fails to instantiate tree with
Blackboard::get() error. Missing key [node]

the bt_navigator node crashes entirely, so it looks like there's something nasty together with nav2 specifically.
a bunch of nav2_behavior_tree unit tests now fail too

[facontidavide]

Sorry, I just implemented it and it works.

I don't know how to help you sorry.

class WaitNode : public BT::StatefulActionNode
{
  std::chrono::system_clock::time_point _deadline;
public:
  WaitNode(const std::string& name, const BT::NodeConfiguration& config) :
    BT::StatefulActionNode(name, config)
  {}

  virtual NodeStatus onStart() override
  {
    double time = getInput<double>("wait_duration").value();
    _deadline =
        std::chrono::system_clock::now() + std::chrono::milliseconds(int(time * 1000));
    return NodeStatus::RUNNING;
  }

  virtual NodeStatus onRunning() override
  {
    return (std::chrono::system_clock::now() < _deadline) ? NodeStatus::RUNNING :
                                                            NodeStatus::SUCCESS;
  }

  virtual void onHalted() override {}

  static BT::PortsList providedPorts()
  {
    return {BT::InputPort<double>("wait_duration")};
  }
};

TEST(SubTree, Autoremap)
{
  BehaviorTreeFactory factory;
  const std::string tree_xml = R"(
<root main_tree_to_execute="Tree">
  <BehaviorTree ID="Tree">
    <Sequence>
        <SubTreePlus ID="Main" __autoremap="true"/>
    </Sequence>
  </BehaviorTree>

  <BehaviorTree ID="Main">
    <Wait wait_duration="1"/>
  </BehaviorTree>
</root>
)";

  factory.registerNodeType<WaitNode>("Wait");
  Tree tree = factory.createTreeFromText(tree_xml);
  auto ret = tree.tickRootWhileRunning();
  ASSERT_EQ(ret, NodeStatus::SUCCESS);
}

[facontidavide]

Also the message:

Blackboard::get() error. Missing key [node]

suggests that you are having a problem with the infamous use of the blackboard to share the ROS node handle.

This is a bad practice and an abuse of the blackboard that I don't support, nor recommend.

I think the solution you need has nothing to do with SubtreePlus and autoremap.

[Rid0n]

I'm aware:c. it is the way it's currently done all over nav2 bt's, unfortunately.
you mentioned this issue in a nav2 thread asking if it breaks something - it does.

[Rid0n]

it does look like you fixed it from your side, but that breaks the nav2 side somehow

[facontidavide]

I don't even know how it is possible that it worked before.

Also, I think that, as you detected it yourself (motivation of this issue), Subtree / __shared_blackboard works, but SubtreePlus / __autoremap does NOT work.

Is that a correct statement? This make sense to me considering what Nav2 does and the way SubtreePlus autoreamp was implemented.

In other words, unless we change something in Nav2 (I have a suggestion), SubtreePlus / __autoremap did not work as YOU expect (wrong expectation) before and it will not work now.

Said that, I DO NEED to know from you what used to work but is not working anymore.

[Rid0n]

I wanted to use SubtreePlus / __autoremap because that would use a separate context and not impact the parent one. A one way data flow. (if I understood the implementation correctly anyway)

I was using SubtreePlus and for some subtrees, __autoremap worked(!!), but for others it didn't(??), I never figured out what that depended on, it seemed random. This is when I created the issue here.

[facontidavide]

I have no idea how it could possibly work before

[facontidavide]

I have an idea, maybe it is possible to fix this, I will ask you later to try a new branch

[Rid0n]

fun to be a part of ! haha

[facontidavide]

Please try this branch issue_563

[Rid0n]

Please try this branch issue_563

nope ;(
Same error...
bt_navigator still crashes..

[facontidavide]

I will need to try this on my machine or debug this with you. Can you arrange a session tomorrow or Thursday to do the investigation together?

The backtrace obtained with GDB would really help

[facontidavide]

Hi.

any additional detail? When you say "crashes" you mean that an exception in thrown? Same message?

[Rid0n]

sorry, don’t have time now.
I think it segfaults. the ros node crashes.
cant you get the nav2_behavior_tree package and debug using its unit tests?

[facontidavide]

cant you get the nav2_behavior_tree package and debug using its unit tests?

Honestly, fixing what Nav2 does is not on top of my priorities; I will prioritize what can be reproduced in my unit tests.
I might have a look at that over the weekeend...

[SteveMacenski]

Nor should you, @Rid0n its on you to provide reproduction help or compile with a debugger to get a trace. Its not @facontidavide 's problem to go into another project he doesn't maintain to chase down your issue 😄

You should provide the level of detail necessary for a responsible maintainer to make actionable progress on, not just "nope ;("

[facontidavide]

@Rid0n to be clear: I am grateful for the time you spent so far. I was able to correct some issues and improve the library thanks to your error report.

But I need more information to help you further.

[Rid0n]

Nor should you, @Rid0n its on you to provide reproduction help or compile with a debugger to get a trace. Its not @facontidavide 's problem to go into another project he doesn't maintain to chase down your issue 😄

You should provide the level of detail necessary for a responsible maintainer to make actionable progress on, not just "nope ;("

I know, but isn’t it a bit of a gray area since it’s something that’s affecting both projects?

I only said that because @facontidavide seems to want to fix this quickly and without fallout to nav2, while I can’t quickly provide what he’s asking for. ;(

[Rid0n]

@Rid0n to be clear: I am grateful for the time you spent so far. I was able to correct some issues and improve the library thanks to your error report.

But I need more information to help you further.

and I will give it to you if I find the time haha 💌

[facontidavide]

as far as I know, this is fixed

[HovorunB]

Bug report:

Hey! I ran into some problems after this issue, type conversion for the arguments I pass to the SubTreePlus stopped working:

<SubTreePlus
  ID="ToggleSomething"
  enabled="True"
/>

ToggleSomething BT:

 <BehaviorTree ID="ToggleSomething">
    <Sequence>
        <ToggleSmth enabled="{enabled}"/> # enabled is bool (BT::InputPort<bool>("enabled"))
        ...
    </Sequence>
  </BehaviorTree>

This fails with the following error:
Original error: The creation of the tree failed because the port [enabled] was initially created with type [std::string] and, later type [bool] was used somewhere else

On version 3.8.3 it worked great, and i think that's expected that it would be converted from string to bool automatically or no?

As for the issue this ticket is about:

I might be missing something but the original report in my understanding sounds like: Blackboard variables (which are not ports) are not remapped by a SubTreePlus with __autoremap="True".

The way I see it its your favorite bug report about SubTrees and nav2 (#189) , or there is more to it, that you tried to fix in this ticket?

[HovorunB]

@facontidavide friendly ping

[m-morelli]

Ciao @facontidavide, same problem as @HovorunB here. I've recently upgraded my ros-humble-navigation2 stack from v1.1.6 to v1.1.8, which in turn upgraded ros-humble-behaviortree-cpp-v3 from v3.8.3 to v3.8.4. Now when my BT executor executes a SubTreePlus like

<SubTreePlus name="Patrol" ID="Patrol" tl1="KITCHEN" tl2="LIVING"/>

I get this error:

[ERROR] [1690826385.151509220] []: Original error: The creation of the tree failed because the port [tl1] was initially created with type [std::string] and, later type [std_msgs::msg::String_<std::allocator<void> >] was used somewhere else.

It was used to work in the previous version. Possibly related to #489, which you already fixed for BT.CPP v4?

(For info: I'm allowing to write to this issue, as the first problem caused by the version upgrade was the one with __autoremap discussed above, which I've already fixed using your solution 2 at ros-navigation/navigation2#3640 (comment). Then I found the issue reported by @HovorunB and I've added my "+1". Do not hesitate to ask me to open a dedicated issue if you prefer.)

[facontidavide]

I am spending some days with my family with little access to the computer.

If anyone can provide a simple example that I can reproduce locally, I will focus my attention to this issue as soon as I can

[facontidavide]

@m-morelli would you be so kind to open a new issue and provide a way to reproduce it?

[m-morelli]

Ciao @facontidavide , sure I'll do it as soon as I have a bit of time. Thanks!

[m-morelli]

Done in #623. Thanks!

[tonynajjar]

This can be closed then I guess