For Loop Fires Once In While Loop
I am sure someone with a better understanding of C# will see what I missed or maybe be able to tell me I am doing this the wrong way.
I am trying to make sure not to randomly instantiate game objects on top of any game objects with the tag "LevelTag" (game level objects). For some reason the while loop works but the for loop only runs once while in the while loop. So all the instantiated game objects ARE a 3 magnitude away but ONLY from the first game object with the "LevelTag" in the for loop (by the way: I take the vector3 and put it into an instantiate command in another part of the script). Thanks for the help!
Vector3 GetRandomPosition2()
{
levelTagGO = GameObject.FindGameObjectsWithTag("LevelTag");
float randomX = Random.Range(minX, maxX);
float randomY = Random.Range(minY, maxY);
bool powerUpOnGO = true;
Vector3 powerUpPosition = new Vector3(Random.Range(minX, maxX), Random.Range(minY, maxY), 0f);
for (int i = 0; i < levelTagGO.Length; i++)
{
while (powerUpOnGO)
{
powerUpPosition = new Vector3(Random.Range(minX, maxX), Random.Range(minY, maxY), 0f);
if ((powerUpPosition - levelTagGO[i].transform.position).magnitude < 3)
{
continue;
}
else
{
powerUpPosition = new Vector3(powerUpPosition.x, powerUpPosition.y, powerUpPosition.z);
powerUpOnGO = false;
}
}
}
return new Vector3(powerUpPosition.x, powerUpPosition.y, 0f);
}
Answer by Bunny83 · Jul 15, 2020 at 08:00 PM
Uhm, this line:
bool powerUpOnGO = true;
has to be inside your for loop before the while loop. Once you set the boolean to false it will stay false. So currently you won't enter the while loop ever again once powerUpOnGO is set to false.
Though I don't really see any point in iterating through the whole for loop anyways. The way it works now is once you have found a location for the first "levelTag" you are essentially done. However if you implement the change as I mentioned you would check every object but you essentially ignore all results you get since you will always run through the whole for loop. So your final position would always be from the last "levelTag"
Also note that your continue
is pretty pointless. You are inside a while loop and you would just start a new iteration in that while loop. This happens anyways since there's no more code after that.
This line is also completely pointless. It literally does nothing:
powerUpPosition = new Vector3(powerUpPosition.x, powerUpPosition.y, powerUpPosition.z);
It's conceptionally the same as
powerUpPosition = powerUpPosition;
To me that looks like you have several flaws in your logic.
If you actually want to find a position that does not overlap with any of your "LevelTag" gameobject you would need to swap your loops around. You would pick a random position and check it against all other positions. If any is overlapping you can abort the for loop and calculate a new position until the for loop completes without any overlap.
NOTE: The approach I just described does work but the time complexity grows unpredictably as the number of gameobjects increases. Also since we have a confined space at some point there's simply no more room to fit another object in. At this point the algorithm would never finish and your game would be stuck in an infinite loop, hangs and need to be forced closed. It's just something you should keep in mind. I would generally recommend to not use a while loop in the first place but instead use another for loop with a max iteration count of for example 20. If after 20 attempts no position is found we just give up. What you want to do in that special case is up to you.
So you probably want something like this:
Vector3 GetRandomPosition2()
{
levelTagGO = GameObject.FindGameObjectsWithTag("LevelTag");
for (int n = 0; n < 20; n++)
{
Vector3 powerUpPosition = new Vector3(Random.Range(minX, maxX), Random.Range(minY, maxY), 0f);
bool powerUpOnGO = true;
for (int i = 0; i < levelTagGO.Length; i++)
{
if ((powerUpPosition - levelTagGO[i].transform.position).magnitude < 3)
{
powerUpOnGO = false; // we have an overlap
break; // so just break out of the inner loop and let the outer loop go around again
}
}
// If we reach that point and powerUpOnGO is still true there was no overlap
// so we have a valid point
if (powerUpOnGO)
return powerUpPosition;
}
// if we get to this point it means we haven't found any free position after 20 tries
// so currently we just return the latest position
return powerUpPosition;
}
If you usually should have enough space to place your power ups this should work just fine. However if you want to prevent the powerup spawning when you run out of space, you need a different method signature since currently you can't really tell the calling code that there's no space left. The way you have setup your method signature, it always has to return a Vector3 position, no matter what.
You are awesome! Thanks a ton for taking all this time to help! I am obviously a beginner/hobbyist and I spent a long time trying to figure this out. I had no idea you could break out a for loop so this is GREAT to learn haha. I changed the magnitude to .6 to be a little closer to my level objects but now it works perfectly. And I will only have one of these on the level at a time so there should always be space BUT I will definitely be using this idea of 20 attempts for future features.
Image just shows 1 magnitude keeping the "power up" gameobjects from instantiating on top of the "level objects" that are the diamond shapes
Just a note for anyone else that uses this, make sure to make the "powerUpPosition" vector3 a global/class variable available in the script (not sure the technical term there).
Your answer
Follow this Question
Related Questions
Error Instantiating Prefab in while look at end of row, Happens after a random amount of rows 0 Answers
Having problem with instantiated prefabs 0 Answers
I want to make make a plant to grow after few seconds. 0 Answers
Unity Freezing after for loop 1 Answer
For loop instantiates object endlessly 0 Answers