5
\$\begingroup\$

I'm in the process of creating a simple SDL2 wrapper for a breakout clone. The real learning objectives for this project are:

Learn how to manage resources properly via wrappers Learn more about SDL2 I'm building these wrapper as I step through Lazy Foo's tutorial and wanted to get some feedback on how I'm doing, and what other things I could be doing.

Here's my SDL_Surface wrapper so far:

// Surface.h
#pragma once
#include <iostream>
#include <SDL.h>
#include <string>
#include "Window.h"


namespace SDL2 {
    class Surface
    {
    private:
        SDL_Surface* ScreenSurface = nullptr;
        bool IsWindowSurface = false;
        std::string ImagePath = "";

    public:
        // This function creates a Surface object associated with a Window (gets cleaned when Window is destroyed)
        Surface(SDL2::Window& window);
        Surface(std::string path);
        // Move constructor
        Surface(Surface&& source);
        // Disable copy constructor
        Surface(const Surface& source) = delete;
        ~Surface();
        // Disable copy by assignment
        Surface& operator=(const Surface& source) = delete;
        // Move assignment
        Surface& operator=(Surface&& source);
        void FillRect();
        void Update(SDL2::Window& window);
        bool LoadBMP();
        // TODO: Add more parameters at future SDL2 tutorial
        void BlitSurface(SDL2::Surface& destination);
    };
}



// Surface.cpp
#include "Surface.h"


SDL2::Surface::Surface(SDL2::Window& window) :
    ScreenSurface{ SDL_GetWindowSurface(window.GetSDLWindow()) },
    IsWindowSurface{ true }
{
}


SDL2::Surface::Surface(std::string path) :
    ImagePath{ path },
    IsWindowSurface{ false }
{
}

SDL2::Surface::Surface(Surface && source) :
    ScreenSurface{ source.ScreenSurface },
    IsWindowSurface{ source.IsWindowSurface },
    ImagePath{ source.ImagePath }
{
    source.ScreenSurface = nullptr;
    source.IsWindowSurface = false;
    source.ImagePath = "";
}


SDL2::Surface::~Surface()
{
    // If surface is not associated with Window, need free the surface with SDL2 call
    if (!IsWindowSurface) {
        SDL_FreeSurface(ScreenSurface);
    }
}


SDL2::Surface & SDL2::Surface::operator=(Surface && source)
{
    if (&source == this)
        return *this;

    // If surface is not associated with Window, need free the surface with SDL2 call
    if (!IsWindowSurface) {
        SDL_FreeSurface(ScreenSurface);
    }

    ScreenSurface = source.ScreenSurface;
    IsWindowSurface = source.ScreenSurface;
    ImagePath = source.ImagePath;

    // Return source to stable state
    source.ScreenSurface = nullptr;
    source.IsWindowSurface = false;
    source.ImagePath = "";

    return *this;

}

// TODO: Define SDL_Rect parameter and Color parameter (make wrappers for these)
void SDL2::Surface::FillRect()
{
    SDL_FillRect(ScreenSurface, NULL, SDL_MapRGB(ScreenSurface->format, 0x00, 0x00, 0xFF));
}


void SDL2::Surface::Update(SDL2::Window& window)
{
    SDL_UpdateWindowSurface(window.GetSDLWindow());
}


// This function is called after a non-window surface is created.  Check on this return value for success/fail.
bool SDL2::Surface::LoadBMP()
{
    // Load image from path
    ScreenSurface = SDL_LoadBMP(ImagePath.c_str());

    if (!ScreenSurface) {
        std::cout << "Unable to load image " << ImagePath << " SDL Error: " << SDL_GetError() << '\n';
        return false;
    }
    return true;
}


// The source Surface is `this`.  This function applies the image to the destination surface (though not updated on the screen)
void SDL2::Surface::BlitSurface(SDL2::Surface & destination)
{
    // Args: Source, FUTURE, Destination, FUTURE
    SDL_BlitSurface(this->ScreenSurface, NULL, destination.ScreenSurface, NULL);
}

Any pointers would be much appreciated!

\$\endgroup\$
  • \$\begingroup\$ Which tutorial are you referring to? \$\endgroup\$ – Mast Aug 20 '18 at 5:30
  • 2
    \$\begingroup\$ Right now just Lessons 1 and 2 here: lazyfoo.net/tutorials/SDL \$\endgroup\$ – Joe Aug 20 '18 at 5:54
5
\$\begingroup\$

Wrapping SDL_Surface like this to get automatic cleanup in the destructor is a good idea. However, the current implementation makes a few assumptions that aren't necessarily correct.


Surface Creation

There are lots of ways to get hold of a surface. Getting it from a Window, loading from a file, manually creating it with SDL_CreateSurface, or using something like SDL_TTF to render a font.

The Surface class shouldn't care about how the surface is created. It only needs to accept a surface, and know whether that surface should be cleaned up on destruction, e.g.:

Surface(SDL_Surface* surface, bool freeOnDestruction);

should replace the current Window and string constructors. The LoadBMP function could be made a free function that takes a string and returns a Surface.


