3
\$\begingroup\$

I've recently had time to devise another program so this time I decided to write the Perlin Noise algorithm in code. I have succeeded and the code works but I can't help but feel that my practices while writing are still not as good as they should be (especially since I'm still pretty new to the topic of programming). Please note that as I am working in Unity I have not separated the class files that I wrote into different source files. The code is comprised of a total of 3 class files: the main MonoBehaviour class named perlin_terrain and the two I have declared - named "Grid" and "Point".

using System;
using System.Collections;
using System.Collections.Generic;
using UnityEngine;

public class perlin_terrain : MonoBehaviour {

    public Terrain terrain;

    public int gridResolution = 512; // Will be adjusted to be divisible by the subdivision value. 
    public int gridSubdivision = 4;  // Value must be a power of 2.

    [Range(0, 1)]
    public float steepness = 0.12f;

    Grid grid;

    private static float[,] heights;

    // Use this for initialization
    void Start()
    {
        if (gridResolution < terrain.terrainData.heightmapResolution)
            throw new ArgumentException("Given resolution must be equal to or exceed terrain heightmap resolution.");

        grid = new Grid(gridResolution, gridSubdivision);
        grid.SteepnessCoefficient = steepness;

        heights = new float[terrain.terrainData.heightmapResolution, terrain.terrainData.heightmapResolution];

        determineTerrain();
    }

    // Update is called once per frame
    void Update()
    {
        // Do nothing.
    }

    void determineTerrain()
    {
        for (int y = 0; y < terrain.terrainData.heightmapResolution; y++)
            for (int x = 0; x < terrain.terrainData.heightmapResolution; x++)
            {
                heights[y, x] = grid.getAmplitude(x, y);
            }

        terrain.terrainData.SetHeights(0, 0, heights);
    }
}

public class Grid
{
    public float steepnessCoefficient = 0.12f; // The closer it is to one, the steeper the terrain. Has to be greater than or equal to zero.

    private static int resolution;
    private static int subdivision;
    private int subdivisionInterval;

    private Point[] points;

    public Grid(int _resolution, int _subdivision)
    {
        if (Mathf.Log(_subdivision, 2) % 1 != 0)
            throw new ArgumentException("Subdivision value MUST be a power of 2.");

        while (_resolution % _subdivision != 0)
        {
            _resolution++;
        }

        resolution = _resolution;
        subdivision = _subdivision;
        subdivisionInterval = resolution / subdivision;

        points = new Point[(int)Mathf.Pow(subdivision + 1, 2)];

        generatePoints();
    }

    private void generatePoints()
    {
        int n = 0;

        for (int i = 0; i <= subdivision; i++)
            for (int j = 0; j <= subdivision; j++)
            {
                points[n] = new Point(j * subdivisionInterval, i * subdivisionInterval);
                n++;
            }
    }

    public float getAmplitude(int x, int y)
    {
        float pointX = ((float)x / (float)subdivisionInterval) % 1f , pointY = ((float)y / (float)subdivisionInterval) % 1f;

        Vector2[] distanceVectors = { new Vector2(pointX, pointY), new Vector2(pointX - 1, pointY), new Vector2(pointX, pointY - 1), new Vector2(pointX - 1, pointY - 1) };

        float value1 = 0, value2 = 0, value3 = 0, value4 = 0;
        float weighed1 = 0, weighed2 = 0;
        float amplitude = 0;

        List<Point> unitPoints = new List<Point>();

        unitPoints = getUnitPoints(x, y);

        int n = 0;

        foreach (Point point in unitPoints)
        {
            switch (n)
            {
                case 0:
                    value1 = Vector2.Dot(point.GradientVector, distanceVectors[0]);
                    break;
                case 1:
                    value2 = Vector2.Dot(point.GradientVector, distanceVectors[1]);
                    break;
                case 2:
                    value3 = Vector2.Dot(point.GradientVector, distanceVectors[2]);
                    break;
                case 3:
                    value4 = Vector2.Dot(point.GradientVector, distanceVectors[3]);
                    break;
            }

            n++;
        }

        weighed1 = Mathf.Lerp(value1, value2, fadeValue(pointX));
        weighed2 = Mathf.Lerp(value3, value4, fadeValue(pointX));

        amplitude = Mathf.Lerp(weighed1, weighed2, fadeValue(pointY));

        return ((amplitude + 1) / 2) * steepnessCoefficient; // Normalize amplitude to range from 0 to 1. (and apply steepness coefficient)
    }

    private float fadeValue(float value)
    {
        return 6 * Mathf.Pow(value, 5) - 15 * Mathf.Pow(value, 4) + 10 * Mathf.Pow(value, 3);
    }

    private List<Point> getUnitPoints(int x, int y)
    {
        int unitNumberX = (int)(x / subdivisionInterval), unitNumberY = (int)(y / subdivisionInterval);
        List<Point> unitPoints = new List<Point>();

        foreach (Point point in points)
        {
            if (point.X == unitNumberX * subdivisionInterval && point.Y == unitNumberY * subdivisionInterval)
            {
                unitPoints.Add(point);
            }
            else if (point.X == unitNumberX * subdivisionInterval + subdivisionInterval && point.Y == unitNumberY * subdivisionInterval)
            {
                unitPoints.Add(point);
            }
            else if (point.X == unitNumberX * subdivisionInterval && point.Y == unitNumberY * subdivisionInterval + subdivisionInterval)
            {
                unitPoints.Add(point);
            }
            else if (point.X == unitNumberX * subdivisionInterval + subdivisionInterval && point.Y == unitNumberY * subdivisionInterval + subdivisionInterval)
            {
                unitPoints.Add(point);
            }
        }

        return unitPoints;
    }

    //------------------------------ Property declarations ------------------------------

    public static int Resolution
    {
        get { return resolution; }
    }

    public static int Subdivision
    {
        get { return subdivision; }
    }

    public float SteepnessCoefficient
    {
        get { return steepnessCoefficient; }
        set { steepnessCoefficient = value; }
    }
}

public class Point
{
    private static Vector2[] vectors = { new Vector2(1, 1), new Vector2(-1, 1), new Vector2(1, -1), new Vector2(-1, -1) };
    private Vector2 gradientVector;

    private int x, y;

    public Point(int _x, int _y)
    {
        x = _x;
        y = _y;

        gradientVector = vectors[UnityEngine.Random.Range(0, 4)];
    }

    //------------------------------ Property declarations ------------------------------

    public int X
    {
        get { return x; }
    }

    public int Y
    {
        get { return y; }
    }

    public Vector2 GradientVector
    {
        get { return gradientVector; }
    }
}

I have some work to do at the moment but I'll be back in a few hours to see you guys' awesome and much appreciated feedback. :)

