Skip to content

Conversation

pakagronglb
Copy link
Contributor

@pakagronglb pakagronglb commented Mar 21, 2025

Implements conditional nodes for behaviour trees (Fixes #5).

Description

Added the ability to conditionally include or exclude nodes in behaviour trees based on argument values. This makes subtrees more configurable and modular by allowing different parts of a tree to be enabled or disabled based on runtime parameters.

Implementation details

  • Added _evaluate_condition method to evaluate conditional expressions with argument substitution
  • Enhanced _build_tree to handle the new <conditional> XML tag
  • Added comprehensive tests for various condition types
  • Returns conditional children directly or wrapped in a sequence as appropriate

Example usage

<!-- Basic string comparison -->
<conditional if="${mode} == 'advanced'">
  <py_trees.behaviours.Success name="Advanced Feature" />
</conditional>

<!-- Numeric comparison -->
<conditional if="${level} > 5">
  <py_trees.behaviours.Periodic name="High Level Feature" n="3" />
</conditional>

<!-- Using with subtrees -->
<subtree include="path_to_subtree.xml">
  <arg name="mode" value="advanced" />
</subtree>

This enables more flexible behaviour tree configurations where different nodes can be included based on argument values.

@erichlf
Copy link
Collaborator

erichlf commented Mar 21, 2025

The review for this will take some more time, since we will have to discuss the design. The ticket for this was pretty new and no discussion of how this should look was done. I am not sure what is implemented is what we would have gone with.

I did take a brief look though and I noticed that you are checking a conditional and then checking args during the evaluation of the conditional. I believe that some of this could have been taken care of if you just placed the evaluation between line 458 and 459. So basically, you process the args check if the node is a conditional and only process the items in the conditional if it is true. Something like that.

EDIT: I do see that you followed a similar pattern as the subtree and maybe that is appropriate.

@pakagronglb
Copy link
Contributor Author

The review for this will take some more time, since we will have to discuss the design. The ticket for this was pretty new and no discussion of how this should look was done. I am not sure what is implemented is what we would have gone with.

I did take a brief look though and I noticed that you are checking a conditional and then checking args during the evaluation of the conditional. I believe that some of this could have been taken care of if you just placed the evaluation between line 458 and 459. So basically, you process the args check if the node is a conditional and only process the items in the conditional if it is true. Something like that.

EDIT: I do see that you followed a similar pattern as the subtree and maybe that is appropriate.

Thank you again for your detailed feedback! It is very much appreciated. I'm still very new to coding so this is extremely helpful.

@dave992
Copy link
Member

dave992 commented Mar 22, 2025

Some suggestions based on my comment in #5.

On parser.py#L452 add:

condition = xml_node.attrib.get("if")
if condition:
  include_node = self._evaluate_condition(condition, args)
  if not include_node:
    return None

This would require the following change in the _build_tree function:

children.append(child)

to:

if child is not None:
  children.append(child)

This will make all nodes able to be conditionals I think.

I am not really sure if we need to do something to support the group tag explicitly as I think this will just be handled by the following code already:

children = list()
for child_xml in xml_node:
self._process_args(child_xml, args)
child = self._build_tree(child_xml, args)
children.append(child)

@pakagronglb
Copy link
Contributor Author

Some suggestions based on my comment in #5.

On parser.py#L452 add:

condition = xml_node.attrib.get("if")
if condition:
  include_node = self._evaluate_condition(condition, args)
  if not include_node:
    return None

This would require the following change in the _build_tree function:

children.append(child)

to:

if child is not None:
  children.append(child)

This will make all nodes able to be conditionals I think.

I am not really sure if we need to do something to support the group tag explicitly as I think this will just be handled by the following code already:

children = list()
for child_xml in xml_node:
self._process_args(child_xml, args)
child = self._build_tree(child_xml, args)
children.append(child)

Thank you for the suggestion! I'll try testing it first then send pr for review.

@erichlf
Copy link
Collaborator

erichlf commented Mar 25, 2025

Seems like the conclusion of the discussion in #5 is that we prefer do add conditionals directly to the node like ROS2 xml launch files. Thank you for your contribution and feel free to suggest or make improvements any time.

@erichlf erichlf closed this Mar 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: The ability to handle conditional would be really helpful.
3 participants