Console RPG - show cycle optimization

Question

I have this RPG game and this has elements such as the map, the monsters and the items. The monsters and the items are stored in Lists and every time I want to show the map around the player, I have to check if the element is within the bounds of the camera.

public void ShowCamera()
    {
        int size = cameraSize/2;
        int topY = player.GetY() - size, bottomY = player.GetY() + size, topX = player.GetX() - size, bottomX = player.GetX() + size;
        int setX = 0, setY = 0;

        //Print Map
        Console.SetCursorPosition(0,0);
        Console.ForegroundColor = ConsoleColor.DarkGreen;
        for (int i = topX; i < bottomX; i++)
        {
            for (int j = topY; j < bottomY; j++)
            {
                Console.ForegroundColor = ConsoleColor.DarkGreen;
                Console.SetCursorPosition(setY,setX);
                Console.Write(map[i, j]);
                if (monstros.Count > 0)
                {
                    Console.ForegroundColor = ConsoleColor.DarkRed;
                    foreach (var elem in monstros)
                    {
                        if (elem.ReX() == i && elem.ReY() == j)
                        {
                            Console.SetCursorPosition(setY, setX);
                            Console.Write(elem);
                        }
                    }
                }
                if (items.Count > 0)
                {
                    Console.ForegroundColor = ConsoleColor.DarkRed;
                    foreach (var elem in items)
                    {
                        if (elem.ReX() == i && elem.ReY() == j)
                        {
                            Console.SetCursorPosition(setY, setX);
                            Console.Write(elem);
                        }
                    }
                }
                setY++;
            }
            Console.Write("\n");
            setY = 0;
            setX++;
        }

So, this works but it is very unoptimized and flickers a lot. I was wondering if there's a way to improve this cycle in any way.

Answer 1

Performance

The main problem seems to be that you're checking each monster and item for each console position. If cameraSize is 80, that means you're checking each monster and item 80 x 80 = 6400 times.

Just draw the map first, and then loop through your monsters and items (once) and draw them if they're within view.

Other notes

You're using a lot of getter methods: GetX(), GetY(), ReX(), ReY(). In C# properties are normally used instead of getter/setter methods.

ReX and ReY are not very descriptive names. Obviously we're dealing with coordinates here, but what does that Re prefix mean?

Since you're dealing with 2D coordinates quite frequently, it could be useful to create a separate Position class for that, with X and Y properties and perhaps some overloaded operators (+, -) and utility methods (int DistanceTo(Position other), and so on).

Calling Console.Write(elem) invokes elem's ToString method. Personally I'd give those monster and item classes a public char Appearance { get; private set; } property, and turn that write call into Console.Write(elem.Appearance), to make it more obvious what's going on, and so I don't have to abuse a ToString method.

Monstro and Item class

These notes refer to the classes you posted in the chat:

new Random() gives you a pseudorandom number generator that's seeded based on the current time. Because each monster has its own Random instance, they will likely move in the exact same way if you create multiple monster instances at the same time (or close enough).

Your movement code can be simplified: if you use a offsetX and offsetY variable, which are set based on the chosen direction, then you don't need that switch-case statement anymore.

Making Item responsible for its own placement is quite inflexible. Just give its constructor an x and y parameter, and let the map-generating/loading code figure out where to place items.

Randomly picking spots until you find an empty one should work just fine if there's plenty of space, but large maps with few empty spots will cause a lot of retries. It'll even loop indefinitely if there is no empty spot available.

SetPickedUp can be simplified to pickedUp = argPickedUp. Or just use a property instead.

Map.CheckTile is not a very descriptive name. Map.IsTileEmpty is more obvious.

You don't need those StringBuilders in your ToString methods. Just return appearance.ToString() or GetType().Name directly.

More notes

Making Item and Monstro abstract implies that you've got several classes that inherit from them. I assume that each different item has a class of its own? That makes sense if different items actually have different behavior, but if they only have different stats, then a single Item class is sufficient. Since Item has an attack, defense, health and rarity field, it can already be used for a variety of different items: weapons (attack), armor (defense), health potions (health).

In larger games, such data is often stored in csv or Excel files, which makes it easy for game-designers to adjust items without the help of a programmer. In your case that's probably not worth the effort, but you can still create a 'table' of item data in code. For a one-man project that's just as easy to modify, and still saves you a ton of useless classes:

var itemBlueprints = Dictionary<string, Item> {
    { "Sword", new Item(5, 0, 0, 0.05, '|') },
    { "Shield", new Item(0, 2, 0, 0.08, 'o') },
    { "Potion", new Item(0, 0, 5, 0.12, '+') },
};

You'll want to add a Name property to the Item class so you can still determine an item's name (the type name no longer tells you what it is). That's more flexible than using a type name anyway.

The above code could be made a little more readable by using optional and named parameters:

new Item("Sword", attack: 5, rarity: 0.05, appearance: '|');
new Item("Shield", defense: 2, rarity: 0.08, appearance: 'o');

Answer 2

it appears that you are duplicating code here

if (monstros.Count > 0)
{
    Console.ForegroundColor = ConsoleColor.DarkRed;
    foreach (var elem in monstros)
    {
        if (elem.ReX() == i && elem.ReY() == j)
        {
            Console.SetCursorPosition(setY, setX);
            Console.Write(elem);
        }
    }
}
if (items.Count > 0)
{
    Console.ForegroundColor = ConsoleColor.DarkRed;
    foreach (var elem in items)
    {
        if (elem.ReX() == i && elem.ReY() == j)
        {
            Console.SetCursorPosition(setY, setX);
            Console.Write(elem);
        }
    }
}

I think that this could be done with a method and then called for either items or monstros

Maybe a method similar to this

public void DrawSprite(List<object> sprite, int x, int y, int newX, int newY)
{
    Console.ForegroundColor = ConsoleColor.DarkRed;
    foreach (var element in sprite)
    {
        if (element.Rex() == x && element.ReY() == y)
        {
            Console.SetCursorPosition(newY, newX);
            Console.Write(element);
        }
    }
}

I used the term sprite but you can name it whatever you like.

I also used a list of type object but I assume that you have an interface or type that encompasses the items and monsters that you could use in its place.

I removed the check for whether or not there are any elements in the list because in a foreach statement if there are no objects in the container that you are enumerating it just won't enumerate, so it was really unnecessary.

I would use some Linq as well, but I don't know exactly how your types work, but this code would be a lot simpler using Linq for the foreach statements.

This is a simple method call now.

DrawSprite(monstros, x, y, setX, setY);
DrawSprite(items, x, y, setX, setY);

The only thing that I don't like about this is the amount of parameters that we are sending to the method, I would recommend creating an XY point class and using points instead of variableX and variableY, this will make your code a lot cleaner, among other things.

---

Now, your code should look like this

public void ShowCamera()
{
    var monstros = new List<bool>();
    int size = cameraSize/2;
    int topY = player.GetY() - size,
        bottomY = player.GetY() + size,
        topX = player.GetX() - size,
        bottomX = player.GetX() + size;
    int setX = 0, setY = 0;

    //Print Map
    Console.SetCursorPosition(0, 0);
    Console.ForegroundColor = ConsoleColor.DarkGreen;
    for (int x = topX; x < bottomX; x++)
    {
        for (int y = topY; y < bottomY; y++)
        {
            Console.ForegroundColor = ConsoleColor.DarkGreen;
            Console.SetCursorPosition(setY, setX);
            Console.Write(map[x, y]);

            DrawSprite(monstros, x, y, setX, setY);
            DrawSprite(items, x, y, setX, setY);

            setY++;
        }
        Console.Write("\n");
        setY = 0;
        setX++;
    }
}

public void DrawSprite(List<object> sprite, int x, int y, int newX, int newY)
{
    Console.ForegroundColor = ConsoleColor.DarkRed;
    foreach (var element in sprite)
    {
        if (element.Rex() == x && element.ReY() == y)
        {
            Console.SetCursorPosition(newY, newX);
            Console.Write(element);
        }
    }
}

Answer 3

@Pieter gives great advice on performance (and other things).

I would like to add that ShowCamera() appears to deal with drawing map and not with showing some camera to somebody, thus I suggest renaming this method.

Eventually, you may get something like this:

    public void DrawMap()
    {
        DrawEmptyMap();

        DrawMonsters();
        DrawItems();
    }

DrawMonsters() and DrawItems() may have logic like this:

    private void DrawMonsters()
    {
        var visibleMonsters = this.monstros.Select(m => this.IsVisible(m));

        foreach (var monster in visibleMonsters)
        {
            monster.Draw();
        }
    }

Here I assume that you store all the map highlights (top points, bottom points...) somewhere in the class. IsVisible() and Draw() methods do some trivial job on comparing coordinates. Further details depend on how you are going to implement inheritance of your sprites and storing their coordinates.

Note also to write only one var declaration per line.