Skip to content

Instantly share code, notes, and snippets.

@jroweboy
Created September 21, 2018 01:45
Show Gist options
  • Save jroweboy/d65018f7fbaef60dcacfcd22d95a72b8 to your computer and use it in GitHub Desktop.
Save jroweboy/d65018f7fbaef60dcacfcd22d95a72b8 to your computer and use it in GitHub Desktop.
Trying to figure out a good way to make handle this scenario
// place where other input state is stored
std::shared_ptr<SDL> state;
// Ties the lifetime of the SDL input code to whatever the frontend decides is important
void Init() {
state = SDL::Init();
}
void Shutdown() {
state.reset();
}
class SDL {
// stuff like threads that we want to determine the order of destruction
};
std::shared_ptr<SDL> killmepls;
std::shared_ptr<SDL> Init() {
killmepls = std::make_shared<SDL>();
return killmepls;
}
InputDevice::GetController() {
// uses `killmepls` to find what controllers have been registered
}
// several other methods in a variety of classes that all use the state in killmepls
@lioncash
Copy link

lioncash commented Sep 21, 2018

Assume all code implemented within the class functions inline in the classes are intended to be implemented in the cpp file

In this case you would likely pass a reference to that state, or specific bits of that state to the relevant classes and provide a helper function, if you want, to get rid of the need to explicitly specify the state. So let's step back and look at this from top down. We have a System class that drives everything, and given SDL is a subsystem, we can't place that directly in the System class, but we can make a class to encapsulate an input subsystem, so let's do that.

A very basic class one might be inclined to write is like so:

// Encapuslates the totality of the input subsystem.
//
// This would ideally be a std::unique_ptr<InputSubsystem> within the System class
// and be instantiated when initializing the system depending on the selected input backend.
//
// Everything would go through this for input-related stuff.
//
class InputSubsystem {
public:
    // We place the definition of this in the cpp file to avoid generating the vtable into every
    // translation unit that uses this class.
    virtual ~InputSubsystem();

    // Initialize the specifics of a backend and return whether or not it succeeded.
    virtual bool Initialize() = 0;

    // Tears down the subsystem.
    virtual void Shutdown() = 0;
};

This has an issue however. Common state that can be shared between all input subsystem implementations needs to be duplicated, as the initialization and shutdown functions are pure virtual. Instead we can make this better by making the actual functions a backend needs to implement as private, and make Initialize() and Shutdown() non-virtual. And so it'll look something like so:

class InputSubsystem {
public:
    // Define this in the cpp file to avoid inlining input code. It's pointless code bloat.
    // If there's ever a case where this not being inlined is a problem, there's even bigger problems
    // that should be looked at. This also makes the class more friendly with forward declared types.
    InputSubsystem();

    // We place the definition of this in the cpp file to avoid generating the vtable into every
    // translation unit that uses this class.
    virtual ~InputSubsystem();

    // Initializes the backend and returns whether or not initialization succeeded.
    bool Initialize() {
        // Initialize common bits here...

        if (!InitializeSubsystemSpecifics()) {
            return false;
        }

        return true;
    }

    // Tears down the subsystem.
    void Shutdown() {
        // Shutdown common bits here...

        ShutdownSubsystemSpecifics();
    }

private:
    // Used to initialize the API-specific parts of an input subsystem.
    virtual bool InitializeSubsystemSpecifics() = 0;

     // Used to shutdown the API-specific parts of an input subsystem.
    virtual void ShutdownSubsystemSpecifics() = 0;
};

Now this gives you a couple of benefits:

  • The actual externally usable portion of the interface does not need to change if only the virtual functions need to change. They're hidden from external code using them, and so it doesn't need to be modified if the internals get shuffled around with regards to lifetimes or init and shutdown

  • You now have a place for common state that isn't dependent on the subsystems directly, and so, you can place that here and expose it as necessary through functions, without having to worry about it sitting in unrelated places. This state would likely be protected so derived classes can see it and alter it as necessary.

Now, it's very important to consider the following.

  • If there is ever a requirement for exposing subsystem specifics externally beyond this interface, then external code needs to be reworked to not require that. A general interface, should not need to expose input subsystem-specific data and handles outside of input common. It's inherently not common, if uncommon things are being done. In other words, if functions exposing SDL-specifics are added to this class, it's a red flag. If functions exposing DInput stuff are added to this class, it's a red flag. Repeat for every other input library on the planet.

So this also creates an explicit boundary point one can see when doing review.