\$\endgroup\$
  • \$\begingroup\$ So every time you construct an instance of Grid you want the static variables resolution and subdivision to be set? Seems odd. Doesn't seem like they need to be static at all. Maybe gridSubdivision in the perlin_terrain class but in the Grid class it doesn't seem like they need to be static. Is there some reason you have them static? \$\endgroup\$ – Shelby115 May 22 '17 at 20:09
  • \$\begingroup\$ Also, are you aware you could do public int X { get; private set; } instead of a getter only and a backing field for your Point class? I do recall something to do with fields and the inspector in Unity but I thought the fields had to be public not private so I'm not sure. \$\endgroup\$ – Shelby115 May 22 '17 at 20:20
  • \$\begingroup\$ To be honest, I don't know why I decided to set them as 'static' now that I'm reviewing the code. I think at the time I did it because I was using the two values to initialize an array, but I ultimately decided to use the 'terrainData' resolution property to initialize it and I seem to have forgotten to delete the static keyword from the two fields. Oh well, what can ya' do... :D I also see now that I probably should have made 'Point' a 'struct' instead of a 'class' but it's whatever... As for the property declarations I'm not exactly sure. Could you maybe elaborate? :) \$\endgroup\$ – Simeon Laplev May 26 '17 at 12:56
2
\$\begingroup\$

I've re-written your point class to be a bit more concise and to-the-point (hehe).

public class Point
{
    private static Vector2[] vectors = {
        new Vector2(1, 1),
        new Vector2(-1, 1),
        new Vector2(1, -1),
        new Vector2(-1, -1)
    };

    public int X { get; private set; }
    public int Y { get; private set; }
    public Vector2 GradientVector { get; private set; }

    public Point (int x, int y)
    {
        X = x;
        Y = y;
        gradientVector = vectors[UnityEngine.Random.Range(0,vectors.length)];
    }
}

The following was done,

  • Properties I made use of private setters with public getters condensing your two variables/members into a single one. Only the code inside the class can set the values of X, Y, and GradientVector but anyone can view it.
  • Magic Numbers Your UnityEngine.Random.Range(0,4) was okay but if you ever add/remove vectors (for whatever reason) form the vectors variable then you might forget to update the constructor. Using vectors.length removes the possibility for that entirely.
  • Lines of Code Additionally instead of the initial 32 lines of code it is now 20 lines of code including the fact that I make vectors take 6 lines instead of 1 line to improve readability.

