4
\$\begingroup\$

I have written this code base on AsciiEngine. How can I improve this code?

#include <iostream>
#include <fstream>
#include <vector>
#include <string>
#include <memory>
#include <algorithm>
#include <unordered_map>
#include <type_traits>
#include <random>
#include <chrono>
#include <cassert>

#include <Windows.h>

enum class WorldSize
{
    ColNum = 80, // width
    RowNum = 50  // hieght
};

enum class Textures
{
    BackGround,
    Tile,
};

enum class Components
{
    Position,
    Sprite,
};

enum class Entities
{
    BackGround,
    Tile,
};

struct NonCopyable
{
    NonCopyable() = default;
    virtual ~NonCopyable() = default;

    NonCopyable(const NonCopyable &) = delete;
    NonCopyable(const NonCopyable &&) = delete;
    NonCopyable& operator = (const NonCopyable&) = delete;
};

struct Vector2D
{
    float x, y;
};

/////////////////////////////////////////////////

template<typename T>
struct Random : public NonCopyable
{
    Random(float min, float max)
        : mUniformDistribution(min, max)
    {}

    T get() { return mUniformDistribution(mEngine); }

    std::default_random_engine mEngine{ std::random_device()() };

    template <typename U>
    static auto dist() -> typename std::enable_if<std::is_integral<U>::value, std::uniform_int_distribution<U>>::type;

    template <typename U>
    static auto dist() -> typename std::enable_if<std::is_floating_point<U>::value, std::uniform_real_distribution<U>>::type;

    decltype(dist<T>()) mUniformDistribution;
};

/////////////////////////////////////////////////

template<typename T, bool enable = std::is_integral<T>::value || std::is_enum<T>::value>
struct range_impl
{
    struct iterator
    {
        const T operator * () const noexcept{ return value; }

        iterator& operator ++() noexcept{ ++value; return *this; }

            friend  const bool operator != (const iterator& lhs, const iterator& rhs) noexcept{
            return lhs.value != rhs.value;
        }

        T value;
    };

    std::size_t size() const{ return last - first; }
    const iterator begin() const noexcept{ return{ first }; }
    const iterator end() const noexcept{ return{ last }; }

    T first, last;
};

template<typename T>
struct range_impl<T, false>
{
    range_impl(T first, T last) : first(first), last(last){}

    std::size_t size() const{ return std::distance(first, last); }
    const T begin() const noexcept{ return{ first }; }
    const T end() const noexcept{ return{ last }; }

    T first, last;
};

template<typename T1, typename T2>
range_impl<typename std::common_type<T1, T2>::type> range(T1 first, T2 last) noexcept
{
    return{ first, last };
}

/////////////////////////////////////////////////

struct Image : NonCopyable
{
    Image(Textures id_) 
        : id(id_)
        , mWidth()
        , mHeight()
        , mChar()
        , mColor()
    {}

    uint8_t chars(int x, int y)
    {
        return mChar[x + y * mWidth];
    }

    uint8_t colors(int x, int y)
    {
        return mColor[x + y * mWidth];
    }

    Image* loadFromFile(const std::string& fileName)
    {
        std::ifstream file(fileName);

        if (!file)
        {
            return nullptr;
        }

        file >> mWidth >> mHeight;

        mChar.resize(mWidth * mHeight);
        mColor.resize(mWidth * mHeight);

        for (const auto& i : range(0, mHeight))
        {
            for (const auto& j : range(0, mWidth))
            {
                int copy = 0;
                file >> copy;
                mChar[j + i * mWidth] = copy;

            }
        }

        for (const auto& i : range(0, mHeight))
        {
            for (const auto& j : range(0, mWidth))
            {
                int copy = 0;
                file >> copy;
                mColor[j + i * mWidth] = copy;
            }
        }

        return this;
    }
    Textures getId() const { return id; }
    std::string getIdReadable() { return toString(id); }

    friend struct Graphic;
private:
    std::string toString(Textures id)
    {
        switch (id)
        {
        case Textures::BackGround:  return "BackGound";
        case Textures::Tile: return "Tile";
        default: return "UNKNOWN";
        }
    }
    Textures id;
    std::size_t mWidth;
    std::size_t mHeight;
    std::vector<uint8_t> mChar;
    std::vector<uint8_t> mColor;
};

