Coroutine confuse

Hi
Let's say I have 2 classes, TileManager and Pathfinding.
Inside TileManager i have a list :

Code (CSharp):

1. List<short> TilesInPath = new List<short>();

and a void:

Code (CSharp):

1. public void FindPath(short start, short end){
2.     TilesInPath.Clear();
3.     TilesInPath = new PathFinding().FindPath(start, end);
4. }

Inside Pathfinding class I have:

Code (CSharp):

1. List<short> ListToSend = new List<short>();
2. public List<short> FindPath(short start, short end){
3.     //make some temporary lists
4.     //check parents etc
5.     // in general - some heavy stuff goes here
6.     CreateList();
7.     return ListToSend;
8. }
9.  
10. private void CreateList(){
11.     //fill the ListToSend - hevy stuff goes here as well
12. }

NOW... question is... how to put this whole process into coroutine with yield return null ?
Is enough to make a void FindPath inside TileManager as ienumerator?

Code (CSharp):

1. public IEnumerator FindPath(short start, short end){
2.     TilesInPath.Clear();
3.     TilesInPath = new Pathfinding().FindPath(start, end);
4.     while(TilesInPath.count ==0){
5.         yield return null;
6.     }
7. }

Or it should be done other way ?

So in general: how to coroutine something like this with yield return null

Code (CSharp):

1. void SomeVoid(){
2.     list = HardCalculations_1();
3. }
4.  
5. list HardCalculations_1(){
6.     //hard stuff here
7.     return HardCalculations_2();
8. }
9.  
10. list HardCalculations_2(){
11.     //hard stuff here
12. }

 

If I were doing it , I would make it so my two heavy load methods are IEnumerators. If they both go over a list of data, I would probably do every 'n number of loops' per yield. You can also yield on the CreateList as a coroutine inside FindPath so that it waits for CreateList to be done. I'm not sure if there's a better way, but I'm fairly confident what I described would work.

 

Coroutines are not threads so if you do a lot of work in between yields there is no benefit. As it will all be done on the same main thread in the same frame.

Coroutines are there to really just help you write code that executes across multiple frames.

I would do what methos5k said. In your heavy work break it up with yields where it's appropriate and wait until the next frame to continue the work.

If it has nothing to do with the unity API then you could look at actually using a real thread.

 

Yes, i know that coroutines are still in the same main thread but doing stuff in background not freezig it.
I solve my problem using Async/Await like this:

Code (CSharp):

1.  
2.     List<int> TilesInPath = new List<int>();
3.  
4.     public void FindPath() {
5.         new PathFinding().PathFind(TilesInPath);
6.     }
7.  
8.     //in other PathFinding class
9.     public async void PathFind(List<int> _list) {
10.         finding = true;
11.         await Task.Run(() => {
12.             //do some heavy stuff here
13.             CallAnotherHeavyVoid(_list);
14.         });
15.         finding = false;
16.     }

Nice and easy and all PathFind void is running as separate thread
I've added global bool 'finding' to avoid clicking on tiles while finding a path is still in progress.
What you think about this approach?

 

Async/await works as long as you're using the "experimental" 4.6 runtime. Are you comfortable with using an experimental runtime in production code?

 

In a world of bugs, fixes, golded status beta versions and dayOne patches ? SURE
But seriously... until I will finish this project, this will be already in "stable" status i suppose
Question is, is this approach any good from coding point of view.

 

Well... firstly you'd want to make sure the list is thread safe. I see a potential issue here:

Code (csharp):

1.  
2.     List<int> TilesInPath = new List<int>();
3.  
4.     public void FindPath() {
5.         new PathFinding().PathFind(TilesInPath);
6.     }
7.  

FindPath() is basically fire and forget, so you'd need a way to make sure TilesInPath is safe to read from, so you need some way to know when the task is completed and the list is available for reading.

In fact, you can avoid that issue altogether by getting a task object, and waiting for that task to complete before extracting the list from it.

I've created a C# console application to demonstrate this. It should be pretty easy to adapt to your Unity project (just don't use Thread.Sleep, it's only there for illustration purposes )

Code (csharp):

