BehaviorTree / BehaviorTree.ROS2

BehaviorTree.CPP utilities to work with ROS2
Apache License 2.0
144 stars 59 forks source link

fix issue #61, remove registerNodesIntoFactory #62

Closed facontidavide closed 4 months ago

facontidavide commented 4 months ago

@MarqRazz as correctly pointed out in issue #61 , we can not invoke a virtual method in the constructor.

This leaves only 2 possibilities:

  1. postpone registration to "later", for instance in the execute method.
  2. Get rid intirely of the registerNodesIntoFactory method and expose the factory in the public API.

I propose solution 2, because my gut feeling is that solution 1 might be error-prone.

This unfortunately remove the ability of the factory to be "reset" if the list of folders changes at run-time, but that is something that is probably unlikely, anyway.

Now, the recommended place to do registration of custom node (or enum, we forgot that!) is the constructor of the derived class.

What do you think?

MarqRazz commented 4 months ago

I'm bummed to loose the ability to dynamically register new Behaviors and trees into the system. One use case I had for this was to add/remove behaviors as the hardware re-configures itself (for example a robot arm performing a tool change from GripperA to GripperB). I wanted the ability to define high level trees like Pick object and depending on the configuration it would automatically select the correct sub-trees based on the behaviors loaded.

We could make option 1 more robust by registering plugins and statically linked behaviors the first time the Action is run.

if(p_->param_listener->is_old(p_->params) || load_behaviors)
  {
    p_->params = p_->param_listener->get_params();
    registerNodesIntoFactory(p_->factory);
    RegisterBehaviorTrees(p_->params, p_->factory, p_->node);
    load_behaviors = false;
  }
kenyoshizoe commented 4 months ago

I also agree to solution 1. The solution 2 implemented in this PR does not resolve the issue of failing to load the BehaviorTree. This is because BehaviorTree loading occurs within the constructor of the base class before the constructor of the derived class is executed.

I have implemented method 2 here.

In addition, I made modifications to directly assign rclcpp::Node::SharedPtr node to BT::RosNodeParams's nh, and it worked perfectly. (I have created another issue regarding this.)

If necessary, I will create PR.

facontidavide commented 4 months ago

@kenyoshizoe ok, I will adopt you solution.

But we should not forget that there are two more things that are registered into the factory:

  1. enums: frequent use case
  2. substitution rules: done only when doing testing

The 2. is not a concewrn, I believe, but keep in mind that 1. will fail if we try to register multiple time the same enums.