array position = NaN

Ok ive got myself into a web of mess, so first of apologies for the code below.

I am attempting to place objects randomly on a board and record the vector position they are placed within an array.
To avoid overlaps i am first running through a bool function to check my next random placement position does not = any of the the other placed objects.

So i have set up my array length of possibly 20 objects and passing through the new object location and then looping through my array.

I am encountering a never ending loop however as when i place my first object the array is empty so upon debugging all array positions are showing as NaN.
This seems to be then confusing my if statement on comparing vector 2 positions and making it return the wrong value.

I have included the full length of code but the main issue is in the checkoverlap function where is checks if selrange != to boxlocation

Any help or guidance would be massively appreciated.

thanks

Code (csharp):

1.  
2.   void BoxSpawn()
3.    {
4.      xPos = Random.Range (minXPos, maxXPos);
5.      yPos = Random.Range (minYPos, maxYPos);
6.  
7.      Vector2 selRange = new Vector2 (xPos, yPos);
8.  
9.  
10.      hit = Physics2D.Raycast (Camera.main.ScreenToWorldPoint(selRange),Vector2.zero);
11.      Debug.DrawRay(Camera.main.ScreenToWorldPoint(selRange),Vector3.forward);
12.      if (hit.collider == null && CheckOverlap(selRange) == true) {
13.        boxLocations[i] = new Vector2(xPos,yPos);  
14.       Instantiate (theBoxes, new Vector2 (xPos, yPos), Quaternion.identity);
15.  
16.        i++;
17.        print ("miss");
18.  
19.      }
20.      else
21.      {
22.        BoxSpawn ();
23.      }
24.    }
25.  
26.    bool CheckOverlap(Vector2 pos)
27.    {
28.      bool nohit = true;
29.      for (int i = 0; i < boxLocations.Length && nohit == true; i++)
30.      {
31.        if (pos != boxLocations [i]) {
32.          nohit = true;
33.        }
34.        else {nohit = false;}
35.      }
36.      if (nohit == true) {
37.        return true;
38.      } else {
39.        return false;
40.      }
41.    }
42.  

 

Do you know the size of your board? Right now you are doing a ton of checks, if you have 20 positions you'll do 20*20 loops (because every position has to check every other).

Why dont you simply fill an array with all possible positions at the beginning and shuffle that. You can then just loop over that array and assign a position to each box.

A small comment on your coding style, i find it quite confusing. You really shouldnt write code like `if(x == true)`, you can just write `if(x)`. Another example; `if(x==true)return true; else return false;` is ofcourse equal to `return x;`
I'd rewrite it to something like this:

Code (csharp):

1.  
2. bool doesOverlap(Vector2 pos) {
3.   for(int i = 0; i < boxLocations.Length; i++)
4.     if(pos == boxLocations[i])
5.       return true;
6.   return false;
7. }
8.  

Though comparing floats, and thus Vector2's is nigh impossible. A float and double represent an irrational number, that is 1.000000 repeating. It goes on indefinitely, which makes comparing them rather impossible. If you really need to you usually substract them and see if the difference is small.

 

Well... you have a lot going on there.

First off, if your raycast hits anything, you could go in to hit an infinite loop. For example if you have a background or UI or whatever, it could easily loop.

Also, the verbosity makes it very hard to read, it may present problems in bug hunting. You could simplify a lot. For example:

Code (CSharp):

1.     bool CheckOverlap(Vector2 pos)
2.     {
3.       bool nohit = true;
4.       for (int i = 0; i < boxLocations.Length && nohit == true; i++)
5.       {
6.         if (pos != boxLocations [i]) {
7.           nohit = true;
8.         }
9.         else {nohit = false;}
10.       }
11.       if (nohit == true) {
12.         return true;
13.       } else {
14.         return false;
15.       }
16.     }

Could be:

Code (CSharp):

1.  bool CheckOverlap(Vector2 pos)
2.     {
3.       for (int i = 0; i < boxLocations.Length; i++)
4.       {
5.         if (pos == boxLocations [i]) return false;
6.       }
7.       return true;
8.     }
9.  

 

Here is an example that doesn't directly do what your previous code did (comments explain the differences), but was actually done by refactoring from your code and simplifying the case a little bit.

It might solve your use case, or it might not, but should give you a good example what direction you could take your code.

Especially note that proper encapsulation (separating different things to different methods/objects) lets the code be flexible. F.e. if you use bigger prefabs or floats for random position, only the PositionIsAvailable method needs changing - the rest of the code doesn't care what the actual position to spawn is.

Code (CSharp):

