Decorators in a tree stored in the register exhibit global state

[Tresky]

Consider the following code where I've created a decorator that will add a custom cooldown timer to a BT that will start as soon as the tree experiences a SUCCESS in the sequence that it is wrapping. This concept works excellent when there is 1 tree present.

However, as soon as I instance the class a second time, the two trees exhibit shared behavior. Meaning, as soon as one of the trees experiences a SUCCESS, both of their decorators fire and the cooldown begins for BOTH trees instead of just the one that had the SUCCESS as you would expect.

// Custom decorator that adds a cooldown after a SUCCESS
// Modified from the CooldownDecorator in the repo.
// Original CooldownDecorator times the cooldown from the START of
// the sequence. I need to time from the end of the sequence getting a SUCCESS.
class SuccessCooldownDecorator extends Decorator {
  nodeType = 'SuccessCooldownDecorator'

  setConfig ({ cooldown }) {
    this.config = { cooldown }
  }

  decorate (run) {
    if (this.lock) {
      return FAILURE
    }
    const result = run()
    if (result === SUCCESS) {
      this.lock = true
      setTimeout(() => {
        this.lock = false
      }, this.config.cooldown * 1000)
    }
    return result
  }
}

// Create a new sequence
BehaviorTree.register('my-sequence', new Sequence({
  nodes: [ ... ]
}))

// Implement a custom Decorator around the Sequence
BehaviorTree.register('decorated', new SuccessCooldownDecorator({
  node: 'my-sequence',
  config: { cooldown: 2 }
}))

// Place the BT inside of a class
class MyContainer {
  contructor () {
    this.tree = new BehaviorTree({
      node: 'decorated',
      blackboard: { ... }
    })
  }

  update () {
    this.tree.step()
  }
}

const inst1 = new MyContainer()
const inst2 = new MyContainer()

---

If I move the decorator being created out of the register and into the class constructor, it works as expected.

class MyContainer {
  contructor () {
    this.tree = new BehaviorTree({
      node: new SuccessCooldownDecorator({
        node: 'my-sequence',
        config: { cooldown: 2 }
      }),
      blackboard: { ... }
    })
  }
  ...
}

I tried looking through the Decorator and Node classes to see if I could identify the issue I'm having, but I couldn't find any instances of any global state being used. I'm not sure why doing new Sequence in the global scope works fine, but doing new Decorator in the global scope doesn't allow this use case.

Is my custom decorator implementation incorrect? Am I doing something else wrong?

[Calamari]

Heyho @Tresky.

What you are observing is the following: If you register a node, you put that node into a global registry, if you then reuse that node in different tree, both trees use the exact copy of that node. So any change you make to this within one tree, is of course immediately done on the other tree, since that is the same node.

Interestingly, I never thought about that case before, or encountered it. Guess I should write some specs for that. And have to think about a way to resolve that case, without impacting the performance. Sharing the instance way faster then having to create new ones for all trees, but on the other hand, how often are new trees created anyways…

[Tresky]

Hey @Calamari! Thanks for the quick reply. I understand why you'd make it that way for lookup times.

It seems to me that you could copy the entire logic tree into the Tree whenever it is created, so you have independent instances of the nodes in each tree. You could potentially create an independent registry inside of each Tree instance that stores instances of the nodes it needs.

You could still get very fast lookup of the nodes in that tree, the performance of creating a new tree would just be impacted due to the copying of the nodes. As you said, though, new trees likely aren't created in large quantities during runtime, but I guess that would also depend on use case.

I realize, though, that it may be a big push to get to something like that because it would require changing a major implementation detail of the library. Not to mention it's risky anytime you change such a core implementation detail. Maybe you, as the author, have some better thoughts than me on what might work, given you understand the code much more than I do.

---

My current use case is in the context of game dev. I'm attempting to have each instance of Enemy own an instance of BehaviorTree that stores the logic for how the AI should act (wandering, chasing, idling, etc).

Could you think of a way to enable this sort of use-case using this library beyond the major changes I suggested above? Is it just to totally exclude the use of the registry altogether?

[Tresky]

I just took another look at the library code. This might actually be a fairly simple change to make.

The library is using a global registry defined here:

BehaviorTree.js/src/BehaviorTree.js

Lines 4 to 8 in 856bb65

	let registry = {}
	export function getRegistry() {
	return registry
	}

Given the constructor of the BehaviorTree class here:

BehaviorTree.js/src/BehaviorTree.js

Lines 21 to 26 in 856bb65

	export default class BehaviorTree {
	constructor({ tree, blackboard }) {
	this.tree = tree
	this.blackboard = blackboard
	this.lastResult = null
	}

You could simply add the following to the constructor and you have an independent registry inside the tree.

this.registry = Object.assign({}, registry)

The only other detail would be to change all registry lookups to look inside the reigstry on the tree instead of the global one.

It's not necessarily 0 performance impact, but I can't imagine it would be very unless your regsitry grows to a large size. At that point, it wouldn't be hard to add some nice optimization where only the nodes needed are copied from the registry.

[Calamari]

Hmm, I thought I posted some thoughts, this morning. Apparently I forgot to hit the comment button. Well, here is a more informed look:

I just checked and it is not as simple. The solution you suggest will copy the registry, but the instances will be still the same, since JavaScript is copying objects by reference. So you would gain nothing.

The best idea I came up with, until now is, for a maybe additional syntax for creating objects that are instantiated on demand, something like:

BehaviorTree.register('my-decorated-node', [MyDecorator, {
  node: 'my-sequence',
  config: { cooldown: 2 }
}])

It feels a bit cumbersome, with that array, but on the otherhand it is maybe nice to serialize, and since passing in a function as creator-function is out, since it is used as a short version to define tasks. (I wonder how many people really use that, and if that is really needed.)

On a slightly related topic:
Since a while, I have the idea in my head, that it might be a useful idea to split out the registry stuff into an own class, so you can attach different registries to different behavior trees. But I still don't see a real use case for that. Thought this might be a case to do this, but I think it will not yield benefits here sadly.