Open Discussion: input/output ports to abstract blackboards

[facontidavide]

Hi everyone.

I want to open a discussion about the future of the library.
The more I think about issue #18 , more I realize (thanks @v-lopez) that the model of ports in SMACH is better to deal with name clashing.

Looking at examples from the people involved in ROS2/Navigation2, I have the feeling that they have their own "work around".

The only conclusion I have is that accessing TreeNode::blackboard() in an implicit way is a recipe for disaster.

I try to summarize my thoughts here: https://github.com/BehaviorTree/BehaviorTree.CPP/blob/output_ports/RoadmapDiscussion.md

Please feel free to add your own two cents, since... most of the solutions I am thinking about will break the API and you will profoundly hate me :(

I am pinging people that showed some interest in the past, since this will affect them.

@mjeronimo @miccol @liuxin00738 @Thordreck @StefanoSoffiaUTRC @orduno

[miccol]

Hi Davide,
Thank you for highlighting this issue and for proposing detailed solutions.

Both solutions are reasonable. I personally prefer Solution B at first sight.
Would it be possible to keep NodeParameters required_parameters in the manifest and add PortsList ports ?
Parameters could be used to define those values that do not change at runtime and defined by the user at design time.

What do you think?

[facontidavide]

The old behavior related to NodeParameter would still work, so no need to make any distintion.

An "input port" might either be some text that is parsed using convertFromString or be the "pointer" to an entry of the Blackboard (or, if you prefer to think in SMACH terms, a "remap").

On the contrary, output port can only have the latter (pointer to BB).

In terms of syntax in the XML, nothing really changes for NodeParameters (AKA Input Ports).

UPDATE: I pushed an extension of the document to make this clear 561533d

[facontidavide]

After a night of sleep and a coffee, I came to the conclusion that whatever we do, this is going to be an annoying and non-backward-compatible change. Sorry users.

Therefore, there is no point to stick with the half baked Solution A.

I unified both Solutions into the single one that seems to me the most future-proof: 9429e7a

[v-lopez]

Very interesting summary, I agree with everything.

Just a question, I have a navigation_path entry in the BB, and I want to feed it to a OptimizePath node that has an inout port path, how would I access the optimized path afterwards?
Or in this case, it would be a better design decision, to provide two separate ports, in_path and optimized_path, for instance.

        <Action ID="CreatePath" />  <!-- It's output goes to a navigation_path port -->
        <Action ID="OptimizePath" path="{navigation_path}" />
        <Action ID="FollowPath"  path="{navigation_path or path}" />

[facontidavide]

According to my plan, the case you described is already covered in my proposal. You can have read write access, since "ports" are actually shared memory under the hood.

[facontidavide]

Ok, I think I am slowly coming to a viable version 3 of the library.

The new discussion document is here and is much more focused the the previous one:

https://github.com/BehaviorTree/BehaviorTree.CPP/blob/ver_3/RoadmapDiscussion.md

The new version is more or less implemented and I need to write unit tests and documentation.

Overall, I believe that version 3 is much better, but there is something that people might dislike.
All the entries of the the Blackboards that are read/written must be specified explicitly in the XML.

This might look cumbersome to people that use BB a "lot", but it is the only way to avoid name clashing.

Any thougth? @miccol @v-lopez @mjeronimo ?

[Thordreck]

Kind of late to the party, but I wanted to give my two cents on this.
I completely agree with the proposal, and as an end user of the library I don't think the proposed changes would be too cumbersome to follow. I have some questions, though.

It's stated in the proposal that:

No distinction is made in the XML between inputs, outputs; additionally, passing static text parameters is still possible

I think knowing if a given port is either input or output can be useful for behaviour designers using graphical front-ends. This could make it possible for external tools to include easy-to-understand symbols next to each port entry in a block, thus making the whole process less error prone. In general, I think all the info in the manifest should be included in the XML as well, otherwise other tools would be missing important info.
Also related to this, does the sentence above imply that the ${...} syntax is completely deprecated and the new port syntax {port} should be used instead, even in external tools like Groot?

I also wonder if ports could allow us to get better sub-trees support, as we are making extensive use of them. A problem we run frequently into is that sub-trees, being composed by multiples nodes, present several internal connections, but there's some info that usually comes from outside (for example, a "Track" subtree may receive a frame to track as input from the parent tree). Would it be possible to somehow define ports as "external" or "internal"? I guess this is something that should be included in the XML schema of a sub-tree? This way other people using that subtree can forget about the internal details and just feed the tree with the info it needs.

What do you think?

[facontidavide]

thanks @Thordreck .

I was thinking about that but I didn-t add my thought to the document.
External tools, such as Groot ,do have access to the TreeNodeManifest, which knows if a port in a input or output.
In the XML I expect something like this:

<BehaviorTree>
     ...
        <MyAction my_input="{the_question}"  arg="42" my_output="{the_answer}" />
    ....
<BehaviorTree>

<TreeNodesModel>
     ...
        <Action ID="MyAction" input_ports="my_input;arg" output_ports="my_output" />
    ....
<TreeNodesModel>

Does it make sense in your opinion?

About Subtrees, I believe you are right, there must be a way to add in/out ports to Subtree.
It is defenetively worth thinking about it now; it is probably going to be the next thing I will concentrate on once I feel that ports has been implemented "right" at the TreeNode level.

If you have any proposal or suggestion I will be very happy to review it :)

