Search Unity

Other Nested for loop doesn't run to completion

Discussion in 'Scripting' started by TheOtherUserName, Dec 4, 2022.

  1. TheOtherUserName

    TheOtherUserName

    Joined:
    May 30, 2020
    Posts:
    136
    I'm creating a List<List<Matrix4x4>> to save the positions of grass that I want to render. I do this in the Awake method but the last Itteration of the nested for loop doesn't run untill it should.

    Code (CSharp):
    1.     public int resolution = 100;
    2.  
    3.     public Transform pivot;
    4.  
    5.     private List<List<Matrix4x4>> batches = new List<List<Matrix4x4>>();
    6.  
    7.     void Awake()
    8.     {
    9.         int addedMatricies = 0;
    10.  
    11.         for(int i = 0; i < resolution; ++i)
    12.         {
    13.             for (int j = 0; j < resolution; ++j)
    14.             {
    15.                 if (addedMatricies < 1000 && batches.Count != 0)
    16.                 {
    17.                     batches[batches.Count - 1].Add(Matrix4x4.TRS(new Vector3(j - resolution / 2f + (UnityEngine.Random.value - .5f), 0, i - resolution / 2f + (UnityEngine.Random.value - .5f)) + (pivot != null ? pivot.position : Vector3.zero), Quaternion.identity, Vector3.one));
    18.                     addedMatricies++;
    19.                 }
    20.                 else
    21.                 {
    22.                     batches.Add(new List<Matrix4x4>());
    23.                     addedMatricies = 0;
    24.                 }
    25.             }
    26.         }
    27.     }
    It should run to 1000 each time as the resolution is 100 meaning 10.000 instances but the last only goes to 990.
    There are no errors or anything, only an IndexOutOfRange exception when calling index 990 on the last list, obviously.

    Solution posted by Bunny83
     
    Last edited: Dec 4, 2022
  2. Kurt-Dekker

    Kurt-Dekker

    Joined:
    Mar 16, 2013
    Posts:
    38,697
    What is going on above? Are you just trying to initialize two nested lists?

    Have you considered just using a two-dimensional array and be done with it?

    Code (csharp):
    1. private Matrix4x4[,] batches;
    And then to initialize:

    Code (csharp):
    1. batches = new Matrix4x4[ resolution, resolution];
    and then just iterate
    [i,j]
    and fill in.

    Also this:

    If you have more than one or two dots (.) in a single statement, you're just being mean to yourself.

    How to break down hairy lines of code:

    http://plbm.com/?p=248

    Break it up, practice social distancing in your code, one thing per line please.

    "Programming is hard enough without making it harder for ourselves." - angrypenguin on Unity3D forums

    "Combining a bunch of stuff into one line always feels satisfying, but it's always a PITA to debug." - StarManta on the Unity3D forums
     
  3. Bunny83

    Bunny83

    Joined:
    Oct 18, 2010
    Posts:
    3,992
    You have a strange approach to this problem. Your issue is most likely that you have an if / else inside your inner loop. Once you reached 1000 elements in a batch, you do not add a new matrix but instead create a new batch. So each time this happens one iteration of j is wasted / ignored.

    You probably want to do something like this instead:
    Code (CSharp):
    1.  
    2.         List<Matrix4x4> batch = new List<Matrix4x4>();
    3.         batches.Add(batch);
    4.         for(int i = 0; i < resolution; ++i)
    5.         {
    6.             for (int j = 0; j < resolution; ++j)
    7.             {
    8.                 if (batch.Count >= 1000)
    9.                 {
    10.                     batch = new List<Matrix4x4>();
    11.                     batches.Add(batch);
    12.                 }
    13.                 batch.Add(Matrix4x4.TRS(new Vector3(j - resolution / 2f + (UnityEngine.Random.value - .5f), 0, i - resolution / 2f + (UnityEngine.Random.value - .5f)) + (pivot != null ? pivot.position : Vector3.zero), Quaternion.identity, Vector3.one));
    14.             }
    15.         }
    As you can see we do unconditionally add a new matrix for every loop iteration. However before we add it we check the element count of the current batch and if it's full, we create a new batch.
     
  4. Bunny83

    Bunny83

    Joined:
    Oct 18, 2010
    Posts:
    3,992
    Note that this:
    is unnecessarily complex. The pivot does not change during the execution of the for loop. So do this evaluation before the loops

    Code (CSharp):
    1. Vector3 offset = Vector3.zero;
    2. if (pivot != null)
    3.     offset = pivot.position;
    Likewise the generation of the position can or should be broken up. The resolution offset can be integrated into the offset, again once before the loop

    Code (CSharp):
    1. offset -= new Vector3(resolution/2f + 0.5f, 0, resolution/2f + 0.5f);
    Then you can calcuate the position like this:
    Code (CSharp):
    1. Vector3 pos = offset + new Vector3(j + Random.value, 0, i + Random.value);
     
  5. TheOtherUserName

    TheOtherUserName

    Joined:
    May 30, 2020
    Posts:
    136
    Thanks! That was my problem!
    The wasted itteration resulted in ten places being lost, what is exactly how many lists I would need. Sometimes you just need someone else to look over your code.
    Regarding the very long .Add(): I did it because I was in a rush to finish the foliage code and just dumped the function call in there to be done with it.

    I update the position of the grass depending on which direction the player is walking towards and having to call the last elements made me aware of these missing positions.