-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add runid #5553
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
base: main
Are you sure you want to change the base?
Add runid #5553
Conversation
Signed-off-by: silanus23 <[email protected]>
Signed-off-by: silanus23 <[email protected]>
Signed-off-by: silanus23 <[email protected]>
Signed-off-by: silanus23 <[email protected]>
@adivardi could you give this a review? I just wondered if I comprehend the idea correctly. |
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 left a few comments, but looks good!
We would need to add it to other nodes as well, all the ones that use BT::isStatusActive()
to initialize (you can see #5035, though may new nodes were added in the meantime).
} | ||
} catch (const std::exception & e) { | ||
// run_id not found on blackboard, use old behavior | ||
if (!BT::isStatusActive(status())) { |
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 am not sure about this, that would be a weird surprise. Maybe just let it crash? It would be the same as if server_timeout
is missing.
|
||
// Subsequent ticks with same RunID should continue without re-initialization | ||
result = tree_->tickOnce(); | ||
EXPECT_EQ(result, BT::NodeStatus::RUNNING); |
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 come all tests have the same EXPECT_EQ(result, BT::NodeStatus::RUNNING);
?
Thank you. |
return; | ||
} | ||
|
||
run_id_++; |
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 we should use a UUID unique hash instead
Basic Info
Description of contribution in a few bullet points
I tried to implement the idea at #5036
I added run_id to the blackboard and it's symetrycal to action node template. So that everytime blackboard ticks nodes one can keep track of nodes init time. This is an insurance for synchronization in the background. Made this activated with a parameter so that it won't interfere with current implementations and break them too.
Description of documentation updates required from your changes
Action nodes now have the parameter
is_global
for this feature.Description of how this change was tested
I have tested this locally with debug lines it was showing so integration tested too. Besides I wrote backward compatibility tests too.
Future work that may be required in bullet points
In future adding current behavior trees
is_global = true
for every nodeAs the final making this a default.
For Maintainers:
backport-*
.