1. using UnityEngine;
2.  
3. public class RandomBoxSpawn : MonoBehaviour
4. {
5.     // assumes prefab is 1x1 or less Unity units in size
6.     // if it's bigger, objects will overlap
7.     //
8.     // uses int instead of float to prevent spawned objects overlap
9.     // and to ease checking for already taken positions
10.     //
11.     // if floats are used, used position check should also take into account
12.     // the bounding box of the object to spawn
13.     //
14.     // works with 2D grid, ignoring Z axis
15.     // if used in 3D environment and spawning on "ground" level
16.     // Y axis should be ignored instead
17.  
18.     public int minXPos = -5;
19.     public int maxXPos = 5;
20.     public int minYPos = -5;
21.     public int maxYPos = 5;
22.     public GameObject BoxPrefab;
23.     public int MaximumBoxes = 20;
24.     public Vector2[] boxLocations;
25.     // boxLocationsIndex is used to keep track how many objects we already have  
26.     private int boxLocationsIndex;
27.  
28.     void Start()
29.     {
30.         boxLocations = new Vector2[MaximumBoxes];
31.         boxLocationsIndex = 0;
32.     }
33.  
34.     void Update()
35.     {
36.         if (Input.GetKeyDown(KeyCode.Space))
37.         {
38.             BoxSpawn();
39.         }
40.     }
41.  
42.     bool CanAddBox()
43.     {
44.         if (boxLocationsIndex < MaximumBoxes)
45.         {
46.             Debug.Log("Can spawn.");
47.             return true;
48.         }
49.         else
50.         {
51.             Debug.Log("BoxLocations capacity reached.");
52.             return false;
53.         }
54.  
55.         // if you don't need the Debug.Log statements
56.         // this can be simplified to:
57.         //
58.         // if (boxLocationsIndex < MaximumBoxes)
59.         //     return true;
60.         // else
61.         //     return false;
62.  
63.     }
64.  
65.     void AddBoxLocation(Vector2 boxLocation)
66.     {
67.         // check here too if you think this method might get called
68.         // without checking capacity first
69.         //
70.         // if (!CanAddBox()) return;
71.  
72.         boxLocations[boxLocationsIndex] = boxLocation;
73.         boxLocationsIndex++;
74.     }
75.  
76.     void BoxSpawn()
77.     {
78.         if (!CanAddBox())
79.             return;
80.  
81.         // using vars to not need to change the code
82.         // if floats are used in the future
83.         var xPos = Random.Range(minXPos, maxXPos);
84.         var yPos = Random.Range(minYPos, maxYPos);
85.  
86.         Vector2 selRange = new Vector2(xPos, yPos);
87.  
88.         if (PositionIsAvailable(selRange))
89.         {
90.             Instantiate(BoxPrefab, selRange, Quaternion.identity);
91.             Debug.Log("Box " + boxLocationsIndex + " spawned at " + selRange);
92.             AddBoxLocation(selRange);
93.         }
94.         else
95.         {
96.             Debug.Log("Random position " + selRange + " already taken.");
97.         }
98.     }
99.  
100.     bool PositionIsAvailable(Vector2 pos)
101.     {
102.         for (int i = 0; i < boxLocationsIndex; i++) // using index instead of length shortens the loop
103.         {
104.             // if floats are used, only this check needs changing
105.             // and BoxSpawn can stay as is
106.             if (pos == boxLocations[i])
107.                 return false;
108.         }
109.         return true;
110.     }
111. }

 

Code (csharp):

1.        // if (boxLocationsIndex < MaximumBoxes)
2.       //     return true;
3.        // else
4.        //     return false;
5.  

Yeah, i really wouldnt do this.

Code (csharp):

1. return boxLocationsIndex < MaximumBoxes;

is far clearer, and used so much more often.

Code (csharp):

1.  
2.        // using vars to not need to change the code
3.       // if floats are used in the future
4.        var xPos = Random.Range(minXPos, maxXPos);
5.  

So this would be the only case i'd say do explicitely use a type. I'm a huge fan of var, but there is a huge caveat here. Random.Range min and max are inclusive when using floats, but when you use ints its exclusive. You really dont want to mess this up, so i'd explicitly state the type just so you know.

 

If taken to the logical conclusion your code is better. I guess it's style preference (if I'm not mistaken it will compile to exact same IL), but you're right - it is clearer that way.
Personally I tend to be more verbose in first iterations of the functions and simplify later when the class is feature-complete and I'm happy with the structure.

Good catch!
I always forget that (need to make a sticky note or sth).

Thanks for valuable input.