meniku / NPBehave

Event Driven Behavior Trees for Unity 3D
MIT License
1.13k stars 194 forks source link

GC/Performance problems #12

Closed myselfshj closed 7 years ago

myselfshj commented 7 years ago

Hello Author

I tried to put a method which calls another component function into the mutiframe action,like this: new Action((bool _shouldCancel) => { if (!_shouldCancel)
{ MoveInComp(true); return Action.Result.PROGRESS; } })

The caller method defined like this: private void MoveInComp(bool isStart) { GetComponent().isStart = true; GetComponent().enemyPos = new Vector3(21f, 2f, 2f); }

And the really working function is in Update() of MoveComponent component void Update() { if (isStart == true) MoveTowardsEnemy(enemyPos); }

    public void MoveTowardsEnemy(Vector3 targetPos)
   {     gameObject.transform.position = Vector3.MoveTowards(gameObject.transform.position, targetPos, this.speed);        }

The running result in Unity is gameObject moved to the targePos just in One frame. My intent is to let it move in multiframe slowly.
How to do it then ?

//////***////// So I tried another way. I didn't use Update() in MoveComponent this time. I just call the plain MoveTowardsEnemy() method. public void MoveTowardsEnemy() //in MoveComponent { if (isStart == true && enemyPos != Vector3.zero) gameObject.transform.position = Vector3.MoveTowards(gameObject.transform.position, enemyPos, this.speed); }

In behavior tree caller is like this: private void MoveInComp(bool isStart) { GetComponent().isStart = true; GetComponent().enemyPos = new Vector3(21f, 2f, 2f); GetComponent().MoveTowardsEnemy() ; }

But the result is same, gameObject moved to the targePos just in One frame. Please give me some advice. Thanks

meniku commented 7 years ago

I guess that your speed is maybe to high? Check the documentation of the MoveTowards. They also multiply it by Time.deltaTime, that's what you should be doing too - the number going in should be pretty small.

Besides from that your code looks okish. I would suggest writing it like this:

new Action(MoveInComp)
...
private Action.Result MoveInComp(bool isCancel)
{
    if( isCancel )
    {
       GetComponent().isStart = false;
       return Action.Result.FAILED;
    }
    else
    {   
        GetComponent().isStart = true;
        GetComponent().enemyPos = new Vector3(21f, 2f, 2f);
        GetComponent().MoveTowardsEnemy() ;
        return Action.Result.PROGRESS;
    }
}

Also note that your task should probably should eventually stop once the target location is reached, so putting another condition in may be good:

private Action.Result MoveInComp(bool isCancel)
{
    if( isCancel )
    {
       GetComponent().isStart = false;
       return Action.Result.FAILED;
    }
    else
    {   
        GetComponent().enemyPos = new Vector3(21f, 2f, 2f);
        if( GetComponent().IsEnemyReached() )
        {
            GetComponent().isStart = false;
            return Action.Result.SUCCESS;
        }
        else
        {
            GetComponent().isStart = true;
            GetComponent().MoveTowardsEnemy() ;
            return Action.Result.PROGRESS;
        }
    }
}

Besides from that: If you're designing a "complex" task like that you may also just consider creating an own subclass of Task.

myselfshj commented 7 years ago

speed is not the reason, I tried. I've thought about creating a new subclass of Task, I did it and it worked. But, I found I created mynewTask instance in the multiframe cycle. That's not efficient. I think I should instantiate mynewTask just once, and call the worker function in multiframe cycle. But I didn't know how to do that. Your code above looks good , so I tried. But the result is same, not fixed. I post my full code below,I wish it maybe helps.

myselfshj commented 7 years ago

using UnityEngine; using NPBehave; public class NPBehaveTest : MonoBehaviour { private Blackboard blackboard; private Root behaviorTree; public GameObject enemy; void Start() { enemy = GameObject.FindGameObjectWithTag("marker"); behaviorTree = CreateBehaviourTree(); blackboard = behaviorTree.Blackboard;

if UNITY_EDITOR

    Debugger debugger = (Debugger)this.gameObject.AddComponent(typeof(Debugger));
    debugger.BehaviorTree = behaviorTree;

endif

    behaviorTree.Start();
}

private Root CreateBehaviourTree()
{
    // we always need a root node
    return new Root(
        // kick up our service to update the "playerDistance" and "playerLocalPos" Blackboard values every 125 milliseconds
        new Service(0.5f, UpdatePlayerDistance,
            new Selector(
                new BlackboardCondition("enemyDistance", Operator.IS_GREATER, 0.2f, Stops.IMMEDIATE_RESTART,
                    new Sequence(
                        new Action(() => Debug.Log("enemy is found..." )),
                        new Action(() => { Debug.Log("enemy is waiting..."); }),
                        new Action(MoveInComp)
                    )
                ),
                new Sequence(
                    new Action(() => SetColor(Color.grey)),
                    new Wait(2f)
                )
            )
        )
    );
}

private void UpdatePlayerDistance()
{
    behaviorTree.Blackboard.Set("enemy", enemy);
    behaviorTree.Blackboard.Set("enemyDistance", (enemy.transform.position - transform.position).magnitude);
}
private void SetColor(Color color)
{
    GetComponent<MeshRenderer>().material.SetColor("_Color", color);
}
private Action.Result MoveInComp(bool isCancel)
{
    if (isCancel)
    {
        GetComponent<MoveComponent>().isStart = false;
        return Action.Result.FAILED;
    }
    else
    {
        GetComponent<MoveComponent>().enemyPos = new Vector3(21f, 2f, 2f);
        if (GetComponent<MoveComponent>().IsEnemyReached())
        {
            GetComponent<MoveComponent>().isStart = false;
            return Action.Result.SUCCESS;
        }
        else
        {
            GetComponent<MoveComponent>().isStart = true;
            GetComponent<MoveComponent>().MoveTowardsEnemy();
            return Action.Result.PROGRESS;
        }
    }
}

}

