Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open discussion: Ports and Subtrees #44

Closed
facontidavide opened this issue Jan 4, 2019 · 15 comments
Closed

Open discussion: Ports and Subtrees #44

facontidavide opened this issue Jan 4, 2019 · 15 comments
Labels
help wanted Extra attention is needed

Comments

@facontidavide
Copy link
Collaborator

facontidavide commented Jan 4, 2019

This issue is about adding input/output ports to Subtrees.
Unfortunately, to do it right, we need to think carefully about the impact in terms of composition and scope. The BB model itself struggle to deal with this.

image

@Thordreck @miccol @v-lopez

@Thordreck suggested to have internal/external ports.
Even if that might look as a solution, it feels to me "wrong".

My gut feeling is telling me that the solution is "scoped blackboards".

Consider this C++ code:

int value = 69;
int num = 11;
{
    int value = 42;
    std::cout << num << " " << value <<std::endl; // will print: 11 42
}
std::cout << num << " " << value <<std::endl; // will print: 11 69

My feeling is that, to address composition using Subtrees, each subtree must have it's scoped Blackboard. Similarly to the C++ example, unless it is remapped, there should be a "priority stack" of blackboards, rather than a single one.

By default, the "local" blackboard is accessed first and then the following ones.

Now I have to think how to implement this and how make it look simple in the XML representation ;)

Any other idea or different direction you would like to propose?

@facontidavide facontidavide added help wanted Extra attention is needed version 3 labels Jan 4, 2019
@facontidavide facontidavide pinned this issue Jan 4, 2019
@facontidavide facontidavide changed the title Ports for Subtrees Open discussion: Ports and Subtrees Jan 4, 2019
@facontidavide
Copy link
Collaborator Author

Just added "scoped/hierarchical" blackboards.

What is left is to define the syntax and semantic of ports. Let's consider an example:
This is the current way to pass data to a SubTree (based on comments from @Masadow in #45):

<BehaviorTree ID="Walk">
    <SequenceStar>
        <MoveTo target="{target_pos}" />
        <FaceTo target="{target_angle}" />
    </SequenceStar>
</BehaviorTree>

<BehaviorTree ID="main">
    <SequenceStar>
        <SetBlackboard output_key="target_pos" value="position_ID" />
        <SetBlackboard output_key="target_angle" value="orientation_ID" />
        <Walk />
    </SequenceStar>
</BehaviorTree>

The problem with this was that the BB key {target_pos} or {target_angle} might be used somewhere else, not just in Walk.

Do not forget that, in this example, "position_ID" and "orientation_ID" are strings, but they could be pointers to other BB entries.

The natural way to look at the problem is to add ports to Subtrees as many of us agree. I propose this syntax:

<BehaviorTree ID="Walk" input_port="target_pos;target_angle" >
    <SequenceStar>
        <MoveTo position="{target_pos}" />
        <FaceTo  angle="{target_angle}" />
    </SequenceStar>
</BehaviorTree>

<BehaviorTree ID="main">
    <SequenceStar>
        <Walk  target_pos="position_ID" target_angle="orientation_ID" />
    </SequenceStar>
</BehaviorTree>

With the new implementation BB are scoped {target} inside Walk can not be seen "outside" Walk itself (no name collision then).

The redirection works as follows:

  • Port position of MoveTo points to BB entry target_pos that is redirected to the value "position_ID".
  • Port angle of FaceTo points to BB entry target_angle that is redirected to the value "orientation_ID".

Can any of you suggest a better syntax or you like this one?

Do you think that the "mapping" from {walk_target} to {Walk::target} should be value semantic (copy) or reference semantic (pointer) ?

I prefer value semantic, because it adds neglegible overhead in most cases and it is easier to implement.

@v-lopez
Copy link
Contributor

v-lopez commented Jan 9, 2019

I like the syntax proposed.

Another option would be:

<BehaviorTree ID="main">
    <SequenceStar>
        <Walk>
           <port external="position_ID"   internal="target_pos"/>
           <port external="orientation_ID"   internal="target_angle"/>
         </Walk>
    </SequenceStar>
</BehaviorTree>

It's more verbose, but if you are performing lots of remappings, it can be better visually. Plus it gives you the option to add more attributes in the future if need be.

Regarding the value/reference semantic, I don't fully understand the question, what is {Walk::target}, does it reference an example that I may have missed?

@facontidavide
Copy link
Collaborator Author

I like it, but I wonder if, since the internal refers to a port, the syntax {target_pos} and {target_pos} should be used instead.

What about:

<BehaviorTree ID="main">
    <SequenceStar>
        <Walk>
           <input_port from="position_ID"     to="{target_pos}"/>
           <input_port from="orientation_ID"   to="{target_angle}"/>
           <output_port from="{error_code}"     to="{walk_error_code}"/>
         </Walk>
    </SequenceStar>
</BehaviorTree>

Added error_code to show the syntax.

@facontidavide
Copy link
Collaborator Author

@Thordreck @miccol @mjeronimo @Masadow @DJuego and anyone else...

I will keep this issue open until the 21th of January. You have 12 days to contribute with suggestions and share your opinion, afterward I will implement this new feature and I hopefully have version 3 ready and documented) by the 1st of February.

