Exit from Subgraph takes a lot of frames depending on its complexity (Major Bug)

Hello, dear Unity Developers!

Some time ago in this post you really helped me to fix issues with the slow rate update for TryInOrder and Sequence nodes: Slow Update Rate in the Behavior Graph

Thank you again for this help, it was very useful and because of this help our team decided to not leave the Behavior Graph (because you literally provided us a solution on a major bug in the Behavior Graph before the official fix).

But unfortunately, I spotted a bug of the same kind with Subgraph running. Below I will present you an example. The example is not real, it’s just to explain the issue.
Here are the details:

Let’s imagine that we have a Graph with the next sequence:

When we run the “ProcessIdle” Subgraph, we know what we are doing, we are ready for this Subgraph to take responsibility for the flow before the IsIdle variable in the Animator becomes false.

Now, let’s see what happens in the ProcessIdle Subgraph:

As you can see, a complex graph is present here. It’s not just a sequence, it’s a composition of sequences, conditions, modifiers, and actions. From the top to the bottom they work perfectly, they do exactly what I need.

But the problem arises when we come to the very bottom point - the Success node (it’s just an Action that returns Status.Success in the OnStart()).
What would you expect from the Graph when we reached the Success node? I would expect it to return Success → Exit the Subgraph → Set the IsIdle of the Animator to false in the Root Graph.

But unfortunately, this is not what happens, at least, not within 1 frame. I deeply debugged the class BehaviorGraphModule and noticed one interesting thing: the more complex your Subgraph, the longer you need to wait to exit it. By more complex I mean a lot of Sequences, Modifiers, and Conditions go one after another and this process repeats. And when I pause the editor, and see that I reached the Success node, but it still takes about 6 frames to exit the Subgraph, I have a desync in the logic and animations. The animation must already be non-Idle, but IsIdle is still true because it takes 6 frames to exit the Subgraph despite the fact that its work is already done.

This happens because when the flow is finished in the Subgraph, it will go backwards from the Success node to the OnStart node to exit the Subgraph. I am not sure why this happens, why we need to approve all the previous nodes because if we already reached the Success node at the bottom, that means all the nodes above already returned Success. Maybe it’s needed to clear arrays in the BehaviorGraphModule class instance, but I think it’s possible to somehow refactor the code to make it not be dependent on the complexity of the Graph.

But debugging of the BehaviorGraphModule.cs shows that when we return the Success node, we return Status.Success → we skip the frame and go to the previous node - Modifier “Wait For Any” - returned Success → we skip the frame and go to the previous node - Sequence with conditional guard - returned Success → we skip the frame and go to the previous node - Modifier “Repeat Until Success” - returned Success → And so on.

Dear Unity Developers, thank you that you helped me previously, but while you are fixing this bug (and I am not sure it’s fast), could you please suggest me a workaround?
Maybe it’s possible somehow to instantly exit the Subgraph and clear the m_RunningNodes and ProcessedNodes?
Maybe in the Tick() method of the BehaviorGraphModule.cs we can modify a bit the part with the “if (status != Status.Running) {…}” block? Right now we end the current node, call the “node.AwakeParents()” and then just wait for the next frame. Maybe we could keep the going backward within the same frame instead of just calling “node.AwakeParents()”?

Please help me with the workaround if you can.

Platform: Windows 10
Behavior Version: 1.0.7
Unity Version: 6000.0.25f1

Thank you very much!

Hello,

Thanks for reporting the issue, a lot of users have also reported the deferred execution being an issue for them and @ShaneeNishry have been hard at work to get rid of it. She posted the following last week in that regards:

It is not going to land for 1.0.8 as it is an important change and we want to allocate more time testing the change to not introduce new regression.

@ShaneeNishry do you have a temporary workaround?

1 Like

Thank you for the reply @MorganHoarauUnity !

What do you think about this solution:

