7
\$\begingroup\$

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:A simple 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);
                }
            }
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Accepted. I will definitely add documentation in the near future and having a static random object was something I forgot about. Also, the reason why the dungeon array uses integers is because different numbers represents different tile IDs that were going to be used to load different textures (0 meaning there is no tile). Using the numbers as references to larger structure could be a good way of keeping generation neat as well. \$\endgroup\$ – Ioan Thomas Aug 29 '18 at 11:01
  • \$\begingroup\$ Now it makes more sense. You can still use integers or rather enum if these can distinguish between room and tunnel (or whatever) or have the tile index in each bigger structure that you can reference in the array or maybe have an array of both tile indexes and objects, if you plan different tiles for e.g. edges than the center. Sometimes you just have to look ahead to select properly, good design can save you a lot of time. \$\endgroup\$ – user52292 Aug 29 '18 at 12:37
0
\$\begingroup\$

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; }

    ...
}
\$\endgroup\$
  • 2
    \$\begingroup\$ There are a couple of things I do not like about your suggestions, hence the down-vote. 1) I don't like static dependency on random generator, as it hinders OP's ability to write unit tests. 2) I don't like that rooms now have a dependency on dungeon grid. IMHO a better approach is to pass dungeon as parameter to Fill method and remove the field. 3) I don't think that rooms should generate themselves, it sound like a violation of SRP to me. I think that separate component should be responsible for building the dungeon, and rooms should not know or care, whether they were autogenerated. \$\endgroup\$ – Nikita B Jul 30 '18 at 9:43
  • 1
    \$\begingroup\$ It is best practice to create a single instance of Random and use it throughout your program - otherwise the results may not be as random. Making the random number generator static is just the simplest way to make it unique; however, for better testability you could inject a Random object into the generator. This would allow you to choose between reproducible random sequences or sequences with a random seed. \$\endgroup\$ – Olivier Jacot-Descombes Jul 30 '18 at 12:23
  • 2
    \$\begingroup\$ yes, I completely agree that having single random generator helps tremendously. I just don't like the idea of accessing it via static reference. Injecting it into dungeon generator directly sounds good though. :) Later on OP can even hide the concrete implementation of random number generator behind an interface, so he can then build test cases that use a sequence of specific non-random edge-case values as a source of "randomness" instead of Random class. \$\endgroup\$ – Nikita B Jul 30 '18 at 13:10

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.