Dynamic selectors/sequencers do not reset re-evaluated connections

NodeCanvas Forums General Discussion Dynamic selectors/sequencers do not reset re-evaluated connections

This topic contains 17 replies, has 2 voices, and was last updated by  timv 2 months, 3 weeks ago.

Viewing 15 posts - 1 through 15 (of 18 total)
  • Author
    Posts
  • #13785

    timv
    Participant

    I have been working with NodeCanvas for quite a while now and I thought I had a good understanding of the basic nodes. Alas, I was wrong. I found that dynamic selectors and sequencers (maybe more?) do not reset connections when they are re-evaluated. In my attached example, the “PlayAudio” task will only ever be triggered once. This was actually quite surprising to me, because the word ‘re-evaluated’ (which is appears as ‘revaluated’ in the description of the nodes) suggests to me that it would evaluate the same thing again from the start. Instead, it ‘continues’ executing connections that already finished (with Failure or Success). I’m not even sure what to expect in all cases. Is this by design?

    Attachments:
    You must be logged in to view attached files.
    #13795

    Gavalakis
    Keymaster

    Hey,

    Dynamic Selector and Sequencers indeed do not Reset their child nodes per-frame. If they were to Reset the child nodes per-frame, then a Dynamic Sequencer or Selector would both simoultanously execute child action nodes and act as a parallel. Even worse and in case of Running (actions for example) the action would simply start and reset (restart) per-frame as well. This is also why the Action Node specifically, only execute assigned Action Task only after the node has been reset, which in your example image, it would have been when the tree is restarted, but due to the “Run Forever” action you are using, the tree is “locked” forever running that node.
    If you were to return Success instead of Failure on your action node on the left, then the tree would work correctly (as far as I understand what the expected behaviour is), since the Selector (regardless of Dynamic or not) looks to higher priority child nodes that return Success (not Failure).

    I hope that made sense (?).
    Thanks.

    #13800

    timv
    Participant

    If they were to Reset the child nodes per-frame, then a Dynamic Sequencer or Selector would both simoultanously execute child action nodes and act as a parallel.

    I’m not sure I follow your explanation. A dynamic selector may already execute multiple child connections right? As long as child connections finish within the same update (without returning Running), the selector will continue iterating over the available alternatives. As soon as a higher priority node returns Running, it resets the other connection that was Running before. Only one connection will ever have the state Running, which is the main difference with Parallel. I don’t suggest changing that.

    Even worse and in case of Running (actions for example) the action would simply start and reset (restart) per-frame as well.

    I would not expect it to reset a connection that is running, because in that case the result is still pending. However, after a Selector connection finishes and returns Failure or even Optional (or maybe anything but Running?), I think it would be more logical to reset the connection before next time it is re-evaluated. In my example resetting the left connection actually makes it feel less like a Parallel node to me. Right now it is keeping track of the state of multiple connections.

    My example is a minimal example which I specifically constructed to show the (for me) unexpected behavior of a dynamic selector. Imagine both the left connection and the right connection are larger trees with much more logic in them. There are several ways in which the left part can fail, but I want to keep trying the left side each update. As long as that keeps failing, I want to continue executing the right hand side, but as soon as the left hand side can be executed, the right had side should be interrupted. (which dynamic selector already does)

    Oh, and in this example, the dynamic selector is the start node, which it often isn’t. In this case you could say “just return success instead and it will work”, but that will complete the selector, which I don’t want in my larger tree.

    #13803

    timv
    Participant

    Here is an example I think should just work, but doesn’t because of the way a dynamic selector works at the moment.
    There are two spheres in the scene that rotate around the origin. If the user clicks the first sphere, they’ll stop rotating and the timeout node will give the user 1 second to click on the second sphere. If the second sphere is clicked within that time, a light goes on.

    The issue here is that if the user only clicks on the first sphere and then misses the timeout, the Sequencer will still remember its last running node and it will keep executing the same child connection even though that connection previously already returned Failure.

    See an animation of the resulting issue here: https://gyazo.com/e56b6c5562d8df2d1ec9d9462b985c18

    I hope you´ll agree that that looks broken.

    Attachments:
    You must be logged in to view attached files.
    #13806

    timv
    Participant

    Here is another example to illustrate my point. The two trees shown in the attached image act identically. Both trees rotate the spheres and allow the user to click the first sphere, then wait, then click the second sphere to turn on a light. I hope you’ll agree that the left part of the dynamic selector version makes no sense. That tree will never work on its own. But because the dynamic selector does not reset the left connection, the conditional nodes start acting as wait until-nodes.

    I must conclude that the dynamic selector (and also dynamic sequencer) is broken as it alters the execution and therefore the meaning of subtrees. On top of that, the way it changes meaning is really unpredictable as it depends on what kind of state remains inside each of the nodes after they finish (returning Failure, Success, Optional etc).

    Attachments:
    You must be logged in to view attached files.
    #13814

    Gavalakis
    Keymaster

    Hello again and sorry for the late reply (my brother got married).
    Thanks a lot for your detailed elaboration on the subject as well as the examples. It is true that Dynamic option in Sequencer and Selector can create some unpredictable results, as in “not the expected behaviour”. I experimented a bit and I think you are right about reseting.
    If you have the time and want to further elaborate, can you please open up both Sequencer.cs and Selector.cs and add these lines of code at the very start of the for-loop iteration and let me know your thoughts?

    Is the new behaviour with the above change, more closely to what you would expect from the dynamic Selector (and Sequencer).
    Let me know.

    Thanks again for your elaboration.

    #13816

    timv
    Participant

    Hi again! No problem, hope you had a good time at the wedding.

    Great to hear that you agree! Your suggested change looks a lot like what I had in mind. When I have the time later this week (probably Thursday), I will investigate and test it more closely.

    #13832

    timv
    Participant

    Hi Gavalakis,

    I have spent some more time looking at the change and testing the effects and I think it is 100% correct this way. (except you missed the after outConnections) I tested both dynamic Selector and Sequencer in various situations. Using “i < lastRunningNodeIndex” as a condition in both Selector and Sequencer is quite an elegant way of making sure you reset all connections that need to be reset.

    The attached Sequencer examples now work correctly. In the first example, the left connection keeps evaluating the entire task list. All of the tasks in that action node finish in the same frame, so the sequencer will keep running the connection on the right side.

    The second example is a bit more contrived, but the point here is that the MouseOver condition is now reevaluated when the right side connection of the dynamic sequencer is running. (If you remove your mouse from the object, the light stays on as expected)

    Of course, there may be some fallout due to this change as some code might (intentionally or unintentionally) depend on this behaviour, so you’d propbably want to add a note about it to the change log. I found at least one thing in our game that broke and I remember I spent quite a lot of time getting that part to work, because I couldn’t figure out what the problem was. I suspect it will be easier to fix the issue now that the behaviour is consistent.

    Thanks,
    Tim

    Attachments:
    You must be logged in to view attached files.
    #13845

    Gavalakis
    Keymaster

    Hello again Tim,

    Thanks a lot for the follow up and testing the change. I am glad that it’s more of the expected behaviour now, which I will have to agree; it does look more consistent with your suggestion and thus I am definetely keeping it for v3.0. I will list it in the “Important Changes” along with some other that will arise for v3.0.

    Thanks again for your elaboration, suggestions and insights on the matter!

    #13857

    timv
    Participant

    Hi Gavalakis,

    I think that unfortunately this issue is not completely fixed yet. I think this part is too conservative when it comes to resetting:

    I’m investigating to find a minimal BT to show you the issue. Most likely all nodes > i and <= lastRunningNodeIndex will need to be reset. Not just the lastRunningNodeIndex one.

    #13860

    timv
    Participant

    I spent some additional time tracking down a minimal example of the issue. See the attached image. This currently only prints “Hello World” once, while I would expect it to print it repeatedly (each second).

    This happens because the center connection is never reset and the ActionNode keeps returning the cached status.

    Attachments:
    You must be logged in to view attached files.
    #13873

    Gavalakis
    Keymaster

    Hey,

    Hmmm. Do you mean something like this instead:

    I need to think this a bit more before completely destroying the way Dynamic Selector/Sequencers are working as of now though 🙂
    Let me know.

    #13885

    timv
    Participant

    I think it should be this:

    – which is what dynamic selector currently uses on Success.

    By the way, is that code actually needed on Success? I would expect a recursive reset to happen (from higher up the tree) before the next call to OnExecute. That would already reset all child connections. Maybe that code is a workaround that is no longer needed?

    I understand that you’re a bit hesitant, but I see it as getting the solid foundation of consistent rules needed to build more complex BTs, like the ones in our game.

    #13910

    Gavalakis
    Keymaster

    Hey,
    Considering the changes we are about to make here, then this code in Success will no longer be needed (since it prety much does the same thing).
    I do indeed hesitate to make big changes in core parts, but I do of course also want to improve what needs improving, and it seems that this one definetely does 🙂
    I will make some more tests and let you know.
    Thanks!

    #13937

    timv
    Participant

    Hi Gavalakis,

    Sorry for not getting back to you sooner. I had quite a busy week. I thought about this some more, but I do think the code also needs to be there in case of success. See the attached two images. When I wrote this I forgot about how NC uses the Status to show the colors of the connections. So, even though in theory it could work, (as long as the parent node issues a recursive Reset eventually,) I don’t think removing the code from the Success case is the right thing to do. The image without_reset_other_on_success.png really looks incorrect.

    On a related node (sorry for the pun), I noticed today that the step sequencer never resets its child nodes. I was also a bit surprised to find out that it increases the index even when the connection returned status Running. I’m now thinking if there is a way to add some asserts that check for missing Resets. Or if we could establish the exact rules and then go over all composite nodes to double check them.

    Tim

    Attachments:
    You must be logged in to view attached files.
Viewing 15 posts - 1 through 15 (of 18 total)

You must be logged in to reply to this topic.