Tree visualization broken in 2.1.5

[eriytt]

In py-trees 2.1.5, I might get for example:

            --> WaitForBlackboardVariable [✓] -- '<some key>' found
            --> WaitForBlackboardVariable [✓] -- '<some key>' found

and in the very next tick, I get:

            --> WaitForBlackboardVariable [-] -- '<some key>' found
            --> WaitForBlackboardVariable [-] -- '<some key>' found

It seems like the behavior status is not sticking any more, like it did in 2.1.4. This makes it incredibly hard to figure out what happened during the run.

You might for example have a large behavior tree with parallel root where one branch is a global timeout. If the timeout triggers, the tree visualization shows the timeout behavior as FAILURE and all the other behaviors in the tree as INVALID, however many of them that actually succeeded.

[stonier]

The parallels visualisation problem has been a long standing one, #132. We've at times, made sure to publish the last N states of the tree for logging purposes on global timeouts. That works around that problem and was actually useful to get the few states leading up to it as well.

There's quite likely some problem inadvertently introduced you're alluding to here though, but from 2.1.4 to 2.1.5 there were only changes around selectors and sequences. Not likely to be about parallels. Do you have a simple example that reproduces the problem? Or at least some more context around those two examples you pasted above (the composites they were under, the siblings to the left and right and their status)?

[eriytt]

In the report we generate, we also include quite a few last states for trouble shooting. A potential problem is that not enough of them are included.

This was trickier than I thought, and it turns out that the very last state is actually the same in both versions when this occurs. However, the state before that differs. And you're right, it has nothing to do with parallels. It's because sequences in 2.1.5 stops child behaviors the tick after they succeded while 2.1.4 did not. I think this was introduced here: 69752f1#diff-365cf7e4f4a8fb1a8e7302c01bf12435187d0798c7c13e80194e367e439fb69cR443

I realize that I do not understand the general use case well enough, but for our use case, it's unfortunate because it makes trouble shooting a bit more difficult.

This is the program I used to test the differences:

from py_trees.decorators import Timeout, FailureIsRunning
from py_trees.behaviours import Running, SuccessEveryN
from py_trees.trees import BehaviourTree, CONTINUOUS_TICK_TOCK
from py_trees.display import unicode_tree
from py_trees.composites import Parallel, Sequence
from py_trees.common import ParallelPolicy, Status

sleep1 = FailureIsRunning(SuccessEveryN("Sleep1", 6))
sleep2 = FailureIsRunning(SuccessEveryN("Sleep2", 8))

tree = BehaviourTree(root=Parallel(policy=ParallelPolicy.SuccessOnOne(), children=[
    Sequence(children=[
        sleep1,
        sleep2
    ]),
    Timeout(
        name='timeout',
        duration=10,
        child=Running()
    )
]))

def tick_handler(tree):
    print(unicode_tree(root=tree.root, show_status=True))
    if tree.root.status != Status.RUNNING:
        tree.interrupt()

tree.tick_tock(
    period_ms=1000,
    number_of_iterations=CONTINUOUS_TICK_TOCK,
    pre_tick_handler=None,
    post_tick_handler=tick_handler
)

[stonier]

69752f1#diff-365cf7e4f4a8fb1a8e7302c01bf12435187d0798c7c13e80194e367e439fb69cR443

Ah, got it. I should probably have delayed this till the 2.2.x series since it is a change, albeit neither an api nor algorithmic one. So one option is to revert this and bring it in for 2.2.x if indeed it is the right thing to do.

So, is it the right thing to do? Example:

# Tick 1 Viz
{} Sequence w/ Memory
  --> B1 [S]
  --> B2 [R]

# Tick 2 Viz
{} Sequence w/ Memory          vs              [] Sequence w/ Memory
  --> B1 [-]                                     --> B1 [S]         # NB: This behaviour was not actually ticked
  --> B2 [R]                                     --> B2 [R]

My hope was that this code would help clearly highlight what was actually ticked. In the first viz, you can safely infer that B1 had hitherto returned success. It also helps highlight the difference in behaviour between w/ memory and w/o memory, but it is in fact not necessary. Sequences w/ and w/o distinguish themselves via the {} and [] symbols (py_trees_js also), so you could do the inference in reverse.

---

Having said all that, neither is technically right or wrong. If showing the ghost of success is useful practically easier, that's ok by me. Worth noting, that the ascii display doesn't fade out unticked cells like py_trees_js does. If the right visitors are enabled, it could.

[stonier]

Re logging the last few states - you might want to consider storing last few tree changes and/or slower periodic snapshots. Some example code which shows what is done in the ROS implementation.

I've often thought about having some lower level, purely pythonic logging implementation that could be replayed from the command line with visualisation from the methods in the display module though. Good for tutorials and simple use cases. Would that be useful to you?

[stonier]

Back to this ticket - short term options:

• Leave it as is, i.e. infer the state
• Revert this and re-introduce it in 2.2.x
• Revert this, leave the ghosts behind

With a long term option of leaving the ghosts and fading out unticked parts of the tree in the display methods. Thoughts?

Notes to self: Keep in mind, S or F ghosts are fine, R need to be handled with prejudice to avoid dangling runners. I do wonder though, that it's maybe time to make ghost states a first class citizen to aid in debugging. i.e. Cache the last non-invalid state and use that for visualisation. This critically depends on having visualisation being aware of what was visited and what was not in the preceding tick. It would resolve #132.

[eriytt]

I think the conflict here is being able to visualize what happened this tick vs. visualizing the entire run, and both cases are totally valid.

I can't say that I fully understand how the fading would look, but from my own experience, hiding parts of the tree could potentially confuse new users, even though with some experience, you realize that it is more effective. This is actually the way we're moving with our use case, using visitors.

After this discussion, I'm totally fine with leaving the behavior as it is. If it becomes too much of a problem, we could probably work around this using an approach similar to what you describe, using ghost states.

That said, it would be nice if this was a supported feature of py_trees, so the user could decide what visualization made most sense to him/her. A ghost state, or maybe we should call it a "result state", of R would be totally fine then, as long as the real state is stopped/invalid.

[stonier]

• Reverted the display change for sequences w/ memory in [composites] show ghost states for sequence children #330. Rationale - it was a suprising/unecessary change in a patch release.
• Notes from this discussion on how to move the display module forward crystalised in Improvements for visualisation #331.

@eriytt thanks for your input!