6
\$\begingroup\$

I've created this simple 3D voxel terrain generator using Perlin Noise. I have a few concerns about it though.

  • Is there any way to make it more performant? It currently takes ~1.5 seconds to generate a 200x200 map.
  • Am I splitting up the different "jobs" of the generator correctly?
  • Is my general code style good?

using UnityEngine;

/// <summary>
/// Terrain generator.
/// </summary>
public class TerrainGenerator : MonoBehaviour 
{
    public GameObject cube;
    public int worldWidthX;
    public int worldWidthZ;

    public float lowerNoiseScaleValue;
    public float upperNoiseScaleValue;

    public float lowerNoiseScaleModifierValue;
    public float upperNoiseScaleModifierValue;

    private float noiseScale;
    private float noiseScaleModifier;

    /// <summary>
    /// Generates the cubes as children of a GameObject.
    /// </summary>
    public void InitalizeCubes()
    {
        for(float x = 0; x <= this.worldWidthX; ++x)
        {
            for(float z = 0; z <= this.worldWidthZ; ++z)
            {
                GameObject cube = Instantiate(
                    this.cube, 
                    new Vector3(x, 0, z), 
                    this.cube.transform.rotation
                ) as GameObject;

                cube.transform.parent = this.transform;
            }
        }
    }

    /// <summary>
    /// Sets the color of the cube based on the cube's height.
    /// </summary>
    /// <param name="cube">Cube.</param>
    /// <param name="cubeHeight">Cube height.</param>
    public void SetCubeHeightColor(Transform cube, float cubeHeight)
    {
        cube.renderer.material.color = new Color(cubeHeight / 5, cubeHeight, cubeHeight / 5);
    }

    /// <summary>
    /// Applies a height value to the transform of a cube.
    /// </summary>
    /// <param name="cube">Cube.</param>
    /// <param name="cubeHeight">Cube height.</param>
    public void ApplyHeightToCube(Transform cube, float cubeHeight)
    {
        int newHeight = Mathf.RoundToInt(cubeHeight * this.noiseScaleModifier);
        Vector3 newPosition = new Vector3(cube.transform.position.x, newHeight, cube.transform.position.z);
        cube.transform.position = newPosition;
    }

    /// <summary>
    /// Updates the positions of the cubes by giving
    /// each cube a specific perlin noise value,
    /// coloring them, and finally, applying the height
    /// values to the transform.position's of the cubes.
    /// </summary>
    public void GenerateCubes()
    {
        foreach(Transform cube in transform)
        {
            float noiseScaleAddition = Random.Range(this.noiseScale / 10, this.noiseScale / 9);
            float cubeHeight = Mathf.PerlinNoise(
                (cube.transform.position.x / (this.noiseScale + noiseScaleAddition)),
                (cube.transform.position.z / (this.noiseScale + noiseScaleAddition))
            );
            SetCubeHeightColor(cube, cubeHeight);
            ApplyHeightToCube(cube, cubeHeight);
        }
    }

    /// <summary>
    /// Start this instance.
    /// </summary>
    void Start()
    {
        this.noiseScale = Random.Range(this.lowerNoiseScaleValue, this.upperNoiseScaleValue);
        this.noiseScaleModifier = Random.Range(this.lowerNoiseScaleModifierValue, this.upperNoiseScaleModifierValue);

        this.InitalizeCubes();
        this.GenerateCubes();
        Screen.lockCursor = true;
    }
}

For those who are curious, here are a few pictures of sample terrain:

enter image description hereenter image description here

If you want to critique or suggest changes to the algorithm, I'm open to them.

\$\endgroup\$
4
\$\begingroup\$

I don't know much about Unity, but one of the Unity operations must be the bottleneck here if it takes 1.5 seconds to process 40k cubes. I'd focus on reducing the number of Unity operations you're doing (manipulating the game objects). For example, you're first placing all the cubes at y=0, then moving them later. Instead, try generating the Perlin Noise first into an array, and then place the cubes at the correct height on the Instantiate call.

Also consider this line:

cube.renderer.material.color = new Color(cubeHeight / 5, cubeHeight, cubeHeight / 5);

You have 40k cubes but only about 20 colors. Is there some way of sharing materials (or even renderers) between the cubes? You may be asking Unity to manage many copies of identical materials. Look into other ways to share resources; is there some way to only create a few cubes with different colors, and render one entire height-layer with copies of it?

As for general coding style:

You have public GameObject cube; as an instance variable, but you also use the word cube all over the place for local variables. Is there a better name you could give this, like templateCube?

This is comment is not necessary:

/// <summary>
/// Sets the color of the cube based on the cube's height.
/// </summary>
/// <param name="cube">Cube.</param>
/// <param name="cubeHeight">Cube height.</param>
public void SetCubeHeightColor(Transform cube, float cubeHeight)

This takes up a lot of lines to give me the same information as your method signature ... although when my eyes see "SetCubeHeight" I assume you're setting the cube's height, but you're actually setting the color. Consider SetCubeColorByHeight instead.

Usually, everything that your class needs to function properly should be an argument in its constructor. This may not be possible with a Unity MonoBehavior, but I thought I'd throw it out there.

\$\endgroup\$

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.