Conversation
ste93
force-pushed
the
feat/parallel_node
branch
from
dcf942b to
0147311
Compare
Signed-off-by: Stefano Bernagozzi <stefano.bernagozzi@iit.it>
Signed-off-by: Stefano Bernagozzi <stefano.bernagozzi@iit.it>
Signed-off-by: Stefano Bernagozzi <stefano.bernagozzi@iit.it>
Signed-off-by: Stefano Bernagozzi <stefano.bernagozzi@iit.it>
Signed-off-by: Stefano Bernagozzi <stefano.bernagozzi@iit.it>
ste93
force-pushed
the
feat/parallel_node
branch
from
ed9c0bb to
3aa4aa4
Compare
Signed-off-by: Stefano Bernagozzi <stefano.bernagozzi@iit.it>
Signed-off-by: Stefano Bernagozzi <stefano.bernagozzi@iit.it>
Signed-off-by: Stefano Bernagozzi <stefano.bernagozzi@iit.it>
Signed-off-by: Stefano Bernagozzi <stefano.bernagozzi@iit.it>
|
@ct2034 @EnricoGhiorzi I've added the parallel node (needed for UC3) can you check this PR and if it is good we can merge it |
|
I tried pulling the PR and building the UC3 model with a parallel node but I see no such nodes in the generated files. Is this to be expected? Is there a way to have a compiled parallel node to test? PS: I managed to compile the bt with the parallel node (I had to uncomment some stuff in PPS: I had forgot to change the property to include the |
EnricoGhiorzi
left a comment
EnricoGhiorzi
left a comment
There was a problem hiding this comment.
Thank you @ste93 for the PR. I think the parallel node is mostly working, also based on empirical sperimentation, but there are a few points in the code (marked with relevant comments) that I think could be improved before we merge this.
| <assign location="success_count" expr="0" /> | ||
| <assign location="failure_count" expr="0" /> | ||
| </onentry> | ||
| <transition cond="children_count == 0" target="wait_for_tick"> |
There was a problem hiding this comment.
On state init the first transition prevents children_count to be equal to 0 so this condition should never be satisfied.
| </transition> | ||
| </state> | ||
|
|
||
| <state id="advance_child"> |
There was a problem hiding this comment.
This state is never reached so it should be safe to delete.
| <transition target="tick_current_child" /> | ||
| </state> | ||
|
|
||
| <state id="eval_thresholds"> |
There was a problem hiding this comment.
This state is never reached so it should be safe to delete.
| <bt_return_status status="SUCCESS" /> | ||
| </transition> | ||
|
|
||
| <bt_child_halted id="current_child_idx" target="halting_after_success_next" /> |
There was a problem hiding this comment.
I think there is no need for the state halting_after_success_next, just modify this transition as:
<bt_child_halted id="current_child_idx" target="halting_after_success">
<assign location="current_child_idx" expr="current_child_idx + 1" />
</bt_child_halted>Similar comments apply for halting_after_failure_next and halting_on_parent_halt_next.
|
|
||
| <!-- Loop and counters --> | ||
| <data id="current_child_idx" type="int8" expr="0" /> | ||
| <data id="success_count" type="int16" expr="0" /> |
There was a problem hiding this comment.
The variables success_count and failure_count are declared as 16-bit integers, but should be the same type as all others, i.e., int8. Even better, all variables could be uint8 as we don't really need them to go negative.
| @@ -0,0 +1,14 @@ | |||
| <!-- Test that the Parallel node fails when failure_threshold is met: | |||
| - 3 children where 1 keeps running, 2 return failure immediately | |||
There was a problem hiding this comment.
Example is inconsistent with description: two nodes always succeed and one always fails.
| @@ -0,0 +1,14 @@ | |||
| <!-- Test that the Parallel node succeeds when success_threshold is met: | |||
| - 3 children where 2 return success immediately, 1 keeps returning running | |||
There was a problem hiding this comment.
Example is inconsistent with description: third node always fails instead of running.
There was a problem hiding this comment.
status_action is never used in the test BTs so its code never runs. I think we should use status_action in the test BTs in place of/in addition to the AlwaysFailure and AlwaysSuccess nodes already there. Similar comments apply to main_test_parallel_running.xml and main_test_parallel_success.xml.
There was a problem hiding this comment.
This test is never invoked so it will not run. To run it we also need a relevant property to check, which I think could be something like "always running".
Signed-off-by: Stefano Bernagozzi <stefano.bernagozzi@iit.it>
|
@EnricoGhiorzi I should have addressed most of the comments |
| "parallel_failure_test", | ||
| expected_result_probability=1.0, | ||
| ) | ||
|
|
There was a problem hiding this comment.
I think the main_test_parallel_running is missing. Are you planning to add that?
| </parameters> | ||
|
|
||
| <behavior_tree> | ||
| <input type="bt.cpp-xml" src="./bt_test_parallel_running.xml" /> |
ct2034
left a comment
ct2034
left a comment
There was a problem hiding this comment.
Thanks Stefano. This looks really good. The tests worked for me.
I just noticed that you also created main_test_parallel_running.xml but not the bt for it. I think that would be worth testing, too.
3 participants
added parallel node for BT, needed for UC3