Simple dungeon generator

Question

I made a simple dungeon generator in C# using MonoGame that works by placing rooms in a grid and connecting them via "tunnels". I would like feedback on how this could be improved in terms of efficiency and code style. Also, please ignore the Render() function in DungeonGenerator.cs as it is only temporary.

Here is one example of a generated dungeon:

SimpleRoomGenerator.cs

using System;

namespace Roguelike
{
    public class SimpleRoomGenerator : DungeonGenerator
    {
        private struct Room
        {
            public const int MinWidth = 5;
            public const int MinHeight = 5;
            public const int MaxHeight = 9;
            public const int MaxWidth = 9;

            public int XPos { get; set; }
            public int YPos { get; set; }
            public int Width { get; set; }
            public int Height { get; set; }
            public int CentreX { get { return XPos + (Width / 2); } }
            public int CentreY { get { return YPos + (Height / 2); } }
        }

        public const int NumRooms = 10;

        public SimpleRoomGenerator(int width, int height) : base(width, height)
        {

        }

        public override void Generate()
        {
            dungeon = new int[width, height];

            Room previousRoom = new Room();

            for (int i = 0; i < NumRooms; i++)
            {
                Room room = new Room();
                room.XPos = random.Next(0, width - Room.MaxWidth);
                room.YPos = random.Next(0, height - Room.MaxHeight);
                room.Width = random.Next(Room.MinWidth, Room.MaxWidth);
                room.Height = random.Next(Room.MinHeight, Room.MaxHeight);

                for (int y = room.YPos; y < room.YPos + room.Height; y++)
                {
                    for (int x = room.XPos; x < room.XPos + room.Width; x++)
                    {
                        dungeon[x, y] = 1;
                    }
                }

                if (i > 0)
                {
                    int startX = Math.Min(room.CentreX, previousRoom.CentreX);
                    int startY = Math.Min(room.CentreY, previousRoom.CentreY);
                    int endX = Math.Max(room.CentreX, previousRoom.CentreX);
                    int endY = Math.Max(room.CentreY, previousRoom.CentreY);

                    if (random.Next(1) == 0)
                    {
                        for (int x = startX; x < endX; x++)
                            dungeon[x, previousRoom.CentreY] = 1;

                        for (int y = startY; y < endY + 1; y++)
                            dungeon[room.CentreX, y] = 1;
                    }
                    else
                    {
                        for (int y = startY; y < endY + 1; y++)
                            dungeon[previousRoom.CentreX, y] = 1;

                        for (int x = startX; x < endX; x++)
                            dungeon[x, room.CentreY] = 1;
                    }
                }

                previousRoom = room;
            }
        }

        public override void Update()
        {

        }
    }
}

DungeonGenerator.cs

using Microsoft.Xna.Framework;
using Microsoft.Xna.Framework.Content;
using Microsoft.Xna.Framework.Graphics;
using System;

namespace Roguelike
{
    public abstract class DungeonGenerator
    {
        public const int TileWidth = 8;
        public const int TileHeight = 8;

        protected Random random;
        protected int[,] dungeon;
        protected int width;
        protected int height;

        public DungeonGenerator(int width, int height)
        {
            this.width = width;
            this.height = height;
            random = new Random();
            dungeon = new int[width, height];
        }

        public abstract void Generate();
        public abstract void Update();

        public void Render(SpriteBatch spriteBatch, ContentManager content)
        {
            for (int y = 0; y < height; y++)
            {
                for (int x = 0; x < width; x++)
                {
                    if (dungeon[x, y] != 0)
                        spriteBatch.Draw(content.Load<Texture2D>(dungeon[x, y].ToString()), new Rectangle(x * TileWidth, y * TileHeight, TileWidth, TileHeight), Color.White);
                }
            }
        }
    }
}

Answer 1

Design, description and documentation

protected int[,] dungeon; - What is the meaning of the values? So far it looks like 0 = wall (cannot pass), 1 = room or tunnel (can pass). Would bool work? How about references to rooms, tunnels and whatever object representing bigger structure? Null would mean wall / place you cannot enter. Could be useful (e.g. for avoiding tunnels next to room enlarging it), but I don't know for the lack of description and bigger picture.

In either case, documentation/description of such things should be part of your code. You yourself may one day be deciphering it as I am doing now (and believe me, I sometimes wonder about my own code I wrote 10 years ago).

SimpleRoomGenerator.Generate is another place you should comment. I got it that the logic randomly decides to connect currently created room to previously either horizontally or vertically (chosen randomly), but such description should be there in comment.

Is piercing other rooms and tunnels with newly created desired or flaw of current implementation? In either case document it, maybe with //TODO: prevent collisions with existing rooms and tunnels, maybe create tunnel junctions.

new Random()

This is very dangerous, because these are seeded by timestamp and can therefore start with same seed if you create many of these at once. So, you better either get it as part of construction: public DungeonGenerator(int width, int height, Random rand) or add static method to generate these, probably both.