1.  
2.     class Program
3.    {
4.        static void Main(string[] args)
5.        {
6.            Console.WriteLine("Generating path...");
7.  
8.            var task = FindPath();  // get FindPath task
9.  
10.            while (!task.IsCompleted) // wait for task to finish
11.            {
12.                Console.Write(".");  // this just shows the UI is still responsive
13.                System.Threading.Thread.Sleep(100); // Don't use this in Unity!  :D
14.            }
15.  
16.            Console.WriteLine();
17.  
18.            List<int> list = task.Result; // get list result from task
19.  
20.            Console.WriteLine("List generated! Count = " + list.Count);
21.  
22.            Console.WriteLine("Hit enter to exit.");
23.            Console.ReadLine();
24.        }
25.  
26.        public static async Task<List<int>> FindPath()
27.        {
28.            List<int> list = await Task.Run(LongRunningTaskInBackground);
29.            return list;
30.        }
31.  
32.        private static async Task<List<int>> LongRunningTaskInBackground()
33.        {
34.            await Task.Delay(5000);  // pretend this task took 5 seconds to finish
35.            return new List<int> { 0, 1, 2, 3, 4, 5, 6 };
36.        }
37.    }
38.  

 

Great one but....
when converted to Unity (Consoles changed to debug) and sleep converted to some real struff to count...
scene (empty test one) is freezing when load.

Second... another issue i found is to implement canceling the token
when mouse exit tile so we can cancel current async task and start new one later when mouse enter new tile
Or just on application/scene quit

 

Ok, my last version looks like this:

Code (CSharp):

1. private List<int> MyPath = new List<int>();
2. private CancellationTokenSource source;
3. public bool StopSearching { get; set; } //changed to true on app/scnene/tile exit
4.  
5. puplic void FindPath(){
6.         try{
7.             Debug.Log ("calculating path start");
8.             source = new CancellationTokenSource();
9.             StopSearching = false;
10.             var task = PathFinding.FindPath(source.Token);
11.             while (!task.IsCompleted) {
12.                     if(StopSearching)source.Cancel();
13.             }
14.             MyPath = task.Result;
15.             Debug.Log ("calculating path end");
16.         }
17.         catch (AggregateException) { Debug.Log ("calculating path canceled"); }
18.         finally{source.Dispose();}
19. }

and in second class:

Code (CSharp):

1. public static async Task<List<int>> FindPath(CancellationToken token)
2. {
3.        List<int> ToSend = new List<int>();
4.        await Task.Run(()=>{
5.             //to generate ToSend list
6.             DoHardStuff_1();
7.             DoHardStuff_2_After_1();
8.         }, token);
9.         return ToSend;
10. }

But... is not working mean not canceling. Probably problem is here:

Code (CSharp):

1. await Task.Run(()=>

so all voids inside don't care about the cancel - but not sure.

 

Yeah... I'm not entirely happy how they handled cancelling tasks. It's one API design I really wish they'd have done better.

The cancellation token is basically a glorified bool you can pass around to check to see if a cancel request was made. So if you want your tasks to be cancellable, then you'll need to add support for that by scattering token.ThrowIfCancellationRequested throughout your code. Do this whereever it's safe to bail out, like at the beginning of a for loop block, etc.

Do you have this in your DoHardStuff_1() and DoHardStuff_2_After_1() code?

 

The FindPath method here is critical and if you call this from Unity, it's just gonna be as fast as a main-thread approach, with the same consequences. It's also got some logic which doesn't make sense if you think about it.

Here's why: You start the async task to find the path (PathFinding.FindPath(...)), which returns a task. As long as this has not finished, you let Unity wait for it in a while loop ... EITHER until the task actually finished, so it's kinda like a synchronous version in the end, OR some thread (cannot be the main thread, unless the task happens to be executed on it) causes StopSearching to be set to true... which (in the end) is like a synchronous approach again.

You could re-introduce a coroutine here or simply await the thread where you actually need it, but this is current implementation wouldn't work as intended (it might appear to work if the task happens to finish quickly enough).

I'm currently not sure where you want to cancel the task from (using the CancellationToken), but the signal cannot come from Unity.

 

Yep, as @Suddoha noted, this will block your thread. It's better to do this as an if() statement in either your Update() function, or do this in a coroutine with yield return null in the while loop.

 

When checked in Unity this async/await approatch looks like is running in a parallel with main thread
Or... just leave those ideas with coroutines, await/async etc and go for pure

Code (CSharp):

1. new Thread(()=> PathFInd());
2. myThread.Start();
3.  
4.     private void PathFind() {
5.         pf = new PathFinding();
6.         PathLIst = pf.FindPath();
7.     }
8.  

But it's really annoying when such an easy thing like run some simple process in parallel is so hard to make.
Coroutines are one big mess, with tasks is a problem with canceling, and finally you have to do this with pure threading what is quite dangerous but way more easy to make.

 

It's not difficult to achieve that, given that you find a properly designed approach for it. The more complex it is, the more time it might take - that's just natural and common in software engineering. Of course, you could abstract this a little more and write some generic and re-usable piece of code that you can easily use in your application again and again.

The issue with your last solution (the threaded one) may suffer from various problems, as it has already been stated by @BlackPete in one of his previous posts. If you wanted to implement this correctly, you'll need to prevent other parts of your code from touching the list during the processing of the path finding.

You can actually do that in a non-heavy and non-locked manner.
You'd always start out with the path list being null, the main thread needs to set to null either when it's grabbed the reference and stored it somewhere else or when it starts the a new path-finding routine. During the path-finding, you'll work on local variables and once you've completed the list, you assign it to the actual variable that you want to access from the main thread.

The other thread, Unity's main-thread in this case, would null-check the list before any access. The result of the evaluation of the null-check at the same time represents whether the pathfinding has completed or not - that's kinda your implicit "isDone" flag.

That would work, but it's the most basic approach and quite intuitive and probably a rather dirty way, it's a detail to always remember.
Like mentioned above, I'd wrap this is a more comprehensible API, because the null-check does not explicitly say why you're doing that, so you either need to remember it all the time or add a descriptive comment.

Talking about a wrapper for it, this is exactly the beauty of async/await and custom yield instructions for coroutines, because you can simply "wait" for something to complete... you wouldn't clutter the code with all the polling mechanisms.

 

If you're specifically talking about threaded pathfinding what we do is have a singleton manager that we pass path requests to. Part of that request is the entity state that requested the path. We have a flag on that state saying whether it's still active. This becomes false when the entity leaves that state. In the path thread we check that flag and if it's false we skip processing that request. The only time you do unnecessary processing is if the state becomes invalid as the request is being processed but honestly this isn't a huge deal.

 

Alright, I sat down and created a full working Unity application, which I've attached to this post.

This demonstrates the following:
1) Using async/await to generate a new randomized path
2) Have an object follow a path while a new path is being generated in the background
3) Demonstrate cancelling a path finding request using a CancellationToken