@facontidavide
Copy link
Collaborator Author

Just to keep things more clear and visual.
I think this component diagram (UML) shows, more or less, what we want to achieve.

image

A Subtree contains an arbitrary numbers of Nodes (the diagram represent composition and dataflow, not the behaviort tree itself).
From the "outside" we can access only the blue ports, whilst the red one are 100% hidden and free of "name clashing".

@facontidavide
Copy link
Collaborator Author

@v-lopez

Regarding the value/reference semantic, I don't fully understand the question.

What I meant is...

So far we called "remapping" how a "port" in the C++ code is mapped to an entry of the BB.

For instance:

   target="{target_pos}" 

Means that port called "target" in the C++ code actually read/write the entry with key "target_pos" in the BB.
This Subtree we want to add another level of indirection, i.e. entry target_posin the scoped BB of Walk actually read write another entry in another BB, let's call it walk_targe_ID.

The question is: do we copy the content of walk_targe_ID into target_pos, or, we use some reference/pointer mechanism to avoid copying?

I invite the community to please prioritize correctness, usability and "the minimum surprise principle" above performance.

@v-lopez
Copy link
Contributor

v-lopez commented Jan 10, 2019

I would go with reference/pointer, because in some situations you may have parallelism and you want to update the contents of the entry from outside the subtree during it's execution.

Plus, if they were on the same BB, it would be reference/pointer right? I think it's less surprising if they have the same behaviour.

@miccol
Copy link
Member

miccol commented Jan 15, 2019

I would go with reference/pointer, because in some situations you may have parallelism and you want to update the contents of the entry from outside the subtree during it's execution.

I would go with reference/pointer too.

@facontidavide
Copy link
Collaborator Author

Soooooooooooooooo.....

The very basic unit test seems to work properly: see here.

https://github.com/BehaviorTree/BehaviorTree.CPP/blob/ver_3/gtest/gtest_factory.cpp#L170-L193

As you may notice this new syntax is used for Subtrees (suggested by @v-lopez )

      <SubTree ID="TalkToMe">
            <remap external="talk_hello" internal="hello_msg" />
            <remap external="talk_bye"   internal="bye_msg" />
            <remap external="talk_out"   internal="output" />
      </SubTree>

When the nodes in TalkToMe try to read/write a port locally, they are actually writing into the blackboard of the Maintree.

This should eventually solve all the problems that you guys reported ;)

On my side, I will spend the week doing more unit test to check corner cases and error report.

@v-lopez
Copy link
Contributor

v-lopez commented Jan 21, 2019

Looking great!

I'm migrating some of our smach_c code to BT using the ver_3 branch, so I'll let you know if I find any issues, I know it's still in development and things can break.

@facontidavide
Copy link
Collaborator Author

FYI:

I decided that I will also support this syntax:

<SubTree ID="TalkToMe" hello_msg="{talk_hello}" bye_msg="{talk_bye}" output="{talk_out}" />

That is more compact and readable when only 1 or 2 ports are remapped.

Implementation wise, I think I have done. I need to rewrite tutorials and documentation before the release (trying to release it the 1st of February).

@facontidavide
Copy link
Collaborator Author

I think it is done, eventually. You can check here https://github.com/BehaviorTree/BehaviorTree.CPP/blob/ver_3/examples/t06_subtree_port_remapping.cpp

I am 95% confident that version 3.0 beta is ready...

Thanks all of you for the great suggestions. Documentation an tutorials are also reasonably up to date.

@miccol it would be VERY nice if you in particular can review the new tutorial and provide some feedback.

@facontidavide facontidavide unpinned this issue Feb 1, 2019
@miccol
Copy link
Member

miccol commented Feb 1, 2019

@facontidavide sure, I will (hopefully in the next week)!

@miccol
Copy link
Member

miccol commented Feb 7, 2019

@facontidavide I have some feedback on the tutorial. Shall I send it over email or do you prefer an issue?
MC

@facontidavide
Copy link
Collaborator Author

probably email makes more sense. thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants