Simple game engine layout

Question

I'm trying to make a game with inheritance & interfaces and I'm wondering if my system could be working without ECS. (I explain at the end why I'm not using components).

To make it easier to understand there are three core class "families" in the code:

Engine related classes:

Graphics, Inputs, Audio, Physics,...

Using E~~~ as a prefix for each class

Interfaces:

I'm using them like properties, they don't contain data, they're contracts, like C# interfaces (and Java I believe?):

Transformable, Destroyable, ...

Using I~~~ as a prefix for each class

"Concrete" classes:

They contain data and implements the interfaces. The could be Player, Bullet, etc.

Using C~~~ as a prefix for each class

Plus a class named Engine that contains every E~~~ class so it makes it easy to access "Engine elements".

Engine-related code:

class EInput {
public:
    enum { keyCount = 64 };
    char state[keyCount];
    char previousState[keyCount];
};

class EGraphics {
public:
    int width, height;
};

class EPhysics {
public:
    float gravity;
};

class EEngine {
public:
    EInput* input;
    EGraphics* graphics;
    EPhysics* physics;

    EEngine() {
        input = new EInput();
        graphics = new EGraphics();
        physics = new EPhysics();
    }

    ~EEngine() {
        delete input;
        delete graphics;
        delete physics;
    }
};

Interface-related code:

class ITransformable {
public:
    virtual float getX() = 0;
    virtual float getY() = 0;

    virtual void setX(float x) = 0;
    virtual void setY(float x) = 0;

    virtual ~ITransformable() {}
}; 

class IDestroyable {
public:
    virtual float getHp() = 0;

    virtual void setHp(float hp) = 0;

    virtual void hurt(float amount) = 0;
    virtual void kill() = 0;
    virtual bool isAlive() = 0;

    virtual ~IDestroyable() {}
};

class IEngine {
public:
    virtual void provideEngine(EEngine* engine) = 0;

    virtual ~IEngine() {}
};

class ITeam {
public:
    virtual int getTeam() = 0;

    virtual void setTeam(int team) = 0;

    virtual ~ITeam() {}
};

Concrete classes-related code:

class CCharacter : public IEngine, public ITransformable, public IDestroyable, public ITeam {
private:
    /* IEngine */
    EEngine* engine;
    /* ITransformable */
    float x, y;
    /* IDestroyable */
    float hp;
    /* ITeam */
    int team;
public:
    CCharacter() : engine(nullptr), x(0.f), y(0.f), hp(0.f) {}

    /* IEngine */
    void provideEngine(EEngine* engine) override { this->engine = engine; }

    /* ITransformable */
    virtual float getX() override { return x; }
    virtual float getY() override { return y; }
    virtual void setX(float x) override { this->x = x; }
    virtual void setY(float x) override { this->y = y; }

    /* IDestroyable */
    virtual float getHp() override { return hp; }
    virtual void setHp(float hp) override { this->hp = hp; }
    virtual void hurt(float amount) override { this->hp -= amount; }
    virtual void kill() override { this->hp = 0.f; }
    virtual bool isAlive() override { return this->hp > 0.f; }

    /* ITeam */
    virtual int getTeam() override { return this->team; };
    virtual void setTeam(int team) override { this-> team = team; };

    virtual void update() = 0;
    virtual void draw() = 0;
};

class CPlayer : public CCharacter {
public:
    CPlayer() {
        setHp(100.f); //...
    }
    void update() override {}
    void draw() override {}
};

enum { T_NONE, T_ALLY, T_ENEMY };
class CAIPlayer : public CCharacter {
private:
    int team;
public:
    CAIPlayer() : team(T_NONE) {}
    void update() override {}
    void draw() override {}
};

And a game would be similar to this, removing the game loop:

int main() {
    EEngine engine;
    //Initialize some values
    {
        EGraphics& g = *engine.graphics;
        g.width = 512;
        g.height = 288;

        EPhysics& p = *engine.physics;
        p.gravity = 0.4f;
    }

    //Create our characters, a factory would be used, this is just to show the idea

    //Our hero
    CPlayer sam; sam.provideEngine(&engine);

    //Two friendlies
    CAIPlayer lambert; lambert.provideEngine(&engine);
    lambert.setTeam(T_ALLY);

    CAIPlayer wilkes; wilkes.provideEngine(&engine);
    wilkes.setTeam(T_ALLY);
    wilkes.kill();

    //The bad guy
    CAIPlayer nikoladze; nikoladze.provideEngine(&engine);
    nikoladze.setTeam(T_ENEMY);
}

• Is this code clean?

• What are the limits of such a system? I mean, I don't see any duplicated behaviour.

  I read some really nice articles and questions on the net, including Stack Overflow, about entity-components-systems and how amazing it is, but it seems (to me) that it is overdone by people, the code is a big mess with templates everywhere and yet there are no results on the screen.

• I know that this code might be weak compared to ECS, but at which point?

I'm trying to keep it simple.

Answer 1

I had a few thoughts whilst reading through your code:

Naming

I don't mind the I prefix for interfaces, I've never really liked C as a prefix for classes although I can live with it. Having another prefix of E for engine specific classes feels wrong/confusing. EEngine is a concrete class, that does stuff, why isn't it ECEngine / CEEngine for example?

Interfaces

Some of your interfaces don't really feel like interfaces. For example, IEngine has one method provideEngine. This isn't what I think of when I think of an Engine interface. It could be an IEngineUser interface but it still feels a bit wrong. I'd expect IEngine to define operations that were then implemented in EEngine.

Initialisation

You're declaring variables and calling methods on them at the same time:

CAIPlayer lambert; lambert.provideEngine(&engine);

This seems error prone. It also flags up a possible design error. Does it make sense for a CAIPlayer to exist without an engine / without a team? If not, then I would expect it to be passed into the constructor, rather than passed in immediately after every construction. It seems like you've done it this way because of the interfaces you've declared which specify class dependencies rather than what a class can do...

Public fields

You're passing an instance of EEngine into every class it has public pointers to other engine classes that have public fields. This feels like way too much exposure of your engines implementation. I would hide this information behind getter methods. Then either pass the intial values into the constructor or provide setter methods. Whilst they may be simple pass throughs initially it will easier in the future if you want to make changes such as supporting different gravity zones Does it really make sense that at the moment any CCharacter can change the gravity in the game?

Answer 2

Namespaces

Your "E" for engine classes, "I" for interfaces, and "C" for concrete classes seems to be reinventing namespaces. I don't mind the "I" prefix for interfaces, but why not use namespaces? You might lay it out something like this:

namespace my_game_engine_name {
    namespace core { // for the "E" classes
        // if there are enough graphics related things, maybe this should be
        // its own namespace
        class Graphics;
        class Inputs;
        class Audio;
        class Physics;
    }
    ... // stuff to do with the interfaces, etc
}

namespace my_game {
    // notice how this is a separate namespace; almost a separate project.
    // The way you have your game set up, you almost have an entirely separate
    // game engine library, so why not make it that way?
    class Character;
    class Player;
    class AIPlayer;
}

Naming

This is a small nitpick that you may or may not agree with, but it's often better for acronyms to not be all uppercase, to ease the use with tools. For example, AIPlayer. There are tools which would think this was an "A I Player" rather than an "AI Player". Consequently, it often works out better if it was AiPlayer, as it makes parsing class names as PascalCase feasible.

Memory safety

class EEngine {
public:
    EInput* input;
    EGraphics* graphics;
    EPhysics* physics;

    EEngine() {
        input = new EInput();
        graphics = new EGraphics();
        physics = new EPhysics();
    }

    ~EEngine() {
        delete input;
        delete graphics;
        delete physics;
    }
};

This can easily delete memory multiple times. For example:

void myFunction() {
    EEngine myEngine;
    doSomethingWithMyEngine(myEngine);
}

This might double-delete if doSomethingWithMyEngine takes the EEngine by value. If you feel the need to use the raw pointers, you need to get the memory safety right. I strongly recommend you don't do this yourself, and use the standard library to take care of it.

You either want std::unique_ptr or std::shared_ptr, depending on whether or not you want to pass the engine by value. I recommend std::unique_ptr until you find that you need std::shared_ptr. You could then implement the class like so:

#include <memory>

...

class EEngine {
public:
    std::unique_ptr<EInput> input;
    std::unique_ptr<EGraphics> graphics;
    std::unique_ptr<EPhysics> physics;

    EEngine()
        : input{ new EInput() }
        , graphics{ new EGraphics() }
        , physics{ new EPhysics() }
    {}
};

Also notice that I used the member initializer list for the constructor; it's good to get in the habit of doing so.

The benefit of this code is that it is now difficult to get the memory safety wrong. If I wrote this function:

void doSomethingWithMyEngine(EEngine engine) {
    ...
}

and tried to call it like before:

void myFunction() {
    EEngine myEngine;
    doSomethingWithMyEngine(myEngine);
}

It would result in a compiler error, since the EEngine class does not have a copy constructor, because we used std::unique_ptr. Instead, the code would have to be used like so:

void doSomethingWithMyEngine(EEngine &engine) {
    ...
}

Or like so:

void myFunction() {
    EEngine myEngine;
    doSomethingWithMyEngine(std::move(myEngine));
    // now myEngine is in an invalid state.
}

This prevents the memory management issues that are present in your code.