3
\$\begingroup\$

I need some help/critique on some logic I am trying to figure out. What I have feels a bit messy and I wanted to reach out and see if there is a better way to approach this.

I am creating a procedurally generated sidescroller in Unity. My Objective is to create a large List of platforms for the character to traverse at runtime. The Terrain/Platforms are selected randomly from a list, and then added into a list where I will sequence them one after another. They are added in consecutively based on a random value from a range Example: I would spawn (Random selected)3 sections of ground (Random) terrain then spawn (Random)5 sections of a cave section. This was trivial.

The hard part is that I need to spawn an event terrain/section at random points. Similar to the normal terrain I will generate in consecutively. However I have specific ranges where I want to spawn them events. Example: Between platforms #20-#30, spawn the event terrain. This section of terrain needs to be spawned several times in the sequence and cannot split up a series of normal terrain. my approach is to pick an INT from a list of ranges, then when I reach that int count I will spawn that series of terrain. I actually have most of this logic already in place. The issue is that I feel like my approach is a messy and could use some restructure. Below is an image of what I am trying to accomplish and the commented code of the script I am running. Please let me know if I need to clarify on anything.

Quick Diagram of what I am trying to accomplish

What I previously had working

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

public class NumberGenerator : MonoBehaviour
{
    // Using INTs to create s
    public List<int> Sequence = new List<int> ();

    // Terrain is class I need to spawn for the platforms. For now I am using ints though
    public List<Terrain> TerrainSequence = new List<Terrain> ();
    // This is the list of Terrains I will spawn, example; Ground, Water, Cave, Tunnel, ect.
    public List<Terrain> TerrainList = new List<Terrain> ();
    // Seperate Terrain class for Events used for something like a chase or boss fight
    public Terrain EventTerrain;
    // Total Number of Terrain I want to generate.
    public int GenerateCount;
    // MinMaxInt is a simple class I made which hold two int for a minimum and maximum range. I can also select a random int from this range
    // in this case I need create several ranges to for Consecutive terrain generation.
    public MinMaxInt MinMaxConsecutiveNormalTerrain;
    public MinMaxInt MinMaxConsecutiveEventTerrain;
    int NormalTerrainCount;
    int CurrentEventPlatform;
    // a list of ranges where I will randomly generate a int to assign to eventpoint. An example would be min=20 and max25,
    public List <MinMaxInt> EventPointsRange = new List <MinMaxInt> ();
    //List of ints I will need generate for
    public List <int> EventPoint = new List <int> ();
    // Use this for initialization
    void Start ()
    {
        GenerateSequence ();
    }


    public void GenerateSequence ()
    {
        //Adding 0 which is the intro/Start Terrain
        Sequence.Add (10);
        //Need to create Random Event Points before adding normal Terrain
        GenerateEventPoints ();

        while (Sequence.Count < GenerateCount) {
            // Once we reach the Event Point we will generate the Event Terrain, else just add the regular terrain
            if (Sequence.Count == EventPoint [CurrentEventPlatform]) {
                AddEventPoints ();
            } else
                GenerateNormalPlatforms ();
        }
    }

    //Creating Points based off a list of ranges. Ex. Range is 20-25
    public void GenerateEventPoints ()
    {
        for (int i = 0; i < EventPointsRange.Count; i++) {
            int RandomChasePoint = EventPointsRange [i].random;
            EventPoint.Add (RandomChasePoint);
        }
    }

    // Main function to add in Terrain Event.
    public void AddEventPoints ()
    {
        int RandomEventPointCount = MinMaxConsecutiveEventTerrain.random;
        for (int i = 0; i < RandomEventPointCount; i++) {
            //Adding in Sequence int 
            Sequence.Add (100);
            //This is the Terrain Event Class I am adding, but simply using Int for right now.
            //TerrainSequence.Add (EventTerrain);
        }
        //incrementing CurrentPlatform by one to work on next one. 
        CurrentEventPlatform++;
    }


    //Generates the Series of platforms based on the random random.
    public void GenerateNormalPlatforms ()
    {
        // Generating random number from list of ints that represent the diffrent types of terrain I and trying to spawn. 
        int RandomPlatformType = Random.Range (0, TerrainList.Count);
        // I need to get the Distance/Value to the next Sequence Event. 
        int DistanceFromEventPoint = EventPoint [CurrentEventPlatform] - Sequence.Count;

        // If the Current Sequence count Less than the Maximum generation count THen just fill up the rest of the rest of the slots before the event. 
        // This is important because I need to spawn transition in the event with out breaking up the list.
        if (DistanceFromEventPoint <= MinMaxConsecutiveNormalTerrain.max) {
            NormalTerrainCount = DistanceFromEventPoint;
            print ("Current sequence count it " + Sequence.Count + ", Adding " + NormalTerrainCount + " to fill in gap between upcoming event");
        } else {
            NormalTerrainCount = MinMaxConsecutiveNormalTerrain.random;
        }

        for (int i = 0; i < NormalTerrainCount; i++) {
            Sequence.Add (RandomPlatformType);
            //TerrainSequence.Add (TerrainList [RandomPlatformType]);
        }
    }

}
\$\endgroup\$
2
\$\begingroup\$

Naming

Your NumberGenerator is actually a TerrainGenerator.


The convention for member variable names in C# is _camelCase with leading underscore. That is, when they are private. Member variables should never be public. If you need to provide access to them from the outside, consider adding a public property with a backing private field.

Private variables are by convention spelled in camelCase, e.g. randomChasePoint instead of RandomChasePoint.


Unfortunately you did not add the code of the MinMaxInt class, but it probably should be called something like Range, or IntRange. Also, it probably should be a struct rather than a class.


A variable's type should not be part of its name. For example List<Terrain> TerrainList could be named List<Terrain> Terrains. This is more intuitive, because a list of terrains contains terrains, and also you could more easily change the variable's type if needed without having to rename it.

On a related note, variables whose types are collections, like lists and arrays etc., should have plural names, because they (at least potentially) contain multiple objects of that type. E.g. List <int> EventPoint would be List<int> EventPoints.

Access modifiers

All of the methods and most of the fields are public. From what I can see in the code supplied, most of both could be private instead. Try to keep as much as possible private, and only expose methods that should actually be able to be called from other classes.

Comments

The code contains quite a lot of comments, almost one comment per line of code. While it is good practice to add a comment now and then to explain the purpose of a complicated piece of code, it is usually counter productive to add redundant comments that only explain what the code itself already says. When you think of adding a comment, try to change the code instead to make it more self explanatory if possible.

A strong example why redundant comments are not only superfluos, but possible very bad:

//Adding 0 which is the intro/Start Terrain
Sequence.Add (10);

When reading this, it is impossible to tell whether the code is wrong or the comment is wrong. Probably both were correct at some point, but then one has been changed while the other has not, which happens very often. To prevent that problem in the above case, you could add a constant const int introTerrain = 0; and then do Sequence.Add(introTerrain);. If later you would have changed the constant to 10, the code could stay the same and the comment would not be needed, as the code would explain itself.

For-Each instead of For

When you iterate over a collection and you don't need to use the iteration counter, you can use a foreach loop rather than a for loop. This often makes the code more readable and easier to understand, and it is more idiomatic.

This

    for (int i = 0; i < EventPointsRange.Count; i++) {
        int RandomChasePoint = EventPointsRange [i].random;
        EventPoint.Add (RandomChasePoint);
    }

could be written like this:

    foreach (var eventPointsRange in EventPointsRanges) {
        int randomChasePoint = eventPointsRange.random;
        EventPoint.Add(randomChasePoint);
    }
\$\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.