I've tried to keep it very simple, and only use a couple of Debug.Log to show what's happening, but if you have any questions, feel free to ask.

The important thing to note is FindPath() is very much a "fire and forget" operation, so the object will still continue to travel on its old path until a new path has been found.

For completeness's sake, I'm also including the script here:

Code (csharp):

1.  
2. using System;
3. using System.Collections.Generic;
4. using System.Diagnostics;
5. using System.Threading;
6. using System.Threading.Tasks;
7. using UnityEngine;
8. using UnityEngine.UI;
9. using Debug = UnityEngine.Debug;
10.  
11. public class AwaitExample : MonoBehaviour
12. {
13.    public float PathFindingTime = 5f;
14.  
15.    private List<Vector3> _waypoints;
16.    private int _currentWaypoint;
17.  
18.    public Button ButtonFindPath;
19.    public Button ButtonCancel;
20.  
21.    private CancellationTokenSource cts;
22.    System.Random randomGenerator = new System.Random();
23.  
24.  
25.    private void Awake()
26.    {
27.        Debug.Assert(ButtonFindPath != null && ButtonCancel != null);
28.  
29.        transform.position = Vector3.zero;
30.        _waypoints = new List<Vector3> { Vector3.zero, Vector3.right * 2f, Vector3.forward * 2f };
31.        _currentWaypoint = 0;
32.  
33.        ButtonFindPath.onClick.AddListener(FindPath);
34.        ButtonCancel.onClick.AddListener(() =>
35.        {
36.            Debug.Log("Cancelled!");
37.            cts.Cancel();
38.        });
39.  
40.        ButtonCancel.interactable = false;
41.    }
42.  
43.    private void Update()
44.    {
45.        var waypoint = _waypoints[_currentWaypoint];
46.  
47.        if (Vector3.Distance(transform.position, waypoint) < 0.1f)
48.            _currentWaypoint = (_currentWaypoint + 1) % _waypoints.Count;
49.        else
50.            transform.position = Vector3.Lerp(transform.position, waypoint, 0.1f);
51.    }
52.  
53.    private async void FindPath()
54.    {
55.        Debug.Log("Finding new path...");
56.  
57.        ButtonFindPath.interactable = false; // disable find path button until new path is found
58.        ButtonCancel.interactable = true;
59.  
60.        if (cts != null) cts.Dispose(); // get rid of old source if one exists
61.        cts = new CancellationTokenSource();
62.  
63.        try
64.        {
65.            var newList = await Task.Run(() => LongRunningBuildPathAsync(cts.Token), cts.Token);
66.  
67.            //We're back on the main thread, so it's safe to replace the list here.
68.            if (newList != null && newList.Count > 0)
69.            {
70.                Debug.Log("New path found!");
71.                _waypoints = newList;
72.                _currentWaypoint = 0;
73.            }
74.        }
75.        catch (Exception e)
76.        {
77.            Debug.Log(e.Message);
78.        }
79.  
80.        ButtonCancel.interactable = false;
81.        ButtonFindPath.interactable = true; // re-enable find path button
82.    }
83.  
84.    private async Task<List<Vector3>> LongRunningBuildPathAsync(CancellationToken token)
85.    {
86.        // Imitate a [PathFindingTime] second delay to find the "path".
87.        // Note that this is broken up into 100ms intervals to check if the task has been cancelled.
88.        Stopwatch timer = new Stopwatch();
89.        timer.Start();
90.  
91.        while (timer.Elapsed.TotalSeconds < PathFindingTime)
92.        {
93.            token.ThrowIfCancellationRequested();  // if cancel was requested, bail out now.
94.            await Task.Delay(100);
95.        }
96.  
97.        return new List<Vector3> { GetRandomPoint(), GetRandomPoint(), GetRandomPoint() };
98.    }
99.  
100.    private Vector3 GetRandomPoint()
101.    {
102.        return new Vector3(Random(-2f, 2f), 0, Random(-2f, 2f));
103.    }
104.  
105.    private float Random(float min, float max)
106.    {
107.        return Mathf.Lerp(min, max, randomGenerator.Next(100) / 100f);
108.    }
109.  
110.    private void OnDrawGizmos()
111.    {
112.        if (_waypoints != null)
113.        {
114.            foreach (var waypoint in _waypoints)
115.                Gizmos.DrawCube(waypoint, Vector3.one * 0.25f);
116.        }
117.    }
118. }
119.  