public void Tick()
{
    bool nodeChangesLocal = m_NodesChanged;

	bool keepProcessing;

	// Keep iterating until no more nodes change status.
	do
	{
		// Rebuild list of running nodes once if needed
		if (nodeChangesLocal)
		{
			RebuildNodeLists();
			nodeChangesLocal = false;
		}

		keepProcessing = false;

		// Update every currently running node
		for (int i = 0; i < m_RunningNodes.Count; i++)
		{
			Node node = m_RunningNodes[i];
			// If this node is no longer in ProcessedNodes (was ended), skip it
			if (!ProcessedNodes.Contains(node))
            {
				continue;
			}

			Status status = node.Update();

			// If this node has finished, end it and awaken its parents
			if (status != Status.Running)
			{
				node.SetCurrentStatus(status);
				if (status is Status.Success or Status.Failure)
				{
					nodeChangesLocal = true;
					EndNode(node);
					node.AwakeParents();
					keepProcessing = true;
				}
			}
		}

		// If we changed any node�s status, we�ll likely need to recalc the running list
		if (nodeChangesLocal)
		{
			OnGraphStatusChange?.Invoke(this);
			keepProcessing = true;
		}

	} while (keepProcessing);
}

Will it break something or seems ok?

Thank you!

Hi @unity_30D89FB79A72844A3914 ,

I’m afraid your solution could cause some issues with regard to the timing of resumed sequences and if you ever have a repeat that could cause an infinite loop.

If you’re willing, I can send you my local changes to try out. That’ll also be useful in case I missed anything and can get early feedback before we release it.

1 Like

Hello @ShaneeNishry ! Thank you for the reply.

Actually, I had an infinite loop in my solution until I decided to use “bool nodeChangesLocal = m_NodesChanged;” within the current scope instead of pure “m_NodesChanged”. After that change, it seems like it works fine for me.
About “could cause some issues with regard to the timing of resumed sequences” - not sure I had this. Could you please provide an example of this case (not a project example, just tell with words) so that I can test it?

“If you’re willing, I can send you my local changes to try out.” - It would be very cool! Could you please send them? :slight_smile:

Thank you!

The infinite loop could happen if you have a repeat node with all its child nodes only take a single frame to pass, so it’s quite unlikely in a real game scenario, but doable.

Here are files related to my change you’ll need to try. Note it also adds a checkbox for Start and Repeat nodes to allow executing their repeats on the same tick, which should generally be safe to use if you don’t have the scenario mentioned before, but we decided to add it in case someone has it and upgrades.

If the graph goes into an infinite loop it’ll break and throw an exception, so you don’t need to worry about Unity hanging/crashing :slight_smile:

It’s quite a few because of the UI changes. Please send any feedback! :slight_smile:

(This is a new file, add it at Runtime\Execution\Nodes]
IRepeater.cs (165 Bytes)

[Runtime\Execution\Nodes\Composites]
SelectorComposite.cs (1.7 KB)
SequenceComposite.cs (1.7 KB)

[Runtime\Execution\Nodes\Decorators]
Start.cs (2.5 KB)
RepeaterModifier.cs (2.5 KB)
RepeatUntilModifierModifier.cs (5.2 KB)
RepeatWhileCondition.cs (3.7 KB)

[Runtime/Execution]
BehaviorGraphModule.cs (17.0 KB)

[Authoring/Asset/Editor/Transformers]
RepeatNodeTransformer.cs (1.0 KB)
StartNodeTransformer.cs (785 Bytes)

[Authoring/Asset/Models]
RepeatNodeModel.cs (3.1 KB)
StartNodeModel.cs (1.3 KB)

[Authoring/Inspector]
BehaviorGraphNodeInspectorUI.cs (10.7 KB)
RepeatNodeInspectorUI.cs (5.1 KB)
StartNodeInspectorUI.cs (2.2 KB)

1 Like

Thank you very much @ShaneeNishry !

I will try your files this week.

Thank you again!

1 Like

Hello again, @ShaneeNishry !

First of all, thank you for waiting for my feedback, and thank you that you provided the code. I understand that the code you provided is still in the process of development and can contain some issues, so, please don’t take the text below personally. Also, maybe there are no issues and I just somehow incorrectly used your changes (please tell me if it’s so).

To the issues:

With the code you provided, I noticed that synchronization issues occurred with the project I am working on. My bosses know that I am talking with Unity developers and they asked me not to share info from the project I am working on, including screenshots, so, I am very sorry, I can talk only with abstract words and notions.
Also, the graphs I have in the project are too complicated to create a separate example for this conversation and they still can expose some info about the project itself.
Again, I am very sorry for the inconvenience, but I hope you understand.

