0
\$\begingroup\$

I'm trying to make a random object spawning script in Unity.

Bellow is the code, any suggestions for improvement / changes? I'm new to both Unity and C#.

// Game objects and Transforms
public Transform playerTransform;
public Transform[] obstaclePrefab;

// spawn params
public float minYDicstane = 6.0f;
public float maxYDistance = 11.0f;
private float boxPositionY;

public float minXDistance = 0.0f;
public float maxXDistance = 3.0f;
private float boxPositionX;

private float minSpawnTime;
private float maxSpawnTime;
private float spawnTime;
private float timeCounter;

// Distances
private float playerDistance;
private float boxDistance;
private float ySpread;

private void Start() 
{
    // Count sequence
    minSpawnTime = 3.0f;
    maxSpawnTime = 8.0f;

    // Count timer
    timeCounter = 0;
    spawnTime = Random.Range(minSpawnTime, maxSpawnTime);

}

private void Update() 
{
    // Count the random spawn time
    timeCounter += Time.deltaTime;
    Debug.Log ("Spawn Time: " + spawnTime + " spawnCount: " + timeCounter);

    if(timeCounter >= spawnTime) 
    {
        // boxDistanceFromPlayer
        playerDistance = playerTransform.position.y;

        // Box spawn distance
        // X position
        boxPositionX = Random.Range(minXDistance, maxXDistance);
        boxPositionX = (boxPositionX-1)*2.0f;

        // Y Position
        boxPositionY = playerDistance + Random.Range(minYDicstane, maxYDistance);

        // Select box color
        int boxColor = Random.Range (0, 4);

        // Let the boxes awake!!!
        Instantiate (obstaclePrefab [boxColor], new Vector2 (boxPositionX, boxPositionY), Quaternion.identity);

        // Make new random spawn time
        spawnTime = Random.Range (minSpawnTime, maxSpawnTime);
        timeCounter = 0;
    }

}

Code after changes, using coroutines

// Game objects and Transforms
public Transform playerTransform;
public GameObject[] obstaclePrefab;

// spawn params
public float minYDicstane = 6.0f;
public float maxYDistance = 11.0f;

public float minXDistance = 0.0f;
public float maxXDistance = 3.0f;

public float minSpawnTime = 2.0f;
public float maxSpawnTime = 5.0f;

public float spawnTime = 4.0f;



IEnumerator SpawnBoxes() 
{
    while (true) 
    {
        float boxPositionY;
        float boxPositionX;

        //Distances
        float playerDistance;

        // Player position
        playerDistance = playerTransform.position.y;

        // Box position
        boxPositionX = Random.Range(minXDistance, maxXDistance);
        boxPositionY = playerDistance + Random.Range(minYDicstane, maxYDistance);

        // Select box
        GameObject box = obstaclePrefab[Random.Range(0, obstaclePrefab.Length - 1)];

        // Instantiate box
        Instantiate (box, new Vector2 (boxPositionX, boxPositionY), Quaternion.identity);

        // Coroutine random amount of time
        yield return new WaitForSeconds(Random.Range(minSpawnTime, maxSpawnTime));
    } 
}

private void Start() 
{
    StartCoroutine(SpawnBoxes());
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to code review! I hope you get some good answers. \$\endgroup\$ – pacmaninbw Sep 24 '16 at 14:58
1
\$\begingroup\$
public Transform[] obstaclePrefab;

That's wrong. Prefabs are of type GameObject, not Transform.


private float minSpawnTime;
private float maxSpawnTime;

Why did you make these private? They should have been public with default values - just as all the others.

private float boxPositionY;
private float boxPositionX;
// Distances
private float playerDistance;
private float boxDistance;
private float ySpread;

These shouldn't even been private, but local to the Update() method, as their values are never reused.


// Select box color
int boxColor = Random.Range(0, 4);

This is very likely to break. Better read the array length instead of hard coding it:

// Select box color
int boxColor = Random.Range(0, obstaclePrefab.Length - 1);

Or just get rid of boxColor all together:

// Select box
GameObject box = obstaclePrefab[Random.Range(0, obstaclePrefab.Length - 1)];

// Count the random spawn time
timeCounter += Time.deltaTime;
if(timeCounter >= spawnTime) 
{
    // Make new random spawn time
    spawnTime = Random.Range (minSpawnTime, maxSpawnTime);
    timeCounter = 0;
}

That's one way to do it. The cleaner method would have been to handle this in a Coroutine and then use WaitForSeconds. Right now, your code runs every single frame, without actually doing anything useful.


Debug.Log ("Spawn Time: " + spawnTime + " spawnCount: " + timeCounter);

Be careful when you log. Logging when something spawns? OK. But spamming a log entry every single frame? Waste of resources.


boxPositionX = (boxPositionX-1)*2.0f;

What is this line supposed to do? That should have been directly computed into minXDistance and maxXDistance, so this line is obsolete.


If this is actually supposed to be C#, the whole script you posted should actually have wrapped in a regular C# class:

using UnityEngine;
using System.Collections;

public class ScriptName : MonoBehaviour {
    // <---- Your stuff goes here
}

Did you just omit this when posting your code here, or did you actually write your scripts without it?

\$\endgroup\$
  • \$\begingroup\$ Thank you for the feedback :) i just removed the "Boilerplate", when i posted the code here. \$\endgroup\$ – Jsf Sep 24 '16 at 18:05
  • \$\begingroup\$ but local to the Update() method, as their values are never reused. allocating memory every single frame ? That doesn't sounds pretty efficient they are good as they are. Logging is almost never a waste of resources because it will be removed when the application is ready to ship anyway. How can you know if the variables should be private or not by seeing only 1 class ? Most of those private variables seems to be calculated from the class or picked as random which basically cant be done from the inspector thus should stay as private, exposing everything from a class is generally a bad idea. \$\endgroup\$ – Denis Sep 24 '16 at 18:54
  • \$\begingroup\$ @denis He simply exported variables from the function scope to private, without any reuse. Allocating a couple of floats on the stack instead of heap doesn't add any measurable cost, that's why I wrote to properly sort the variables into public and method local scope where they belong, and retain the private modifier only for those variables where state is stored. \$\endgroup\$ – Ext3h Sep 24 '16 at 19:39
  • \$\begingroup\$ @Ext3h Thanks for the feedback / help on the topic. I have now done some changes to the code using coroutine. \$\endgroup\$ – Jsf Sep 24 '16 at 22:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.