myselfshj commented 7 years ago

using System; using UnityEngine;

public class MoveComponent :MonoBehaviour
{

    private float speed = 0.02f;    
    public float arriveDistance = 0.01f; 
    public bool isStart { get ; set ; }

    public Vector3 enemyPos;
   public bool IsEnemyReached()
   {
       if((gameObject.transform.position - enemyPos).magnitude < arriveDistance)
            return true;
       return false;

   }

    public void MoveTowardsEnemy()
   {
       if (isStart == true && enemyPos != Vector3.zero)
           gameObject.transform.position = Vector3.MoveTowards(gameObject.transform.position, enemyPos, this.speed);
   }

}
myselfshj commented 7 years ago

Please forgive my poor English, I am trying my best. cheers.

myselfshj commented 7 years ago

OMG. I found the reason. " public Vector3 enemyPos; " in MoveComponent should has getter and setter. That's my fault. It works now. Thank's for your help. You may modify and use my code as your example code. Cheers.

myselfshj commented 7 years ago

I think it's better to put the worker functions outside the behavior tree structure, and the behavior tree just only do workflow decision. That‘ll make the behavior tree neat and trim. Your NPBehave works great.

myselfshj commented 7 years ago

Since separated the heavy functional Action methon outside the behavior tree structure, The core job left is to design the game logic in the tree which gives me a lot of fun. How about this design blow ? Any Advice ? I mean If I used these nodes properly ?

private Root CreateBehaviourTree()
{
    return new Root(

        new Service(0.5f, UpdatePlayerDistance,

            new Selector(
                new BlackboardCondition("enemyDistance", Operator.IS_GREATER, 0.5f, Stops.IMMEDIATE_RESTART,
                    new Sequence(
                        new Action(ActionWander),
                        new Action(ActionPatral),
                        new Action(ActionSearch),
                        new Action(ActionSeek)
                    )
                ),

                new BlackboardCondition("enemyIsAlive", Operator.IS_EQUAL, true, Stops.IMMEDIATE_RESTART,
                    new Sequence(
                        new Action(ActionPrepareForCombate),
                        new NPBehave.Random(70, new Action(ActionPhyicalAttack)),
                        new NPBehave.Random(20, new Action(ActionMagicAttack)),
                        new NPBehave.Random(10, new Action(ActionTactfulAttack))
                        )
                    ),

                new BlackboardCondition("AmIAlive", Operator.IS_EQUAL, false, Stops.NONE,
                    new Sequence(
                        new Action(ActionPlayTriumphMusic),
                        new Action(ActionUpdateGameData),
                        new Action(ActionUpdateGUI)
                        )
                    ),

                new Sequence(
                    new Wait(2f),
                    new Action(ActionDrawBackCamera)
                )
            )
        )
    );
}
meniku commented 7 years ago

I've thought about creating a new subclass of Task, I did it and it worked. But, I found I created mynewTask instance in the multiframe cycle. That's not efficient. I think I should instantiate mynewTask just once, and call the worker function in multiframe cycle. But I didn't know how to do that.

when you create a custom task you don't wrap that inside an Action or anything. It's just a new Task type that you could place in your behavior tree and replace the multiframe Action entirely with that new task.

I think it's better to put the worker functions outside the behavior tree structure, and the behavior tree just only do workflow decision. That‘ll make the behavior tree neat and trim. Your NPBehave works great.

Exactly, that's the way I also do it most of the time. And thanks for the compliment.

How about this design blow ? Any Advice ? I mean If I used these nodes properly ?

That really depends on what you want to achieve and the way you want to do it. I don't know your exact use-case. But from looking at your suggested structure, my feeling is that things like "Camera" or "Music" wouldn't go in a behaviour tree that's used for an Enemy AI, as you probably have multiple instances of that running at the same time. However it could actually make sense for other scenarios, for example if you want to control a your global game state with a BT ( I never used it for such things, but I could think that it could make sense in some scenarios ). Like I said, it really depends, it's hard to give a general advice here.

myselfshj commented 7 years ago

Thank you a lot . I am a little over excited . I am trying make some game with your BPBehave, and I hope what I‘ll do can be a little helpful to this AI tree and appreciate your help and kindness.

myselfshj commented 7 years ago

I made a scene of 50 bad guys as NavMeshAgent chased a hero ,when they reached they play some attack animation.

