libgdx / gdx-ai

Artificial Intelligence framework for games based on libGDX or not. Features: Steering Behaviors, Formation Motion, Pathfinding, Behavior Trees and Finite State Machines
Apache License 2.0
1.21k stars 243 forks source link

Discussing Behavior Trees #12

Open davebaol opened 10 years ago

davebaol commented 10 years ago

I've opened this to discuss behavior trees API enhancements.

@implicit-invocation Resuming discussion https://github.com/libgdx/gdx-ai/pull/4#issuecomment-56509640 ... Not sure why you don't like the XML format, but I think that it has some advantages:

So I'm playing with the XML format just to see what can be done. Currently I can successfully load a behavior tree from this file:

<BehaviorTree>
  <Import task="com.badlogic.gdx.ai.tests.btree.dogtasks.BarkTask" as="Bark"/>
  <Import task="com.badlogic.gdx.ai.tests.btree.dogtasks.CareTask" as="Care"/>
  <Import task="com.badlogic.gdx.ai.tests.btree.dogtasks.MarkTask" as="Mark"/>
  <Import task="com.badlogic.gdx.ai.tests.btree.dogtasks.RestTask" as="Rest"/>
  <Import task="com.badlogic.gdx.ai.tests.btree.dogtasks.WalkTask" as="Walk"/>
  <Root>
    <Selector>
      <Parallel>
        <com.badlogic.gdx.ai.tests.btree.dogtasks.CareTask urgentProb="0.8"/>
        <AlwaysFail>
          <com.badlogic.gdx.ai.tests.btree.dogtasks.RestTask/>
        </AlwaysFail>
      </Parallel>
      <Sequence>
        <Bark times="3"/>
        <Walk/>
        <Bark/> <!-- times defaults to 1, see BarkTask source code -->
        <com.badlogic.gdx.ai.tests.btree.dogtasks.MarkTask/>
      </Sequence>
    </Selector>
  </Root>
</BehaviorTree>

I added the "Import" tag to improve readability. It allows you to use the given alias in place of the fully qualified class name of the task. Actually Selector, Parallel, etc.. are predefined imports. Also, the "as" attribute is optional, meaning that the simple class name is used as the alias, i.e.

  <Import task="com.badlogic.gdx.ai.tests.btree.dogtasks.BarkTask"/>

creates the task alias "BarkTask". Also, I added task parameters, see urgentProb in CareTask and times in BarkTask. The attribute value is parsed according to the type of the corresponding field of the task class. For example, urgentProb is a float and times is an int. Supported types are: int, Integer, float, Float, boolean, Boolean, long, Long, double, Double, short, Short, char, Character, byte, Byte, and String.

Of course, we can maintain both formalisms as long as they have the same structural features. I mean, unlike task parameters, imports are just a syntactic sugar so they are not mandatory for the inline tab-based formalism.

I think we can use a "btree" branch in order to experiment with BT improvements while keeping the master branch clean.

davebaol commented 9 years ago

Just completed parallel description in the wiki page.

Also, I've updated all behavior tree tests. Now they use the tree viewer that is a good learning tool for beginners IMO.

SemaphoreGuardTest

There's just a little problem in the SemaphoreGuardTest. Currently the user can save and load the two trees independently from each other, meaning that he can easily break the semaphore and get both WalkTask running at the same time. I think I will inhibit load and save buttons for this test.

davebaol commented 9 years ago

I'd like to add the capability to clone trees with external libraries like kryo in order to free the user from the responsibility of overriding copyTo properly for each task.

I'm thinking of something like this:

public interface TaskCloner {

    public <T> Task<T> cloneTask (Task<T> task);

}