Alright, so we have our basic area to place stuff, now how do we create the necessary input devices and also allow it to access that common state?

Well, it's already mentioned that the common state would be protected, so that's easily accessible, for this example we'll call it common_state and give it the type name of CommonState from here on. Now the SDL specifics need to be taken care of. So let's do that. We can define the subclass like so:

class SDLInputSubsystem final : public InputSubsystem {
public:
    SDLInputSubsystem();
    ~SDLInputSubsystem() override;

private:
    bool InitializeSubsystemSpecifics() override {
        // Here we can initialize the specifics, such as creating the controllers, etc.
        return true;
    }

    void ShutdownSubsystemSpecifics() override {
        // Shutdown SDL specifics here.
    }

    struct Impl;
    std::unique_ptr<Impl> impl;
};

We use the PImpl idiom here, because ideally we want to keep all SDL specifics out of the header, since the source file that includes the header this class is defined in will be a general source file that ties everything together, and we don't want anything from SDL being included in that, as it could clash with symbols in some other hypothetical input library. So this would look something like:

// In the header for this subsystem:

class SDLInputSubsystem final : public InputSubsystem {
public:
    SDLInputSubsystem();
    ~SDLInputSubsystem() override;

private:
    bool InitializeSubsystemSpecifics() override;
    void ShutdownSubsystemSpecifics() override;

    struct Impl;
    std::unique_ptr<Impl> impl;
};


// In the cpp file for this subsystem:

struct SDLInputSubsystem::Impl {
    struct SDLState {
        // SDL specific state that can't be encapsulated within
        // joystick classes, or other related input classes for SDL
        // will go here.
    };

    SDLState sdl_state;
};

SDLInputSubsystem::SDLInputSubsystem() = default;

SDLInputSubsystem::~SDLInputSubsystem() = default;

bool SDLInputSubsystem::InitializeSubsystem() {
    // Initialize all necessary state in Impl
    return true;
}

void SDLInputSubsystem::ShutdownSubsystem() {
    // Shutdown SDL stuff here
}

Now we have places for both the common shared state, and the SDL state. Now all that remains is to modify the necessary interfaces to take the reference of either one of (or both of) CommonState and SDLState and access the data. For example, if you need the SDLSpecific state in a hypothetical SDLJoystick class, one would do:

SDLJoystick::SomeFunction(SDLState& sdl_state) {
    // Use state here for whatever stuff you need it for
}

or if you need that state to always be accessible to the class, take a reference to it in the constructor

SDLJoystick::SDLJoystick(SDLState& state) : sdl_state{state} {
    // sdl_state would be some member variable like: "SDLState& sdl_state;"
    // within SDLJoystick
}

With this approach, you have the guarantee that the overall subsystem class will outlive everything created under it, as the shutdown call in the interface should tear down everything before the subsystem class is destroyed.

For example, lets do GetSDLJoystickByGUID() from the codebase:

static std::shared_ptr<SDLJoystick> GetSDLJoystickByGUID(SDLState& state, const std::string& guid, int port) {
    std::lock_guard<std::mutex> lock(state.joystick_map_mutex);

    const auto it = state.joystick_map.find(guid);
    if (it != state.joystick_map.end()) {
        while (it->second.size() <= port) {
            auto joystick = std::make_shared<SDLJoystick>(guid, it->second.size(), nullptr,
                                                          [](SDL_Joystick*) {});
            it->second.emplace_back(std::move(joystick));
        }
        return it->second[port];
    }

    auto joystick = std::make_shared<SDLJoystick>(guid, 0, nullptr, [](SDL_Joystick*) {});
    return state.joystick_map[guid].emplace_back(std::move(joystick));
}

We just pass in the state as a parameter. If the shared common state would ever be needed, then that can be passed as a parameter too.


Now we just need to tie it all together. In some common header/source file pair we'd have the following:

// Header
enum class InputSubsystemType {
    Null,
    SDL,
    // etc.
};

std::unique_ptr<InputSubsystem> MakeInputSubsystem(InputSubsystemType type);

// In cpp file:

std::unique_ptr<InputSubsystem> MakeInputSubsystem(InputSubsystemType type) {
    switch (type) {
    case InputSubsystemType::Null:
        return std::make_unique<NullSubsystem>();
    case InputSubsystemType::SDL:
        return std::make_unique<SDLSubsystem>();
    }

    UNREACHABLE_MSG("Invalid input subsystem");
    return nullptr;
}

Then just use that to initialize it. Hopefully that answers the question thoroughly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment