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

Expected behavior of Delay Decorator Node? #413

Closed
weeshal opened this issue Aug 11, 2022 · 2 comments
Closed

Expected behavior of Delay Decorator Node? #413

weeshal opened this issue Aug 11, 2022 · 2 comments

Comments

@weeshal
Copy link

weeshal commented Aug 11, 2022

I was testing some trees with the Delay decorator and observed some behavior that did not align with what I expected. My intuitive assumption behind the delay was wait for n secs, then start ticking my child normally, however it seems to operate as a delay between each tick of the child wait for nsecs, tick the child, and repeat. I wrote a test to demonstrate the difference between these 2 cases.

I would think the current implementation is more of a throttle decorator, i.e. tick my node every n seconds, and a delay, unless halted, would only delay ticking of a child one time. This might just be my own misunderstanding of the decorator's intention, but wanted to give it some visibility in case the expressiveness of language used can be improved.

struct DelayTest : testing::Test
{
    BT::DelayNode root;
    BT::AsyncActionTest action;

    DelayTest() : root("delay", 300)
      , action("action" )
    {
        root.setChild(&action);
    }
    ~DelayTest() = default;
};

TEST_F(DelayTest, DelayTickChild)
{
    action.setExpectedResult(NodeStatus::RUNNING);
    BT::NodeStatus state = root.executeTick();
    while (action.tickCount() == 0)
    {
      state = root.executeTick();
      std::this_thread::sleep_for(std::chrono::milliseconds(10));
    }
    state = root.executeTick();

    ASSERT_EQ(1, action.tickCount());
    ASSERT_EQ(NodeStatus::RUNNING, state);
    ASSERT_EQ(NodeStatus::RUNNING, action.status());
    
    state = root.executeTick();
    // what it currently is
    ASSERT_EQ(1, action.tickCount());
    // what we expected (fails)
    ASSERT_EQ(2, action.tickCount());
}
@facontidavide
Copy link
Collaborator

I think I know what you mean. Yes, I think there is an error in the implementation. I will push the fix on both master and v3.8 branch

@facontidavide
Copy link
Collaborator

The intention of the delay is to add that delay every time we execute a new action. Consider this:

<Delay delay_msec="200">
   <Sleep msec= "300">
</Delay>

The expected behavior is to keep this entire branch in RUNNING state for 500 ms (with Sleep being executed with a delay of 200 ms).

facontidavide added a commit that referenced this issue Nov 22, 2022
facontidavide added a commit that referenced this issue Nov 22, 2022
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

No branches or pull requests

2 participants