and in Task class

    public static TaskCloner TASK_CLONER = null;

    /** Clones this task to a new one.
     * @return the cloned task
     * @throws TaskCloneException if the task cannot be successfully cloned. */
    @SuppressWarnings("unchecked")
    public Task<E> cloneTask () {
        if (TASK_CLONER != null) {
            try {
                return TASK_CLONER.cloneTask(this);
            } catch (Throwable t) {
                throw new TaskCloneException(t);
            }
        }
        try {
            return copyTo(ClassReflection.newInstance(this.getClass()));
        } catch (ReflectionException e) {
            throw new TaskCloneException(e);
        }

Actually, copyTo is still the default but you can easily use kryo (no GWT support), for example:

        // Use kryo to clone tasks
        Task.TASK_CLONER = new TaskCloner() {
            Kryo kryo;
            @Override
            public <T> Task<T> cloneTask (Task<T> task) {
                if (kryo == null) {
                    kryo = new Kryo();
                    kryo.setInstantiatorStrategy(new StdInstantiatorStrategy());
                }
                return kryo.copy(task);
            }
        };

However, I'm not very happy with the static TASK_CLONER field. Any cleaner solutions?

sergeyzhulin commented 9 years ago

Why don't use cloner as parameter for method cloneTask?

In this case developer can decide is cloner static or something else, also if parameter is null standard cloner could be used

davebaol commented 9 years ago

@sergeyzhulin Thanks, your idea is interesting but looks like there are 2 drawbacks:

piotr-j commented 9 years ago

Static feels like only options that doesnt require adding another param to a lot of things. OT: BranchTask.removeChild(int) ?

davebaol commented 9 years ago

@piotr-j

Static feels like only options that doesnt require adding another param to a lot of things.

Ok then static will be fine

OT: BranchTask.removeChild(int) ?

Why not Task.removeChild(int) ? Then you want BehaviorTree.Listener.childRemoved(Task<E> task, int index) I guess.

EDIT: Hmmm.... Maybe I'm exaggerating :)

piotr-j commented 9 years ago

I guess it could be in Task, but most of the things can have only 0/1 children anyway. It would be more consistent i guess.

davebaol commented 9 years ago

Damn stupid javadoc! It's so fucking hard to get embedded code shown properly. I had to use a combination of <pre>, <code>, {@lileral ...} and finally {@code ...} for generics :flushed: https://github.com/libgdx/gdx-ai/blob/5839294958e78c49dfe2dfb0ec9b7188c6114adb/gdx-ai/src/com/badlogic/gdx/ai/btree/Task.java#L55-L70

davebaol commented 9 years ago

Wiki page updated.

davebaol commented 9 years ago

Combining Behavior Trees and State Machines

davebaol commented 9 years ago

Added test "Predators: Parallel vs. Sequence" that visually shows through steering behaviors the difference between sequence task and parallel task with sequence policy.

davebaol commented 8 years ago

Damn, I'm really tempted to change the API. Especially, I'd like to remove success, fail and running methods. Here's my story. I'm working on the behavior tree of dogs in this project and I was getting crazy because tree evaluation suddenly messed up. Then, after some hours, I found out the problem in the code below.

    @Override
    public void run () {
        DogCharacter dog = getObject();
        if (......) {
            dog.calculateNewPath();
            success();
        }
        fail();
    }

When the if condition is true both success and fail methods are executed. This was messing up the parent branch task. Simply adding the missing return after success fixed the problem.

Also, if you forget to call success, fail or running you're messing up things once again.

So, the problem with current API is that breaking the whole tree evaluation is too easy and very hard to debug.

In order to minimize error probability, I think that previous code should change like that

    @Override
    public Task.Status run () {
        DogCharacter dog = getObject();
        if (......) {
            dog.calculateNewPath();
            return Task.Status.SUCCESS;
        }
        return Task.Status.FAILED;
    }

What do you think?

piotr-j commented 8 years ago

Bit me as well. Forcing return sounds like a decent solution. Alternatively it could throw an exception if it is changed more then once per run.

davebaol commented 8 years ago

We should also throw an exception if none of success(), fail() and running() has been called.

I think I found a simpler solution. Since most if not all tasks written by developers are leaf tasks, we can fix the issue in the LeafTask class and keep the rest of the API unchanged:

public abstract class LeafTask<E> extends Task<E> {

    ...

    /** This method contains the update logic of this leaf task. The actual implementation MUST return one of {@link Status#RUNNING},
     * {@link Status#SUCCEEDED} or {@link Status#FAILED}. Other return values will cause an {@code IllegalStateException}.
     * @return the status of this leaf task */
    public abstract Status execute();

    @Override
    public final void run () {
        Status result = execute();
        if (result == null) throw new IllegalStateException("Invalid status 'null' returned by the update method");
        switch (result) {
        case SUCCEEDED:
            success();
            return;
        case FAILED:
            fail();
            return;
        case RUNNING:
            running();
            return;
        default:
            throw new IllegalStateException("Invalid status '" + result.name() + "' returned by the update method");
        }
    }
}
davebaol commented 8 years ago

Done :)

