Fix bug in Parallel and add a While composite node.

[DanTulovsky]

[askft]

Hi @DanTulovsky, and thanks for your PR!

Indeed there was a bug in the Parallel node.

As for the While node, you can replicate that behavior in the following way:

UntilFailure {
    Sequence {
        Condition (custom node)
        Action (custom node)
    }
}

This also allows you to have multiple conditions and multiple actions (just put them after each other in the sequence).

So for instance, instead of while (x < 5) x++ you would have something like UntilFailure(Sequence(LessThan(x, 5), AddTo(x, 1))). So here, when LessThan fails, the Sequence node will fail, and the UntilFailure node will succeed, moving on to the next node in the tree.

Your suggested While node does obviously make the code simpler in the case of there being only one condition and one action, but it doesn't scale beyond that, so I will not pull that part of the code due to its redundancy and over-specificity. If you want to use a While node in your own code, you are of course able to use components from the core package to do so.

As for PersistentRandomSelector: it just starts from a random child and no shuffling takes place. This is not what one would expect from a decorator called PersistentRandomSelector. If you wanted to, I suppose you could make a "proper" PersistentRandomSelector by borrowing both from the PersistentSequence code and the RandomSelector code. As it looks now I will not pull this part of the code either.

If you would like to either make a separate pull request for the bugs and comments, or just update this one, I will pull it.

Thank you.

[DanTulovsky]

For PersistentRandomSelector, what I wanted was a random selector that would continue Running the same child until that child finished, but would pick which child to run at random. Which, I think, is what the code does right now.

Is that now what it's doing? It seems to work properly in my app. RandomSelector shuffles which child it updates on every Tick(), which is not what I wanted.

Pick a random child. Update it until it finishes (status != running) On next try, pick a random child again.

Hence the "persistent" part in the name...

But maybe I am still confused by how this works :)

On Fri, Apr 3, 2020, at 3:01 PM, Alexander Skafte wrote:

Hi @DanTulovsky https://github.com/DanTulovsky, and thanks for your PR!

Indeed there was a bug in the Parallel node.

As for the While node, you can replicate that behavior in the following way:

UntilFailure { Sequence { Condition (custom node) Action (custom node) } } This also allows you to have multiple conditions and multiple actions (just put them after each other in the sequence).

So for instance, instead of while (x < 5) x++ you would have something like UntilFailure(Sequence(LessThan(x, 5), AddTo(x, 1))). So here, when LessThan fails, the Sequence node will fail, and the UntilFailure node will succeed, moving on to the next node in the tree.

Your suggested While node does obviously make the code simpler in the case of there being only one condition and one action, but it doesn't scale beyond that, so I will not pull that part of the code due to its redundancy and over-specificity. If you want to use a While node in your own code, you are of course able to use components from the core package to do so.

As for PersistentRandomSelector: it just starts from a random child and no shuffling takes place. This is not what one would expect from a decorator called PersistentRandomSelector. If you wanted to, I suppose you could make a "proper" PersistentRandomSelector by borrowing both from the PersistentSequence code and the RandomSelector code. As it looks now I will not pull this part of the code either.

If you would like to either make a separate pull request for the bugs and comments, or just update this one, I will pull it.

Thank you.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/askft/go-behave/pull/2#issuecomment-608607453, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAD4PPZQVDC7EPSH3BKLQKTRKYXBRANCNFSM4LUSAR3Q.

[askft]

@DanTulovsky Upon closer inspection, you are right. Your code takes the RandomSelector and adds persistence to it. It's been a long time since I touched this code.

The confusion is probably due to me not naming things well enough... Selector and RandomSelector currently do quite different things—the Selector runs all children in order and returns as soon as one succeeds, while the RandomSelector simply randomly selects a child to run. Your PersistentRandomSelector acts in accordance with the RandomSelector. Perhaps at a later point I will invent a better naming for the nodes. I'll have to think about that more, but I will accept this part of your pull request too. :)

[askft]

Hey @DanTulovsky if you update the pull request to only reflect the changes in common/composite/parallel.go (removal of logging and the bug), I will accept it. Otherwise I'll close the PR and do it on my own.

[DanTulovsky]

Oh sorry.. please go ahead on your own, I don't have time right now...

On Thu, Apr 30, 2020, at 2:29 PM, Alexander Skafte wrote:

Hey @DanTulovsky https://github.com/DanTulovsky if you update the pull request to only reflect the changes in common/composite/parallel.go (removal of logging and the bug), I will accept it. Otherwise I'll close the PR and do it on my own.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/askft/go-behave/pull/2#issuecomment-622025382, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAD4PP2A4LASYKDZ6TID72DRPG7SHANCNFSM4LUSAR3Q.