/////////////////////////////////////////////////

template <typename Identifier>
struct ResourceHolder
{
    static ResourceHolder<Identifier>* getInstance() { return &resourceHolderInstance; }

    void load(Identifier id, const std::string& filename)
    {
        std::unique_ptr<Image> resource(new Image(id));
        if (!resource->loadFromFile(filename))
        {
            throw std::runtime_error("ResourceHolder::load - Failed to load " + filename);
        }

        insertResource(id, std::move(resource));
    }

    Image& get(Identifier id)
    {
        auto found = mResourceMap.find(id);
        assert(found != mResourceMap.end());
        return *found->second;
    }
    const Image& get(Identifier id) const
    {
        auto found = mResourceMap.find(id);
        assert(found != mResourceMap.end());
        return *found->second;
    }


private:
    static ResourceHolder<Identifier> resourceHolderInstance;

    void insertResource(Identifier id, std::unique_ptr<Image>& resource)
    {
        auto inserted = mResourceMap.insert(std::make_pair(id, std::move(resource)));
        assert(inserted.second);
    }

    std::unordered_map<Identifier, std::unique_ptr<Image>> mResourceMap;
};

using TexturesHolder = ResourceHolder<Textures>;

template <typename Identifier>
ResourceHolder<Identifier> ResourceHolder<Identifier>::resourceHolderInstance;

/////////////////////////////////////////////////

struct Graphic : NonCopyable
{
    Graphic(std::size_t width, std::size_t height)
        : mHeight(height)
        , mWidth(width)
        , mBuffer(width * height)
    {}
    static Graphic* getInstance() { return &graphicInstance; }

    bool screenBoundCheck(int x, int y)
    {
        return (x < static_cast<int>(mWidth) && x > -1 && y < static_cast<int>(mHeight) && y > -1) ? true : false;
    }

    bool writeImageToScreen(const Textures& id, int x, int y)
    {
        auto resourceHolder = TexturesHolder::getInstance();

        auto image = &resourceHolder->get(id);

        if (!image)
        {
            return false;
        }

        for (const auto& i : range(0, image->mHeight)) 
        {
            for (const auto& j : range(0, image->mWidth))
            {
                if (screenBoundCheck(j + x, i + y) && image->mChar[j + image->mWidth * i] != 0 /*TRANSPERANT*/)
                {
                    mBuffer[(j + x) + mWidth * (i + y)].Char.AsciiChar = image->mChar[j + image->mWidth * i];
                    mBuffer[(j + x) + mWidth * (i + y)].Attributes = image->mColor[j + image->mWidth * i];
                }
            }
        }

        return true;
    }

    void clearScreen()
    {
        for (const auto& i : range(0, mHeight))
        {
            for (const auto& j : range(0, mWidth))
            {
                mBuffer[j + mWidth * i].Char.AsciiChar = 0;
                mBuffer[j + mWidth * i].Attributes = 0;
            }
        }
    }

    void renderScreen()
    {
        COORD charBufferSize{ mWidth, mHeight };
        COORD characterPosition{ 0, 0 };
        SMALL_RECT writeArea{ 0, 0, mWidth - 1, mHeight - 1 };

        WriteConsoleOutputA(GetStdHandle(STD_OUTPUT_HANDLE), mBuffer.data(), charBufferSize, characterPosition, &writeArea);
    }

private:
    static Graphic graphicInstance;

    std::vector<CHAR_INFO> mBuffer;
    std::size_t mHeight;
    std::size_t mWidth;
    const uint8_t TRANSPERANT = 255;

};

Graphic Graphic::graphicInstance(static_cast<std::size_t>(WorldSize::ColNum), static_cast<std::size_t>(WorldSize::RowNum));

/////////////////////////////////////////////////

struct ComponentHolder;

struct Entity : virtual NonCopyable
{
    Entity(Entities cid) : id(cid){}
    Entities getId() const { return id; }
    std::string getIdReadable(){return toString(id);}

    virtual void update(float dt) = 0;
    virtual void draw() = 0;

    std::shared_ptr<ComponentHolder> componentHolder;

    bool active = true;

private:
    std::string toString(Entities id)
    {
        switch (id)
        {
        case Entities::BackGround:  return "BackGound";
        case Entities::Tile: return "Tile";
        default: return "UNKNOWN";
        }
    }
    Entities id;
};

/////////////////////////////////////////////////

struct Component : NonCopyable
{
    Component(Components cid) : id(cid), entity(nullptr), image(nullptr){};
    Component(Components cid, Entity *e, Image *i = nullptr) : id(cid), entity(e), image(i) {}

    Components getId() const { return id; }
    std::string getIdReadable() { return toString(id); }

    virtual void update(float dt) = 0;
    virtual void draw() = 0;

protected:
    Entity *entity;
    Image *image;

private:
    std::string toString(Components id)
    {
        switch (id)
        {
        case Components::Position:  return "Position";
        case Components::Sprite: return "Sprite";
        default: return "UNKNOWN";
        }
    }
    Components id;
};

/////////////////////////////////////////////////

struct ComponentHolder : NonCopyable
{
    using Ptr = std::unique_ptr<Component>;

    template<typename T, typename... Targs>
    Component& addComponent(Targs&&... args)
    {
        Ptr ptr(std::make_unique<T>(std::forward<Targs>(args)...));

        auto inserted = mComponents.insert(std::make_pair(ptr->getId(), std::move(ptr)));

        return *inserted.first->second.get();
    }

    Component& getComponent(Components id)
    {
        auto found = mComponents.find(id);
        assert(found != mComponents.end());

        return *found->second;
    }
    const Component& getComponent(Components id) const
    {
        auto found = mComponents.find(id);
        assert(found != mComponents.end());

        return *found->second;
    }

private:
    std::unordered_map<Components, Ptr> mComponents;
};

/////////////////////////////////////////////////

struct Position : Component
{
    Position(Components cid, float x, float y) : Component(cid)
    {
        setPosition(x, y);
    }

    void setPosition(float x, float y)
    {
        position.x = x;
        position.y = y;
    }

    Vector2D getPosition() const
    {
        return position;
    }

    void update(float dt) override{};
    void draw() override {};

private:
    Vector2D position;
};

/////////////////////////////////////////////////

struct Sprite : Component
{
    Sprite(Components cid, Entity *e, Image *i) :Component(cid, e, i) {}

    void draw() override
    {
        auto graphic = Graphic::getInstance();

        auto position = dynamic_cast<Position*>(&entity->componentHolder->getComponent(Components::Position));

        if (image)
        {
            graphic->writeImageToScreen(image->getId(), static_cast<int>(position->getPosition().x), static_cast<int>(position->getPosition().y));
        }
    }

    void update(float dt) override {}
};

/////////////////////////////////////////////////

struct BackGround : Entity
{
    BackGround(Entities cid, float x, float y) :Entity(cid)
    {
        auto resourceHolder = TexturesHolder::getInstance();

        componentHolder = std::make_shared<ComponentHolder>();

        auto sprite = &componentHolder->addComponent<Sprite>(Components::Sprite, this, &resourceHolder->get(Textures::BackGround));
        auto position = &componentHolder->addComponent<Position>(Components::Position, x, y);
        // use sprite, position 
    }

    void update(float dt) override
    {

    }

    void draw() override
    {
        componentHolder->getComponent(Components::Sprite).draw();
    }
};

/////////////////////////////////////////////////

struct Tile : Entity
{
    Tile(Entities cid, float x, float y) :Entity(cid)
    {
        auto resourceHolder = TexturesHolder::getInstance();

        componentHolder = std::make_shared<ComponentHolder>();

        auto sprite = &componentHolder->addComponent<Sprite>(Components::Sprite, this, &resourceHolder->get(Textures::Tile));
        auto position = &componentHolder->addComponent<Position>(Components::Position, x, y);
        // use sprite, position 
    }

    void update(float dt) override
    {

    }

    void draw() override
    {
        componentHolder->getComponent(Components::Sprite).draw();
    }
};

/////////////////////////////////////////////////

struct EntityManger : NonCopyable
{
    using Ptr = std::unique_ptr<Entity>;

    template<typename T, typename... Targs>
    Entity& createEntity(Targs&&... args)
    {
        Ptr ptr(std::make_unique<T>(std::forward<Targs>(args)...));

        auto inserted = mEntities.insert(mEntities.end(), std::move(ptr));

        return **inserted;
    }

    void createFromFile(const std::string& fileName)
    {
        std::ifstream file(fileName);

        if (!file)
        {
            return;
        }

        std::vector<std::string> words;

        int index = 0;
        for (std::string word; file >> word;)
        {
            index++;
            words.push_back(word);

            if (index == 3)
            {
                if (words[0] == "BackGround")
                {
                    createEntity<BackGround>(Entities::BackGround, atof(words[1].c_str()), atof(words[2].c_str()));
                }
                else if (words[0] == "Tile")
                {
                    createEntity<Tile>(Entities::Tile, atof(words[1].c_str()), atof(words[2].c_str()));
                }
                else
                {
                    return;
                }

                index = 0;
                words.clear();
            }
        }
    }
    void update(float dt)
    {
        for (auto& i : mEntities)
        {
            if (!i->active)
            {
                deleteEntity(i);
            }

            i->update(dt); // dummy value
        }
    }

    void draw()
    {
        for (auto& i : mEntities)
        {
            if (!i->active)
            {
                deleteEntity(i);
            }

            i->draw();
        }
    }

    void deleteAll()
    {
        while (!mEntities.empty())
        {
            mEntities.back().release();
            mEntities.pop_back();
        }
    }

private:

    void deleteEntity(Ptr& entity)
    {
        auto it = std::find_if(mEntities.begin(), mEntities.end(),
            [&](Ptr& p)->bool
        {
            return entity->getId() == p->getId();
        });

        assert(it != mEntities.end());

        mEntities.erase(it);
    }

    std::vector<Ptr> mEntities;
};

/////////////////////////////////////////////////

struct Game : NonCopyable
{
    Game()
    {
        mEntities.createFromFile("../Game Assets/level.txt");
    }

    void run()
    {
        auto graphic = Graphic::getInstance();

        auto start = std::chrono::high_resolution_clock::now();

        while (true)
        {
            auto end = std::chrono::high_resolution_clock::now();

            double timeTakenInSeconds = (end - start).count()
                * (static_cast<double>(std::chrono::high_resolution_clock::period::num)
                / std::chrono::high_resolution_clock::period::den);

            if (timeTakenInSeconds > mWaitTime)
            {
                update();

                start = std::chrono::high_resolution_clock::now();
            }

            draw();

            graphic->renderScreen();
        }
    }

private:
    void update()
    {
        mEntities.update(0.f);
    }
    void draw()
    {
        mEntities.draw();
    }

    EntityManger mEntities;

    const double mWaitTime = 0.1;
};

/////////////////////////////////////////////////

class FileSystem
{
public:

    void loadAllFiles(const std::string& directory)
    {
        WIN32_FIND_DATAA fileData = { 0 };
        std::string searchString = "*.txt";
        std::string entireSearchString = directory + searchString;

        HANDLE handle = FindFirstFileA(entireSearchString.data(), &fileData);

        do
        {
            loadFile(directory, fileData.cFileName);

        } while (FindNextFileA(handle, &fileData) != 0);
    }

    void loadFile(const std::string& folder, const std::string& fileName)
    {
        std::string file = folder + fileName;

        Textures id;

        if (fileName == "BackGround.txt")
        {
            id = Textures::BackGround;
        }
        else if (fileName == "Tile.txt")
        {
            id = Textures::Tile;
        }
        else
        {
            return;
        }

        auto resourceHolder = TexturesHolder::getInstance();

        resourceHolder->load(id, file);
    }
};

/////////////////////////////////////////////////

int main()
{
    try
    {
        FileSystem file;
        file.loadAllFiles("../Game Assets/");

        Game game;
        game.run();
    }
    catch (std::exception& e)
    {
        std::cerr << e.what() << std::endl;
    }
    catch (...)
    {
        std::cerr << "An error occured." << std::endl;
    }
}

level.txt

BackGround 
30.00 
5.00

Tile 
31.00 
6.00

Tile 
45.00 
6.00

Tile 
31.00 
18.00

Tile 
45.00 
18.00

BackGround.txt

17
15
255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255
255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255
255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255
255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255
255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255
255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255
255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255
255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255
255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255
255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255
255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255
255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255
255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255
255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255
255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255

112 112 112 112 112 112 112 112 112 112 112 112 112 112 112 112 112
112 159 159 159 159 159 159 159 159 159 159 159 159 159 159 159 112
112 159 159 159 159 159 159 159 159 159 159 159 159 159 159 159 112
112 159 159 159 112 159 159 159 159 159 159 159 112 159 159 159 112
112 159 159 112 112 112 159 159 159 159 159 112 112 112 159 159 112
112 159 159 159 112 159 159 159 159 159 159 159 112 159 159 159 112
112 159 159 159 159 159 159 159 159 159 159 159 159 159 159 159 112
112 159 159 159 159 159 159 159 159 159 159 159 159 159 159 159 112
112 159 159 159 112 159 159 159 159 159 159 159 112 159 159 159 112
112 159 159 159 112 159 159 159 159 159 159 159 112 159 159 159 112
112 159 159 159 112 159 159 159 159 159 159 159 112 159 159 159 112
112 159 159 159 112 159 159 159 159 159 159 159 112 159 159 159 112
112 159 159 159 112 159 159 159 159 159 159 159 112 159 159 159 112
112 159 159 159 159 159 159 159 159 159 159 159 159 159 159 159 112
112 112 112 112 112 112 112 112 112 112 112 112 112 112 112 112 112

Tile.txt

1
1
219

10
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Unnecessary overhead of NonCopyable

Because your NonCopyable has a virtual destructor, it forces any class inheriting from it to also become virtual: it will now need to have a vtable, which increases its size. Since you don't ever access your classes via the virtual base class, this is unnecessary overhead.

Also consider whether you need to make things non-copyable to begin with. For example, why is Image non-copyable? If you would make a copy of it, your program would continue to run perfectly fine, since everything stored in it is by value.

You also don't need to explicitly make a class non-copyable if it has member variables that are non-copyable. Consider ComponentHolder: it has this member:

std::unordered_map<Components, std::unique_ptr<Component>> mComponents;

Since std::unique_ptr is non-copyable, the std::unordered_map becomes non-copyable, and therefore ComponentHolder automatically becomes non-copyable.

There are other issues with your NonCopyable: it not only prevents copying, it is also non-movable since you deleted to move constructor (but not the move assignment operator?).

Naming things

There are several questionable naming choices in your code. Consider Textures. Because it's the plural of a noun, it sounds like this is a collection of Textures. But instead it is the ID of a texture. So I would name this TextureID (singular).

Graphic is a weird name, I think you would call this an Image instead, because apart from its member function renderScreen(), everything else is just image manipulation. Consider creating a Screen class that has a renderImage(const Image &image) member function, or at least rename renderScreen() to renderToScreen().

Several names are just misleading: writeImageToScreen() doesn't write anything to the screen, it just copies a texture into the image buffer. So copyTexture() or drawTexture() would be better names.

Some things are not spelled correctly, like TRANSPERANT (should be TRANSPARENT), hieght (height), BackGound (BackGround) and occured (occurred). Consider enabling spell checking in your editor, and/or use a spell checking tool like codespell.

Use of enum for constants

An enum is a great way to give names to a set of possibilities, like IDs. But they are not a good fit for constants that have a specific value, like the number of rows and columns. Use static constexpr for those constants:

static constexpr std::size_t ColNum = 80;
static constexpr std::size_t RowNum = 50;

Note how you no longer need the static_casts anymore when using these constants.

Unnecessary use of class

In C++ you can have functions outside classes. If you have a class which has no state at all, and all its member functions could have been made static, you probably don't need to put them in a class at all. This is the case for example with class FileSystem. Just make free functions loadFile() and loadAllFiles():

static void loadFile(const std::string& filder, const std::string& filename)
{
    ...
}

static void loadAllFiles(const std::string& directory)
{
    ...
    loadFile(directory, fileData.cFileName);
    ...
}

int main()
{
    try
    {    
        loadAllFiles("../Game Assets/");
        ...
    }
    ...
}

Arguably, class Game is also unnecessary; while it does have member variables, consider that these could have been made local variables inside run() instead, in which case there's just a single run() function that's only called once. So you could have made a free function void runGame() instead.

Reading images

In Image::loadFromFile(), you use doubly-nested loops to read in the values from the file. However, since you are storing it in a one-dimensional std::vector, there is no need to do that, you can use a single for-loop instead. Furthermore, you con't need to first read it into an int and then copy it into the vector, you can do this instead:

mChar.resize(mWidth * mHeight);

for (const auto& c: mChar) {
    file >> c;
}

Note that calling resize() will actually cause all the elements in the vector to be value-initialized, but then you are going to overwrite them, so this is not optimal. It's not really important here, but the best way would be to call reserve() instead of resize(), and then push_back() the values you read in. You can also the STL algorithm std::copy_n() in combination with std::back_inserter to read in the file without any loops:

mChar.reserve(mWidth * mHeight);
std::copy_n(std::istream_iterator<uint8_t>(file), mWidth * mHeight,
            std::back_inserter(mChar));

But probably a simple for-loop is more clear here.

Unnecessary abstractions

Your class ResourceHolder, on closer inspection, is just a wrapper around a std::unordered_map. It doesn't add any important features, apart from perhaps a getInstance() function. But instead of creating a singleton class, in C++ you can just declare a global variable:

static std::unordered_map<TextureID, Image> textureHolder;

Note that instead of using "Holder" in the name, I would use the plural of the type to indicate that this is a collection of things, so:

static std::unordered_map<TextureID, Image> textures;

Instead of TexturesHolder::getInstance().load(id, filename), consider that you can just write:

textures.emplace(id, id);
textures[id].loadFromFile(filename);

Or with C++17 you can write:

resources.emplace(id, id).loadFromFile(filename);

Note that there is no need to use std::unique_ptr here; std::unordered_map already manages the memory for the keys and values it stores.

Instead of get(), you can use at() directly on the map:

auto& image = textures.at(id); // or textures[id] if you don't need bounds checking

The same goes for ComponentHolder.

Missing error checking

In loadFromFile(), you check if the file was opened correctly, but you didn't do any further error checking. Consider that errors can happen at any time when reading or writing files; these could be actual I/O errors, but it can also be that the contents of the file are not what you expected.

While checking for errors after every little I/O operations would be very tedious, luckily stream object remember their error state. So you can read everything like you already did, but only check at the end that everything was read correctly:

Image* loadFromFile(const std::string& fileName)
{
    std::ifstream file(fileName):

    file >> mWidth >> mHeight;
    ...

    return file ? this : nullptr;
}

Beware though that if reading mWidth or mHeight fails, those variables might not be written to. Since you never initialized those variables, they can contain random values. Either add a check that file.good() right after reading these two, or ensure they are initialized to zero before trying to read them.

throw early

If you are using exceptions, then throw as soon as you encounter an error. The benefit of exceptions are that code that cannot directly handle the error doesn't have to deal with error conditions, they can just leave it up some higher layer to catch the exception. Consider loadFromFile(): it potentially returns a nullptr, so in ResourceHolder::load() you had to check for an error condition. If you would instead a throw std::runtime_error directly from loadFromFile, then load() wouldn't need to do any error handling.

Another benefit is that you can then pass a filename to the constructor of Image, so you could have simplified load() to:

void load(Identifier id, const std::string& filename) {
    insertResource(id, std::make_unique<Image>(id, filename));
}

Or if you removed ResourceHolder and just used global map variables, then with C++17 you could have written:

textures.try_emplace(id, id, filename);

Beware of iterator invalidation

When you are iterating over a container, be careful when adding or removing items at the same time, as these can cause the iterator to be invalidated. This happens in update() and draw(). Even worse, those functions seem to explicitly dereference the iterators after deleting the item they point to! One possible solution is to use std::erase_if() to erase inactive elements, and then to iterate over the remaining ones:

void update(float dt) {
    std::erase_if(mEntities, [](auto& entity) {
        return !entity->active;
    });

    for (auto& entity: mEntities) {
        entity->update(dt);
    }
}

You could optimize this by keeping track of whether any entities were deactivated since the last call to update(), so that you can avoid having to call std::erase_if() unnecessarily. Even more advanced would be to check if an element is active in the for-loop, and if not swap it with the last element; this wouldn't invalidate any iterators, and at the end of the loop all the inactive elements are at the end. If you kept a count, you can then simply resize() the vector to get rid of them.

Do you need an Entity Component System?

At the moment, your code only draws tiles. Instead of using an ECS for that, I think it would be better to implement a tilemap renderer. This would be a single entity in a game, optimized at rendering the map using tiles, instead of having many individual tile entities in the ECS.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.