Issue 1: Selectors and Sequences
I noticed that your code is a bit different from what you have sent me in this conversation: Slow Update Rate in the Behavior Graph

Instead of this code:

protected Status StartChildNodesUntilCompleteOrWaiting(int childIndex)
{
	m_CurrentChild = childIndex;
	while (true)
	{
		if (m_CurrentChild >= Children.Count)
		{
			return Status.Success;
		}

		var status = StartNode(Children[m_CurrentChild]);
		if (status == Status.Success)
		{
			// Move on to the next child.
			++m_CurrentChild;
			continue;
		}
		if (status == Status.Running)
		{
			return Status.Waiting;
		}
		return status;
	}
}

You have this code:

protected Status StartChild(int childIndex)
{
    if (m_CurrentChild >= Children.Count)
    {
        return Status.Success;
    }
    var childStatus = StartNode(Children[childIndex]);
    return childStatus switch
    {
        Status.Success => childIndex + 1 >= Children.Count ? Status.Success : Status.Running,
        Status.Running => Status.Waiting,
        _ => childStatus
    };
}

As you can see, in your new implementation (StartChild()) the maximum of child nodes you can go through within one frame is 2 (one called in the OnStart() and another one in the OnUpdate()).

But what if I have more than 2 children in Selector or Sequence? Does that mean I have to wait until the next frame? If yes, that means it will cause a desync in the nodes’ logic.

Please correct me if I am wrong about your new StartChild() method. But I can certainly tell you that desync issues occurred in my project when I applied your code (problems occurred in logic flows and character animations).

Issue 2: BehaviorGraphModule
I debugged the changes you made to BehaviorGraphModule and it seems like it works mostly like the changes in my messages above (subgraphs are instantaneously finished, which is good). But for some reason, your changes broke flows with RestartIf at the root of the branch in my Graphs.
I still don’t understand the whole problem that I faced, so, the explanation will be somewhat abstract.

The explanation: I have a restart bool in one of the Subgraphs: I need this bool in order to restart the topmost branch if I want to repeat the same flow. Initially, this bool is false and I set it to true when a flow is finished. It is something like “I finished chasing my prey, but at the end of the flow I noticed that the prey is still in the line of sight and I have to repeat the chasing flow again”. And when I applied your changes to the BehaviorGraphModule.cs, I noticed that the RestartIf modifier didn’t work despite the fact that the bool is true. It is only a supposition, but maybe somewhere in the code (BehaviorGraphModule.cs), the nodes under the RestartIf are blocked by the containers you used (m_NodesToEnd, m_NodesToTick, m_ActiveNodes, m_ActiveNodes).

I am sorry for such an abstract description of the problem, but I had no time to deeply debug why it happens.
But what I can recommend in order to simulate my cases locally is to use the approach with States that we were talking about here: OnExit Node in State Machines, and each state are just like common Behavior Trees. Like we have states: Idle, Patrol, Chase, and they are running in parallel in the main graph, and when we set state to Chase, we switch to the Chase subgraph (state changed with help of throwing event in the root graph and listener in subgraphs will react on it) and execute it until it finishes its flow or the flow resets the branch, or the higher priority state interrupts this flow. And in each state, I have a lot of Sequences and Selectors with a lot of children and conditions. Also, you can manipulate with Animator properties in order to track characters visually. Sorry again that I cannot show you my graph, but this is all I can tell you to reproduce the issues I found.

There is a chance that I just don’t understand your changes and may be wrong about all this or just incorrectly used your changes. Please tell me what do you think.

Thank you!

Hey @unity_30D89FB79A72844A3914 ,

If you take a look at the graph module, awaken nodes will now Update again on the same frame. You do need to toggle the option I mentioned in the Repeat / Start nodes. Did you check that? :slight_smile:

I suspect this is the issue for both of your cases.

1 Like

1 Like

@ShaneeNishry , thank you for such a quick reply!

Ok, let’s try to discuss this new Repeat feature. Seems like I indeed underestimated this change in your files.

Let me first explain where I use the Repeat modifiers in my project.