static Random seedRandom = new Random();
static Random CreateRandom() { return new Random(seedRandom.Next()); }
public DungeonGenerator(int width, int height, Random rand = null) {
    this.random = rand ?? CreateRandom();

The default seed value is derived from the system clock and has finite resolution. As a result, different Random objects that are created in close succession by a call to the default constructor will have identical default seed values and, therefore, will produce identical sets of random numbers. This problem can be avoided by using a single Random object to generate all random numbers. You can also work around it by modifying the seed value returned by the system clock and then explicitly providing this new seed value to the Random(Int32) constructor. For more information, see the Random(Int32) constructor.

You may later decide to add unit tests and for that you would probably want stable pseudo-random that you can repeat the test with same results. You can simply modify seedRandom = new Random() or CreateRandom (and that is the reason I made it a function / static method and not static variable or property). You can also choose to use single Random for them all just by modifying the CreateRandom() => seedRandom; Up to you and your needs, the static method is single place you have to modify.

Answer 2

I would make the random generator static and public to make it accessible from various classes (e.g. from rooms):

public abstract class DungeonGenerator
{
    public static readonly  Random Random = new Random();
    ...
}

Then I would generalize the concept of Room and make it a Shape that can write (Fill()) itself into the dungeon.

public class Shape
{
    protected int[,] dungeon;

    public Shape(int[,] dungeon, int xPos, int yPos, int width, int height)
    {
        this.dungeon = dungeon;
        XPos = xPos;
        YPos = yPos;
        Width = width;
        Height = height;
    }

    public int XPos { get; }
    public int YPos { get; }
    public int Width { get; }
    public int Height { get; }
    public int CentreX { get { return XPos + (Width / 2); } }
    public int CentreY { get { return YPos + (Height / 2); } }

    public void Fill()
    {
        for (int y = YPos; y < YPos + Height; y++) {
            for (int x = XPos; x < XPos + Width; x++) {
                dungeon[x, y] = 1;
            }
        }
    }
}

Lines can now also be formulated as Shapes.

public class HorizontalLine : Shape
{
    public HorizontalLine(int[,] dungeon, int startX, int endX, int yPos)
        : base(dungeon, startX, yPos, endX - startX + 1, 1)
    {
    }
}

public class VerticalLine : Shape
{
    public VerticalLine(int[,] dungeon, int xPos, int startY, int endY)
        : base(dungeon, xPos, startY, 1, endY - startY + 1)
    {
    }
}

The room itself that I renamed to allow adding more room types in future (it can be a nested class in SimpleRoomGenerator or a standalone class):

private class SmalllRandomRoom : Shape
{
    public const int MinWidth = 5;
    public const int MinHeight = 5;
    public const int MaxHeight = 9;
    public const int MaxWidth = 9;

    public SmalllRandomRoom(int[,] dungeon)
        : base(dungeon,
                DungeonGenerator.Random.Next(0, dungeon.GetLength(0) - MaxWidth),
                DungeonGenerator.Random.Next(0, dungeon.GetLength(1) - MaxHeight),
                DungeonGenerator.Random.Next(MinWidth, MaxWidth),
                DungeonGenerator.Random.Next(MinHeight, MaxHeight))
    {
    }
}

The generator is now more focused on the generating algorithm and the naming of the line types makes visible what you had to discover by dissecting the for statements.

public override void Generate()
{
    dungeon = new int[width, height];
    SmalllRandomRoom previousRoom = null;

    for (int i = 0; i < NumRooms; i++) {
        var room = new SmalllRandomRoom(dungeon);
        room.Fill();
        if (previousRoom != null) {
            int startX = Math.Min(room.CentreX, previousRoom.CentreX);
            int startY = Math.Min(room.CentreY, previousRoom.CentreY);
            int endX = Math.Max(room.CentreX, previousRoom.CentreX);
            int endY = Math.Max(room.CentreY, previousRoom.CentreY);

            HorizontalLine hLine;
            VerticalLine vLine;
            if (DungeonGenerator.Random.Next(1) == 0) {
                hLine = new HorizontalLine(dungeon, startX, endX, previousRoom.CentreY);
                vLine = new VerticalLine(dungeon, room.CentreX, startY, endY);
            } else {
                vLine = new VerticalLine(dungeon, previousRoom.CentreX, startY, endY);
                hLine = new HorizontalLine(dungeon, startX, endX, room.CentreY);
            }
            hLine.Fill();
            vLine.Fill();
        }

        previousRoom = room;
    }
}

---

There is one detail that could become a problem if we create bigger rooms. We calculate the room position dependent on the maximum room size instead of its actual size. We should change that.

public SmalllRandomRoom(int[,] dungeon)
    : base(dungeon)
{
    Width = DungeonGenerator.Random.Next(MinWidth, MaxWidth);
    Height = DungeonGenerator.Random.Next(MinHeight, MaxHeight);
    XPos = DungeonGenerator.Random.Next(0, dungeon.GetLength(0) - Width); // Actual width!
    YPos = DungeonGenerator.Random.Next(0, dungeon.GetLength(1) - Height); // Actual height!
}

But this requires a base constructor having only the dungeon as parameter and we must be able to assign the position and size properties from the derived class. Therefore we change the Shape class as follows:

public class Shape
{
    protected int[,] dungeon;

    public Shape(int[,] dungeon)
    {
        this.dungeon = dungeon;
    }

    public Shape(int[,] dungeon, int xPos, int yPos, int width, int height)
        : this(dungeon)
    {
        XPos = xPos;
        YPos = yPos;
        Width = width;
        Height = height;
    }

    public int XPos { get; protected set; }
    public int YPos { get; protected set; }
    public int Width { get; protected set; }
    public int Height { get; protected set; }

    ...
}