Multiplayer GameObject design

I created a really basic game class. A game has GameObject instances, which currently have a position only.

This code is running on the client, and in each loop iteration:

• The client receives the updates from the server

• It executes the received events

• The client updates the game state

I created a map to store game objects. The reason why I chose std::map is because I need to access game objects by their index, to update them. And sometimes an object may be deleted, so there would be holes in the std::vector if I would use it.

I created an Updateable class, so when I execute events, and one of them is affected by the properties of the other objects, then the execution order does not matter (it may be parallelized later).

The classes were originally in separate files, and have header files and a namespace too. I just put it together, to show them:

```
#include
#include
#include

/ Vec2 struct /
struct Vec2 {
double x;
double y;

Vec2() {
x=0.0;
y=0.0;
}

Vec2(double x, double y) :
x(x),
y(y)
{}

friend Vec2 operator+(const Vec2 &v1, const Vec2 &v2) {
return Vec2(v1.x+v2.x, v1.y + v2.y);
}
};

/ Updateable class /
template
class Updateable {

T current;
T next;

public:

void setNext(T val) {
next = val;
}

T getCurrent() const {
return current;
}

T getNext() const {
return next;
}

void update() {
current = next;
next = T();
}

};

/ GameObj class /
class GameObj {

const int obj_id;

public:

Updateable position;

GameObj(int obj_id) : obj_id(obj_id) {

}

virtual ~GameObj() {}

};

/ Event class /
class Game;

class Event {
public:

virtual void do_event(Game* game) const = 0;

virtual ~Event() {}

};

/ Game class /
class Game {

std::queue events;

std::map objects;

public:

GameObj* findObj(int id) {
auto it = objects.find(id);

Don't roll your own math libraries

When making a game you will need a lot of math, rolling your own is time consuming and error prone and unless you've got a very strong background in computational mathematics likely to be slower than using a library.

Personally I often use Eigen.

Use default constructors to reduce code

You can replace this:

double x;
double y;

Vec2() {
    x=0.0;
    y=0.0;
}

Vec2(double x, double y) : 
    x(x),
    y(y)
{}

by this with the exact same functionality:

double x = 0.0;
double y = 0.0;

Vec2() = default;

Vec2(double x, double y) 
  : x(x), y(y)
{}

If you are going to roll your own math you need to provide all the operators for vectors. When implementing operators there are certain tricks that you can use to reduce code. For example you can implement all the relational operators (not that you would for a vector, just showing a concept) like this:

bool opeartor  (const T& rhs) const {
   return rhs = (const T& rhs) const {
   return !(*this  < rhs); // Implement using '<'
}

And for example you can implement addition like this:

T& operator += (const T& rhs){
    // Implement this
}

T operator + (const T& rhs) const {
    T ans(*this);
    ans += rhs; // Implement using copy ctor and '+='
    return ans;
}

Once you realise the above you can use the Barton-Nackman trick for making all these operators easy to implement in many classes.

Using raw pointers is bad

Using raw pointers is considered bad practice in modern C++. For some interesting reading check this question.

By using smart pointers (such as std::shared_ptr since C++11) your code would simplify as you would no longer have to care about calling delete at various points so cleanup would be easier and add would simplify too.

Use std::unordered_map

The standard library std::map is implemented using a tree (typically a Red-Black tree. As such it has logarithmic insertion, lookup and removal. But whats worse than the logarithmic time complexity is that it is a tree and traversing a tree is prone to cache misses which will severely impact your performance.

On the other hand std::unordered_map has (amortised) constant time lookup, insertion and removal which is faster and because it has to be implemented using a Hash Table the elements will be in a more or less contiguous memory region making your CPU cache perform better.

Use enhanced for loops

Replace this kind of code:

// update objects
    for(auto iterator = objects.begin(); iterator != objects.end(); iterator++) {
        iterator->second->position.update();
    }

with this:

// update objects
    for(auto& object : objects) {
        object.second->position.update();
    }

Use of white space

You have a lot of randomly added line breaks, this makes your code difficult to read because I have to scroll a lot. Please keep your code tidy.

Don't do unnecessary work

For example in your Game destructor you do this:

~Game() {

    while(!events.empty()) {
        Event* event = events.front();
        events.pop();
        delete event;
    }

    for(auto iterator = objects.begin(); iterator != objects.end(); iterator++) {
        delete iterator->second;
    }

    objects.clear();

}

If you were using std::shared_ptr you could replace all of the above by:

~Game() = default;

because both the queue and objects's destructors will clear them and if each of them only contain objects that clean up them selves like std::shared_ptr then you don't need to do anything.

I will refrain from reviewing high level concepts of the implementation because I think it is too hard get an overview of your code due to how it is formatted.

StackExchange Code Review Q#149862, answer score: 5