Remaking a 1998 RTS game in C++

Question

I am remaking a very obscure old game called Rival Realms.

The full source is here if you're interested.

I come from a Java background so C++ is still fairly new to me.

I recently performed a refactor to move all the textures and sprites into one class. The intention is that I will create a Resources instance when the game first starts, which will load all the required resources, and then I can pass around a pointer to this object so that other classes can easily retrieve the resources they need. This object will be "alive" until the game exits.

Does this sound like a reasonable approach? How can the code be improved?

In particular I am still a little confused by the rule of 5, and when to use smart pointers. For example, here I have a std::unique_ptr<std::map<UnitType, Spritesheet>> - but would it be better to have a std::map<UnitType, std::unique_ptr<Spritesheet>> instead?

Resources.h

#ifndef RESOURCES_H
#define RESOURCES_H

#include <map>
#include <string>
#include <vector>

#include "Palette.h"
#include "Spritesheet.h"
#include "Texture.h"
#include "Unit.h"

namespace Rival {

    class Resources {

    public:

        // Directories
        static const std::string mapsDir;
        static const std::string txDir;

        Resources();
        ~Resources();

        // Prevent moving or copying (rule of 5)
        Resources(const Resources& other) = delete;
        Resources(Resources&& other) = delete;
        Resources& operator=(const Resources& other) = delete;
        Resources& operator=(Resources&& other) = delete;

        // Retrieval
        Texture& getPalette() const;
        Spritesheet& getTileSpritesheet(int index) const;
        std::map<UnitType, Spritesheet>& getUnitSpritesheets() const;
        Spritesheet& getMapBorderSpritesheet() const;

    private:

        // Texture constants
        static const int numTextures = 96;
        static const int txIndexUnits = 0;
        static const int txIndexTiles = 50;
        static const int txIndexUi = 53;

        // Loaded textures
        std::unique_ptr<std::vector<Texture>> textures =
            std::make_unique<std::vector<Texture>>();
        std::unique_ptr<Texture> paletteTexture;

        // Spritesheets
        std::unique_ptr<std::map<UnitType, Spritesheet>> unitSpritesheets =
            std::make_unique<std::map<UnitType, Spritesheet>>();
        std::unique_ptr<std::map<int, Spritesheet>> tileSpritesheets =
            std::make_unique<std::map<int, Spritesheet>>();
        std::unique_ptr<Spritesheet> mapBorderSpritesheet;

        // Initialisation
        void loadTextures();
        void initPaletteTexture();
        void initUnitSpritesheets();
        void initTileSpritesheets();
        void initUiSpritesheets();
        void initUnitSpritesheet(UnitType type, int txIndex);
        void initTileSpritesheet(int type, int txIndex);

    };

}

#endif // RESOURCES_H

Resources.cpp

#include "pch.h"
#include "Resources.h"

#include "RenderUtils.h"

namespace Rival {

    const std::string Resources::mapsDir = "res\\maps\\";
    const std::string Resources::txDir = "res\\textures\\";

    Resources::Resources() {
        loadTextures();
        initPaletteTexture();
        initUnitSpritesheets();
        initUiSpritesheets();
        initTileSpritesheets();
    }

    Resources::~Resources() {
        // Delete Textures
        for (Texture& texture : *textures.get()) {
            const GLuint texId = texture.getId();
            glDeleteTextures(1, &texId);
        }
        textures->clear();
    }

    void Resources::loadTextures() {

        textures->reserve(numTextures);

        // Units - Human
        textures->push_back(Texture::loadTexture(txDir + "unit_human_ballista.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_human_battleship.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_human_bowman.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_human_chariot_of_war.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_human_fire_master.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_human_knight.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_human_light_cavalry.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_human_peasant.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_human_pegas_rider.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_human_priest.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_human_sea_barge.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_human_thief.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_human_wizard.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_human_zeppelin.tga"));

        // Units - Greenskin
        textures->push_back(Texture::loadTexture(txDir + "unit_greenskin_balloon.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_greenskin_catapult.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_greenskin_gnome_boomer.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_greenskin_horde_rider.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_greenskin_landing_craft.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_greenskin_necromancer.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_greenskin_priest_of_doom.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_greenskin_rock_thrower.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_greenskin_rogue.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_greenskin_serf.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_greenskin_storm_trooper.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_greenskin_troll_galley.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_greenskin_warbat.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_greenskin_warlord.tga"));

        // Units - Elf
        textures->push_back(Texture::loadTexture(txDir + "unit_elf_archer.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_elf_arquebusier.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_elf_bark.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_elf_bombard.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_elf_centaur.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_elf_druid.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_elf_dwarf_miner.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_elf_enchanter.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_elf_mage.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_elf_magic_chopper.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_elf_scout.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_elf_sky_rider.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_elf_warship.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_elf_yeoman.tga"));

        // Units - Monsters
        textures->push_back(Texture::loadTexture(txDir + "unit_monster_devil.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_monster_dragon.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_monster_golem.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_monster_gryphon.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_monster_hydra.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_monster_sea_monster.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_monster_skeleton.tga"));
        textures->push_back(Texture::loadTexture(txDir + "unit_monster_snake.tga"));

        // Tiles
        textures->push_back(Texture::loadTexture(txDir + "tiles_meadow.tga"));
        textures->push_back(Texture::loadTexture(txDir + "tiles_wilderness.tga"));
        textures->push_back(Texture::loadTexture(txDir + "tiles_fog.tga"));

        // UI
        textures->push_back(Texture::loadTexture(txDir + "ui_cursor_select.tga"));
        textures->push_back(Texture::loadTexture(txDir + "ui_map_border.tga"));
    }

