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

How to execute "cleanup action" after halting nodes #547

Open
alsora opened this issue Apr 18, 2023 · 8 comments
Open

How to execute "cleanup action" after halting nodes #547

alsora opened this issue Apr 18, 2023 · 8 comments
Assignees
Labels
enhancement New feature or request

Comments

@alsora
Copy link
Contributor

alsora commented Apr 18, 2023

Let's consider the following scenario:

  • we have an action node "drive robot" which moves a robot forward of a certain distance (this node will take a while to complete and it will return RUNNING until it's done).
  • we have an action node that turns on or off the lights (for the sake of simplicity, let's say that this is a SYNC action).

The robot should have the lights off during normal operations, but they should be on while executing the "drive robot" action node.
A trivial implementation would be the following:
Screenshot from 2023-04-18 12-38-31

The last action node in the sequence (i.e. the one setting the lights to OFF) is what I call a "cleanup" action".
It must be executed (no matter what) when we stop running the "drive robot".

This can be problematic if using the trivial tree shown above as part of a larger behavior, because the execution of "drive robot" can be halted from many different parts of the behavior tree (e.g. this subtree could be inside of a reactive sequence that checks for obstacles).

I can think of some solutions, all which seem suboptimal:

  1. Use post conditions such as _onSuccess=" lights_on:=true " and _onSuccess=" lights_on:=false " when we execute the different SetLights nodes. Then the BT can check this and call "turn off lights" if it realizes that "drive robot" was halted and we still have lights on. The problem here is that if there are multiple places where this can be halted from (e.g. nested reactive behaviors), we need to check the blackboard variable in multiple places.
  2. Create a new control node that has 2 children: the first is the main child, the action to execute (drive robot), while the second is the cleanup action (turn off lights). The cleanup action is executed when the main child reaches a terminal state or when the control node is halted.
    At a first glance, this second approach seemed to me the ideal solution to the problem, but while implementing it, I noticed several issues.
    2.a) we are adding complexity to the halt operation; maybe other nodes would expect it to be instantaneous.
    2.b) how to deal with a FAILURE in the cleanup action? the halt method does not return anything. we could use a blackboard variable, but then we would be back to the scenario of 1). Ignoring the failure is equally bad, because then we would risk of "keep running with the lights on", whereas different parts of the behavior may decide to handle a failure there differently (e.g. retrying or aborting everything).

Is there a recommended approach to deal with this type of situations?
Thank you

@alsora
Copy link
Contributor Author

alsora commented Apr 19, 2023

This, unresolved, stack overflow question deals with the same problem https://gamedev.stackexchange.com/questions/189352/behaviour-trees-how-to-clean-up-when-a-sequence-is-interrupted

They propose a variation of the solution (1) I described above, i.e. whatever new behavior will start running after halting, will need to check its pre-conditions (e.g. are the lights in the expected configuration?) before starting.
I think that this makes sense semantically, but it may still require to add this check to a lot of places (that maybe shouldn't care about the fact that the behavior they halted had modified something and it didn't clean up).

@facontidavide facontidavide self-assigned this Apr 19, 2023
@facontidavide
Copy link
Collaborator

I have been thinking about this and it lookas as a common problem.

To solve this and other problem, why not implement a SequenceAll Control Node?

The logic would be:

  • execute ALL the children
  • return FAILURE if at least 1 of them failed, SUCCESS otherwise.

What do you think?

@facontidavide facontidavide added the enhancement New feature or request label Apr 27, 2023
@alsora
Copy link
Contributor Author

alsora commented Apr 27, 2023

What would this SequenceAll Control Node do if it receives an halt signal?
I think that making sure that the cleanup is still executed after an halt is the main pain-point.

@alsora
Copy link
Contributor Author

alsora commented Apr 27, 2023

I'm currently using a ActionWithCleanup Control Node with the following semantics:

  • takes 2 children
  • first child is the main action to execute, second child is the cleanup action
  • when the main action terminates (success, failure or halt) the cleanup action is executed
  • the cleanup action must be synchronous (i.e. it can't return RUNNING)
  • if the cleanup action returns FAILURE, then throw a runtime error
  • the output of the control node will coincide with the output of the main action

It's definitely not ideal.
More complex solutions would require to let nodes register "cleanup actions" in the tree.
When nodes are halted, whoever halted them will have to execute the registered cleanup action (if any) before proceeding with its business
This means that the cleanup is now executed in the subtree/context of the node that triggered the halt (rather than being executed as part of the halt operation itself as with the node I described before).

@facontidavide
Copy link
Collaborator

I Think Both option are equally valid, because if you pass only two children to my hypothetical SequenceAll, the overall effect would be mostly the same.

Solution 1: pre/post conditions

image

Solution 2: IfTheElse

image

Solution 3: SequenceAll (proposed)

image

@alsora
Copy link
Contributor Author

alsora commented Apr 27, 2023

Thank you for the illustrated solutions.
However, I still think that if the Sequence (or SequenceAll) is halted while it's executing DriveRobot, the SetLight cleanup action won't be executed.

That would require an explicit handling of the halt signal, which Sequence and IfThenElse don't do.
Would the new SequenceAll handle halt differently?

@facontidavide
Copy link
Collaborator

you are correct about halting, indeed, I did not think about that....

But given the name All, maybe the logic is that halting will be propagated to the RUNNING child, but the sequence will continue with the next on.

@zflat
Copy link
Contributor

zflat commented Sep 8, 2023

I think that due to the way behaviortree_cpp halts the tree, the best way to ensure cleanup taking halting into account is to perform this type of "transactional" functionality in a decorator.

The decorator will do the "setup" first, then tick the single child, then do "cleanup" when the child is finished. When the decorator is halted, it will halt the child (which is handled by calling the base class DecoratorNode::halt()) and then do the "cleanup" work.

If there are multiple setup and corresponding cleanup steps to perform they can be done in nested decorators. For example, there could be the following decorators:

  • WithLightsOn: Sets the light on when running and off when not running
  • WithTankPressurized: Builds pressure then runs the child node and releases pressure when the child finishes
  • WithBrakeReleased: Sets the break released when the child node is running and then engages the break when the child node finishes

They would be nested like this:

<WithLightsOn>
    <WithTankPressurized pressure_setpoint_kpa="600" pressure_release_point_kpa="30">
        <WithBrakeReleased>
            <DriveRobot />
        </WithBrakeReleased>
    </WithTankPressurized>
</WithLightsOn>

Using a decorator like this ensures that halting will do the cleanup we expect. Using an ActionWithCleanup Control Node as described in #547 (comment) has the issue that the cleanup child can't be a sequence or other node that breaks the semantics of what is needed by ActionWithCleanup which seems hard to verify or enforce.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants