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

fix issue #61, remove registerNodesIntoFactory #62

Closed
wants to merge 1 commit into from

Conversation

facontidavide
Copy link
Collaborator

@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
Copy link
Collaborator

MarqRazz commented May 6, 2024

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
Copy link

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.

@@ -91,7 +91,6 @@ TreeExecutionServer::TreeExecutionServer(const rclcpp::NodeOptions& options)

// register the users Plugins and BehaviorTree.xml files into the factory
RegisterBehaviorTrees(p_->params, p_->factory, p_->node);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @kenyoshizoe pointed out we can't load the Tree.xml files that are pass in via parameters before the statically linked behaviors are loaded.

@facontidavide
Copy link
Collaborator Author

@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.

@facontidavide facontidavide deleted the issue_61_factory branch June 11, 2024 09:04
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.

3 participants