next »

[eyeliner]

Hi guys and girls! How about if we posted some (or all) bits of code we come up with and submit them for peer review?
It won't hurt, promise, and a second opinion is always good to have.

So, here's my code for those of you who might want do something different and help a fellow out.
Wanted:
I'm looking to see if there's some good practices to follow that I haven't, some performance improvements you might believe are possible and all around messups.

Description:
After reading a bit about dungeon generation, I decided to make mine. It's based on random/drunken walking, and works by setting the room width and height and the number of cells to create. After that, a random position is selected and it starts from there, carving holes, adding the counter. If a tile that was marked as ground, it is not added to the counter, and the algo proceeds to search.

So, here it is:
Cell.cs

Code:

    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

Code:

    public enum CellType
    {
        WALL, GROUND, START
    }

Direction.cs

Code:

enum Direction
{
    UP, DOWN, LEFT, RIGHT
}

DungeonGenerator.cs

Code:

        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;
                    }
                    //There is a wall, let's mark it as ground
                    //DungeonCells[startX, startY].CellType = CellType.GROUND;
                    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);
                    }
                }

            }
        }

What say you?
Have something to share or review?Have something to say about mine? I'd really appreciate it.
Thanks.

[bateleur]

This code basically looks fine to me (I have a stylistic preference for switch over else if chains, but I don't think it matters). The one line I'd maybe tweak a bit is this:

Code:

Direction direction = (Direction)values.GetValue(random.Next(values.Length));

The random selection is a core part of your algorithm and whilst I have no doubt the above works absolutely fine, there are three things which strike me:

* Using "values.Length" in that way and then casting back to a Direction is a dirty hack. Having made the decision to be elegant and use and enum instead of ints, don't then treat them as ints!
* You probably want to split out your random selection into a separate function anyway, because if this code is used in a larger project you're likely to want a way to deterministically reproduce the same programmatic layouts, which is much easier to do if you control the random number generation. (For now the function can just be a thin wrapper to the same approach.)
* This is C#, but it's unclear whether you're using Unity or not. If you are, you should probably opt for UnityEngine.Random over System.Random. (I know you're using the latter, because the former doesn't have Random.Next().)

These are all very minor things, though!

For me, the real interest of code like this isn't in the coding style so much as the algorithm. These algorithms are very much a matter of taste, but the one you've picked has a couple of properties I'm not too fond of. First, it doesn't scale well to large rooms. The maths are hard to do, but there's a real possibility of carving out a large empty patch and then struggling to reach more non-floor cells in a reasonable time. Second, the shapes made by a random walk will tend to head off in one direction like wiggly corridors, which doesn't really end up looking like either natural caves or architecture.

[whistlerat]

Having made the decision to be elegant and use and enum instead of ints, don't then treat them as ints!

How would you generate a random enum from all possible enums of a type? Just asking because I'm pretty sure I've used that dirty hack myself

[bateleur]

How would you generate a random enum from all possible enums of a type? Just asking because I'm pretty sure I've used that dirty hack myself

There is no nice way.

But OK, here are the actual problems so that you can think about how one ought to approach it in general:

1) Enums do not, in general, involve a continuous sequence of values starting from zero. As such, if you write code which assumes they do you're asking for trouble later when you innocently amend the Enum itself.
2) Further to the above, getValues isn't guaranteed to return a distinct value for each element of an enum. Calling GetName on the resulting values isn't even guaranteed to work (because in the presence of duplicated values it's impossible).

In this case eyeliner's code isn't bad, because the Enum is right there and we can see it leaves values to be autoassigned, which will have the properties required. But if one wanted to be more robust then writing a getRandomDir() helper function that generates a random int and then maps case by case (eg. using switch) to a Direction would be the obvious alternative. Other than style pedantry it does have some real advantages. For example, if later in the project a need arises to add a 'None' direction this wouldn't automatically become a possible output from getRandomDir().

(I hope writing all this doesn't make it sound as though I think it's a big deal. It really isn't!)

[eyeliner]

Thanks for even looking!

I just made the whole this from the top of my head by reading articles with barely no code on them, so yeah, it might be a bit messy here and there.

[whistlerat]

Thanks for the clarification, eyeliner, it's appreciated Sounds like it roughly aligns with some of my thoughts on using it, which is reassuring. I'll probably revisit my instances of using enums like this to see if I can improve them.

Good idea for a thread, btw, bateleur

[BorisTheBrave]

My biggest gripe is that Cell doesn't do anything except wrap CellType. This would be fine if you were planning Cell to become larger, but I don't think you are. Why not just use the enum directly, it would be a lot less verbose.

It's also generally best to avoid classes having an uninitialized state, when some methods are illegal to call on them. Easily fixed by making CreateDungeonRoom a constructor.

CreateGround should be CanCreateGround, as it doesn't do any actual creation.

Instead of LogDungeon it would be more natural to have string ToAsciiMap(). Then the caller can decide if they want to log it or deal with it another way.

As you write more code, you'll find writing 4 cases, one per direction gets increasinly tedious. It's best to use or make a Point class with appropriate methods on it, so you can handle all the cases without if statements.

[eyeliner]

Thanks, Boris. Notes taken. Will improve further.

[cykboy]

First issue I have is inconsistent code style, just because it may fit on one line doesnt mean you should break conventions. EG:

Code:

if (direction == Direction.LEFT)
{
  startX -= 1;
  if (startX < minValueXY) { startX = minValueXY; }
}

Should be:

Code:

if (direction == Direction.LEFT)
{
  startX -= 1;
  if (startX < minValueXY)
  {
    startX = minValueXY;
  }
}

But I'd personally use

Code:

if (direction == Direction.LEFT) {
  startX -= 1;
  if (startX < minValueXY) {
    startX = minValueXY;
  }
}

If you care about line usage or w.e, it is arguably cleaner. Second I see is this:

Code:

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;
    }
    //There is a wall, let's mark it as ground
    //DungeonCells[startX, startY].CellType = CellType.GROUND;
    return true;
}

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

