A level generation script in Unity

Question

I'm doing my first big unity project and I'm really unsure about my current code I use for my level generation.

I would greatly appreciate any feedback. I'm very new to c# and I'm not at all aware of all of my options with MonoBehaviour and such.

On the editor side of unity I have Prefabs that have sprite renderers with unique sprites, a Colors script that can also be found below and optionally an edge collider. I also have a 2100x2100 map of pixels that are assigned a tile according to color in the script.

In the Colors script there is at least one color for foreground and optionally colors for background

Main Script:

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

public class LevelGenerator : MonoBehaviour
{
    public Player player; // The player script
    public Texture2D map; // map of the 1-color tiles to be deciphered
    public GameObject[] gameobjects; // tile gameobjects with sprite renderers, Colors script featuring foreground and backgrounds colors and a bool for constant background. Optionally also an edge collider2D

    private Vector2Int playerPos; // player's position int
    private Vector2 _playerPos; // player's position float
    private const int radius = 8; // radius of generation

    private Dictionary<GameObject, Pair> prefabs; // prefabs to be populated with gameobjects and their foreground and background colors as pairs
    private Dictionary<Vector2, List<GameObject>> posDictionary; // Dictionary of all active tile positions in the level
    private Dictionary<Vector2, List<GameObject>> tilePosDictionary; // Dictionary of all active tile positions in the level
    private Dictionary<Color, List<KeyValuePair<GameObject, Pair>>> colorDictionary; // Dictionary of all colors and the gameobjects associated with them

    // Start is called before the first frame update
    void Start()
    {
        prefabs = new Dictionary<GameObject, Pair>();
        posDictionary = new Dictionary<Vector2, List<GameObject>>();
        tilePosDictionary = new Dictionary<Vector2, List<GameObject>>();
        colorDictionary = new Dictionary<Color, List<KeyValuePair<GameObject, Pair>>>();

        AssignPrefabs(); // populate prefabs dictionary

        playerPos = new Vector2Int((int)Mathf.Round(player.transform.position.x), (int)Mathf.Round(player.transform.position.y)) + new Vector2Int(map.width / 2, map.height / 2); // center map to player
        _playerPos = (Vector2)player.transform.position + new Vector2(map.width / 2, map.height / 2);

        GenerateLevel();
    }

    private void AssignPrefabs()
    {
        foreach (GameObject go in gameobjects)
        {
            prefabs.Add(go, new Pair(go.GetComponent<Colors>().colors, go.GetComponent<Colors>().backgrounds));
        }
    }

    void Update()
    {
        _playerPos = (Vector2)player.transform.position + new Vector2(map.width / 2, map.height / 2);
        float movementDelta = (playerPos - _playerPos).magnitude;
        if (movementDelta >= 1)
        {
            playerPos = new Vector2Int((int)Mathf.Round(player.transform.position.x), (int)Mathf.Round(player.transform.position.y)) + new Vector2Int(map.width / 2, map.height / 2);
            GenerateLevel();
        }
    }

    void GenerateLevel()
    {
        CheckBounds(); // create bounds around player and remove any additional tiles from level
        
        for (int y = playerPos.y - radius; y < playerPos.y + radius; y++)
        {
            for (int x = playerPos.x - radius; x < playerPos.x + radius; x++)
            {
                if (!Generated(new Vector2(x, y)))
                {
                    if (tilePosDictionary.ContainsKey(new Vector2(x, y))) // check if it has already been created
                    {
                        foreach (GameObject go in tilePosDictionary[new Vector2(x, y)])
                            go.SetActive(true);
                    }
                    else
                        GenerateTile(x, y);
                }
            }
        }
    }

    private void CheckBounds()
    {
        Bounds b = new Bounds(new Vector3(playerPos.x - 0.5f, playerPos.y - 0.5f), new Vector3(radius * 2, radius * 2, 0));
        List<Vector2> toDestroy = new List<Vector2>();
        foreach (Vector2 pos in posDictionary.Keys)
        {
            if (!tilePosDictionary.ContainsKey(pos))
                tilePosDictionary.Add(pos, posDictionary[pos]);

            if (!b.Contains(pos))
            {
                toDestroy.Add(pos);
                foreach (GameObject prefab in posDictionary[pos])
                {
                    prefab.SetActive(false);
                }
            }
        }

        foreach (Vector2 pos in toDestroy)
        {
            posDictionary.Remove(pos);
        }
    }

