Perlin Noise terrain in Unity3D

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 unitPoints = new List();

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(val

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 getUnitPoints(int x, int y)
{
    int unitNumberX = (int)(x / subdivisionInterval) * subdivisionInterval;
    int unitNumberY = (int)(y / subdivisionInterval) * subdivisionInterval;
    List unitPoints = new List();

    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 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]) fo

StackExchange Code Review Q#163302, answer score: 2