koobas.hobune.stream
Simplify this code?
So i have just made a spawning script that spawns enemies just outside the visible screen. The method i used under void Update () seems so overcomplicated and hacky though. The problem with this is that the x or the y viewport coordinate needs to be either -0.05 or 1.05 (for x) and -0.06 or 1.06 (for y) which made me use yolorolls, making the code too long. I used decimals so that the enemies would spawn just a bit outside the screen. Here is the code by the way (Just ignore everything outside the update function).
using UnityEngine;
using System.Collections;
public class SwordsmanSpawn1 : MonoBehaviour {
public GameObject Swordsman;
public int ZombieCount;
public float SpawnwaitMin;
public float SpawnwaitMax;
public int BuyTime;
public Vector3 CameraViewportCoordinates;
public Vector3 CameraViewportCoordinates2;
public Vector3 WorldSpawnCoordinates;
public int YoloRoll;
public int YoloRoll2;
public int YoloRoll3;
private float y;
private float x;
void Start () {
StartCoroutine (SpawnZombies ());
}
void Update () {
YoloRoll = Random.Range (0, 2);
YoloRoll2 = Random.Range (0, 2);
YoloRoll3 = Random.Range (0, 2);
if (YoloRoll == 0)
{
y = -0.06f;
}
else
{
y = 1.06f;
}
if (YoloRoll2 == 0)
{
x = -0.05f;
}
else
{
x = 1.05f;
}
if (YoloRoll3 == 0)
{
WorldSpawnCoordinates = Camera.main.ViewportToWorldPoint (CameraViewportCoordinates);
}
else
{
WorldSpawnCoordinates = Camera.main.ViewportToWorldPoint (CameraViewportCoordinates2);
}
CameraViewportCoordinates = new Vector2 (Random.Range (-0.05f, 1.05f),y);
CameraViewportCoordinates2 = new Vector2 (x, Random.Range (-0.06f, 1.06f));
}
IEnumerator SpawnZombies()
{
yield return new WaitForSeconds (BuyTime);
for (int i = 0; i < ZombieCount; i++)
{
Instantiate(Swordsman, WorldSpawnCoordinates, transform.rotation);
yield return new WaitForSeconds (Random.Range (SpawnwaitMin,SpawnwaitMax));
}
}
}
Any thoughts on how to shorten the update function?
Thanks in advance!
//Taegos
Question: Why are you calculating spawn coordinates in your update function?
You only need them when you spawn something, so why calculate them every frame?
So, well, in this case I would shorten your update function by...removing everything in it (and putting it in a seperate method).
Other than that, I don't really think your code is that long.
You could rewrite
if (YoloRoll == 0)
{
y = -0.06f;
}
else
{
y = 1.06f;
}
to
y = (YoloRoll == 0) ? -0.06f : 1.06f;
if you like that better, but that is not a huge gain.
Rather than instantiating 3 random objects, could you simplify by calling .Next on just one?
In addition to the wasting processing power from generating new coordinates every Update (as covered by barbe) the code also wastes memory and confuses the programmer with alot of variables that are used only in one method but are stored by the class.
For instance the two lines
public int YoloRoll;
...
YoloRoll = Random.Range (0, 2);
would be better if you used local variables and declared YoloRoll as it was being assigned a value
int YoloRoll = Random.Range (0, 2);
Even better, why use a variable to store YoloRoll at all, the variable is used once and getting the value is almost as short as the name. You could just use replace
int YoloRoll = Random.Range (0, 2);
...
if (YoloRoll == 0)
with
if(Random.Range(0, 2)==0)
In this case, since there are only 4 cases it is also a waste of space to have 3 if/else statements with 2 conditions each. By combining the information from the other answers you can replace your update function with
Vector3 GetRandomPosition() {
switch(Random.Range(0, 4)) {
case 0:
return Camera.main.ViewportToWorldPoint(new Vector2(Random.Range(-0.05f, 1.05f), -0.06f));
case 1:
return Camera.main.ViewportToWorldPoint(new Vector2(Random.Range(-0.05f, 1.05f), 1.06f));
case 2:
return Camera.main.ViewportToWorldPoint(new Vector2(-0.05f, Random.Range(-0.06f, 1.06f)));
case 3:
return Camera.main.ViewportToWorldPoint(new Vector2(-1.05f, Random.Range(-0.06f, 1.06f)));
}
return Vector3.zero;
}
Which lets you get the spawn position once per spawn instead of constantly generating new spawn positions. It will also let you ditch 2/3 of the variables (which, once again, should not have been class variables in the first place).
I got many informative answers but i will accept this one because it is the most accurate answer to my question, thank you! I have never used switch but thank you for introducing them to me! :)
First don't use an update if you only use the values on the coroutine. Then you don't need the brackets when you only have one job to do after a condition. That saves some space. ;)
You can also makes more job in one line and use function to save some space (but it depends on how much the funtion would be called).
My idea here would be to use a bit of all those and switches along with an enum for clarity :
enum Position{Top, Bottom, Left, Right};
IEnumerator SpawnZombies()
{
yield return new WaitForSeconds (BuyTime);
for (int i = 0; i < ZombieCount; i++)
{
switch(GetRandomPosition())
{
case Position.Top:
Instantiate(Swordsman, Camera.main.ViewportToWorldPoint(new Vector2 (Random.Range (-0.05f, 1.05f),1.06f)), transform.rotation);
break;
case Position.Bottom:
Instantiate(Swordsman, Camera.main.ViewportToWorldPoint(new Vector2 (Random.Range (-0.05f, 1.05f),-0.06f)), transform.rotation);
break;
case Position.Left:
Instantiate(Swordsman, Camera.main.ViewportToWorldPoint(new Vector2 (-0.05f, Random.Range (-0.06f, 1.06f))), transform.rotation);
break;
case Position.Right:
Instantiate(Swordsman, Camera.main.ViewportToWorldPoint(new Vector2 (1.05f, Random.Range (-0.06f, 1.06f))), transform.rotation);
break;
}
yield return new WaitForSeconds (Random.Range (SpawnwaitMin,SpawnwaitMax));
}
}
Position GetRandomPosition()
{
int randomPos = Random.Range(0,4);
switch (randomPos)
{
case 0:
return Position.Top;
break;
case 1:
return Position.Bottom;
break;
case 2:
return Position.Left;
break;
case 3:
return Position.Right;
break;
}
return Position.Top; // I think it needs a return here or it won't compile
}
That beeing said you have plenty of solutions and it mainly depends on how you like to code, who is working with you, how much you need an operation...
For example you could remove the function, the enum and make all in the coroutine. Up to you!
using UnityEngine;
using System.Collections;
public class SwordsmanSpawn1 : MonoBehaviour
{
public GameObject Swordsman;
public int ZombieCount;
public float SpawnwaitMin;
public float SpawnwaitMax;
public int BuyTime;
public Vector3 CameraViewportCoordinates;
public Vector3 CameraViewportCoordinates2;
public Vector3 WorldSpawnCoordinates;
public int YoloRoll;
public int YoloRoll2;
public int YoloRoll3;
private float y;
private float x;
private float random;
void Start()
{
StartCoroutine(SpawnZombies());
}
void Update()
{
random = Random.Range(0, 2);
YoloRoll = YoloRoll2 = YoloRoll3 = random;
if (YoloRoll == 0)
y = -0.06f;
else
y = 1.06f;
if (YoloRoll2 == 0)
x = -0.05f;
else
x = 1.05f;
if (YoloRoll3 == 0)
WorldSpawnCoordinates = Camera.main.ViewportToWorldPoint(CameraViewportCoordinates);
else
WorldSpawnCoordinates = Camera.main.ViewportToWorldPoint(CameraViewportCoordinates2);
CameraViewportCoordinates = new Vector2(Random.Range(-0.05f, 1.05f), y);
CameraViewportCoordinates2 = new Vector2(x, Random.Range(-0.06f, 1.06f));
}
IEnumerator SpawnZombies()
{
yield return new WaitForSeconds(BuyTime);
for (int i = 0; i < ZombieCount; i++)
{
Instantiate(Swordsman, WorldSpawnCoordinates, transform.rotation);
yield return new WaitForSeconds(Random.Range(SpawnwaitMin, SpawnwaitMax));
}
}
}