    void Resources::initPaletteTexture() {
        paletteTexture = std::make_unique<Texture>(
                Palette::createPaletteTexture());
    }

    void Resources::initUnitSpritesheets() {

        int nextIndex = txIndexUnits;

        // Human
        initUnitSpritesheet(UnitType::Ballista, nextIndex++);
        initUnitSpritesheet(UnitType::Battleship, nextIndex++);
        initUnitSpritesheet(UnitType::Bowman, nextIndex++);
        initUnitSpritesheet(UnitType::ChariotOfWar, nextIndex++);
        initUnitSpritesheet(UnitType::FireMaster, nextIndex++);
        initUnitSpritesheet(UnitType::Knight, nextIndex++);
        initUnitSpritesheet(UnitType::LightCavalry, nextIndex++);
        initUnitSpritesheet(UnitType::Peasant, nextIndex++);
        initUnitSpritesheet(UnitType::PegasRider, nextIndex++);
        initUnitSpritesheet(UnitType::Priest, nextIndex++);
        initUnitSpritesheet(UnitType::SeaBarge, nextIndex++);
        initUnitSpritesheet(UnitType::Thief, nextIndex++);
        initUnitSpritesheet(UnitType::Wizard, nextIndex++);
        initUnitSpritesheet(UnitType::Zeppelin, nextIndex++);

        // Greenskin
        initUnitSpritesheet(UnitType::Balloon, nextIndex++);
        initUnitSpritesheet(UnitType::Catapult, nextIndex++);
        initUnitSpritesheet(UnitType::GnomeBoomer, nextIndex++);
        initUnitSpritesheet(UnitType::HordeRider, nextIndex++);
        initUnitSpritesheet(UnitType::LandingCraft, nextIndex++);
        initUnitSpritesheet(UnitType::Necromancer, nextIndex++);
        initUnitSpritesheet(UnitType::PriestOfDoom, nextIndex++);
        initUnitSpritesheet(UnitType::RockThrower, nextIndex++);
        initUnitSpritesheet(UnitType::Rogue, nextIndex++);
        initUnitSpritesheet(UnitType::Serf, nextIndex++);
        initUnitSpritesheet(UnitType::StormTrooper, nextIndex++);
        initUnitSpritesheet(UnitType::TrollGalley, nextIndex++);
        initUnitSpritesheet(UnitType::Warbat, nextIndex++);
        initUnitSpritesheet(UnitType::Warlord, nextIndex++);

        // Elf
        initUnitSpritesheet(UnitType::Archer, nextIndex++);
        initUnitSpritesheet(UnitType::Arquebusier, nextIndex++);
        initUnitSpritesheet(UnitType::Bark, nextIndex++);
        initUnitSpritesheet(UnitType::Bombard, nextIndex++);
        initUnitSpritesheet(UnitType::Centaur, nextIndex++);
        initUnitSpritesheet(UnitType::Druid, nextIndex++);
        initUnitSpritesheet(UnitType::DwarfMiner, nextIndex++);
        initUnitSpritesheet(UnitType::Enchanter, nextIndex++);
        initUnitSpritesheet(UnitType::Mage, nextIndex++);
        initUnitSpritesheet(UnitType::MagicChopper, nextIndex++);
        initUnitSpritesheet(UnitType::Scout, nextIndex++);
        initUnitSpritesheet(UnitType::SkyRider, nextIndex++);
        initUnitSpritesheet(UnitType::Warship, nextIndex++);
        initUnitSpritesheet(UnitType::Yeoman, nextIndex++);

        // Monsters
        initUnitSpritesheet(UnitType::Devil, nextIndex++);
        initUnitSpritesheet(UnitType::Dragon, nextIndex++);
        initUnitSpritesheet(UnitType::Golem, nextIndex++);
        initUnitSpritesheet(UnitType::Gryphon, nextIndex++);
        initUnitSpritesheet(UnitType::Hydra, nextIndex++);
        initUnitSpritesheet(UnitType::SeaMonster, nextIndex++);
        initUnitSpritesheet(UnitType::Skeleton, nextIndex++);
        initUnitSpritesheet(UnitType::Snake, nextIndex++);
    }

