Cannot set SubTree ports without using a SetBlackBoard

[Acwok]

Hello,

It seems that it is not possible to directly set a value to an input port of a SubTree.
I have to map the input port to a key of the BlackBoard otherwise the value is [undefined].
Here is a snapshot of my tree:

And here is the terminal output when I load the tree:

root@pc047:/workspace/sequencer/build# ./bin/run_sequencer ../trees/assemble_inner_ring_up.xml 
Register Node ClearArm
Register Node ClearTileFromAnchorObject
Register Node EyeInHandVisualServoing
Register Node EyeToHandVisualServoing
Register Node GetAnchorSI
Register Node LockSI
Register Node MoveArmToSI
Register Node UnlockAllSiblings
Register Node UnlockSI
Register Node GetParametersString
Register Node GetParametersDouble
Tick : MoveArmToSI
    IN: ee_si : [undefined]
    IN: goal_si : [undefined]
    IN: approach_offset : 1000
MoveArmToSI - Invalid input

As you can see, the port approach_offset of the action MoveArmToSI is correctly set.
But its ports ee_si and goal_si are [undefined] albeit I have set the values of the GrabTileFromContainer ports.

Am I missing something, or is it a requirement to use a SetBlackBoard to initialise SubTree ports ?

[Acwok]

I think it would be nice to use the syntax port={key} (instead of port=key) in the SubTrees like it is done in the ActionNodes.
This would reduce the number of SetBlackBoard and make the tree clearer.

[Acwok]

To illustratre my point, here is a snapshot of what the working tree looks like with the setBlackBoard:

[facontidavide]

I should find some time to look into this during the weekend

[facontidavide]

Whe you use a subtree, you would like to:

1. either remap a certain InputPort to another port in the father tree or
2. use a hard-coded string to directly set the value of that port.

Is that correct?

