Seneral / Node_Editor_Framework

A flexible and modular Node Editor Framework for creating node based displays and editors in Unity
https://nodeeditor.seneral.dev
MIT License
2.01k stars 414 forks source link

Functional Nodes that are not tied to the Node Editor Window #201

Closed TimOgden closed 2 years ago

TimOgden commented 2 years ago

I've spent a decent amount of time creating a Behavior Tree system that works really well, and I would like to release it to the general Unity audience at some point, but I've got to address some pretty major issues before I consider doing that.

The main issue I would really like to fix is the fact that the Behavior Tree is very GUI dependent. I can only run one BT at a time, which is the one in the Node Editor window. If the Node Editor window is not open, the BT won't run. Currently I use the Node Editor window to create "blueprints" of behavior trees where I set up the flow of the graph, use the RTEditorGUI to input parameters that my leaf nodes need to run, and then when I save the canvas, I save the .asset file of the canvas.

I'm thinking that I need to break up the "Node" aspect and the logic execution of my objects. The BaseBTNode class will handle the display of the nodes, displaying the input/output ports, assigning them the color based on their type, and handling the RTEditorGUI stuff to grab parameter values. The LogicExecutor class, which would be a ScriptableObject, would take over the responsibility of calculating the node logic, including the Init, Start, and Tick functions. Here are the few issues I need to address and the solutions I think might be viable. Any input and direction would be greatly appreciated.

Issue: Do I need subclasses of both BaseBTNode and LogicExecutor for every type of node? I have a lot of different types of nodes that I have constructed so far. It would be a pain to say create a InverterNode and InverterLogicExecutor, instead of just the one Inverter node class I have now. Possible Solution: I don't need a node sub-class for every node type. I can use smarter inheritance and have CompositeNode, DecoratorNode, and LeafNode derive from BaseBTNode, which will handle the display of the input/output ports, the size, and color of the node in the window. The only issue here is that different node types under the same subclass of BaseBTNode will have different parameters.

Issue: Different node types under the same subclass of BaseBTNode will have different parameters. For example, currently a "MoveTo" leaf node has a TextField for a blackboard key, and a float field for the speed to move, but a "WaitXSeconds" leaf node only has a float field for the number of seconds to wait. Possible Solution: Change how parameters work. Instead of each non-abstract node subclass defining the floats and strings that make up its parameters, LogicExecutor will have a Dictionary<string, object> parameters field, and the BaseBTNode will loop through, display the parameters, and set them accordingly. Would have to do some typecasting logic, or instead do a Dictionary<string, float> float_params, Dictionary<string, string> str_params, Dictionary<string, bool> bool_params kind of approach.

Issue: Which contains which? I can have every BaseBTNode contain a LogicExecutor or every LogicExecutor contains a BaseBTNode. Possible Solution: I think it makes more sense for every BaseBTNode to contain a LogicExecutor. Then OnBeforeSaveCanvas(), the LogicExecutor will update its list of children LogicExecutors, and the root node LogicExecutor will be saved (like a LinkedList, I think we only need to save the root LogicExecutor since they are storing references to each other all the way down the tree).

After all that work, and assuming everything works out, my thinking is that I can give any GameObject a BTManager component with my root node LogicExecutor SO as the "blueprint", and the BTManager will call ScriptableObject.CreateInstance(blueprint) to make a copy of the tree and will call Tick() on the root node. LogicExecutor is not derived from Node so it will be able to execute without the need of the Node Editor window, and I can reuse the same blueprint for many different GameObjects. What are your thoughts? Thanks to @Seneral or anyone who wants to contribute to this!

Seneral commented 2 years ago

I think you're overcomplicating this. What is your exact reason why you need to separate the GUI code from the Logic code? I understand why you'd entertain it, but as you have demonstrated it won't be particularly nice either. Take a look at the RTCanvasCalculator example, it does not initiate the GUI function and does not load any GUI resources, but it allows you to load the canvas and manipulate it and execute traversal routines (or your own implementation of these).

TimOgden commented 2 years ago

Yeah, I've been browsing through the RTCanvasCalculator, NodeCanvas, and NodeCanvasTraversal classes, and that seems like a much better built-in solution. Definitely shouldn't reinvent the wheel here.

One reservation I still have with this approach, would I be able to run more than one of the same behavior tree at the same time with this? I can't tell if there are any restrictions against that.

Seneral commented 2 years ago

No, that should be fine. There is a global nodeCanvas to refer to the currently edited canvas, but that is only used for the editor, and since there is no true concurrency in editor applications, even that is not hindering you from having multiple editors at the same time (with a bit of work).

TimOgden commented 2 years ago