At the same time, I check the memory analysis tool, I found the GC Allocation of UnityContext.Update() in BehaviourUpdate is aways 2.3-5k in every second.

Will that lead a performance defect? Any possibility to optimize? I check the source code , there are many foreach cycle and timer arrays reside here, and several Clear() methods in this Clock.Upadate(). I guess it's the place where mutipleframe functions execult.

I have no idea how to solve . I just point it out and I hope that can help you improve the performance. Maybe it's not a issue.

meniku commented 7 years ago

I have not yet made a game with NPBehave and more than around 5 enemies in the scene, so I did never run into such problems. If there really is a problem we'll have to check how to reduce those allocations.

How big is the performance problem you have (how much of a frame's time is spend doing the GC?)? Also note that every behavior tree is different, so my main question is how frequently is your tree traversed? you can check the debugger on how fast the "Total Starts" counters go up. For instance for one of my bots in ace of traze, I got roughly 50 starts per second, which is quite high (but it's a huge tree, so quite ok).

Also note that depending on the game you may think about some kind of "LOD" mechanismn for AI's far away or not visible, meaning you may want to have a system where AI's that are not visible anyway do update much less frequently or even suspend completely. This is a general thing, you also should do that when not using NPBehave.

MohHeader commented 7 years ago

I believe there are some performance & GC issues. For example the Asserts, are creating a lot of strings. I will check once more and get back to you :)

Also @myselfshj ,,, you can try to use Deep profile,, sometimes it could be within your code it self.

meniku commented 7 years ago

So at least for the Assertions we could either #if them out entirely or just remove any concatenation with variables in the strings, so they are just plain string literals which should not be collected at all. Maybe we could just use #if UNITY_EDITOR to ensure they are ripped entirely from release builds.

For the problems with the Clock's dictionaries I have no real clue right now, any suggestions?

myselfshj commented 7 years ago

@meniku

I don't think that's a problem.

Unity Profiler shows GC Allocations per Frame: 60/2.3 KB. I don't know what's 60 means. Eachone of 50 bad guys has a behavior tree, runs all the time. Total Starts of eachone is 8 which maybe is nodes number, Active Timers are around 50-60

When I run the scene CPU value in Profiler Window is about 10ms, that's why I think it's not a problem. Compare to the GUI GCalloc, it's quite low, according to the bad guy number.

I am thinking split my scene to severl gamezones, eachone is a small zone. Just like what you said "AI's that are not visible anyway do update much less frequently or even suspend". But I wander how to disable AI tree in script? gameObjec.GetComponent.active = false? I'll check if there is this kind of API in source code.

myselfshj commented 7 years ago

@MohHeader Thanks for your advice.
NPBehave do its real job in paramters not in functions, that make it hard to use breakpoint to debug. Maybe that's why so many assertions. But it's not a problem, not when release you game.

meniku commented 7 years ago

But I wander how to disable AI tree in script? gameObjec.GetComponent.active = false?

For NPBehave it should be enough to just Stop() the Root node and Start() it again later.

This is usually sufficient, however if you want to go the more complicated route and freeze the AI in it's current state and resume from the exact same state later, the best would be to build the tree with a custom Clock instance. You have then complete control over the clock and have to call Update() on it every frame while the AI is active and stop calling Update() while it's inactive. (The constructer of the Root-node allows you to set a custom Clock instance). Or you could just throttle the updates for AI's further away if you still want them to process the tree, but at a lower rate

myselfshj commented 7 years ago

OK,I see. I'll try to use Stop() and Start() to improve the AI frequence. Writing custom Clock is a bit hard for me for now. I'll try later. I am attempting to use NPBehave and FSM in different scenario in one game. I hope that can improve efficency more and make programming easier.

meniku commented 7 years ago

No, you don't need to write an own Clock implementation, you just create a new instance instead of using the default global one from the UnityContext (which the default Root's constructor uses).

Here is an example that throttles the tree to be updated every second:

public class NPBehaveExampleThrottling : MonoBehaviour
{
    const float updateFrequency = 1.0f; // update eveyr second

    private Clock myClock;
    private Root behaviorTree;

    private float accumulator = 0.0f;

    void Start()
    {
        Node mainTree = new Service(() => { Debug.Log("Test"); },
            new WaitUntilStopped()
        );
        myClock = new Clock();
        behaviorTree = new Root(new Blackboard(myClock), myClock, mainTree);
        behaviorTree.Start();
    }

    void Update()
    {
        accumulator += Time.deltaTime;
        if(accumulator > updateFrequency)
        {
            accumulator -= updateFrequency;
            myClock.Update(updateFrequency);
        }
    }
}

you can also share this clock between multiple trees if you like, so you could for example have a group of 10 AIs in a same area driven by the same clock and having 10 other AIs in a different area driven by a different clock in a different speed.

I cannot think about how to make that simpler TBH. The current implementation allows you to do whatever you like with little code to write. Although as always I'm open for suggestions.

myselfshj commented 7 years ago

Thanks for your kindness. I'll practice these all. It's a huge job to make a game, I am working on it. Your NPBehave and All you advice is helpful.