I use Repeat modifiers for the following things:

  1. For conditions checking: I add a RepeatUntilSuccess → ConditionGuard → thing that I need to execute if passed the Condition Guard. In this case I don’t really need everything to be within one tick or instantaneous. I am ok that it will be split into several ticks.
  2. In the Root Graph where I tell what state must be currently set. In this case, I need the Repeat modifier to act quickly, and not split several conditions into several frames.

So, for case 1 I don’t need AllowMultipleRepeatsPerTick.

For case 2 I suppose I need it (if a player is in line of sight, I want the enemy to react to it quickly and switch to the Chase state).
I just set it to true and got the exception you were talking about several messages ago.

Here is how my OnStart(Repeatable) looks like:

[OnStart_Repeatable]
[Selector: Root]
├─ [Sequence: Death]
│
├─ [Selector]
│ ├── [Sequence: Patrol]
│ └── [Sequence: Chase]
│
└─ [Sequence: Idle]

These sequences are not complex, they are just made of Conditions, Set Variable Value (to set current state) and Send Event nodes.

Why it happened? Because it took only 1 frame to process the branch?
Could you please help me? What should I do in this case?

Thank you again!

Yes exactly. If it takes a single frame to finish the branch, you can get an infinite loop. This means you’re not doing any work in it and you should either keep AllowMultipleRepeatsPerTick off and let it repeat the next frame instead, or add a Wait (Frames) action (which I didn’t send to you, but I’ll add it here) somewhere where you might expect it to occur.

Are you expecting your graph to finish in a single frame each time? Do you need it to repeat instantly?

1 Like
using System;
using Unity.Behavior;
using UnityEngine;
using Action = Unity.Behavior.Action;
using Unity.Properties;

[Serializable, GeneratePropertyBag]
[NodeDescription(
    name: "Wait (Frames)",
    description: "Waits for a specified number of frames.",
    story: "Wait for [NumFrames] Frames",
    category: "Action",
    id: "2292810d494a4c941a71cbe358e46920")]
internal partial class WaitFramesAction : Action
{
    [Tooltip("The number of frames to wait.")]
    [SerializeReference] public BlackboardVariable<int> NumFrames = new(1);
    private int m_FrameTarget;
    [CreateProperty] private int m_FramesRemaining;

    protected override Status OnStart()
    {
        int currentFrame = Time.frameCount; 
        m_FrameTarget = currentFrame + Mathf.Max(0, NumFrames.Value);
        if (currentFrame >= m_FrameTarget)
        {
            return Status.Success;
        }
        return Status.Running;
    }

    protected override Status OnUpdate()
    {
        int currentFrame = Time.frameCount;
        if (currentFrame >= m_FrameTarget)
        {
            return Status.Success;
        }
        return Status.Running;
    }
    
    protected override void OnSerialize()
    {
        m_FramesRemaining = m_FrameTarget - Time.frameCount;
    }

    protected override void OnDeserialize()
    {
        m_FrameTarget = Time.frameCount + m_FramesRemaining;
    }
}

Thank you for the reply @ShaneeNishry !

If it takes a single frame to finish the branch, you can get an infinite loop

Hm, this sounds not too effective in terms of synchronization. I thought it’s totally ok to have a graph that executes so fast.

Let’s discuss this example:
Imagine that my enemy is chasing a player and when the player starts running faster, the enemy must also run faster. So, at some point, my graph has entered a frame and checks Conditional Guards. Conditional Guards are passed, the player is indeed faster, then the enemy must be too, good, the next step is just under the conditional guard - set of the state to ChaseFast. It’s here, just under the Conditional Guard, but Behavior Tree will not do it right now, it will do it in the next frame causing desync in the game.

What do you think?

I’m not blaming anyone right now I just would like to understand how everything works and why you used this approach. I just want to show the exact case of why I would have problems in the game with this approach.

Thank you again!

Hey @unity_30D89FB79A72844A3914, let’s figure this out. I hope my questions below help us figure out what’s going on, I feel like maybe there is a misunderstanding about what’s happening here :slight_smile:

In my experience games always have to have a check at some reasonable point in time. Usually if you have this on a script, every frame you check one’s velocity and adjust accordingly, this is also true in Physics and it’s why things are an approximation. A good talk about the subject is here: https://www.youtube.com/watch?v=yGhfUcPjXuE

