Redundant stop() in Parallels

stonier commented 2 years ago

Can provoke it with this test script:

#!/usr/bin/env python3
# -*- coding: utf-8 -*-

import py_trees

class TestBehaviour(py_trees.behaviour.Behaviour):
        def __init__(self, name: str):
            super().__init__(name=name)

        def initialise(self):
            self.terminate_count = 0

        def terminate(self, new_status):
            print(f"Terminate {self.name} - {self.status} [{new_status}]")
            self.terminate_count += 1

        def update(self):
            return py_trees.common.Status.RUNNING

if __name__ == '__main__':
    py_trees.logging.level = py_trees.logging.Level.DEBUG

    foo = TestBehaviour(name="Foo")
    bar = py_trees.behaviours.Running(name="Running")
    foobar = py_trees.composites.Parallel(name="FooBar", children=[foo, bar])
    count = py_trees.behaviours.Periodic(name="Count", n=1)
    root = py_trees.composites.Parallel(
        name="Root",
        policy=py_trees.common.ParallelPolicy.SuccessOnSelected(children=[count]),
        children=[count, foobar]
    )
    print(py_trees.display.unicode_tree(root, show_status=True))
    for node in root.tick():
        pass
    print(py_trees.display.unicode_tree(root, show_status=True))
    for node in root.tick():
        print(f"Ticked {node.name}")
    print(py_trees.display.unicode_tree(root, show_status=True))

Output:


[DEBUG] Root                 : Parallel.tick()
[DEBUG] Count                : Periodic.tick()
[DEBUG] Count                : Periodic.stop(Status.RUNNING->Status.SUCCESS)
Ticked Count
[DEBUG] FooBar               : Parallel.tick()
[DEBUG] Foo                  : TestBehaviour.tick()
Ticked Foo
[DEBUG] Running              : Running.tick()
[DEBUG] Running              : Running.update()
Ticked Running
Ticked FooBar
[DEBUG] Root                 : InParallel.stop()[Status.RUNNING->Status.SUCCESS]
[DEBUG] FooBar               : InParallel.stop()[Status.RUNNING->Status.INVALID]
[DEBUG] Foo                  : TestBehaviour.stop(Status.RUNNING->Status.INVALID)
Terminate Foo - Status.RUNNING [Status.INVALID]
[DEBUG] Running              : Running.stop(Status.RUNNING->Status.INVALID)
[DEBUG] FooBar               : Parallel.stop()[Status.RUNNING->Status.INVALID]
[DEBUG] Foo                  : TestBehaviour.stop(Status.INVALID)
Terminate Foo - Status.INVALID [Status.INVALID]
[DEBUG] Running              : Running.stop(Status.INVALID)
[DEBUG] Root                 : Parallel.stop()[Status.RUNNING->Status.SUCCESS]
Ticked Root
/_/ Root [✓]
    --> Count [✓] -- flip to success
    /_/ FooBar [-]
        --> Foo [-]
        --> Running [-]

liangfok commented 2 years ago

The reason why the redundancy is unexpected is because the documentation for Behaviour.terminate() specifies that it should only be called when the behavior finishes execution or was interrupted.

https://py-trees.readthedocs.io/en/devel/modules.html#py_trees.behaviour.Behaviour.terminate

philip-n commented 1 year ago

Thanks for the library in general and for fixing this issue in particular (as it is currently bothering me)!

The contributing guidelines say that

If you are interested in seeing your changes make it into a release (sooner rather than later) and distributed on PyPi, PPA or via the ROS ecosystem, please make the request via a comment in your PR or in an issue.

Could you release the current version of py-trees, so this fix gets available on PyPi etc? That would be great!

splintered-reality / py_trees

Redundant stop() in Parallels #345