    bool Generated(Vector2 pos)
    {
        if (posDictionary.ContainsKey(pos)) // if it is in current active tiles
            return true;
        return false;
    }

    void GenerateTile(int x, int y)
    {
        Color pixelColor = map.GetPixel(x, y); // store current pixel's color

        if (pixelColor.a == 0) // skip if transparent
            return;

        posDictionary[new Vector2(x, y)] = new List<GameObject>();

        KeyValuePair<GameObject, Pair> foreground = new KeyValuePair<GameObject, Pair>();
        GameObject background = null;

        if (colorDictionary.Keys.Contains(pixelColor)) // if it's color has already been encountered
        {
            foreach (var prefab in colorDictionary[pixelColor]) // store at least the foreground object
            {
                if (foreground.Key == null)
                {
                    foreground = prefab;
                }
                else
                {
                    background = prefab.Key;
                }
            }
        }

        if (foreground.Key == null) // if earlier didn't do the trick
        {
            colorDictionary[pixelColor] = new List<KeyValuePair<GameObject, Pair>>();

            foreach (var prefab in prefabs)
            {
                foreach (Color color in prefab.Value.item1) // iterate foreground colors of each prefab
                {
                    if (color == pixelColor) // if it matches the pixel color
                    {
                        foreground = prefab;
                    }
                }
            }
            foreground.Key.GetComponent<Colors>().constbg = false; // wont set by default so set here
        }

        if (foreground.Key != null) // if a foreground object is located
        {
            if (background == null)
            {
                background = GetBackground(foreground, new Vector2(x, y)); // final attempt to get background
            }

            if (colorDictionary[pixelColor].Count == 0) // if there are no elements in the color dictionary for the pixel color
            {
                colorDictionary[pixelColor].Add(foreground); // always same foreground for color (...)
                if (foreground.Key.GetComponent<Colors>().constbg || background == null) // if background is constant such as constbg and null
                {
                    colorDictionary[pixelColor].Add(new KeyValuePair<GameObject, Pair>(background, null));
                }
            }

            Vector2 position = new Vector2(x - map.width / 2, y - map.height / 2); // offset to center map to center of scene

            var tmp = Instantiate(foreground.Key, position, Quaternion.identity, transform);
            posDictionary[new Vector2(x, y)].Add(tmp);

            if (background != null) // if there is a background to be instantiated
            {
                tmp = Instantiate(background, position, Quaternion.identity, transform);
                posDictionary[new Vector2(x, y)].Add(tmp);
            }
        }
    }

    private GameObject GetBackground(KeyValuePair<GameObject, Pair> prefab, Vector2 pos)
    {
        // different methods will be added later
        return CheckAround(pos, prefab); // get the most encountered tile out of possible background colors
    }

    private GameObject CheckAround(Vector2 pos, KeyValuePair<GameObject, Pair> prefab)
    {
        if (!prefab.Key.GetComponent<Colors>().constbg) // dont attempt again if the background is constant
        {
            if (prefab.Value.item2.Length == 0)
            {
                prefab.Key.GetComponent<Colors>().constbg = true;
                return null;
            }
            else if (prefab.Value.item2.Length == 1)
            {
                prefab.Key.GetComponent<Colors>().constbg = true; // null and single backgrounds will be constant
                return prefab.Value.item2[0];
            }
            else // if there are multiple possible backgrounds
            {
                int[] score = new int[prefab.Value.item2.Length];
                for (int i = 0; i < 9; i++) // create a 3x3 excluding middle tile (the current)
                {
                    if (i == 4) continue;
                    int xi = (i % 3) - 1;
                    int yi = (i / 3) - 1;

                    for (int j = 0; j < prefab.Value.item2.Length; j++)
                    {
                        foreach (Color col in prefab.Value.item2[j].GetComponent<Colors>().colors)
                        {
                            if (col == map.GetPixel((int)pos.x + xi, (int)pos.y + yi))
                            {
                                score[j]++;
                            }
                        }
                    }
                }
                return prefab.Value.item2[Array.IndexOf(score, score.Max())]; // return the tile that was encountered most often
            }
        }
        return null;
    }
}

