HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Console RPG - show cycle optimization

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
showcycleoptimizationrpgconsole

Problem

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  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.

Solution

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 {
    { "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');

Code Snippets

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, '+') },
};
new Item("Sword", attack: 5, rarity: 0.05, appearance: '|');
new Item("Shield", defense: 2, rarity: 0.08, appearance: 'o');

Context

StackExchange Code Review Q#133396, answer score: 6

Revisions (0)

No revisions yet.