davebaol commented 8 years ago

Guys, I'd recommend you to take a look at the open source project GdxDemo3D if you're interested in non-trivial use of behavior trees. Dog's brain is not complete yet (it will just piss instead of taking the stick and bringing it back, for instance), but already able to coordinate several 3D animations and behaviors properly. @jsjolund and me are still working on it. Also, that guy is great when it comes to 3D modelling :sunglasses:

davebaol commented 8 years ago

Updated the wiki page with the link to the new code I added to the GdxDemo3D in order to combine behavior trees and state machines based on approach 1 described here.

I have to admit that I'm very happy with the new API. It has proven to be really versatile during the development of dog's behaviors in the GdxDemo3D.

New release coming soon.

davebaol commented 8 years ago

gdx-ai 1.7.0 is out

davebaol commented 8 years ago

From the JBT user manual:

  • Dynamic Priority List: task that executes the child with the highest priority whose guard is evaluated to true. At every AI cycle, the children's guards are re-evaluated, so if the guard of the running child is evaluated to false, it is terminated, and the child with the highest priority starts running. The Dynamic Priority List task finishes when no guard is evaluated to true (thus failing) or when its active child finishes (returning the active child's termination status).
  • Static Priority List: task that executes the child with the highest priority whose guard is evaluated to true. Unlike the Dynamic Priority List, the Static Priority List does not keep evaluating its children's guards once a child is spawned. The Static Priority List task finishes when no guard is evaluated to true (thus failing) or when its active child finishes (returning the active child's termination status).

These tasks solve a typical issue with behavior trees, i.e. they tend to be reasonably clunky when representing state-based behaviors. One possible solution is to combine behavior trees and state machines as described here.

With priority lists you can do all of that quite comfortably with behavior trees. For instance, in GdxDemo3D the dog uses these 3 trees and a state machine to determine which behavior tree is currently running. These are the 3 states deciding when to switch behavior tree (note the overriden canEnter methods which are the condition guards you would use with a priority list task).

In short, the 3 trees and the state machine above might become something like that

import calculatePathToHuman:"com.mygdx.game.objects.dog.CalculatePathToHumanTask"
import calculatePathToStick:"com.mygdx.game.objects.dog.CalculatePathToStickTask"
import followPath:"com.mygdx.game.objects.dog.FollowPathTask"
import lieDown:"com.mygdx.game.objects.dog.LieDownTask"
import piss:"com.mygdx.game.objects.dog.PissTask"
import setThrowButton:"com.mygdx.game.objects.dog.SetThrowButtonTask"
import sit:"com.mygdx.game.objects.dog.SitTask"
import spinAround:"com.mygdx.game.objects.dog.SpinAroundTask"
import spinAroundToFaceHuman:"com.mygdx.game.objects.dog.SpinAroundToFaceHumanTask"
import stand:"com.mygdx.game.objects.dog.StandTask"
import stateMachineTransition:"com.mygdx.game.objects.dog.StateMachineTransition"
import stickThrown?:"com.mygdx.game.objects.dog.StickThrownCondition"
import wander:"com.mygdx.game.objects.dog.WanderTask"
import whine:"com.mygdx.game.objects.dog.WhineTask"

