Problem
I decided to make my own dungeon generator, after reading a bit about it. This is my first effort. I can only say that it works, but I believe there’s room for some improvement, because I believe my self-taught coding skills are basic.
To use this code, call:
dungeonGenerator = new DungeonGenerator();
dungeonGenerator.CreateDungeonRoom(115, 32);
dungeonGenerator.CellsToGenerate = 1500;
dungeonGenerator.CreateDungeonScenery();
dungeonGenerator.LogDungeon();
Tile.cs
using System;
using System.Collections.Generic;
using System.Text;
namespace DungeonGenerator
{
struct Cell
{
/// <summary>
/// Location in the grid
/// </summary>
public CellType CellType;
public Cell(CellType cellType)
{
CellType = cellType;
}
public override string ToString()
{
return "Type: " + CellType;
}
}
}
CellType.cs
/// <summary>
/// If the cell is walkable or not
/// </summary>
public enum CellType
{
WALL, GROUND, START
}
Direction.cs
enum Direction
{
UP, DOWN, LEFT, RIGHT
}
DungeonGenerator.cs
using System;
namespace DungeonGenerator
{
public class DungeonGenerator
{
public int Width { get; set; }
public int Height { get; set; }
public int CellsToGenerate { get; set; }
Cell[,] DungeonCells { get; set; }
public void CreateDungeonRoom(int RoomWidth, int RoomHeight)
{
Width = RoomWidth;
Height = RoomHeight;
DungeonCells = new Cell[Width,Height];
for (int y = 0; y < Height; y++)
{
for (int x = 0; x < Width; x++)
{
CellType cellType = CellType.WALL;
DungeonCells[x, y] = new Cell(cellType);
}
}
}
/// <summary>
/// Populate the insides of the room
/// </summary>
public void CreateDungeonScenery()
{
//Make it so that the outside walls are untouched
int minValueXY = 1;
int maxValueX = Width - 2;
int maxValueY = Height - 2;
//Choose a random position in the room
Random random = new Random();
int startX = random.Next(minValueXY, maxValueX);
int startY = random.Next(minValueXY, maxValueY);
//Mark it as the starting position, for player placement, maybe?
DungeonCells[startX, startY] = new Cell(CellType.START);
//Get directions to an array for random choosing
Array values = Enum.GetValues(typeof(Direction));
//From the starting position, proceed to create X number of ground cells
int cellCount = 0;
while (cellCount < CellsToGenerate)
{
//Choose a direction at random
Direction direction = (Direction)values.GetValue(random.Next(values.Length));
if (direction == Direction.UP)
{
startY -= 1;
if (startY < minValueXY) { startY = minValueXY; }
}
else if (direction == Direction.DOWN)
{
if (startY < maxValueY) { startY += 1; }
}
else if (direction == Direction.LEFT)
{
startX -= 1;
if (startX < minValueXY) { startX = minValueXY; }
}
else if (direction == Direction.RIGHT)
{
if (startX < maxValueX) { startX += 1; }
}
//From the position chosen, mark it as ground, if possible
if (CreateGround(startX, startY))
{
//Mark the cell as ground
DungeonCells[startX, startY].CellType = CellType.GROUND;
//Add one to cells created
cellCount++;
}
}
}
private bool CreateGround(int startX, int startY)
{
//There's not a wall there, so there's nothing to be done here
if (DungeonCells[startX, startY].CellType != CellType.WALL)
{
return false;
}
return true;
}
public void LogDungeon()
{
Console.Clear();
for (int y = 0; y < Height; y++)
{
string line = "";
for (int x = 0; x < Width; x++)
{
if (DungeonCells[x, y].CellType == CellType.GROUND)
{
line += "O";
}
else if (DungeonCells[x, y].CellType == CellType.WALL)
{
line += "█";
}
else if (DungeonCells[x, y].CellType == CellType.START)
{
line += "H";
}
}
Console.WriteLine(line);
}
}
}
}
From the start, I realize I can improve the code:
By preventing backtracking, but backtracking sometimes makes some nice crevices here and there. Resolve this by making impossible to select the direct opposite of the previous direction?
I want to figure out a way to improve that direction choosing part (currently I don’t know how).
On purpose, I left the number of cells to generate out of the CreateDungeonRoom() method because I’m still deciding if I really want to make users insert the number of ground tiles as a specific number or a percentage, or enum.
We will see.
Here is an example of a dungeon with 115 width, 32 height and 1500 ground tiles and the player starting position (it’s the H somewhere)
Solution
Clean Your Code
- Convert
if-elseladder toswitch. Switch is much faster. startY -=tostartY--- Same goes for
LogDungeon()method
Before
if (direction == Direction.UP)
{
startY -= 1;
if (startY < minValueXY) { startY = minValueXY; }
}
else if (direction == Direction.DOWN)
{
if (startY < maxValueY) { startY += 1; }
}
else if (direction == Direction.LEFT)
{
startX -= 1;
if (startX < minValueXY) { startX = minValueXY; }
}
else if (direction == Direction.RIGHT)
{
if (startX < maxValueX) { startX += 1; }
}
After
switch (direction)
{
case Direction.UP:
startY--;
if (startY < minValueXY)
startY = minValueXY;
break;
case Direction.DOWN:
if (startY < maxValueY)
startY++;
break;
case Direction.LEFT:
startX--;
if (startX < minValueXY)
startX = minValueXY;
break;
case Direction.RIGHT:
if (startX < maxValueX)
startX++;
break;
}
Simplify Methods & Conditions
Before
private bool CreateGround(int startX, int startY)
{
//There's not a wall there, so there's nothing to be done here
if (DungeonCells[startX, startY].CellType != CellType.WALL)
{
return false;
}
return true;
}
After
private bool CreateGround(int startX, int startY) =>
DungeonCells[startX, startY].CellType == CellType.WALL;
Observations
Place the following code outside of while loop if you don’t really need it inside the loop.
int cellCount = 0;
while (cellCount < CellsToGenerate)
{
//Choose a direction at random
Direction direction = (Direction)values.GetValue(random.Next(values.Length))
switch (direction)
{
case Direction.UP:
startY--;
if (startY < minValueXY)
startY = minValueXY;
break;
case Direction.DOWN:
if (startY < maxValueY)
startY++;
break;
case Direction.LEFT:
startX--;
if (startX < minValueXY)
startX = minValueXY;
break;
case Direction.RIGHT:
if (startX < maxValueX)
startX++;
break;
}
}
Suggestions
- Please avoid using global variable/properties inside a class. Instead use parameters to pass values inside method and make use of return type. If you need to return multiple values, create a new type and return it. This will help avoid confusion.
- Use correct naming guidelines. You have some incorrect naming conventions in the
CreateDungeonRoommethod. Parameter names should always be in camelCase.
Hope this helps
Naming
DungeonGenerator, to me, would mean that it is a class that generates a Dungeon. But the way you use it, DungeonGenerator is itself, a Dungeon. I would drop the “Generator” part of the name, or refactor it so it is in-fact a dungeon generator.
Additionally you have a DungeonGenerator namespace and a DungeonGenerator class within that namespace. I would avoid having a class the same name as its containing namespace. Otherwise you need to write confusing things like:
var x = new DungeonGenerator.DungeonGenerator();
Avoid Mutable Properties
Your DungeonGenerator has a few externally mutable properties that I don’t see a reason for them to be mutable. It would be better if these weren’t mutable:
public class DungeonGenerator
{
public int Width { get; private set; }
public int Height { get; private set; }
public int CellsToGenerate { get; private set; }
private Cell[,] DungeonCells;
public void CreateDungeonRoom(int RoomWidth, int RoomHeight) { ... }
/// <summary>
/// Populate the insides of the room
/// </summary>
public void CreateDungeonScenery() { ... }
private bool CreateGround(int startX, int startY) { ...}
public void LogDungeon(){ ... }
}
Additionally, DungeonCells doesn’t really need to be a property at all, so just make it a private field.
Consistent Documentation
You documented the CreateDungeonScenery method, but nothing else that is public. You should be “all-in” with documentation on methods, and make it descriptive (don’t just re-word the method name). If the method has side-effects, mention them in the <remarks></remarks> section.
Consistent Method Parameter Names
You got the names in CreateGround as camelCased but you have the names in CreateDungeonRoom as PascalCased. According to the .NET Naming Guidelines you should be using PascalCased for method/property names and camelCased for method parameters.
Code Use
I don’t see any reason why you would want to create a new DungeonGenerator and not run CreateDungeonRoom, so why not make that into the constructor?
public class DungeonGenerator
{
public int Width { get; private set; }
public int Height { get; private set; }
public int CellsToGenerate { get; private set; }
private Cell[,] dungeonCells;
public void DungeonGenerator(int roomWidth, int roomHeight, int cellsToGenerate) { ... }
/// <summary>
/// Populate the insides of the room
/// </summary>
public void CreateDungeonScenery() { ... }
private bool CreateGround(int startX, int startY) { ...}
public void LogDungeon(){ ... }
}
Additionally the constructor can call the CreateDungeonScenery method as well. So now instead of the original way, you can consolidate code:
var dungeon = new DungeonGenerator(115, 32, 1500);
Method Name
The LogDungeon method name is confusing, because the word “Log” to most developers means some kind of diagnostic routine that writes data to a file or console purely for debugging purposes. You are using it to take over the console (by clearing it) and output the text graphic of the dungeon to the console. I think a better name would be something like FlushToConsole or something like that.
However I think you shouldn’t be having Console methods anywhere in this class, let the user decide what to do with it. If a class has a valid string representation, you should be overriding the ToString() method instead, so LogDungeon becomes ToString():
public override void ToString()
{
StringBuilder dungeon = new StringBuilder();
for (int y = 0; y < Height; y++)
{
StringBuilder line = new StringBuilder();
for (int x = 0; x < Width; x++)
{
switch (DungeonCell[x, y].CellType)
{
case CellType.GROUND:
line.Append("O"); break;
case CellType.WALL:
line.Append("█"); break;
case CellType.START:
line.Append("H"); break;
}
}
dungeon.AppendLine(line.ToString());
}
return dungeon.ToString();
}
Now the user of your class can decide to do something else with it, such as write it to a file, send it over the network, print it on a webpage, etc. The user is no longer tied to the console.
Some general comments about how your code “reads”:
Usage
You wrote: “To use this code, call:”
dungeonGenerator = new DungeonGenerator();
dungeonGenerator.CreateDungeonRoom(115, 32);
dungeonGenerator.CellsToGenerate = 1500;
dungeonGenerator.CreateDungeonScenery();
dungeonGenerator.LogDungeon();
First, let me say that I support the idea that the dungeon generator and the dungeon object should be different. The code to randomly create rooms has nothing to do with the code that decides whether a user has stepped on a trap, etc. So I like the idea of a separate class/object/function.
That said, this is a very awkward set of calls. It doesn’t “scan” well (to use a term from literature). I suspect that all these fine details need to be wrapped up into a single factory function that a user can call:
cavern = Dungeon.Generate(width: 115, height: 32, min_cells: 1500);
cavern.WriteTo(Console.Out);
CellType
Your cell type is an enumeration, which seems obvious but actually isn’t very useful. The thing you mainly do with the cell type is display it, and sadly C# enums cannot have char as their underlying type. But you can work around that using an int and just assigning char values:
public enum CellType
{
WALL = '█',
GROUND = 'O',
START = 'H'
};
Just be careful about what size characters you use (16 bit only). If you have to use a full-out string, consider the solution in this answer.
I suggest this because your output code would benefit from having “concrete” values. (The rest of your code is happy with the CellType being an abstract enum.)
switch (DungeonCell[x, y].CellType)
{
case CellType.GROUND:
line.Append("O"); break;
Note: As a rule of thumb, if you find yourself executing a switch on internal data it’s a good indicator that you should consider a new type. Switching on external data might be a correct way to deal with user input. But switching on internal data frequently means “these things should be different objects, or different classes”. In this case, the enum values are serving as proxies for objects that have their own output string representation.
It would be better, IMO, to simply emit the value, or a method of the value (as in the linked answer):
line.Append(DungeonCell[x, y].cellType);
// -or-
line.Append(DungeonCell[x, y].cellType.ToFriendlyString());
(but see below about that x, y thing!)
Direction
Direction is another enum that doesn’t quite do what you need. Currently, you write code like this (code modified for size):
//Choose a direction at random
Direction direction = (Direction)values.GetValue(random.Next(values.Length));
if (direction == Direction.UP)
if (startY > minValueXY)
--startY;
else if (direction == Direction.DOWN)
if (startY < maxValueY)
++startY;
else if (direction == Direction.LEFT)
if (startX > minValueXY)
--startX;
else if (direction == Direction.RIGHT)
if (startX < maxValueX)
++startX;
//From the position chosen, mark it as ground, if possible
Instead of making Direction an enum, what if you make it a class? You can then have Direction.AtRandom() return what you want without having to cast it.
What’s more, consider what you are doing: you immediately decode the direction into an X or a Y offset. You then either adjust the variable startX or startY.
Why are those two variables different? What is startX? What is startY? Aren’t they part of a greater whole, called Position (or Location or Coords something)?
If you had a Position type, you could just have start instead of startX and startY. You might have to adjust start.X and start.Y, but probably not, because Direction should be a vector.
If Direction.North is a Vector defined as { δx = 0, δy = -1 } then you can define addition of a Position and a Vector in the obvious way (x+δx, y+δy) and then simplify your code:
public static Position operator +(Position pos, Vector v)
{
return new Position(pos.x + v.δx, pos.y + v.δy);
}
Then:
// Choose a direction at random (but don't go out-of-bounds)
dir = Direction.AtRandom();
if (dungeon.Contains(start + dir))
start += dir;
// From the position chosen, mark it as ground, if possible
Note: .Contains may be the wrong name, since there is that implicit rule
about not modifying the outer edges. Maybe “CanBeRoom()”?
Position
If you define an indexer for your Dungeon, you can use the Position directly instead of reaching in for the x and y values. This will cost some performance, but that likely doesn’t matter during generation. Alternatively, you might just write a Dungeon.At(Position) method.
Dungeon Rules
I saw one “implicit” rule in your code: the dungeon’s outside walls are inviolable. I’d suggest that you make those rules explicit, and encode the operations you are currently implementing with just a paragraph or two of code:
//Make it so that the outside walls are untouched
int minValueXY = 1;
int maxValueX = Width - 2;
int maxValueY = Height - 2;
//Choose a random position in the room
Random random = new Random();
int startX = random.Next(minValueXY, maxValueX);
int startY = random.Next(minValueXY, maxValueY);
This can be replaced by defining methods:
topLeft = dungeon.PlayerTopLeft(); // returns a Position, (1,1)?
botRight = dungeon.PlayerBottomRight(); // returns a Position.
start = Position.AtRandom(min: topLeft, max: botRight);
Iterators
I’d suggest writing at least one position iterator for the Dungeon. Something like Dungeon.PositionsInViewOrder(), that would yield the cell positions in the right order for display.
(Note: if Cell objects knew their own Position, I’d suggest just writing a Cell iterator and skipping the Position. But as things are, this is the way to go.)
You could rewrite your LogDungeon function as a Dungeon method:
public void WriteTo(System.IO.TextWriter out)
{
// Only care about Y
int last_pos = TopLeft() + Vector(δx:0, δy:-1);
for (Position pos in PositionsInViewOrder())
{
if (last_pos.y != pos.y)
out.WriteLine();
last_pos = pos;
out.Write(dungeon[pos].cellType);
}
out.WriteLine();
}
You might find that many dungeon creation tasks are made easier by defining Position iterators. For example, if you want to randomly generate rooms and hallways you could define Position.WithinRect(topLeft:Position, bottomRight:Position) and Position.AlongLine(from:Position, to:Position). (Be consistent on how you handle the end position: inclusive or exclusive!)