I must admit that I didn't consider this case and, what is worth, changing this would break everybody's code :(

Let me explain with an example:

This is the current syntax

    <BehaviorTree ID="MainTree">
        <Sequence>
            <SetBlackboard output_key="greetings"  value="hello"" />
            <SubTree ID="SaySubtree" message="greetings" />
        </Sequence>
    </BehaviorTree>

    <BehaviorTree ID="SaySubtree">
            <SaySomething  message="{greetings}" />
    </BehaviorTree>

That remaps the port "greetings" to "message"

You may want this:

    <BehaviorTree ID="MainTree">
        <Sequence>
            <SetBlackboard output_key="greetings"  value="hello"" />
            <SubTree ID="SaySubtree" message="{greetings}" />
            <SubTree ID="SaySubtree" message="just say goodbye" />
        </Sequence>
    </BehaviorTree>

    <BehaviorTree ID="SaySubtree">
            <SaySomething  message="{greetings}" />
    </BehaviorTree>

Note that in this way, similarly to other ports, we use the { } syntax to mean "reference to blackboard".

Unfortunately, changing this now would break any existing BT code :(

Another solution could be to use something new (and not consistent with the existing syntax) to mean "this is a string, not the name to another port", like:

    <SubTree ID="SaySubtree" message="[[just say goodbye]]" />

[facontidavide]

A solution that comes to my mind (that is also backward compatible) is to create a new kind of node with a different name and a new syntax that accommodates your particular use case.

[Acwok]

Hi @facontidavide, thanks for the quick and complete reply !

Whe you use a subtree, you would like to:

1. either remap a certain InputPort to another port in the father tree or
2. use a hard-coded string to directly set the value of that port.

Is that correct?

Yes correct, I think that would be more user-friendly.
The examples you give are exactly what I had in mind.

Unfortunately, changing this now would break any existing BT code :(

I don't think it would break the code, only the xml, right ?
Updating a graph/xml is not the most time consuming thing, I think users would agree with that, and they will also benefit from better readability for their graphs.
It would also be more intuitive and consistent with the way actions work.

Another solution could be to use something new (and not consistent with the existing syntax) to mean "this is a string, not the name to another port", like:

<SubTree ID="SaySubtree" message="[[just say goodbye]]" />

I've been thinking about that too, but it would make things too complex in my opinion...

A solution that comes to my mind (that is also backward compatible) is to create a new kind of node with a different name and a new syntax that accommodates your particular use case.

I'm not against the idea. Making a new kind of subtree, right?
It would allow people to slowly migrate to this new solution.
Maybe we could specify in the documentation that the current subtree will be "deprecated" in the next versions to incite them migrating.
And when there will be enough feedback the current subtree could be removed.

[facontidavide]

DONE:

https://github.com/BehaviorTree/BehaviorTree.CPP/blob/subtree_plus/include/behaviortree_cpp_v3/decorators/subtree_node.h#L31-L71

This new Subtree Solve all your problem and also add an optional way to remap ports withthe very same name automatically.

I will mergi it to master soon-ish.

Try it and tell me if it works for you.

[Acwok]

Hi @facontidavide, thank you very much !

I've tried the SubTreePlus on my graph, the three remapping methods works.

The __autoremap feature is awesome btw !
However, I've noticed that it only works alone. When combined with other remapping methods, all the subtree ports are set to undefined.
(Or maybe the actions of the subtree cannot remapping to them)

Here is an example that works well with __autoremap being used alone:

<BehaviorTree ID="BehaviorTree">
    <Sequence>
        <SetBlackboard output_key="config_filename" value="../../file.xml"/>
        <SetBlackboard output_key="approach_offset" value="0.5"/>
        <SetBlackboard output_key="clear_offset" value="0.5"/>
        <SubTreePlus ID="AssembleInnerRingUp" __autoremap="1"/>
    </Sequence>
</BehaviorTree>

<BehaviorTree ID="AssembleInnerRingUp">
    <Sequence>
        <SubTreePlus ID="GrabTileFromContainer" approach_offset="{approach_offset}" clear_offset="{clear_offset}" ee_si="ARM_SI_0" list_si_pair="ARM_SI_0;TILE_0_SI_0" tile_si="TILE_0_SI_0"/>
        <SubTreePlus ID="PutTile" approach_offset="{approach_offset}" clear_offset="{clear_offset}" ee_si="TILE_0_SI_3" goal_si="ASSEMBLY_SI_2" list_si_pair="TILE_0_SI_3;ASSEMBLY_SI_2"/>
        <SubTreePlus ID="GrabTileFromContainer" approach_offset="{approach_offset}" clear_offset="{clear_offset}" ee_si="ARM_SI_0" list_si_pair="ARM_SI_0;TILE_1_SI_2" tile_si="TILE_1_SI_2"/>
        <SubTreePlus ID="PutTile" approach_offset="{approach_offset}" clear_offset="{clear_offset}" ee_si="TILE_1_SI_5" goal_si="ASSEMBLY_SI_3" list_si_pair="TILE_1_SI_5;ASSEMBLY_SI_3 TILE_1_SI_0;TILE_0_SI_2"/>
        <SubTreePlus ID="GrabTileFromContainer" approach_offset="{approach_offset}" clear_offset="{clear_offset}" ee_si="ARM_SI_0" list_si_pair="ARM_SI_0;TILE_4_SI_0" tile_si="TILE_4_SI_0"/>
        <SubTreePlus ID="PutTile" approach_offset="{approach_offset}" clear_offset="{clear_offset}" ee_si="TILE_4_SI_3" goal_si="ASSEMBLY_SI_4" list_si_pair="TILE_4_SI_3;ASSEMBLY_SI_4 TILE_4_SI_4;TILE_1_SI_4"/>
    </Sequence>
</BehaviorTree>

<BehaviorTree ID="GrabTileFromContainer">
    <Sequence>
        <Action ID="MoveArmToSI" approach_offset="{approach_offset}" ee_si="{ee_si}" goal_si="{tile_si}"/>
        <Action ID="EyeInHandVisualServoing" tile_si="{tile_si}"/>
        <Action ID="LockSI" list_si_pair="{list_si_pair}"/>
        <Action ID="UnlockAllSiblings" disconnect_both_sides="true" list_exception_si="{list_si_pair}" tile_si="{tile_si}"/>
        <Action ID="ClearTileFromAnchorObject" anchor_object="CONTAINER" clear_offset="{clear_offset}" tile_si="{tile_si}"/>
    </Sequence>
</BehaviorTree>

<BehaviorTree ID="PutTile">
    <Sequence>
        <Action ID="MoveArmToSI" approach_offset="{approach_offset}" ee_si="{ee_si}" goal_si="{goal_si}"/>
        <Action ID="EyeToHandVisualServoing" ee_si="{ee_si}" goal_si="{goal_si}"/>
        <Action ID="LockSI" list_si_pair="{list_si_pair}"/>
        <Action ID="UnlockSI" disconnect_both_sides="true" list_si_to_disconnect="ARM_SI_0"/>
        <Action ID="ClearArm" clear_offset="{clear_offset}" from_si="ARM_SI_0"/>
    </Sequence>
</BehaviorTree>

And here is the same example that doesn't work as expected, with __autoremap used next to other remapping methods:

<BehaviorTree ID="BehaviorTree">
    <Sequence>
        <SetBlackboard output_key="config_filename" value="../../file.xml"/>
        <SetBlackboard output_key="approach_offset" value="0.5"/>
        <SetBlackboard output_key="clear_offset" value="0.5"/>
        <SubTreePlus ID="AssembleInnerRingUp" __autoremap="1"/>
    </Sequence>
</BehaviorTree>

<BehaviorTree ID="AssembleInnerRingUp">
    <Sequence>
        <SubTreePlus ID="GrabTileFromContainer" __autoremap="1" ee_si="ARM_SI_0" list_si_pair="ARM_SI_0;TILE_0_SI_0" tile_si="TILE_0_SI_0"/>
        <SubTreePlus ID="PutTile" __autoremap="1" ee_si="TILE_0_SI_3" goal_si="ASSEMBLY_SI_2" list_si_pair="TILE_0_SI_3;ASSEMBLY_SI_2"/>
        <SubTreePlus ID="GrabTileFromContainer" __autoremap="1" ee_si="ARM_SI_0" list_si_pair="ARM_SI_0;TILE_1_SI_2" tile_si="TILE_1_SI_2"/>
        <SubTreePlus ID="PutTile" __autoremap="1" ee_si="TILE_1_SI_5" goal_si="ASSEMBLY_SI_3" list_si_pair="TILE_1_SI_5;ASSEMBLY_SI_3 TILE_1_SI_0;TILE_0_SI_2"/>
        <SubTreePlus ID="GrabTileFromContainer" __autoremap="1" ee_si="ARM_SI_0" list_si_pair="ARM_SI_0;TILE_4_SI_0" tile_si="TILE_4_SI_0"/>
        <SubTreePlus ID="PutTile" __autoremap="1" ee_si="TILE_4_SI_3" goal_si="ASSEMBLY_SI_4" list_si_pair="TILE_4_SI_3;ASSEMBLY_SI_4 TILE_4_SI_4;TILE_1_SI_4"/>
    </Sequence>
</BehaviorTree>

<BehaviorTree ID="GrabTileFromContainer">
    <Sequence>
        <Action ID="MoveArmToSI" approach_offset="{approach_offset}" ee_si="{ee_si}" goal_si="{tile_si}"/>
        <Action ID="EyeInHandVisualServoing" tile_si="{tile_si}"/>
        <Action ID="LockSI" list_si_pair="{list_si_pair}"/>
        <Action ID="UnlockAllSiblings" disconnect_both_sides="true" list_exception_si="{list_si_pair}" tile_si="{tile_si}"/>
        <Action ID="ClearTileFromAnchorObject" anchor_object="CONTAINER" clear_offset="{clear_offset}" tile_si="{tile_si}"/>
    </Sequence>
</BehaviorTree>

<BehaviorTree ID="PutTile">
    <Sequence>
        <Action ID="MoveArmToSI" approach_offset="{approach_offset}" ee_si="{ee_si}" goal_si="{goal_si}"/>
        <Action ID="EyeToHandVisualServoing" ee_si="{ee_si}" goal_si="{goal_si}"/>
        <Action ID="LockSI" list_si_pair="{list_si_pair}"/>
        <Action ID="UnlockSI" disconnect_both_sides="true" list_si_to_disconnect="ARM_SI_0"/>
        <Action ID="ClearArm" clear_offset="{clear_offset}" from_si="ARM_SI_0"/>
    </Sequence>
</BehaviorTree>

the last example ouputs:

Tick : MoveArmToSI
    IN: ee_si : [undefined]
    IN: goal_si : [undefined]
    IN: approach_offset : [undefined]
MoveArmToSI - Invalid input

[facontidavide]

Thanks for reporting, I will double check.

[facontidavide]

I (hopefully) fixed the issue and merged the branch to master

https://github.com/BehaviorTree/BehaviorTree.CPP/blob/master/tests/gtest_subtree.cpp#L174-L203

[facontidavide]

Please remember that nothing I just did will probably work in Groot!!!!!

I need to check that out, but my priority right now is the BehaviorTree.CPP library itself.

[Acwok]

Hi @facontidavide,
I just noticed that you have integrated SubTreePlus into Groot, thank you! It works great!

[BenArtes]

Unless I'm mistaken, I think he just allowed it to be imported; if you save in Groot it will get exported as a regular SubTree which will break the BT. I've opened an issue in Groot about adding support.

Thanks for the SubTreePlus, I think for common cases it is significantly cleaner and will be using it myself.

I've opened an issue to discuss implementing it.

[Acwok]

@BenArtes Indeed, it only works when imported