[mjeronimo]

I'm just back from vacation and catching up on this thread...

I agree that a port abstraction that allows for both static and dynamic parameters, introspection, and remapping is a superior model. Also, not concerned about the breaking changes to the API; we can accommodate that without much trouble.

However, I didn't understand this statement:

The problem is that SimpleActionNodes and SimpleDecoratorNodes will loose the ability to access ports.

I would expect all node types to be able to use the ports.

[facontidavide]

I would expect all node types to be able to use the ports.

This is a reasonable expectation ;)

The main problem is that the TreeNodeModel is created using the static method:

   static const PortsList& MyNode::providedPorts();
   // formerly known as 
   //static NodeParameters MyNode::requiredNodeParameters()

Since, by definition, the purpose of SimpleNodes is to reduce the boilerplate associated with inheritance, the "static method" trick can't be used.

In other words, the manifest in BehaviorTreeFactory can't see ports.

The only solution I see is to change the signature of the registering function:

void registerSimpleAction(const std::string& ID, const TickFunctor& func, PortsList ports);

In other words, if the user really wants a SimpleNode with ports, he needs to specify them.

Any other suggestion?

[LoyVanBeek]

To make things more complicated: should Ports be typed?

Not sure if this is possible yet, but I'm quite interesting in using this library, having type checks would be another reason to use it!

I'm a heavy SMACH user for RoboCup@Home with @tue-robotics, but once our library of reusable behaviors got bigger and the team as well, messing up types for SMACH user data happened a lot.
Another problem often encountered was that some user_data might not be set

I eventually decoupled dataflow from behavior by using so-called Designators that encapsulate & reuse the process of how a concrete value is resolved. E.g. find any Arm of the robot that is empty to grab an object with or to find an entity in the world model matching some requirements.

[facontidavide]

@LoyVanBeek
Usually, I am a very strong advocate of type safety in C++, including strong types.
But the primary goal of State Machines and Behavior Trees is coordination, not dataflow.
On the other hand, we do know that, sometimes, it is useful to share data between states.

I am afraid that adding types to ports might increase the complexity of the library and create an entry barrier; more features attract advanced users but intimidate newbie and, very often, may limit adoption.

I might add types, though, as an optional signature/metadata of the port that is used only if defined by the user, in other words, I can add this as an optional feature.

[Masadow]

@facontidavide To address #18, have you looked at how Unreal Engine handles this ? Since they use the same mechanisms and concept for their behavior tree.

Actually relying on this library, I'm not sure how much it's going to break. This is mainly due to the fact that you said BB are going to be deprecated but you still seems to rely on them to initialize the tree or adjust the values from outside dynamically.

[facontidavide]

have you at how Unreal Engine handles this ? Since they use the same mechanisms and concept for their behavior tree

The problem I am trying to solve is related to the fact that you can interact with the BB in your C++ code, in a way that is invisible from the"outside". As far as I understood, Unreal doesn't have this problem because you do not write custom Tasks in C++.

The only practical difference in the new version is that, currently, you can access any entry in the BB, whilst in the future you will need to specify explicitly which entries of the BB are written/read.

Let me rephrase: you will need to create ports and use input ports to read from the BB and output ports to write on the BB.

Actually relying on this library, I'm not sure how much it's going to break.