    void Resources::initUnitSpritesheet(UnitType type, int txIndex) {
        unitSpritesheets->emplace(std::piecewise_construct,
            std::forward_as_tuple(type),
            std::forward_as_tuple(
                    textures->at(txIndex),
                    RenderUtils::unitWidthPx,
                    RenderUtils::unitHeightPx));
    }

    void Resources::initUiSpritesheets() {
        mapBorderSpritesheet = std::make_unique<Spritesheet>(
                textures->at(txIndexUi + 1),
                RenderUtils::tileSpriteWidthPx,
                RenderUtils::tileSpriteHeightPx);
    }

    void Resources::initTileSpritesheets() {
        int nextIndex = txIndexTiles;
        initTileSpritesheet(0, nextIndex++); // Meadow
        initTileSpritesheet(1, nextIndex++); // Wilderness
        initTileSpritesheet(2, nextIndex++); // Fog
    }

    void Resources::initTileSpritesheet(int type, int txIndex) {
        tileSpritesheets->emplace(std::piecewise_construct,
            std::forward_as_tuple(type),
            std::forward_as_tuple(
                    textures->at(txIndex),
                    RenderUtils::tileSpriteWidthPx,
                    RenderUtils::tileSpriteHeightPx));
    }

    Spritesheet& Resources::getTileSpritesheet(int index) const {
        return tileSpritesheets->at(index);
    }

    std::map<UnitType, Spritesheet>& Resources::getUnitSpritesheets() const {
        return *unitSpritesheets.get();
    }

    Spritesheet& Resources::getMapBorderSpritesheet() const {
        return *mapBorderSpritesheet.get();
    }

    Texture& Resources::getPalette() const {
        return *paletteTexture.get();
    }

}

Answer 1

Does this sound like a reasonable approach?

What is your target system? Although I had a look at a gameplay video, I am not sure if sprites need dynamic loading. I guess not, so the answer is: Yeah, sure. If your target has enough resources (TM)

In particular I am still a little confused by the rule of 5, and when to use smart pointers.

For rule of five; you can look it up on google: An adequately informative result. In case these do not make sense; I see you use a user-defined destructor, so you may need to implement user-defined copy constructor and user-defined copy assignment operator. Though I don't think you will move your Resources object, so no need for "rule of five" in this case.

What I would do in your case though is not to bother and just define constructor & destructor since I would not use unique_ptr, and you are not using anything else that is not RAII-compatible.

Use unique_ptr if you have to and use shared_ptr otherwise for a "safe" start. If you utilise generic programming practices, you can easily change the declaration later on for some performance benefits and better declaration of your intent.

For example, here I have a std::unique_ptr<std::map<UnitType, Spritesheet>> - but would it be better to have a std::map<UnitType, std::unique_ptr<Spritesheet>> instead?

I vote for neither since I see no need for pointers at all. My answer would change if had looked at your code (which you provided, yes).

Other improvements:

• Your function void Resources::loadTextures() seems unnecessarily long. You can define a list of strings and iterate over it for this redundant procedure.

textures->push_back(Texture::loadTexture(txDir + "unit_human_ballista.tga"));
textures->push_back(Texture::loadTexture(txDir + "unit_human_battleship.tga"));
textures->push_back(Texture::loadTexture(txDir + "unit_human_bowman.tga"));
textures->push_back(Texture::loadTexture(txDir + "unit_human_chariot_of_war.tga"));

would be easier to manage with

std::list<std::string> t = { "unit_human_ballista.tga", "unit_human_battleship.tga", "unit_human_bowman.tga", "unit_human_chariot_of_war.tga" /* etc */ };
for ( auto it = t.begin(); it != t.end(); ++t ) {
    textures->push_back(Texture::loadTexture(txDir + *t));
}

• If you want to use range-based for loops, do it as you wish after you watch this wonderful presentation specifically at 42 minutes mark.

• Do these two functions need to be seperated?

void Resources::initTileSpritesheet(UnitType type, int txIndex)
void Resources::initTileSpritesheets()

You can combine them and get a single function that does the same thing. Please refer to this another great video.

Same goes for void Resources::initUnitSpritesheets() and void Resources::initUnitSpritesheet(int type, int txIndex) [grunt] and the rest of your code, within reason.

[grunt] Actually I do not like this default-constructed-object-as-type approach in initUnitSpritesheets but I can't think of a better alternative. You can probably hear me grunting.

