behavior3 / behavior3js

Behavior3 client library for Javascript (Behavior Trees for Javascript)
MIT License
402 stars 105 forks source link

MemSequence bug? #24

Open lmessinger opened 7 years ago

lmessinger commented 7 years ago

Hi,

Thanks so much for a VERY comprehensive library. It's a work of art. We like it a lot.

i wonder if there's a bug in MemSequence, when it uses the open() method to reset the 'runningChild' index.

consider a MemSequence with 3 actions children.

  1. action1 starts, return RUNNING for a few ticks
  2. MemSequence is opened (stored at the open node list)
  3. action1 ends, return SUCCESS.
  4. MemSequence is closed
  5. action2 starts, return RUNNING
  6. MemSequence is opened
  7. runningChild is reset to 0!

if thats indeed a bug, i wondered where and when runningChild should be reset? is it a functional decision, based on some other parameters, or should it reset always when it reaches the end of the sequence?

also, what is the use for the openNode list?

thank you for any idea Lior

--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/40606176-memsequence-bug?utm_campaign=plugin&utm_content=tracker%2F18331363&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F18331363&utm_medium=issues&utm_source=github).
lmessinger commented 7 years ago

It seems the bug wasnt at the memSequence itself but rather at the code that closes the not-recently-opened-nodes. I wonder what do you think of this fix?

in BehaviorTree.js, I put:

// does not close if it is still open in this tick
        var start = 0; 
        var nodesToClose= [];
        var leaveOpen = [];
        for (var i=0; i<lastOpenNodes.length; i++) {
            var foundItem = currOpenNodes.find(function(a) {
                return a===lastOpenNodes[i];
            })
            if (foundItem) {
                leaveOpen.push(foundItem);
            }
            else {
                nodesToClose.push(lastOpenNodes[i])
            } 
        }

        // close the nodes (reverse loop has no special reason)
        for (var i=nodesToClose.length-1; i>=0; i--) {
            nodesToClose[i]._close(tick);
        }

instead of

var start = 0;
      var i;
      for (i=0; i<Math.min(lastOpenNodes.length, currOpenNodes.length); i++) {
        start = i+1;
        if (lastOpenNodes[i] !== currOpenNodes[i]) {
          break;
        } 
      }

      // close the nodes
      for (i=lastOpenNodes.length-1; i>=start; i--) {
        lastOpenNodes[i]._close(tick);
      }
danielepolencic commented 7 years ago

@lmessinger could you submit a PR with the failing test?