Here's where I'm at. I have a BehaviorTreeCanvas that derives from NodeCanvas and a BehaviorTreeManager which is a monobehaviour.

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using NodeEditorFramework;
using NodeEditorFramework.Standard;

public enum TaskResult { NOT_ACTIVE, SUCCESS, FAILURE, RUNNING };

public class BehaviorTreeManager : MonoBehaviour
{
    public BehaviorTreeCanvas behaviorTree;
    public float tickingFrequency = .2f;
    public bool debug = false;

    private IEnumerator coroutine;

    void Awake()
    {
        coroutine = EvalTree();
    }

    // Start is called before the first frame update
    void Start()
    {
        StartCoroutine(coroutine);
    }

    public void AssureCanvas()
    {
        if (behaviorTree == null)
            throw new UnityException("No canvas specified to calculate on " + name + "!");
    }

    private void SetupTree()
    {
        AssureCanvas();
        NodeEditor.checkInit(false);
        behaviorTree.Validate();
    }

    public IEnumerator EvalTree()
    {
        if(debug)
            Debug.Log("Starting evaluation of tree with status: " + behaviorTree.rootNode.GetStatus());
        while (true)
        {
            SetupTree();
            TaskResult status = behaviorTree.rootNode.Tick(this);

            if (!(status == TaskResult.NOT_ACTIVE || status == TaskResult.RUNNING))
            {
                if (debug)
                    Debug.Log(transform.name + " (" + behaviorTree.name + " behavior tree) returned: " + status);
                yield return status;
                yield return new WaitForSeconds(tickingFrequency); // pause after full tree traversal
                behaviorTree.rootNode.DeactivateStatus();
            }
            yield return new WaitForSeconds(tickingFrequency); // ticking frequency
        }
    }

}

I looked into making a BehaviorTreeCanvasTraversal class, but overriding the TraverseAll method won't be that helpful to me since I need to pass the 'this' reference to the manager Monobehaviour to access components like NavMeshAgent and Animator, and I also need to return a TaskResult. Not sure if there is a way around this.

I still have the issue that only the last loaded NodeCanvas within the Node Editor window will work, any other will throw a NullReferenceException when the root node iterates through its children since the size of the children length stays correct, but all elements are null. Not sure how to go about fixing this, I assume it's an issue on my end but I really don't know what it could be.

My BaseBTNode class has fields that should be serialized:

[SerializeField]
public BaseBTNode[] children = new BaseBTNode[0];
[SerializeField]
public BaseBTNode parent = null;

and the RecursivelyFindChildren method is called in the BehaviorTreeCanvas.OnBeforeSaveCanvas method

public void RecursivelyFindChildren()
{
    if (inputPorts.Count > 0)
        parent = inputPorts[0].connections[0].body as BaseBTNode;
    if (outputPorts.Count < 1)
    {
        children = new BaseBTNode[0];
        return;
    }
    children = outputPorts[0].connections.Select(connection => connection.body as BaseBTNode).ToArray();
    Array.Sort(children, (o1, o2) => o1.position.x.CompareTo(o2.position.x));
    foreach (BaseBTNode child in children)
    {
        child.RecursivelyFindChildren();
    }
}

Let me know if there is anything else you need, I'm at a loss here. Could it have to do with me not using a Traversal to traverse the graph? You can also check out my forked repo on the Examples/AISystems branch if you want all the code. It's a (pretty) minimal example.

Seneral commented 2 years ago

Why have the children array? It won't save since they are references to ScriptableObjects. That means they need to be saved as files in the saved canvas. NodeEditorSaveManager.CreateWorkingCopy is responsible for this behaviour, it is needed to make sure the canvas the editor holds in memory and the last saved canvas on disk is separate, else a ton of other weird behaviour by unity kicks in. In other words, you either need to tell the save system that you hold these references (override these two) or better, just use the output ports directly. As for references to components, you could have them as explicit inputs to the canvas and set them on the input nodes (or specifically those that need it) before calculating. E.g. if one node type needs it, filter by node type and set it. Or you can have it be a field in your custom canvas, each node can access it using the canvas member. Worst case, just modify the framework, that's always an option

TimOgden commented 2 years ago

I figured early on that having to typecast the Nodes of outputPorts[i].connections[j].body to BaseBTNodes in order to call Tick() (a BaseBTNode method) would be bad on performance and that it would be better to save them as BaseBTNodes in an array so I only have to typecast once. Decided to skip over that step and try just using the outputPorts directly. Much much better, I can use any tree I want now and it's lovely! Not sure what kind of performance hit I am getting, but that is something I can test later. Also will consider which method of referencing components is best in the next few days.

Thanks a lot @Seneral ! I should've gone to you a lot sooner 😄