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

Create BT for each navigation (fixes #1285) #1322

Closed
wants to merge 1 commit into from
Closed

Create BT for each navigation (fixes #1285) #1322

wants to merge 1 commit into from

Conversation

mjeronimo
Copy link

Rather than trying to re-use the behavior tree, create the behavior tree each time NavigateToPose is called. This avoids some problems in the BT library when using CoroActionNodes (coroutines).

…ree each

time NavigateToPose is called. This avoids some problems in the BT library
when using CoroActionNodes (coroutines).
@orduno
Copy link
Contributor

orduno commented Oct 31, 2019

Previously, we had noticed the robot would take considerable time from the moment the goal pose was provided when it started moving, we thought it had to do with the BT tree constructions. Does this re-introduce that problem?

@SteveMacenski
Copy link
Member

How long does it take to create the tree? That seems* like something relatively heavy and would add unacceptable latency.

  • intuition, but I think a change like that should have some numbers associated with them.

@@ -197,8 +186,11 @@ BtNavigator::navigateToPose()
}
};

// Create the Behavior Tree from the XML input
BT::Tree tree = bt_->buildTreeFromText(xml_string_, blackboard_);
Copy link

Choose a reason for hiding this comment

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

So now the tree could change between navigations if the user modifies the bt xml file during run-time?

@ghost
Copy link

ghost commented Nov 11, 2019

@mjeronimo Can you file a ticket in the behaviortree repo that describes the issue?

@bpwilcox
Copy link

@mjeronimo What's the status of this PR? Is this intended to be merged? I think it is a high priority to fix #1285 for Eloquent.

@SteveMacenski
Copy link
Member

+1 on @bpwilcox's comment. Is there a blocker?

@mkhansenbot
Copy link
Collaborator

@mjeronimo - can you resolve the conflicts on this, make sure it passes CI and then I think it can be merged?

@SteveMacenski
Copy link
Member

SteveMacenski commented Nov 25, 2019

I'd like to know what the latency introduced is for creating the BT before merging. I want us to know what this overhead is and make a conscious decision that its acceptable.

@ghost ghost assigned bpwilcox Nov 25, 2019
@ghost
Copy link

ghost commented Nov 25, 2019

@bpwilcox To take a second look and see if there is another way to deal with this problem

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.

5 participants