Now you’re talking about the case where you have one object and another trying to follow it. Why do you need to check its velocity multiple times in the same frame? If you have had your check return false, why does the repeat need to happen again in the very same frame and not the next one?

Maybe I’m missing something, but I’m wondering if there is an incorrect use case here.

I’d like to ask you: How would you handle the case of a potential infinite loop when you have a Repeat and a condition that is expected to return false each time in the frame? When do you expect the condition to change?

Thank you for the reply @ShaneeNishry !

Why do you need to check its velocity multiple times in the same frame? If you have had your check return false, why does the repeat need to happen again in the very same frame and not the next one?

I’d like to ask you: How would you handle the case of a potential infinite loop when you have a Repeat and a condition that is expected to return false each time in the frame? When do you expect the condition to change?

I am sorry, I suppose this is just a misunderstanding. I don’t need to check its velocity multiple times in the same frame (or I would have an infinite loop). I would like a branch of my Graph, if it has no “async” nodes, to be fully executed from start to end in the frame it was called.

Like this simple branch: Repeat → TryInOrder → Sequence → IsPlayerRunning → SetState “ChaseFast” → and maybe call a couple of other simple sync Actions. That’s it. If the IsPlayerRunning returns false - I am totally ok that it will happen on the next frame. But from your description of the Repeat in the previous messages, I understood that sometimes (if AllowMultipleRepeatsPerTick is disabled), even if IsPlayerRunning is equal to true, we can call SetState “ChaseFast” only on the next frame.

Thank you!

Everything under the repeat should run on the same frame. AllowMultipleRepeatsPerTick == false will only mean that after everything under the repeat finished running, the next iteration will for the repeat will wait for the next frame. I hope that makes sense? In your case, SetState should happen immediately.

Does it mean something still isn’t working for you?

Thank you for the reply!

Repeat is not a problem here, I think. Everything that happens under Repeat is defined also by Modifiers. And as I can see, the SequenceComposite and SelectorComposite will not process all their children within the same frame, because it looks like this:

internal partial class SequenceComposite : Composite
{
	[CreateProperty] int m_CurrentChild;

	/// <inheritdoc cref="OnStart" />
	protected override Status OnStart()
	{
		m_CurrentChild = 0;
		return StartChild(m_CurrentChild);
	}

	/// <inheritdoc cref="OnUpdate" />
	protected override Status OnUpdate()
	{
		var currentChild = Children[m_CurrentChild];
		Status childStatus = currentChild.CurrentStatus;
		if (childStatus == Status.Success)
		{
			return StartChild(++m_CurrentChild);
		}
		return childStatus == Status.Running ? Status.Waiting : childStatus;
	}

	protected Status StartChild(int childIndex)
	{
		if (m_CurrentChild >= Children.Count)
		{
			return Status.Success;
		}
		var childStatus = StartNode(Children[childIndex]);
		return childStatus switch
		{
			Status.Success => childIndex + 1 >= Children.Count ? Status.Success : Status.Running,
			Status.Running => Status.Waiting,
			_ => childStatus
		};
	}
}

I don’t see here that a third child will be processed. Maybe you can explain in detail why all children of the Sequence will execute in the same frame when we have this code?

Thank you!

Ahh sorry I misunderstood you. Yes, for sure. The changes to the BehaviorGraphModule now use m_NodesToTick in the loop. Also AwakeNode was changed to the following:

        internal void AwakeNode(Node node)
        {
            if (m_NodesToEnd.Contains(node)
                || m_NodesToTick.Contains(node)
                || !m_ActiveNodes.Contains(node)
                || node.CurrentStatus is not (Status.Waiting or Status.Running))
            {
                return;
            }
            m_NodesToTick.Insert(0, node);
            node.SetCurrentStatus(Status.Running);
            m_NodesChanged = true;
        }

This means that even if the child will finish in StartNode while the Sequence is still running, the Sequence will be inserted straight back to the start of the queue and will be ran again immediately after it finished its first iteration.

Thank you for the reply!

So, from your description, as I understand, everything must normally end in a Sequence or Selector within one frame if their children (and children of their children) are sync.

Ok, seems like at some point I have to deeply debug what is happening in the BehaviorGraphModule.cs because for me it doesn’t work as expected. But not sure I can debug today or tomorrow.

