Skip to content

Commit

Permalink
Removed "Solution A" and more details in what was before Solution B
Browse files Browse the repository at this point in the history
  • Loading branch information
facontidavide committed Dec 20, 2018
1 parent 561533d commit 9429e7a
Showing 1 changed file with 32 additions and 65 deletions.
97 changes: 32 additions & 65 deletions RoadmapDiscussion.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,7 @@ Therefore, the only reasonable thing to do is to deprecate `TreeNode::blackboard
The problem is that `SimpleActionNodes` and `SimpleDecoratorNodes`
will loose the ability to access ports.

## 2.2 Solution A

### 2.2.1 NameParameters as input ports
## 2.2 NameParameters as input ports

We know that NodeParameters are a mechanism to add "arguments" to a Node.

Expand All @@ -65,36 +63,38 @@ As a consequence, we may consider NodeParameters a valid implementation of an
__input port__.

From a practical point of view, we should encourage the use of
`TreeNode::getParam` as much as possible and deprecate `TreeNode::blackboard()::get`

### 2.2.1 Output Ports
`TreeNode::getParam` as much as possible and deprecate `TreeNode::blackboard()::get`.

We need to add automatically the output ports to the TreeNodeManifest.
Furthermore, it make sense, for consistency, to rename `getParam` to __getInput__.

To do that, we can just add the static method
## 2.3 Output Ports

const std::set<std::string>& providedOutputPorts() //outputs
for consistency, we might consider to change the signature of `requiredNodeParameters()` to

const std::set<std::string>& requiredNodeParameters() //inputs
We need to add the output ports to the TreeNodeManifest.

In other words, requiredNodeParameters provides only the key, but not a default value;
in fact, we have seen that there is little practical use for a default value.
To do that, we should change `requiredNodeParameters` and use instead:

The new manifest definition would become:

```c++

enum PortType { INPUT, OUTPUT, INOUT };

typedef std::unordered_map<std::string, PortType> PortsList;

// New Manifest
struct TreeNodeManifest
{
NodeType type;
std::string registration_ID;
std::set<std::string> required_parameters;
std::set<std::string> provided_outputs;
PortsList ports;
};
```

About remapping, to avoid name cashing it is sufficient to provide remapping
// What was previously MyNode::requiredNodeParameters() becomes:

static const PortsList& MyNode::providedPorts();

```
About remapping, to avoid name clashing it is sufficient to provide remapping
at run-time __only for the output ports__.
We don't need remapping of input ports, because the name of the entry is
Expand All @@ -105,45 +105,14 @@ From the user prospective, `TreeNode::blackboard()::set(key,value)` is replaced
Example:
If the remapping __["goal","navigation_goal"]__ is passed and the user invokes
If the remapping __["goal","navigation_goal"]__ is passed as a `NodeParameter'
and the user invokes
setOutput("goal", "kitchen");
The actual entry to be written will be the `navigation_goal`.

## 2.3 Solution B

An alternative solution is to make no distintion between input and output ports.

This would make the code more consistent with the old one, but would break the API.

### 2.3.1 New manifest

```c++

enum PortType { INPUT, OUTPUT, INOUT };

typedef std::unordered_map<std::string, PortType> PortsList;

// New Manifest
struct TreeNodeManifest
{
NodeType type;
std::string registration_ID;
PortsList ports;
};

// What was previously MyNode::requiredNodeParameters() becomes:

static const PortsList& MyNode::providedPorts();

```
In other words, requiredNodeParameters, which used to focus only on inputs,
is substituted by another static method that provides both inputs and outputs.
### 2.3.1 from XML attributes to ports in/out/remaping
## 2.4 Code example
Let's illustrate this change with a practical example.
Expand All @@ -153,8 +122,8 @@ in `FollowPath`.
```XML
<SequenceStar name="navigate">
<Action ID="SaySomething" message="hello World"/>
<Action ID="ComputePath" endpoints="${navigation_endpoints}" path="${navigation_path}" />
<Action ID="FollowPath" path="${navigation_path}" />
<Action ID="ComputePath" endpoints="{navigation_endpoints}" path="{navigation_path}" />
<Action ID="FollowPath" path="{navigation_path}" />
</SequenceStar>
```

Expand Down Expand Up @@ -184,11 +153,14 @@ class SaySomething: public SyncActionNode

NodeStatus tick() override
{
auto end_points = getInput<std::string>("message");
std::cout << message << std::endl;
auto msg = getInput<std::string>("message");
if( !msg) // msg is optional<std::string>
{
return NodeStatus::FAILURE;
}
std::cout << msg.value() << std::endl;
return NodeStatus::SUCCESS;
}

static const PortsList& providedPorts()
{
static PortsList ports_list = { {"message", INPUT} );
Expand All @@ -209,7 +181,6 @@ class ComputePath: public SyncActionNode
setOutput("path", my_computed_path);
// return result...
}

static const PortsList& providedPorts()
{
static PortsList ports_list = { {"endpoints", INPUT},
Expand All @@ -230,17 +201,13 @@ class FollowPath: public AsyncActionNode
// do your stuff
// return result...
}

static const PortsList& providedPorts()
{
static PortsList ports_list = { {"path", INPUT} };
return ports_list;
}
};
```
Notice that the method `getInput` is used instead of `getPatam` for consistency
with the new methos `setOutput`.
```.
The user's code doesn't need to know if inputs where passed as "static text"
or "blackboard pointers".
Expand Down

0 comments on commit 9429e7a

Please sign in to comment.