New BT Builder and Plugin Interface

[jjzapf]

The main changes proposed in this pull request are as follows.

1. Create a BT builder plugin interface.
2. Move the current BT builder implementation to a plugin named SimpleBTBuilder.
3. Create a new and improved STN-based BT builder plugin named STNBTBuilder.

The BT builder plugins are located in the plansys2_executor package with the header and source files placed in sub-directories labeled bt_builder_plugins. As noted above, there are two plugins available. SimpleBTBuilder is a port of the existing implementation to a plugin. STNBTBuilder is a new an improved implementation based on the notion of Simple Temporal Networks (STNs). See description below for more details.

All unit tests have been updated to use the plugin interface. However, these unit tests currently only exercise the SimpleBTBuilder plugin. Future pull requests will be created to introduce unit tests for the STNBTBuilder plugin. Nonetheless, the STNBTBuilder plugin has been tested with a handful of examples and should work with any example currently supported by PlanSys2. In addition, the new plugin more fully captures the semantics of PDDL and thus allows for the execution of more complex plans that cannot be handled by the current implementation. To allow for more vetting time, the default value for bt_builder_plugin is currently set to SimpleBTBuilder. Once we have some unit tests for STNBTBuilder and have verified that it works on more examples, the default should be set to STNBTBuilder.

Brief Description of STNBTBuilder

The current BT builder implementation does not capture precisely the semantics of complex time triggered plans. In some instances, this even leads to triggering actions at the wrong time. Most notably, the current implementation only allows for establishing links between the end of one durative action and the start of another. However, the PDDL standard allows for far more complex interactions. For example, the start of one durative action can trigger the start of another durative action. The STNBTBuilder plugin seeks to capture a more complete subset of the full PDDL semantics. To this end, the STNBTBuilder currently preforms 2 major steps. First, the PDDL plan is transformed into a Simple Temporal Network (STN). Second, the STN is transformed into a Behavior Tree (BT). The full details of the STN-based BT builder algorithm will be provided at a later time TBD.

[sarcasticnature]

Obviously I'm not the maintainer so my opinion isn't of the foremost importance, but I would suggest the default plugin be set to the SimpleBTBuilder for now due to the fact that:

1. The SimpleBTBuilder (if it is solely a port of the existing code) has been proven on existing codebases instead of a 'handfull' of examples and is less likely to introduce new bugs that may have been missed during initial testing.
2. STNBTBuilder does not have unit tests

If it were defaulted to STN I would be certain to pin my fork to SimpleBTBuilder until the STNBTBuilder was vetted further (not that I don't trust @jjzapf, I just can't afford for an important component of my codebase to break)

[sarcasticnature]

I would also request that this PR be delayed until after the popf issue has been resolved, which you can read more about in:

• Check domain validity always fails #232
• Fixes for bt_actions and popf_plan_sover #236
• etc

As well as the fix to the tests mentioned in

• Change CI to dockers #239 (comment)

[jjzapf]

Obviously I'm not the maintainer so my opinion isn't of the foremost importance, but I would suggest the default plugin be set to the SimpleBTBuilder for now due to the fact that:

1. The SimpleBTBuilder (if it is solely a port of the existing code) has been proven on existing codebases instead of a 'handfull' of examples and is less likely to introduce new bugs that may have been missed during initial testing.
2. STNBTBuilder does not have unit tests

If it were defaulted to STN I would be certain to pin my fork to SimpleBTBuilder until the STNBTBuilder was vetted further (not that I don't trust @jjzapf, I just can't afford for an important component of my codebase to break)

Hi @sarcasticnature. The above comments make total sense to me. As I mentioned before, I would be fine setting the default value to SimpleBTBuilder. @fmrico has also been involved in the development of these changes, so I will defer to him.

One thing to note, however, is that you can easily switch between the two plugins using the provided parameter. So even if the STNBTBuilder is chosen to be the default value, you can always choose SimpleBTBuilder by setting the appropriate launch file parameter. See link below for example.

https://github.com/PlanSys2/ros2_planning_system/pull/240/files#diff-55ace32d7dc0371ee0394e670c9ef90dc4c9c3b050fa5bcf8c8f209924819cb0R73

[xydesa]

I agree with @sarcasticnature about leaving the default builder plugin at SimpleBTBuilder for now given that it is the only one with unit tests, and it has been proven out in many months of use of this library. Once the new STNBTBuilder plugin has had more usage and testing (unit tests would be a big plus too), I can see changing the default in a new minor or major version number increase.

[roveri-marco]

I agree with the comments, but note that the SimpleBTBuilder is buggy and not reflecting the PDDL semantics of a plan!
See the issues that lead to the creation of the new one! Given this, IMHO the default should be the new one, with the possibility to switch to the old one for a short period for backward compatibility.

[fmrico]

Dear all,

Yes, the default value should be SimpleBTBuilder until we develop more unit tests and do a more extensive validation. I do not doubt that STNBTBuilder will be the default in the medium-term because it provides many advantages, as @roveri-marco said.

I hope to have time in a few days to review all this. I am quite busy with the ICRA deadline and fixing CI and popf issues, as @sarcasticnature said.

Thank you all!!

[jjzapf]

Thanks everyone. I've updated the PR so that the default BT builder plugin is now SimpleBTBuilder. To test with the new STNBTBuilder, all you should need to do is set the bt_builder_plugin parameter to STNBTBuilder.

https://github.com/PlanSys2/ros2_planning_system/pull/240/files#diff-55ace32d7dc0371ee0394e670c9ef90dc4c9c3b050fa5bcf8c8f209924819cb0R73

[fmrico]

Merging!! Thanks @jjzapf 🚀