Window

The dependency of Surface on Window seems backwards. Since an SDL_Window owns an SDL_Surface of its own, it would make more sense for the dependency to be the other way around. Thus the Window class could have a GetSurface() function, returning a Surface, and the Update() function would make more sense as UpdateSurface() in the Window class (it doesn't make sense for non-window surfaces anyway).


Interface and Error Checking

BlitSurface is confusing because you have to remember whether to call source.BlitSurface or dest.BlitSurface. As member functions it might be better to have both a BlitFrom(source) and BlitTo(dest), that both call a single Blit function.

It might be better to use free functions instead:

void FillSurface(Surface& surface, Color const& color); // fill entire surface!
void FillSurface(Surface& surface, Rect const& rect, Color const& color); // fill a rect on the surface
void BlitSurface(Surface& source, Surface const& dest); // blit whole surfaces
// TODO: rect versions...

SDL_BlitSurface has some conditions you should check before and after (it doesn't work on locked / null surfaces, it returns an error code on failure). These should be handled and output a message / throw an error / terminate the program.

It would make sense to have a GetSDLSurface() function in the Surface class (similar to the GetSDLWindow() function). We can check (assert / throw / print message) that the surface pointer is non-null in this function before returning it. If the Blit and Fill functions are free functions, and not members, they can't access the private member directly and must use this function. This means they will automatically do the non-null error check when using the surface.


Using the Standard Library

The C++ Standard Library already contains a class to manage an object's lifetime like this: std::unique_ptr.

Instead of checking whether we have to free the surface in copy / move constructors, we can wrap it in a unique_ptr with a custom deleter. We can then use the default move constructor and move operators:

#include <iostream>
#include <functional>
#include <memory>
#include <cassert>

// placeholder for testing...
struct SDL_Surface
{
    SDL_Surface() { std::cout << "constructor" << std::endl; }
    ~SDL_Surface() { std::cout << "destructor" << std::endl; }
};

// placeholder for testing...
void SDL_FreeSurface(SDL_Surface* surface)
{
    delete surface;
}

class Surface
{
public:

    Surface(SDL_Surface* sdlSurface, bool freeOnDestruction);

    Surface(Surface&&) = default;
    Surface& operator=(Surface&&) = default;

    Surface(Surface const&) = delete;
    Surface& operator=(Surface const&) = delete;

    SDL_Surface* GetSDLSurface() const; // Blit / Fill etc. should use this to get the SDL_Surface.

private:

    using SDLSurfacePtrDeleterT = std::function<void(SDL_Surface*)>;
    using SDLSurfacePtrT = std::unique_ptr<SDL_Surface, SDLSurfacePtrDeleterT>;
    SDLSurfacePtrT m_sdlSurfacePtr;
};

Surface::Surface(SDL_Surface* sdlSurface, bool freeOnDestruction)
{
    assert(sdlSurface != nullptr); // no null surfaces!
    // (means we don't have to check the inner pointer every time we e.g. blit).

    auto deleter = freeOnDestruction ?
        SDLSurfacePtrDeleterT([] (SDL_Surface* surface) { SDL_FreeSurface(surface); }) :
        SDLSurfacePtrDeleterT([] (SDL_Surface*) { });

    m_sdlSurfacePtr = SDLSurfacePtrT(sdlSurface, deleter);
}

SDL_Surface* Surface::GetSDLSurface() const
{
    assert(m_sdlSurfacePtr != nullptr); // check this surface hasn't been moved from
    // (if the m_sdlSurfacePtr is valid, the inner surface should be valid (or at least non-null) due to the assert in the constructor).

    return m_sdlSurfacePtr.get();
}

int main()
{
    std::cout << "Construct + Destruct:" << std::endl;

    {
        auto sdlSurface = new SDL_Surface();
        auto surface = Surface(sdlSurface, true);
        // should get destructor call...
    }

    std::cout << "Construct + Leak:" << std::endl;

    {
        auto sdlSurface = new SDL_Surface();
        auto surface = Surface(sdlSurface, false);
        // will not get destructor call...
    }

    std::cout << "Construct + Move + Destruct:" << std::endl;

    {
        auto sdlSurface = new SDL_Surface();
        auto surface = Surface(sdlSurface, true);
        auto surface2 = std::move(surface);
        // should get destructor call...
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for such a thorough response! When you say "The LoadBMP function could be made a free function that takes a string and returns a Surface", what do you mean by a "free function"?? \$\endgroup\$ – Joe Aug 20 '18 at 18:39
  • \$\begingroup\$ By "free function" I just mean a non-member function (i.e. not part of the Surface class). So Surface LoadBMP(std::string const& path);. \$\endgroup\$ – user673679 Aug 20 '18 at 19:12
3
\$\begingroup\$
#pragma once

Be aware that you are giving up portability here as this is, while common, a non-standard compiler extension. For nearly all applications, as long as you use implementations that support it, neither physically nor logically copy files around, and the filesystem doesn't trigger a false-positive, then #pragma once is fine. Otherwise, stick with standard include guards and give some effort to differentiate the guard name.


#include <iostream>
#include <SDL.h>
#include <string>
#include "Window.h"

Headers should not be dependent upon other headers being included first. One way to ensure this is to include your headers before any other headers.

Latent usage errors can be avoided by ensuring that the .h file of a component parses by itself – without externally-provided declarations or definitions... Including the .h file as the very first line of the .c file ensures that no critical piece of information intrinsic to the physical interface of the component is missing from the .h file (or, if there is, that you will find out about it as soon as you try to compile the .c file).

That is to say, your includes should be in the following order:

  1. Prototype/Interface. (surface.h)
  2. Project/Private headers (window.h)
  3. Headers from non-standard, non-system libraries (QT, Eigen, etc...)
  4. Standard C++ headers (vector, string, cstdint, etc...)
  5. System headers (windows.h, dirent.h, etc...)

As you move down the list, libraries are more stable and more widely used (and therefore tested). Further ordering each subgroup, say lexicographically by path/name, makes it easier for maintainers to quickly find an include when the include list becomes long.

Do not #include <iostream> in header files. Many C++ implementations transparently inject a static constructor into every translation unit that includes <iostream>, even if the client never uses the <iostream> facilities.


        bool IsWindowSurface = false;
        std::string ImagePath = "";

Whenever you have a boolean tied to the existence of some other value, use an optional type (Boost, Mnmlstc, Folly, C++17).

Does your surface really need to know where it was created from?


        Surface(std::string path);

Copying a std::string may be expensive. Pass this input parameter by reference to const.

SDL2::Surface::Surface(std::string path) :
    ImagePath{ path },
    IsWindowSurface{ false } {}

If you want to treat the parameter as a sink argument, then std::move(path) into ImagePath.


bool SDL2::Surface::LoadBMP() {
    // Load image from path
    ScreenSurface = SDL_LoadBMP(ImagePath.c_str());

What happens here if the surface already exists? If the surface was a window surface copy, then you leak the old surface and the ScreenSurface will point to null because ImagePath is empty. If the source was a previous BMP, then the old surface still leaks and ScreenSurface points to clean version of the BMP.


Rather than wrapping every function into RAII objects, use std::unique_ptr to manage the lifetimes object types. As long as you don't construct your std::unique_ptr with an instance of a destructor that can't be optimized away, then std::unique_ptr is practically free.

With all of your resources, you've likely noticed a pattern where you create the resource and check the handle to ensure it was created. We can create a generic helper for creating these resource types.

template<typename Result, typename Creator, typename... Arguments>
auto make_resource(Creator c, Arguments&&... args)
{
    auto r = c(std::forward<Arguments>(args)...);
    if (!r) { throw std::system_error(errno, std::generic_category()); }
    return Result(r);
}

Now we need a way to hand std::unique_ptr a deleter without incurring a cost. We can wrap it in a constant value and return a deleter function whenever std::unique_ptr requests it.

template <typename T, std::decay_t<T> t>
struct constant_t {
    constexpr operator T() noexcept const { return t; }
}

Using the two above helpers, we can churn out resources. First we define the pointer type.

using surface_ptr = std::unique_ptr<
    SDL_Surface, constant_t<decltype(SDL_FreeSurface), SDL_FreeSurface>>;

Then the factory functions.

inline auto make_surface(const char* filename) {
  return make_resource<surface_ptr>(SDL_LoadBMP, filename);
}

inline auto make_surface(SDL_Window* ptr) {
  return make_resource<surface_ptr>(SDL_GetWindowSurface, ptr);
}

inline auto make_surface(
    std::uint32_t flags, int width, int height, int depth, 
    std::uint32 Rmask, std::uint32 Gmask, std::uint32_t Bmask, std::uint32_t Amask) {
  return make_resource<surface_ptr>(
      SDL_CreateRGBSurface, flags, width, height, depth, 
      Rmask, Gmask, Bmask, Amask);
}

Then you can use this surface_ptr with the existing SDL library.

void meow() {
    auto w = make_window("Purr", SDL_WINDOW_POS_UNDEFINED, SDL_WINDOW_POS_UNDEFINED, 
                              640, 480, 0));
    auto s = make_surface(w.get());
    SDL_FillRect(s.get(), NULL, SDL_MapRGB(s->format, 255, 0, 0));
    SDL_UpdateWindowSurface(w.get());
}

Note - The constant_t helper above was needed for C++11 as there was a defect with std::integral_constant not being evaluated as a constexpr expression. With C++14, you can do the following

template <typename T, std::decay_t<T> t>
using constant_t = std::integral_t<T, t>;

using surface_ptr = std::unique_ptr<SDL_Surface, constant_t<decltype(SDL_FreeSurface)*, SDL_FreeSurface>>;

With C++17 allowing non-type template arguments with auto, the verbosity can be eliminated.

template <auto t>
using constant_t = std::integral_constant<std::decay_t<decltype(t)>, t>;

using surface_ptr = std::unique_ptr<SDL_Surface, constant_t<SDL_FreeSurface>>;
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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