Skip to content

Conversation

indraneelpatil
Copy link
Contributor

@indraneelpatil indraneelpatil commented Apr 26, 2020

  • This pull request is with reference to this issue 92

  • Two new decorator nodes are proposed in this pull request called Delay Node and Wait for Enter Press node which I found personally very useful in my application

  • The delay node allows the user to add a fixed millisecond delay before ticking the child

  • The Wait for Enter Press Node as the name suggests stops execution of the child until enter key is pressed by the user

  • I am very new to contributing code online and I welcome constructive criticism about this work.

I am also thankful to the owner of the library and all maintainers!

@indraneelpatil indraneelpatil mentioned this pull request Apr 26, 2020
@facontidavide
Copy link
Collaborator

First, thanks a lot for contributing.

I think the WaitForKeyPressed should use ncurses, as https://github.com/BehaviorTree/BehaviorTree.CPP/blob/master/src/controls/manual_node.cpp

@facontidavide
Copy link
Collaborator

I see apractical problem with the DelayNode .

As you may now, Reactive nodes gives the illusion that the BT is multi-threaded, when actually it is single-threaded.

To achieve this, any code must block as little as possible. A "delay", therefore, must return RUNNING and stop execution, not freeze.

What if the DelayNode is a child of the ReactiveSequence and it has a sibling that preempt it?

@indraneelpatil
Copy link
Contributor Author

@facontidavide Thanks for your kind words! I had no idea Manual Selector node existed, thanks for pointing it out. Its a much better solution than what I had pushed, so I removed Wait for Key Press node in the latest commit.

@indraneelpatil
Copy link
Contributor Author

indraneelpatil commented Apr 30, 2020

Regarding the delay node, I have added a TimerQueue timer to the node to keep a track of the delay and the node now has these three states ->

  • If the timer has not yet triggered then the node will return RUNNING
  • If the timer has triggered, then the node will tick the child and return the result of the child
  • and if the delay node is preempted by its sibling (like in the example you specified) then the timer will be cancelled, child will not be ticked and the delay node will return FAILURE

Please let me know if this satisfies your requirement!

@indraneelpatil
Copy link
Contributor Author

@facontidavide Hey, have you gotten any time to review this pull request?
Thanks in advance!

Copy link
Contributor

@gramss gramss left a comment

Choose a reason for hiding this comment

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

Looks solid to me. I cross checked it with https://github.com/BehaviorTree/BehaviorTree.CPP/blob/master/src/decorators/timeout_node.cpp

Just some small adjustments in style and clarity and it should be fine 👍

@indraneelpatil
Copy link
Contributor Author

@gramss Thank you for your suggestions! I have pushed them in the latest commit.

Copy link
Contributor

@gramss gramss left a comment

Choose a reason for hiding this comment

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

Changes are looking good. Only already introduces methods are used like in other decorators. @facontidavide should be ready to be merged 🙂

@facontidavide facontidavide merged commit 8f1c491 into BehaviorTree:master Sep 1, 2020
@facontidavide
Copy link
Collaborator

thanks and sorry for the delay. overwhelmed with other things right now

@indraneelpatil
Copy link
Contributor Author

indraneelpatil commented Sep 1, 2020 via email

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