import alreadyCriedForHumanDeath?:"com.mygdx.game.objects.dog.AlreadyCriedForHumanDeathCondition"
import isHumanDead?:"com.mygdx.game.objects.dog.IsHumanDeadCondition"
import isHumanInRange?:"com.mygdx.game.objects.dog.IsHumanInRangeCondition"
import humanWantToPlay?:"com.mygdx.game.objects.dog.HumanWantToPlayCondition"

subtree name:"feelSadForHumanDeathGuard"
  sequence
    invert
      alreadyCriedForHumanDeath?
    isHumanDead?
    isHumanInRange? meters:20

subtree name:"feelSadForHumanDeath"
  sequence
    calculatePathToHuman
    followPath gait:"run"
    whine
    parallel policy:"selector"
      wait seconds:"uniform,10,25"
      lieDown
    whine
    parallel policy:"selector"
      wait seconds:"uniform,5,9"
      sit
    setAlreadyCriedForHumanDeath   # this makes the task's guard fail; the dog will start acting on his own

subtree name:"playWithManGuard"
  humanWantToPlay?

subtree name:"playWithMan"
  sequence
    calculatePathToHuman
    followPath gait:"run"
    spinAroundToFaceHuman
    setThrowButton enabled:true
    parallel policy:"selector" # wait for the man to throw the stick
      stickThrown?
      repeat
        sequence
          parallel policy:"selector"
            wait seconds:"uniform,3,5"
            randomSelector
              stand
              sit
          spinAround
    setThrowButton enabled:false
    calculatePathToStick  # actually we just calculate a random point
    followPath gait:"run"

subtree name:"actOnYourOwnGuard"
  invert
    selector
      humanWantToPlay?
      isHumanDead?

subtree name:"actOnYourOwn"
  selector
    sequence
      random success:0.1
      piss
    parallel policy:"selector"
      wait seconds:"uniform,3,6"
      randomSelector
        wander gait:"run"
        wander gait:"walk"
        lieDown
        sit

root
  dynamicPriority
    [$feelSadForHumanDeathGuard] $feelSadForHumanDeath
    [$playWithManGuard]          $playWithMan
    [$actOnYourOwnGuard]         $actOnYourOwn

where the main tree is at the end of the file (identified by the keyword root) and each child of the dynamicPriority task has a guard in square brackets (actually a previously defined subtree that models the conditions of the canEnter method in the corresponding state) followed by the actual task (which is another previously defined subtree). You can think of the $subtreeName syntax like a kind of macro.

An important aspect to support priority lists is to keep the text format as simple as possible, since I don't want to get crazy while writing the parser. So, if you have better ideas, I'm all ears :ear:

davebaol commented 8 years ago

Also if I got it right the static priority

staticPriority
  [guard1] task1
  [guard2] task2
  [guard3] task3

acts like the more verbose tree below

selector
  sequence
    guard1
    alwaysSucceed
      task1
  sequence
    guard2
    alwaysSucceed
      task2
  sequence
    guard3
    alwaysSucceed
      task3

with the difference that the latter always returns SUCCEEDED when its active child task finishes, instead of returning the active child task's termination status.

davebaol commented 8 years ago

I've just realized that, at the API level, we can take into consideration the idea to add a guard to the Task base class, meaning that any task in any part of the tree can be conditioned. In normal situations, if the task's guard succeeds the task is evaluated otherwise FAILURE status is returned. On the other hand, if the task is a child of a priority branch, either dynamic or static, its guard is evaluated according to its parent policy.

For instance, in absence of a priority branch parent, the tree below

    sequence
      random success:0.1
      piss

can be expressed simply by

    [random success:0.1]  piss

Also, task's guards are optional, meaning that they implicity evaluate to SUCCESS. So, the tasks piss, [] piss and [success] piss are unconditioned and totally equivalent