I guess quite a lot, if you use BB extensively. But as I said, it is necessary for the future of this library.
Sorry :(
The only thing I can do is to offer you some of my software development hours to help you refactoring your code, if it is open source.

[miccol]

Overall, I believe that version 3 is much better, but there is something that people might dislike.
All the entries of the the Blackboards that are read/written must be specified explicitly in the XML.

I actually like that also from a design point of view. If I want to replace a node, I need to know which entries in the blackboard will be affected without going into the C++ code.

As usual, awesome job Davide!.

[facontidavide]

@mjeronimo I added the possibility of adding ports tosimple nodes (tutorials updated too).

@LoyVanBeek after thinking carefully... I understand the value of typed ports, but it is really difficult to integrate this in the library in an elegant way (if you disagree, feel free to propose a pull request).

Implementation wise, the only thing left is to add ports to Subtrees, that I will discuss in #44 and #45

Thanks for your feedback!

[facontidavide]

@LoyVanBeek I am glad to say that I was wrong...
I just "finished" adding types to ports in the branch ver_3.

Ports type is optional, BUT if you specify the type in providedPorts as shown here, then during the creation of the tree from XML, it is automatically checked that you don't connect ports of different type.

This is, of course, still experimental, but very promising.

[LoyVanBeek]

It's nice to be wrong sometimes :-).

[v-lopez]

I have a question related to this, is it possible to have optional input ports?

I understand there are ways around it, such as passing a special value that signifies no parameter, or wrapping them in a boost::optional or similar construct.

But since getInput() already gives the option of returning false or empty optionals, it may be interesting to define in the GUI if a port must be provided always or is optional.

[facontidavide]

I have mixed feelings about this... since right now all the inputs ARE optional.
The challenge is to make them mandatory, actually. Because you need to check the result of getInput anyway.
And what is a "mandatory" input port? How is it implemented? I don't see any clear answer to this question.

[v-lopez]

I guess part of the idea here, is that people can build trees without having to look at the code.

Maybe I'm just mixing stuff, right now I still have to document somewhere what my Node does, and what my input and output is. Is there a standard or recommended way of doing it? Should an optional text description be part of the PortsList?

[facontidavide]

I can surely add some text description to the PortList, no problem.

Let me extend my previous answer, to be less ambiguous (spoiler alert: long explanation coming):

let's consider this Node:

<MyNode input_A="{A}" input_B="{B}" />

Where the model of the ports C++ is:

PortsList ports = { {"input_A, PortType::INPUT}, {"input_B, PortType::INPUT} };

Let's suppose that we change it to:

PortsList ports = { 
    {"input_A, PortType::INPUT, OPTIONAL}, 
    {"input_B, PortType::INPUT, REQUIRED} };

We have to distinguish between checks/validation done at deployment time (potentially statically verified) and at run-time (may fail during the execution of MyNode::tick() )

In terms of model, OPTIONAL means that input_A doesn't need to be specified in the XML, whilst REQUIRED means that input_B must to be specified in the XML.

The C++ code is responsible for run-time validation; in other words, when getInput() is called you must decide what to do if it returns false or if it throws an exception.

Are you just ignoring the returned value or not catching the exception? Well.. I don't know what to do about it :(

What will happen when an OPTIONAL port is not passed to the Node? Up to you.

What will happen when an REQUIRED port is not passed to the Node? You got my point...

I don't know how to be sure that the C++ is consistent with the OPTIONAL/REQUIRED policy.

May be I should just ignore the problem and let the user code throw exceptions if the "mandatory/optional" contract is not consistent between model and implementation, but, even so, I have to check if it actually possible to implement these ideas.

[v-lopez]

Maybe with the description is enough, since we can write there whether it's optional or not in a purely informative and non-enforceable way.

But if not, getInput (or a similar function to getInput) can throw if a required arg is missing, and return false silently if it's an optional missing.

In my case it's annoyting that the application prints that it's failing to get what I consider to be optional inputs:

getInput() failed because it was unable to find the key [pose_offset] remapped to [pose_offset]
getInput() failed because it was unable to find the key [initial_search_state] remapped to [initial_search_state]
getInput() failed because it was unable to find the key [pose_offset] remapped to [pose_offset]
getInput() failed because it was unable to find the key [initial_search_state] remapped to [initial_search_state]

[facontidavide]

Well, thta printf must die ;)

It just came to my mind that I may replace std::optional with std::expected to include the reason of the failure.

https://github.com/TartanLlama/expected
https://github.com/martinmoene/expected-lite

[facontidavide]

@v-lopez you are definitively right, getInput must not print anything on console. As I said, std:.expected is the right interface to use in this case, because is like std::optional but we can pack an error message too.

I just pushed the changes. f8128a0

As you may notice, I now use expected, renamed Optional. It works like this:

if( auto res = getInput<double>("port_name") )
{
       double value = res.value();
}
else{
    std::cerr << res.error() << std::endl;
    throw std::runtime_error( res.error() );
}

[facontidavide]

I am closing this issue, because I believe that I have done with the refactoring

Thak you all for the feedback and brilliant ideas you shared.

And by the way... @LoyVanBeek you can't imagine how much challenges I had to add typed ports !!!
Something that looked easy actuallt opened a pandora box of potential issues and pitfalls.

In short, I had to upgrade my C++ skills to bend the very fabric of reality...

But I am super happy with the result.

I am currently working on more tutorials and documentation.

You can get a feeling of the new API in version 3 here: https://github.com/BehaviorTree/BehaviorTree.CPP/tree/ver_3/examples