C++ SDL2 Space Invader C++ questions

Question

The whole project would be a bit much to post, but there were a couple of questions that came up while I worked on this project. This is a fully working space invader game I coded in C++ with SDL2. I am an experienced C# developer and first done this in C# using MonoGame and then "ported" it over to SDL2. Platform is Windows 10 using Visual Studio 2019.

1: I do have a class tree where the base class is Object, and other classes such a Player, Enemy, Bullet and Bunker inherit from it. My ObjectManager keeps track of them all and updates, checks collisions and draws them. My ObjectManager holds them in a vector:

    std::vector<std::unique_ptr<Object>> m_colliableObjects;

I'm using a unique_ptr because unlike C# (which uses ptrs too of course) I can't just use the base class in a vector. Now to add an object to this vector, I have this template function:

template<class T>
void addColliableObject(T* co)
{
    static_assert(std::is_base_of<Object, T>::value, "T must be Object.");
    m_colliableObjects.push_back(std::unique_ptr<T>(co));
}

To make the registration, each class inheriting "Object" implements a reg function:

void Player::reg()
{
   ObjectManager::getInstance()->addColliableObject<Player>(this);
}

Which must be called from the constructor from all classes that inherit. Is there a way to implement this function once in the Object base class, so I can also call it straight from the base class constructor? Meanwhile the opposite destroy() function I was able to implement in the base class easily.

2: I always read to use std::string and not wstring. But the windows API in visual studio defaults to UNICODE. Which I can turn off, but what's the best to use? I want to be able to store normal UTF8 strings, Ascii is too limited.

3: I used a unique_ptr to store this struct in the vector so I do not need a copy and move constructor..is this a wise move? Also is ownership clear in this struct? Because Texture is just pointing to a texture which is owned in the Content namespace

   struct SpriteFrame
   {
    SDL_Texture* Texture;
    float Time;
    std::unique_ptr<SDL_Rect> SourceRect;

    SpriteFrame(SDL_Texture* texture, const float time, SDL_Rect* sourceRect)
        : Texture(texture), Time(time), SourceRect(sourceRect)
    {

    }
};

std::vector<std::unique_ptr<SpriteFrame>> m_frames; 

4: Is there any point of a unique_ptr in a namespace class other than showing ownership? Because these variables never go out of the stack I still need to call reset manually:

namespace Content
{

SDL_Texture* getSpriteSheet();
SDL_Texture* getSpaceBackground();

void loadTextures(const std::string& root);
TTF_Font* loadFont(const std::string& name, int size);
TTF_Font* getFont(const std::string& name, int size, bool loadIfNotFound);
void reset();
namespace
{
    SDL_Texture* loadTexture(const std::string& root, const std::string& name);
    std::unique_ptr<SDL_Texture, SDL_Deleter> Spritesheet;
    std::unique_ptr<SDL_Texture, SDL_Deleter> SpaceBackground;
    std::map<std::string, std::unique_ptr<TTF_Font, SDL_Deleter>> fonts;
    std::string fontToKey(const std::string& name, int size);
}

}

5: Similar to 4, to ease access to the ObjectManager I implemented a getInstance function. But since the instance member is stored static, I still need to reset/delete it manually again. Is there a better way?

static ObjectManager* getInstance()
{
    if (!m_instance)
        m_instance.reset(new ObjectManager());

    return m_instance.get();
}

static void reset()
{
    m_instance.reset();
}

6: I'm hosting enemies in a nested array. Which works, but it's not C++ 11 of course. Enemy itself is already owned by ObjectManager, but as soon I use unique_ptr for the array the [] operator does not work. But I need to fill the array quiet dynamically as you can see below:

typedef Enemy** EnemyArr;
EnemyArr*   m_enemies;

void EnemyManager::createEnemies(const int xCount, const int yCount, const bool initial)
{

this->m_xCount = xCount;
this->m_yCount = yCount;

m_enemies = new EnemyArr[xCount]);
for (int x = 0; x < xCount; ++x)
{
    m_enemies[x] = new Enemy*[yCount];
    for (int y = 0; y < yCount; ++y)
    {
        m_enemies[x][y] = new Enemy(this,{
            x * Defs::EnemySpacing + Defs::EnemyMarginIn,
            y * Defs::EnemySpacing + Defs::EnemyMarginIn });

        m_enemies[x][y]->m_ignoreCollision = y < yCount - 1;
        
    }
}

m_currentMoveAmount = 0;
m_moveToRight = 0;
m_reachedPlayer = false;
if (initial)
{
    m_moveDelay = Defs::EnemyMoveDelay;
    m_shootDelay = Defs::EnemyShootDelay;
}
else
{
    m_moveDelay /= Defs::EnemyIncreaseRoundSpeedMult;
    m_shootDelay /= Defs::EnemyIncreaseRoundShootSpeedMult;
}


}

