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

Hi @unity_30D89FB79A72844A3914 ,

I’m sorry something is still not working for you. Since you replaced things with my sequence/selector and other nodes, it should work, unless there were other changes you made? I did make a bunch of internal tests for running sequences and selectors and made sure they all run in a single frame, so I’m curious where’s the issue :thinking:

Please keep communicating so we can figure it out! Sorry about that.

Hello again @ShaneeNishry !

I found some time to understand what exactly was broken in your new code compared to the previous one!

And I finally found it! It’s RestartIf modifier. I am not saying that the problem is exactly in this modifier because you didn’t change it, but in some parent classes (parent in terms of composition).

Now, to the example that you can easily reproduce on your side:

This is the gif of how a simple RestartIf modifier worked in a pure 1.0.7 version:
PreviousRestartIf

And this is the gif of how it works with the changes that you provided in this post:
NewRestartIf

As you can see, the branch doesn’t restart even if the condition was satisfied. The branch just succeeds.

Could you please check it and try to fix it? I am sure this is a major bug.

Thank you very much!

Hi @unity_30D89FB79A72844A3914 ! Thank you so much for testing and getting back to me with this. I’ll investigate it asap and try and get you a file today. I’m sorry for the issue, this was something I missed.

1 Like

@MorganHoarauUnity ,

Sorry for the delay in this. I see what’s going on now.

Since the Restart If node checks the conditions on each update, setting the ShouldRestart to true now finishes the sequence and the Restart If returns success. We can easily make it check the conditions even if the sequence succeeded.

However I wonder if we should also ensure it can break mid frame if the conditions have changed to require a restart. This is a bit of an interesting edge case.

Let’s say you had the sequence as such:

  1. Restart if
  2. Set Should Restart to false
  3. Set Should Restart to true
  4. Log some message

As all of these should run on the same frame, would you expect the message to be logged or not?

Another interesting thought, if you set the value at the end in that example, it’s kinda like having a “Repeat While” instead of a Restart :thinking: So while the question of the callback vs update check of conditions is one thing, I’m also curious if it should restart at the end if conditions have met or not. To be fair, if we change it to use a callback it would regardless.

Hello @ShaneeNishry ! Thank you for the response!

Logically speaking I expect from this flow to not log the message on the end.
But I see that it’s nearly impossible.
In general, this is indeed quite a difficult problem, because you cannot just terminate a thread from execution (I mean, you can with some C++ low-level approaches, but you mustn’t do it in a game, especially in the main thread). If you use callbacks, then you have an infinite loop.

So, I personally would like to not see the log message you are talking about in this example, but I understand that it’s nearly impossible, so right now I am totally ok if the branch will not be interrupted in the same frame.

Yes, in some cases it can work as RepeatWhile, that’s true, but in a real-case scenario, I don’t have this RepeatWhile because in my game, my branch is designed to not be looped. Also, in my game, I have an OnExit node (in case something in the hierarchy of branches interrupts the current flow), where I set ShouldRestart to false. So, having this ShouldRestart variable implies that you cover edge cases that occur with it.

So, to conclude: having this unlogical situation when the log is displayed is ok for me because I know it’s hard to handle. I am more scared that the code you provided in this conversation if you want to keep it, will cause a lot of broken logic for Unity Behavior Graph users, because previously (1.0.7) we had RepeatWhile-like behavior, but with the new code the flow just succeeds and never repeats. So, I am afraid that the logic that I made in 1.0.7 will not be working in the new version.
But this is why we are discussing this right now (your code is not a release yet, you are in process of fixing it), thank you for sharing your thoughts with the community!

Thank you!

Thank you @unity_30D89FB79A72844A3914 for the great communication :slight_smile:

I’ll investigate restarting it on the callback. I expect the same as you said.

As for the Restart, you can try this replacement to see how it works. Also I recommend applying the changes I gave you to 1.0.8 as we’ve made many fixes there to improve stability :slight_smile:
RestartModifier.cs (2.6 KB)

I am more scared that the code you provided in this conversation if you want to keep it, will cause a lot of broken logic for Unity Behavior Graph users, because previously (1.0.7) we had RepeatWhile-like behavior, but with the new code the flow just succeeds and never repeats. So, I am afraid that the logic that I made in 1.0.7 will not be working in the new version.

Oh 100%, I’m already working on fixing the behavior! I’m with you. Thank you for finding this issue early!

1 Like