Can be refactored into:

Code:

private bool CreateGround(int startX, int startY)
{
    return DungeonCells[startX, startY].CellType == CellType.WALL;
}

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

Then you have to ask yourself, wtf are you doing? If cell is wall then cell is ground? That makes no sense to me. You have essentially made CellType.WALL pointless to have because if a cell is of that type it becomes ground. There is also no need for a function to do this if you really want to. EG:

Code:

//From the position chosen, mark it as ground, if possible
if (DungeonCells[startX, startY].CellType == CellType.WALL)
{
    //Mark the cell as ground
    DungeonCells[startX, startY].CellType = CellType.GROUND;
    //Add one to cells created
    cellCount++;
}

[bateleur]

Then you have to ask yourself, wtf are you doing? If cell is wall then cell is ground? That makes no sense to me. You have essentially made CellType.WALL pointless to have because if a cell is of that type it becomes ground.

Well no, it's not pointless, because the algorithm is carving out ground patterns from the walls. The reason it decks for wall cells is to prevent double counting if it tries to place ground where there already is some.

[cykboy]

The reason it decks for wall cells is to prevent double counting if it tries to place ground where there already is some.

except it places ground where walls exist due to your CreateGround function...

[oahda]

I'm guessing that only happens conditionally, but that the condition was left out of the code that was pasted here? So not all walls invariably turn into ground?

[eyeliner]

The reason it decks for wall cells is to prevent double counting if it tries to place ground where there already is some.

except it places ground where walls exist due to your CreateGround function...

No, it does not.
If passing 500 ground tiles to create, it creates 500, ignoring whatever cells that are already round, and looks for another one.
I concur the code can be improved, as it was just an exercise and produces the expected output.
I even made use of some input and rounded it of. One of these days, I'll put the code up again.

I'm diving in Dart/Flutter now, sorry.

[cykboy]

No, it does not.

As much as you say that, your code says otherwise after refactoring. If I am wrong here please explain why.

Code:

//From the position chosen, mark it as ground, if possible
if (DungeonCells[startX, startY].CellType == CellType.WALL)
{
    //Mark the cell as ground
    DungeonCells[startX, startY].CellType = CellType.GROUND;
    //Add one to cells created
    cellCount++;
}

[eyeliner]

Wait a sec... What is the real problem here?
I mean... The code works as intended. If a tile is a wall tile, mark it as ground. That's the whole logic to it. At first pass,the whole set of tiles are ground, only to be carved.
It's supposed to be that way.

Sorry about my previous answer. I misread. You are correct.