Please let me know if you need more info because the code is quite straight to the point, I just didn't want to post code of 30 files but I'm happy to share whatever you need, the whole project too.

Answer 1

No need for a template if you use pointers to base class

You made addColliableObject() a template, but that is unnecessary. Since it accepts a pointer to an object that should always be derived from Object, you can write this instead:

void addColliableObject(Object* co)
{
    m_colliableObjects.push_back(std::unique_ptr<Object>(co));
}

You can simplify this even further by writing:

void addColliableObject(Object* co)
{
    m_colliableObjects.emplace_back(co);
}

So now that you no longer need to push the derived type, you can just push a pointer to the base class to m_colliableObjects from the constructor of Object. But be aware that the constructor of the base class is called before the derived class, so you can't have anything use the pointer in m_colliableObjects until the constructor of the derived class has finished.

You can also destroy it in the base class in a similar way. The destructor of the base class is run after the destructor of the derived class.

Unicode is not an encoding

See this question for a discussion about std::string vs. std::wstring. But in short: std::string can hold UTF-8 strings without issues, but it is not Unicode-aware. It just treats everything as bytes. That's perfectly fine in most cases though; for example the SDL_ttf functions to render text also expect UTF-8 strings in the same format. So it all depends on what other functions need to parse your strings. If it's mostly the Windows API, then perhaps using std::wstring is better.

Using std::unique_ptr

Using std::unique_ptr like you do for m_colliableObjects is perfectly fine and exactly what you need here. It will also work fine for m_frames. However, it should not be necessary to store pointers to SpriteFrames here. With the definition you provided, std::vector<SpriteFrame> m_frames should work as well, without needing to add copy or move constructors. But what you probably noticed is that you cannot do something like this:

SpriteFrame frame(...);
m_frames.push_back(frame);

The reason is that the default copy constructor of SpriteFrame is implicitly deleted, with good reason! It contains a std::unique_ptr, and you cannot copy unique pointers, as then they wouldn't be unique anymore. You should not attempt to fix this by creating a copy constructor, but instead you should use std::move to move frame into the vector:

m_frames.push_back(std::move(frame));

Alternatively, you can emplace a new SpriteFrame directly into the vector:

m_frames.emplace_back(...);

As for the usefulness of a std::unique_ptr: besides ownership semantic, it also ensures the memory is automatically cleaned up correctly, so it is a good way to manage storage, and is preferably over manual use of new and delete.

Avoid the singleton pattern

It seems like you are leaning heavily on the singleton pattern. But do you really need them? Instead of having:

class Foo {
    static Foo *m_instance;
public:
    static Foo *getFoo() {
        if (!m_instance)
            m_instance.reset(new Foo);
        
        return m_instance;
    }
};

You can just write:

class Foo {
    ...
};

Foo theFoo;

Here theFoo is a global variable, everyone can access it, and there is only one theFoo. Especially if Foo doesn't depend on other things to be initialized first, this is easy to do. If you can use this, I would recommend it.

Anyway, in both your code using the singleton pattern, and my example here, you don't need to explicitly reset or delete it, this will be done automatically once the program terminates. Of course, if the order is important, you might have to manually call reset(), or ensure the order is guaranteed in some other way. There are various ways to approach this, but I cannot tell what the best way would be for your project.

Use std::vector for variable sized arrays

Avoid raw new and delete for creating arrays, use std::vector for that. If you want a 2D array, you could nest std::vectors like so:

std::vector<std::vector<Enemy>> m_enemies;

But this approach, and yours too, is inefficient. It is much better to have a one-dimensional array so everything is layed out consecutively in memory, and manually calculate the right index given the x and y coordinates. Here is how to create such an array:

std::vector<Enemy> m_enemies;

// remember the actual dimensions
size_t m_xCount;
size_t m_yCount;

...

void EnemyManager::createEnemies(const int xCount, const int yCount, const bool initial)
{
    m_xCount = xCount;
    m_yCount = yCount;

    for (int y = 0; y < yCount; ++y)
    {
        for (int x = 0; x < xCount; ++x)
        {
            m_enemies.emplace_back(this, {
                x * Defs::EnemySpacing + Defs::EnemyMarginIn,
                y * Defs::EnemySpacing + Defs::EnemyMarginIn });

            m_enemies.back().m_ignoreCollision = y < yCount - 1;
        }
    }
}

And if you later need to access the enemy at location {x, y}, use:

m_enemies[x + y * m_xCount]

This small calculation is much faster for the CPU to execute than following multiple pointers.

Again I've shown m_enemies here storing Enemys directly, it's also possible to make this a std::vector<std::unique_ptr<Enemy>> of course.