What do you think?

@implicit-invocation I'd like to ear your opinion too :)

davebaol commented 8 years ago

BTW, since guards are nothing more than good old tasks, guarding the guard would come for free. I mean something like

[[[guard0] guard1] guard2] task

or maybe

[guard0] [guard1] [guard2] task

This would provide a simple inline syntax for guard sequences, which are a viable alternative to a guard tree whose root is a sequence. As long as you use short guard sequences (for instance, 0 to 4 guards) this construct remains pretty readable. But there's nothing - apart from common-sense, of course - stopping you from guarding dozens of guards.

To keep the logic simple for the user (and the implementation simple for me LOL) I'd just impose the limitation that guards can't run over multiple ticks, meaning that they must terminate with SUCCEEDED or FAILED immediately while RUNNING is an illegal status (should likely cause a runtime exception).

davebaol commented 8 years ago

I got task guards and dynamic priorities working at the API level just like described above. Now the most boring part will be writing the parser. :expressionless:

Also, since any task can have a guard, I think that a specific task for staticPriority is no longer necessary because now you can easily get the same result with selector by guarding its children. In text format it would be something like that

selector
  [guard1] task1
  [guard2] task2
  [guard3] task3

Since guards can be either something simple like a single task or something complex like a whole tree, I'm convinced that AI designers and developers will really benefit from such a feature. :bowtie:

MartinSojka commented 8 years ago

Dropping by from the Reddit thread ...

Interesting that you got the guards working already. I'm not sure I explained the priorities idea properly though, so let's try again, this time with an example.

Boring old Java code stuff. First, a leaf task which supplies its own priority.

class PlayHappyAnimation extends LeafTask<MyBlackboard> {
    @Override public Status execute() {
        // Run the animation and so on
        // Always succeed
        return success();
    }
    // This one is new, defaults to returning 0
    @Override public int getPriority() {
        // Needs return values 1-10
        return 100 / getObject().needForFood() / getObject().needForShelter() - 10;
    }
}

This one calculates some priority for another task (or branch)

class NeedPriority implements TaskPrioritizer<MyBlackboard> {
    @TaskAttribute
    public String need;

    @Override public int getPriority(Task task, MyBlackboard blackboard) {
        switch( need ) {
            case "food": return blackboard.needForFood();
            case "shelter": return blackboard.needForShelter();
            default: return Integer.MIN_VALUE;
        }
    }
}

Let's put it all together. Priorities are in parentheses before the tasks. Default priority is 0.

prioritySelector type:static runUntil:success
    (NeedPriority need:food) sequence
        # Search for food sequence here
    (NeedPriority need:shelter) sequence
        # Search for shelter sequence here
    PlayHappyAnmation # returns its own priority

Now, if the priorities for food or shelter search are above those for playing a happy animation, the one with the bigger need (= priority) gets used. If the need for both isn't that great, the happy animation plays, and the whole task succeeds. If they are great, but fail, PlayHappyAnimation gets played anyway. The AI might be hungry and without a roof above their head, but it can at least still smile.

davebaol commented 8 years ago

I see your point and I think that my proposal is a superset of yours :) For instance, you should be able to implement the desired behavior like that

class CheckPriorityCondition extends LeafTask<MyBlackboard> {

    public enum Need {
       Food, Shelter
    };

    @TaskAttribute(required=true) public Need need;

    public CheckPriorityCondition () {
    } 

    @Override public Status execute() {
        return needs(getObject(), need) ? Status.SUCCEEDED : Status.FAILED;
    }

    // No @Override; this method only exists for this task
    public boolean needs(MyBlackboard blackboard, Need need)  {
        // Use whatever formula you want and determine whether the given need is actually needed or not
    }
}
dynamicGuardSelector
    [checkPriority need:"food"] sequence
        # Search for food sequence here
    [checkPriority need:"shelter"] sequence
        # Search for shelter sequence here
    PlayHappyAnimation

