Search Unity

Why the hell is this an endless while loop?!?

Discussion in 'Scripting' started by mrliioadin, Dec 27, 2016.

  1. mrliioadin

    mrliioadin

    Joined:
    Jul 13, 2013
    Posts:
    21
    --EDIT-- Issue identified in the second reply. Solution posted below.

    -------Original post---------

    This seems like it should be a very straightforward while loop... but, I've definitely isolated that this is the set of code that's causing my crash. It is like it isn't reevaluating line 12 on each iteration. Also, Debug.Log commands don't run even on the first iteration if placed inside the while loop? Apologies for the confusion.

    If you need the context, the list is populated in one script at runtime, I basically need this method to wait until the list is populated in GeographyManager then grab it for this script to use. It only needs to do it once. If I take out the try{} command, it will crash when the list can't populate.

    Code (CSharp):
    1.     void loadtheloclist()
    2.     {
    3.         bool listempty = true;
    4.  
    5.         if (loclist.Any())
    6.         {
    7.             listempty = false;
    8.         }
    9.         else
    10.         {
    11.  
    12.             while (listempty)
    13.             {
    14.                 try
    15.                 {
    16.                     loclist = GameObject.Find("GeographyManager").GetComponent<GeographyManager>().localitieslist;
    17.                     if (loclist.Count > 0)
    18.                     {
    19.                         listempty = false;
    20.                     }
    21.                     else
    22.                     {
    23.                         listempty = true;
    24.                     }
    25.                 }
    26.                 catch
    27.                 {
    28.  
    29.                 }
    30.             }
    31.         }
    32.     }
    I've also tried replacing the try's with:

    Code (CSharp):
    1. if (GameObject.Find("GeographyManager").GetComponent<GeographyManager>().localitieslist.Count > 0)
    2.                 {
    3.                     loclist = GameObject.Find("GeographyManager").GetComponent<GeographyManager>().localitieslist;
    4.                     listempty = false;
    5.                 }
    Same result.

    -----------Solution------------

    Code (CSharp):
    1. IEnumerator loadloclist()
    2.     {
    3.         if (loclist.Any())
    4.         {
    5.             yield return null;
    6.         }
    7.         else
    8.         {
    9.             try {
    10.                 loclist = GameObject.Find("GeographyManager").GetComponent<GeographyManager>().localitieslist;
    11.                 Debug.Log("There are " + loclist.Count + " in the localities list");
    12.                 StopCoroutine(loadloclist());
    13.             }
    14.             catch
    15.             {
    16.  
    17.             }
    18.         }
    19.         yield return new WaitForSeconds(1);
    20.     }
     
    Last edited: Dec 27, 2016
  2. gorbit99

    gorbit99

    Joined:
    Jul 14, 2015
    Posts:
    1,175
    Don't use GameObject.Find(), either use GameObject.FindWithTag() or assign it to a public variable
     
  3. AndyGainey

    AndyGainey

    Joined:
    Dec 2, 2015
    Posts:
    216
    Where is loadtheloclist() called from? And where is the code that actually populates the list?

    Unless you're doing explicit multithreading, it looks like your problem is that loadtheloclist() is called first, and the code that populates the list won't ever get an opportunity to execute until loadtheloclist() finishes, which it never will because the list remains permanently empty. If they're both set to execute on the same thread, then it's impossible for them to run at the same time. Between one iteration of your loop and the next, nothing else happens that could possibly change the length of the list.

    If you are doing multithreading, then you have a serious lack of mutual exclusion to prevent race conditions, but that's another matter entirely.

    The easiest way to fix this is to use Unity's coroutines. To do so, 1) change loadtheloclist()'s signature to return System.Collections.IEnumerator instead of void. 2) Within the while loop, include a line "yield return null;" to cause it to pause execution of this function until the next frame, giving other code a chance to execute. 3) Wherever loadtheloclist() is called, change it to "StartCoroutine(loadtheloclist());".

    Also, I'd recommend moving the GameObject.Find() stuff out of the while loop, if it can be guaranteed that the game object and component can be found at the very beginning and never changes.

    As for Debug.Log() never showing up, realize that it is most certainly outputting a massive amount of info to the Unity console, which is incredibly slow when done in bulk, but because the function never finishes, Unity doesn't even have a chance to redraw the UI. Thus it appears at though Debug.Log() is doing nothing. In reality, Unity is hung (more accurate than saying it "crashed") specifically because all it is doing is Debug.Log() in a tight loop that never lets anything else execute..
     
    mrliioadin likes this.
  4. Laperen

    Laperen

    Joined:
    Feb 1, 2016
    Posts:
    822
    Since your whileloop depends on listempty being true, its more likely the problem lies with how you are defining loclist in the whileloop.

    And if you need this to be executed once, I don't really see why you need to do it the way you have. I'm assuming loadtheloclist is like, being run every frame. If what you are trying to achieve is an order of execution, I'd say delegate events or UnityEvents would be an easier implementation.
     
  5. LaneFox

    LaneFox

    Joined:
    Jun 29, 2011
    Posts:
    6,458
    If the Find does not find anything then loclist is null, which means it cant look at loclist.Count. Thats no bueno. Add a null check to drop out of the loop.

    Also, never use GameObject.Find(). IMO the practice of using that method is so bad it should probably be removed. There are some safe uses for it, but the alternatives are so much better that it's not even worth considering using it.
     
  6. mrliioadin

    mrliioadin

    Joined:
    Jul 13, 2013
    Posts:
    21
    You're exactly right. That has to be it. I actually had it set up as a coroutine first, but poorly designed. This put me back on the right path. Thanks Andy.
     
  7. mrliioadin

    mrliioadin

    Joined:
    Jul 13, 2013
    Posts:
    21
    Solution posted. Thanks for the prompt feedback everyone.