koobas.hobune.stream
Level genration loop crashing unity3D
I have an editor script that generates a random level, I have a for loop that checks for duplicates of certain objects and is supposed to reset and check if it finds one but it keeps crashing and freezing unity. What is wrong with my loop?
private int CheckForMultiple(int testBuilding, int roomIndex)
{
int newBuilding = testBuilding;
for(int i = 1; i < roomIndex; i++)
{
if(_indexes[i] == newBuilding)
{
newBuilding = (int)Random.Range(0, backgroundElements.Count);
i = 1;//reset loop and start over
}
}
return newBuilding;
}
So it doesn't always crash but when it does I have no indication as to why and I've been at this for hours with nothing but a head ache....
Ughhhh... I finally fixed it after several hours. I found unity's log file in the AppData folder and found my issue. So I switched to a while loop to make things a little easier and found I wasn't updating one of my variables after assigning a new variable causing the while loop and for loop to both stick into an infinite loop and thus crash unity. With out that log file I doubt I would of fixed this.
This is extremely dangerous and extremely inefficient code. It's pretty obvious why the loop causes a hang (not a crash btw.). From your code it's hard to tell what this method actually should do. Though if it hangs it means you send it into an infinite loop. That means your for loop will never exit. That means you always enter your if statement at some iteration of your for loop. This also means that you probably have just as many elements in your "_indexes" array / list as you have elements in your "backgroundElements" List. So each index from 0 up to backgroundElements.Count is already used at some of the "_indexes" elements. So no matter which random number you roll it will always appear in the "_indexes" and therefore you restart the loop endlessly.
Another point which is confusing is that you start your loop at index 1 instead of 0 which makes this code even stranger. Since we don't have the full picture (we don't know what "_indexes" actually is, what values it contains, how they are filled, how many elements it has and how many elements are inside backgroundElements).
If you want to avoid picking the same background element twice you should use a "bucket list" where you keep either the remaining / unused indices or the background objects itself. Each time you pick / use one you would remove that index / object from the bucket so you ever have only unused objects in your bucket. So you can safely pick a random index / object from the bucket. Also you immediately see when the bucket is empty and no unused elements are left so you either have to create an error at this point or return some default value depending on what exact behaviour you need.
That's the new code I'm using.
private int CheckDuplicates(int buildingID)
{
int newID = buildingID;
CityBuilding building = backgroundElements[newID].GetComponent<CityBuilding>();
if(!building.allow$$anonymous$$ultiple)
{
Debug.LogWarning("These buildings are not allowed duplicates...");
if(_indexes.Contains(newID))
{
while(_indexes.Contains(newID) && !building.allow$$anonymous$$ultiple)
{
newID = (int)Random.Range(0, backgroundElements.Count);
if (newID == backgroundElements.Count)
newID--;
building = backgroundElements[newID].GetComponent<CityBuilding>();
}
}
}
return newID;
}
I started it at 1 because of the way I have my map set up. The beginning and ending instances don't change just the buildings in between.
This is just as inefficient as your previous code. However as long as you have at least 1 building where "allow$$anonymous$$ultiple" is true this code at least will ter$$anonymous$$ate eventually.
Note: _indexes.Contains() will actually iterate through all elements in that List / array so the complexity of your code stays the same. The more elements you have in your backgroundElements list the more iterations you need if _indexes contains all types at least once.
Next thing is that Random.Range has two overloads. One that takes integer $$anonymous$$ / max values and one that takes float parameters. The int version does already return an int value. Also the int version does already exclude the max limit (but includes the $$anonymous$$ limit). So your following if statement will never be true since newID can never be "backgroundElements.Count" with the only exception of $$anonymous$$ and max are both 0 in which case Random.Range will return the $$anonymous$$ value. However that would mean backgroundElements is empty and neither "0" nor "-1" are valid indices.
Finally you really shouldn't use an array / List of gameobjects. Just use an array / List of CityBuildings. That way you don't need to call GetComponent all the time. Of course if you change the array / List to CityBuilding you have to reassign all your object in the inspector.
Thanks for the input, a little explanation as to what my code is supposed to do would pry be helpful as this is just a small part of it. I have a side scrolling city level that is generated at random. The list backgroundElements stores all available buildings the game can generate, it chooses buildings from that list as it creates the level.. Each building is a GameObject with its own properties like only allowing one of its kind to generate. As the list of buildings is being checked and populated it also instantiates the buildings GameObjects into the level placing them accordingly as the are created. In my game I only want one of specific buildings hence the allow multiple variable. As far as RandRange goes I found it can still choose the max number which would go over my lists array size which is why I have that check in there, I chose not to subtract one from the array size and made it the way I did because when I convert to an int it will cut off the decimal thus making it very unlikely but still plausible to choose the last index. How could I make this more efficient is my main question?