What do you think?

MartinSojka commented 8 years ago

Ok, so ...

The idea with the guards is roughly the same for a static tree, though the needs(...) method gets quite a bit more complicated (it has to check for all the needs and compare them, not just the one passed).

And when you add another need, the formula needs to get updated as well, to pick whichever need is the strongest and if it is the one we're checking for, and if all the needs together are strong enough to not rather play a happy animation instead.

And then you try to add a need at runtime, which means that you suddenly have to write needs(...) to be able to deal with a variable number of needs, not just an enum of them.

And then you decide to add another bunch of potential subtrees, each of them evaluating the needs and other information to decide on priority, and each needing to know the exact formula for priority all the other, potentially many and changing at runtime branches use.

Essentially: Replacing priorities (which each subtree can calculate for itself not caring what other subtrees are doing, or which subtrees there are in the first place) with guards requires that the guards are way more complicated and tightly coupled with each other.

MartinSojka commented 8 years ago

Here's an idea how guards and priorities can work together to their strengths: A "planning selector".

Assume the selector has a bunch (more than 10, possibly more than 100) different possible branches. Assume we can also estimate the cost of executing each branch (either by querying each of them for the estimated cost, or via some external cost estimation class). The algorithm is thus:

  1. Pre-filter the branches: Make a list of all where the guards evaluate to "true".
  2. Estimate the costs of each branch in the list (that doesn't run them!).
  3. Assign the negative of those costs as priorities (least costly = highest priority).
  4. Pick and run the one with the highest priority.
  5. (If runUntil:success) Repeat until a branch returns with a success or running out of branches. Else (if runUntil:finish) just return the result of the first branch.
implicit-invocation commented 8 years ago

To deal with changes at runtime, should we just allow referencing callbacks/functors for attributes (with an additional paramater in the annotation as well)?

@davebaol great work :+1: I've been through some kind of crisis :smile: fortunately I'm alive again

davebaol commented 8 years ago

@implicit-invocation

To deal with changes at runtime, should we just allow referencing callbacks/functors for attributes (with an additional paramater in the annotation as well)?

Sounds like a nice idea for future extensions.

I've been through some kind of crisis :smile: fortunately I'm alive again

Nice to hear it. Welcome back! :smiley:

@MartinSojka TBH I'm not sure I've fully understood your use case. Maybe I just need a more concrete example. Anyways, I've used enum just for the sake of clarity. There's nothing stopping you from using a HashMap or any other data structure (which you usually access to through the blackboard). Also, there are some aspects I really like in this solution:

davebaol commented 8 years ago

Done! Here is a working tree using guards, DynamicGuardSelector and subtree references. I'll update the wiki page after the end of the LibGDX Jam, I guess. :smiley:

scooterman commented 8 years ago

@davebaol, sorry to hijack the thread, but how's the best way to do a "while" loop using behavior trees? I want to iterate over a list of possible targets and apply a selector over them. UntilFail and UntilSuccess won't resolve the problem since they can possibly end on a infinite loop.

davebaol commented 8 years ago

@scooterman I don't see what's the problem with UntilFail/UntilSuccess.

untilFail
  sequence
    next
    processItem

where

scooterman commented 8 years ago

@davebaol thanks for the reply. If processItem does run and I return success (meaning that, for example, I can execute an attack on the selected item, hence making sequence return true) untilFail will exit? I can let the iteration end but the ideal (in my case) would be to stop right after a successful sequence.

davebaol commented 8 years ago

@scooterman No, untilFail will exit only if the child sequence fails. You can decorate processItem with ìnvert, alwaysFail or alwaysSucceed based on your needs.

scooterman commented 8 years ago

great, thanks for the clarification.

scooterman commented 8 years ago

in fact, it won't work @davebaol. take a look on this snippet:

subtree name:"seekAndDestroy"
    sequence
        untilSuccess # loop through every entity in range
            sequence
                nextEnemyInRange
                canAttack?
                attackEnemy # try to attack

Suppose I have iterated over all my enemies in range and haven't attacked any of them. How I'm supposed to exit this loop since selectEnemyInRange will return false and the condition too?

Also it would be interesting have something like a "nop" task that wouldn't interfere in the current processing result, so I could use nextEnemyInRange on both selectors and sequences without having to invert it according to the logic.

davebaol commented 8 years ago

Well, you have to distinguish between end-of-iteration condition and in-range condition:

subtree name:"seekAndDestroy"
  untilFail    # loop through every entity
    sequence
      nextEntity   # only fails if there's no next entity
      alwaysSucceed  
        sequence
          isEnemyInRange?
          canAttack?
          attackEnemy # try to attack

BTW, in your tree the first sequence (the root of the subtree) is useless.

scooterman commented 8 years ago

@davebaol sorry for annoying, if you prefere another conversation mechanism like irc just say :smile:

The problem with this approach is that I lose information on one thing: if my attack succeed or not for that specific entity. My blackboard is the brain for one entity, and this tree is to check if I can attack another one in range. The ideal for this subtree was to shortcut or break the result of attackEnemy returning SUCCESS if one of it managed to attack, otherwise iterate over the next entity until exhausted. If exhausted and no one managed to attack, the subtree should return FAIL meaning that no enemy was found to attack. I could add a check after alwaysSucceed and then one after the untilFail but that's not composable, say I decide to implement a new verification for a different kind of operation. Imo this is looking like I'm trying to program procedurally which it's not desirable, but I can't see another way to do that without iteration, specially when starting to compose behaviors.

This may be related to the priority discussion above, imo.

davebaol commented 8 years ago

@scooterman

subtree name:"seekAndDestroy"
  sequence
    setFlag value:false
    untilFail
      sequence
        isFlag? value:false
        nextEntity  # only fails if there's no next entity
        alwaysSucceed
          sequence
            isEnemyInRange?
            canAttack?
            attackEnemy
            setFlag value:true
    isFlag? value:true    
scooterman commented 8 years ago

@davebaol I ended implementing an "exhaust" branch that simplified this case. Do you have any interest on an pull request for this?

davebaol commented 8 years ago

@scooterman Sure, if it's general enough I'll merge it.

mgsx-dev commented 7 years ago

I don't know if it's the right place to post but I have some suggestions :

First of all I think current API is complete specially with guards and dynamic guard selector, I personally always found a way to implement logic with provided generic tasks without hacking a re-code things (nice job BTW).

My concern right now is about pooling strategy :

My conclusion is that pooling strategy is game specific : recycle whole trees, recycle tasks, don't recycle is a design choice. But we need a mechanism to allow individual tasks recycling though and I think using Poolable interface is the best choice we have.

@davebaol What do you think ? Did you faced this problem ? I could provide a PR if you don't have time to implement it but I need your approval first.

davebaol commented 7 years ago

@mgsx-dev

@davebaol What do you think ? Did you faced this problem ? I could provide a PR if you don't have time to implement it but I need your approval first.

I've never needed to instantiate/destroy trees in game. I always do it during the initialization phase of the level, but I do recognize your use case. So, yes, PR is welcome. 😄

It seams impossible to use LibGDX Poolable interface with Task because of API conflicts (both reset methods have same signature but not the same contract). The only way I found so far would be to rename Task.reset to Task.resetTask and let Task implements Poolable interface. This change the current API.

Sounds good to me

there is no way to remove children from task in order to recycle. Note that possibility to remove children would be a useful feature for editors as well.

Yeah, this would be an interesting feature. Just notice that when you remove a child the parent task MUST update his own internal status accordingly. This operation is task-specific, of course.

My conclusion is that pooling strategy is game specific : recycle whole trees, recycle tasks, don't recycle is a design choice. But we need a mechanism to allow individual tasks recycling though and I think using Poolable interface is the best choice we have.

Couldn't agree more