• Why do you use

std::unique_ptr<std::vector<Texture>> textures = std::make_unique<std::vector<Texture>>();

in the header file when you can initialize textures at the constructor?

• *unitSpritesheets.get(); at definition of getUnitSpritesheets()

Either your code does not compile with this, or I am missing something. I think you meant

return *unitSpritesheets;

See this example.

Answer 2

Transitioning between languages ain't easy. I remember my move from C to Java, where I had to learn that Java passes everything via pointer. Now that I know C++ I would argue that it passes everything by std::shared_ptr. So, the main element of learning a new language, beside the syntax, is to learn about good practices, which are different based on the language. Some constructs of Java, like the way they deal with enumerations are frowned upon in the C++ world, see this stack overflow post.

In general, I think your approach is sound. I'm positively surprised to see you using a std::map<UnitType, ...> with this UnitType being a enum class. To be fair, as soon as you start optimizing, you want to get rid of std::map as it is way too much overhead and you better use a std::vector (This can even result in O(1) lookup and a lot less cache misses). Though, for the time being, it works and it allows you to get to get everything working.

As my intro already alluded to, java uses pointers for everything. Most likely that's why you are using std::unique_ptr all over the place. C++ uses value semantics. In short: Instead of allocating all data on the heap, we store this on the stack by default. This causes huge performance gains as data is packed closed together and one has less cache misses. Not to mention all the allocate/deallocate code that doesn't need to be executed. With modern C++ (which is now the C++17 standard), there ain't that much reason to allocate. See more about that in an earlier post of mine on stackoverflow: when (not to) allocate memory. In this specific case, remove ALL std::unique_ptr from the Resource class.

Than, we have the rule of 5 you asked about. Again, this has something to do with value semantics.

void f(Class c);         //< Function has it's own copy of Class, similar to being called with f(c.clone()); in Java
void g(Class &c);        //< Function can adapt the original instance
void h(const Class &c);  //< Function gets the original instance, though, ain't allowed to modify (If the original gets modified another way, it will see it)
void i(Class &&c);       //< Function requires you to move the original instance. (Original instance stays in valid but unspecified state)
void j(const Class &&c); //< Really useless signature.
void k(Class *c);        //< Same as Class &c, however, c is allowed to be nullptr
void l(const Class *c);  //< Same as const Class &c, however, c is allowed to be nullptr

The above are all the different ways a function can be defined and let's the function implementor decide what behaviour the argument should have. This is really important to understand.

So what happens when we call these functions?

Class a;                  //< Class can be changed
const Class b;            //< No changes allowed after construction
Class *c{nullptr};        //< Pointer to a changable class, can be nullptr and can be updated to refer to another class
const Class *d{nullptr};  //< Pointer to a class that can't be changed via d, can be nullptr and can be updated to refer to another class
Class &e = a;             //< See c, can't be nullptr
const Class &f = a;       //< See d, can't be nullptr

So let's assume we can call this (ignoring the double name of f):

f(a); //< Passes a copy of a to f
g(a); //< g gets a reference to a, can update a
h(a); //< h gets a reference to a, can't update
i(a); //< Doesn't compile, needs to be i(std::move(a))
k(&a);
l(&a);

(I'll leave the others as exercise to the reader)

What does this have to do with the rule of 5? It requires you to define what the Class executes as code when copied, moved, assigned.

Class(); //< Default constructor (not part of rule of 5)
Class(int i); //< Regular constructor (not part of rule of 5
~Class(); //< Destructor: Gets executed when the class gets destroyed (C++ has deterministic destruction, so you can actually write useful code here, as closing a file handle. Often linked to RAII (You can Google this))

 Class(const Class &other); //< Copy constructor: How should the class be copied, should the new class share some elements with the `other` instance or not? Implementer can decide.
 Class(Class &&other); //< Move constructor: Similar to the copy constructor, however, one is allowed to `steal` from the `other` instance, as long as it leaves it in a valid/unspecified state (aka: program doesn't crash when destructor gets called)

 Class &operator(const Class &rhs); //< Copy assignment: In short: Destructor followed by Copy Construction, with some details for self-assign and optimization.
 Class &operator(Class &&rhs); //< Move assignment: In short: Destructor followed by Move Construction, with some details for self-assign and optimization.

The rule of 0-or-5 states that you should specify either none or all 5 of:

• Destructor
• Copy constructor
• Move constructor
• Copy assignment
• Move assignment

This in order to have something easily understandable for the reader, while ensuring bug-free usage of your classes. (= delete is considered an implementation, stating: This ain't allowed to be used)

More about the rule of 5 on the cpp core guidelines

Let me stop here with the review, I have some other remarks which I'll keep for myself as I find it more important for you to first grasp these idioms.