public class Pair // custom mutable Pair class
{
    public Color[] item1;
    public GameObject[] item2;

    public Pair(Color[] item1, GameObject[] item2)
    {
        this.item1 = item1;
        this.item2 = item2;
    }
}

Colors:

using UnityEngine;

public class Colors : MonoBehaviour
{
    public Color[] colors;
    public GameObject[] backgrounds;

    [HideInInspector]
    public bool constbg = false;
}

Answer 1

Let's start with Colors class.

• It's not a bad idea to add [DisallowMultipleComponent] attribute to a class if you're not planning to add this component multiple times to the same gameobject.

---

• Public fields are considered a bad practice. They are used in tutorials to keep code as simple as possible only. You should use properties instead. But there is a problem: properties are not displayed in the inspector.

The solution is to use public properties with private fields:

[SerializeField] // Attribute to show private field in the inspector.
private List<GameObject> backgrounds;
public List<GameObject> Backgrounds => backgrounds; // Auto-property with get-only access.

To improve the piece of code even more, use property with IEnumerable instead of List: now data is nicely protected inside the object and there is no way to accidentally break the list or modify it from external classes. Usually you want to protect data as much as possible to reduce potential bug count in the future - public fields does not provide such encapsulation.

---

• constbg name is not very descriptive.

I'd recommend not to be afraid to give long names to variables (although someone might disagree):

// Property with get and set access and descriptive name
public bool IsConstantBackground { get; set; } 

The final version could look like this:

public class Colors : MonoBehaviour
{
    [SerializeField]
    private List<Color> foregroundColors;
    [SerializeField]
    private List<GameObject> backgrounds;

    public bool IsConstantBackground { get; set; }
    public IEnumerable<Color> ForegroundColors => foregroundColors;
    public IEnumerable<GameObject> Backgrounds => backgrounds;
}

IReadonlyCollection is another nice immutable wrapper for public collections. Use IEnumerable if you want to get a collection and iterate through it; use IReadonlyCollection when you need access to indexes and Count.

Warning! If you're writing performant code inside the Update() which could run hundreds of times per second, you should consider to keep using List instead of abstract wrappers. Immutable wrappers are fantastic when you're writing clean, protected, maintainable code, but they are slightly less performant than List and could generate a little amount of garbage. It's not big deal usually, but inside the Update() it might be crucial when you're targeting low-end devices.

---

Now, the Pair class.

Consider using Tuples instead. They are pretty much the same thing you implemented, build-in into .net, readonly and convenient to use.

I didn't find in your example the reason to make Pair mutable. Immutable structs are the best data holders for a lot of reasons:

• Hard to break.
• Easy to use.
• More performant than classes.
• Easy to maintain (no need to check all struct's users to find if someone trying to modify the struct).

---

LevelGenerator class:

• GetComponent is pretty costly function.

It is better to use it once and store the result inside a variable:

foreach (GameObject go in gameobjects)
{
    var colorsHolder = go.GetComponent<Colors>();
    prefabs.Add(go, new Pair(colorsHolder.colors, colorsHolder.backgrounds));
}

---

• LevelGeneratorcollects some data and tracks the player and keeps map instantiated only around the player and generates tiles.

Single Responsibility Principle states that a class should have only one thing to do. Well, "one thing" is not very accurate term: class can do several actions grouped into one functionality which still considered "a one thing". Still, I'd try to distribute LevelGenerator functionality into several classes:

• LevelManager - manager that sends commands to other classes to do an actual work.
• TileGenerator - generation functionality.
• PlayerTracker - boundaries detection functionality.
• MapUpdater - receives boundaries and activates/deactivates tiles.

If classes are kept as small as possible, it is easier to modify or extend existing code. For example, you could implement very complex tile generation inside external class without worry that your LevelGenerator class becomes a nightmare with 1000 lines of code.

---

One more thing. MonoBehaviours are not the only tool for convenient work with data inside the Unity. If you're not familiar with ScriptableObjects, I'm strongly recommend to watch/read some tutorials: they are fantastic tool to work with data.

In your example you could make Colors class a ScriptableObject instead of MonoBehaviour and track tile prefabs separately or inside it. It's not necessarily a better approach, but definitely an option. It might be more convenient and clean, or might not.

I'm usually prefer to store data inside ScriptableObjects, not MonoBehaviours.