-
Notifications
You must be signed in to change notification settings - Fork 0
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
Meta PR: try inheritance instead of callbacks #6
Conversation
@@ -41,9 +41,13 @@ ActionServerBT::ActionServerBT(const rclcpp::NodeOptions& options, const UserCal | |||
|
|||
// register the users Plugins and BehaviorTree.xml files into the factory | |||
register_behavior_trees(params_, factory_, node_); | |||
registerNodesIntoFactory(factory_); | |||
|
|||
// No one "own" this blackboard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we want this to persist, by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would a user go about clearing the global blackboard? Seems like some runs they could want values to persist but others should start clean. Should we consider adding an option to the Action goal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this also leads into the feature of the user waning to clear and ImportBlackboardFromJSON
. Another nice addition to the Action goal would be the ability to specify a path to a JSON file that should be loaded before running the tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can still access it and clean it in onTreeCreated
.
I would use that callback to do all this initialization and let the user take care of it, instead of tryingto figure out all the potential things they may want to do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I was thinking about that as an option too but it could lead to a few situations. Let's say they offer a service to set the blackboard. What would stop them from mutating the BB while the tree is running? Does BT.CPP have guards to protect from reading and writing BB variables at the same time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like how clean this ended up, great work @facontidavide!
@@ -41,9 +41,13 @@ ActionServerBT::ActionServerBT(const rclcpp::NodeOptions& options, const UserCal | |||
|
|||
// register the users Plugins and BehaviorTree.xml files into the factory | |||
register_behavior_trees(params_, factory_, node_); | |||
registerNodesIntoFactory(factory_); | |||
|
|||
// No one "own" this blackboard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would a user go about clearing the global blackboard? Seems like some runs they could want values to persist but others should start clean. Should we consider adding an option to the Action goal?
I just removed the method Brute force cleanup static auto clean_bb = BT::Blackboard::create();
clean_bb->cloneInto(*global_blackboard); |
RCLCPP_ERROR(kLogger, action_result->error_message.c_str()); | ||
tree.haltTree(); | ||
goal_handle->canceled(action_result); | ||
abort_action(status, "Action Server canceling and halting Behavior Tree"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically this should return goal_handle->canceled()
. If we move this statement out of the lambda we can support both canceled
and aborted
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After we fix the return on cancel I think this is good to go.
What about this?