Reduce & Cache Calculations

private List<Point> getUnitPoints(int x, int y)
{
    int unitNumberX = (int)(x / subdivisionInterval) * subdivisionInterval;
    int unitNumberY = (int)(y / subdivisionInterval) * subdivisionInterval;
    List<Point> unitPoints = new List<Point>();

    foreach (Point point in points)
    {
        var isCompatibleX = point.X == unitNumberX 
                         || point.X == unitNumberX + subdivisionInterval;

        var isCompatibleY = point.Y == unitNumberY 
                         || point.Y == unitNumberY + subdivisionInterval;

        if (isCompatibleX && isCompatibleY)
        {
            unitPoints.Add(point);
        }
    }

    return unitPoints;
}

Things I noticed with your getUnitPoints(..) function.

  • Reduce unitNumberX and unitNumberY can include the multiplication with subdivisionInterval. They're not used separately anywhere else in the function. This reduces the number of times you have to calculate it.
  • Simplify Your if-elseif chain consisted of 4 conditions in which they all resulted in the same action. This should be able to be simplified to the above (don't just take the word of a stranger on the internet though double-check that it works the same as yours before; both solutions that should work and shouldn't).

Arrays & More Reduction

public float getAmplitude(int x, int y)
{
    float sub = (float)subdivisionInterval; // example
    float pointX = ((float)x / sub) % 1f;
    float pointY = ((float)y / sub) % 1f;

    Vector2[] distanceVectors = { new Vector2(pointX, pointY), new Vector2(pointX - 1, pointY), new Vector2(pointX, pointY - 1), new Vector2(pointX - 1, pointY - 1) };

    float amplitude = 0;
    float weighed1 = 0, weighed2 = 0;

    List<Point> unitPoints = getUnitPoints(x, y);
    float[] values = new float[unitPoints.length];

    for (int n = 0; n < unitPoints.length && n < values.length; n++)
    {   
        values[n] = Vector2.Dot(unitPoints[n].GradientVector, distanceVectors[n]);
    }

    weighed1 = Mathf.Lerp(value1, value2, fadeValue(pointX));
    weighed2 = Mathf.Lerp(value3, value4, fadeValue(pointX));

    amplitude = Mathf.Lerp(weighed1, weighed2, fadeValue(pointY));

    return ((amplitude + 1) / 2) * steepnessCoefficient; // Normalize amplitude to range from 0 to 1. (and apply steepness coefficient)
}
  • Arrays When you see something like value1, value2, etc it's a huge indicator an array might be better. If you switch your 4 value variables into an array it opens a door. Suddenly your switch statement no longer needs to be a switch statement because value1, value2, etc can become values[n] and from there you can reduce your switch statement into a single line.
  • Loops foreach loops are for when you don't need an index. for loops are for when you need an index. You're using a foreach then keeping your own index, that can be reduced to a single for loop.
  • Calculations vs Memory Generally speaking games require quick performance. The tradeoff of quick performance is usually memory (aka another variable). You might consider caching calculations/casts/etc that you use more than once. This is an extreme example and probably unnecessary (but for the sake of example) you could make a float variable and store subdivisionInterval casted as a float so you don't need to cast it a second time (i.e. pointX and pointY). Again, a cast probably doesn't need cached as it's nearly negligible but it can be helpful to keep this concept in mind.

Final words

I noticed you have a Point[] for your grid. One thing you could do is make it a jagged array (i.e. Point[][] an array of arrays) this way you can make each point at its coordinate position (i.e. Point[x][y]) for quick and simplistic access conceptually.

That said you have a working solution so unless you're having trouble conceptually keeping track of your current solution I wouldn't change it.

I hope that you find this helpful.

\$\endgroup\$
  • \$\begingroup\$ Thoroughly explained and well written. Thank you so much for the helpful tips. I wish you a very nice day. Gladly accepting your answer. :) \$\endgroup\$ – Simeon Laplev Jun 7 '17 at 10:00
  • \$\begingroup\$ By the way the part where you store the booleans in a variable is sooo clever. I haven't really thought about that before to shorten checks.... Very helpful tip. :) \$\endgroup\$ – Simeon Laplev Jun 7 '17 at 10:07

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.