Anyway, thank you for your time!
Below I would like to provide you the code that I used before you provided me your files. It’s mostly the code you provided me in other discussions. And it worked well, everything was running one after another. Maybe this is the reason your current code is not working for me - because I exploited the bad code I pasted into the package earlier. If it’s so - I’ll have to remake everything in my Graphs.

Here are what was changed in my package:

BehaviorGraphModule.cs

public void Tick()
{
	bool nodeChangesLocal = m_NodesChanged;

	bool keepProcessing;

	// Keep iterating until no more nodes change status.
	do
	{
		// Rebuild list of running nodes once if needed
		if (nodeChangesLocal)
		{
			RebuildNodeLists();
			nodeChangesLocal = false;
		}

		keepProcessing = false;

		// Update every currently running node
		for (int i = 0; i < m_RunningNodes.Count; i++)
		{
			Node node = m_RunningNodes[i];
			// If this node is no longer in ProcessedNodes (was ended), skip it
			if (!ProcessedNodes.Contains(node))
			{
				continue;
			}

			Status status = node.Update();

			// If this node has finished, end it and awaken its parents
			if (status != Status.Running)
			{
				node.SetCurrentStatus(status);
				if (status is Status.Success or Status.Failure)
				{
					nodeChangesLocal = true;
					EndNode(node);
					node.AwakeParents();
					keepProcessing = true;
				}
			}
		}

		// If we changed any node�s status, we�ll likely need to recalc the running list
		if (nodeChangesLocal)
		{
			OnGraphStatusChange?.Invoke(this);
			keepProcessing = true;
		}

	} while (keepProcessing);
}

SelectorComposite.cs

internal partial class SelectorComposite : Composite
{
	[CreateProperty] private int m_CurrentChild;

	protected override Status OnStart()
	{
		m_CurrentChild = 0;
		return StartChildNodesUntilCompleteOrWaiting(m_CurrentChild);
	}

	protected override Status OnUpdate()
	{
		var currentChild = Children[m_CurrentChild];
		Status childStatus = currentChild.CurrentStatus;
		if (childStatus == Status.Failure)
		{
			return StartChildNodesUntilCompleteOrWaiting(++m_CurrentChild);
		}
		if (childStatus == Status.Running)
		{
			return Status.Waiting;
		}
		return childStatus;
	}

	protected Status StartChildNodesUntilCompleteOrWaiting(int childIndex)
	{
		m_CurrentChild = childIndex;
		while (true)
		{
			if (m_CurrentChild >= Children.Count)
			{
				return Status.Failure;
			}

			var status = StartNode(Children[m_CurrentChild]);
			if (status == Status.Failure)
			{
				// Move on to the next child.
				++m_CurrentChild;
				continue;
			}

			if (status == Status.Running)
			{
				return Status.Waiting;
			}

			return status;
		}
	}
}

SequenceComposite.cs

internal partial class SequenceComposite : Composite
{
	[CreateProperty] int m_CurrentChild;

	/// <inheritdoc cref="OnStart" />
	protected override Status OnStart()
	{
		m_CurrentChild = 0;
		return StartChildNodesUntilCompleteOrWaiting(m_CurrentChild);
	}

	/// <inheritdoc cref="OnUpdate" />
	protected override Status OnUpdate()
	{
		var currentChild = Children[m_CurrentChild];
		Status childStatus = currentChild.CurrentStatus;
		if (childStatus == Status.Success)
		{
			return StartChildNodesUntilCompleteOrWaiting(++m_CurrentChild);
		}
		if (childStatus == Status.Running)
		{
			return Status.Waiting;
		}
		return childStatus;
	}

	protected Status StartChildNodesUntilCompleteOrWaiting(int childIndex)
	{
		m_CurrentChild = childIndex;
		while (true)
		{
			if (m_CurrentChild >= Children.Count)
			{
				return Status.Success;
			}
			
			var status = StartNode(Children[m_CurrentChild]);
			if (status == Status.Success)
			{
				// Move on to the next child.
				++m_CurrentChild;
				continue;
			}
			if (status == Status.Running)
			{
				return Status.Waiting;
			}
			return status;
		}
	}
}

Thank you very much! I hope soon I’ll find out what is wrong with my project.