If you don't want to import the attached unitypackage (or maybe in a couple of years this thread gets necroed and the file format isn't compatible by then), you can easily recreate the project by creating a gameobject, and attach this script to it. You'll also need to create a couple of UI buttons and link to them in the inspector to find a path, and to cancel finding the path.

 

That's why I'm always voting for Unity over UE4 - because of the community!

List = task.Result was freezing main thread (just made some rotating box to check it) - so you were right

Now I'm a bit more confused because even without using async/await my new approach (which I made before this post above from BlackPete) seems to work - not blocking cube from rotating.
This how it looks now.

Code (CSharp):

1.     public void FindApath() {
2.         CancelOldSearch();
3.         cts = new CancellationTokenSource();
4.         MyTask = Task.Run(() => new PathFinding().getPath(cts.Token), cts.Token);
5.     }

Then in separate class:

Code (CSharp):

1.     List<int> ToSend = new List<int>();
2.     public void getPath(CancellationToken token) {
3.         buildFakeLists(token); //just 2 lists with 300 and 10 elements for testing
4.         Finding(token);
5.         if (token.IsCancellationRequested) ToSend.Clear();
6.         AsyncTest_3.Instance.SearchingEnded(ToSend);
7.     }
8.  
9.     private void Finding(CancellationToken token) {
10.         foreach (int i in FakeList_1) {
11.             foreach (int a in FakeList_2) {
12.                 if (token.IsCancellationRequested) break;
13.                 Debug.Log("searching... ");
14.                 if (!ToSend.Contains(i)) ToSend.Add(i);
15.             }
16.         }
17.     }

And back to main class

Code (CSharp):

1.     public void SearchingEnded(List<int> _path) {
2.         MyPath = new List<int>(_path); //my main List is not blocked during the search
3.         cts.Dispose(); //to avoid memory leaking
4.         Debug.Log("ended - "+ MyPath.Count); //canceled or notfound gives 0
5.     }

And canceling: (used also when pointer leaving area or goes over GUI)

Code (CSharp):

1.     public void CancelSearching() {
2.         if (MyTask == null || MyTask.IsCompleted) return;
3.         try { Debug.Log("canceling... "); cts.Cancel(); }
4.         catch (OperationCanceledException) { }
5.         finally { cts.Dispose(); }
6.         while (!MyTask.IsCompleted) { Debug.Log("sleeping..."); Thread.Sleep(1); }
7.     }

This sleeping is just to be sure that previous search is ended.
Most of the time it takes like 1ms. Greates number I get was 3ms (with debug.log) so not big deal i hope.

 

Glad you got it working!

A couple of comments/questions:

Code (csharp):

1.  
2. new PathFinding().getPath(cts.Token)
3.  

Why not just make that a static function instead of creating a pathfinding object instance to feed the garbage collector?

Also, a word of caution on using Thread.Sleep... this sleeps the main thread, which means Unity itself can't update until the task finishes. Could be bad for networking, could create noticeable stutters, etc...

 

Yes, it will be changed to new Pathfinding().
And yes, i know it freezes main thread but somehow task have to wait for cancel to be finished. But will change this and I will put this inside a task, so task will be waiting to complete cancelation of previous task if such exist, so all without blocking the main thread.
But in general... yeah, all this task/thread/coroutine stuff is pain in the ass