Storing struct instances in a std::map

Question

I'm trying to map some structs to some other instances, like this:

template <typename T>
class Component {
public:

    typedef std::map<EntityID, T> instances_map;

    instances_map instances;

    Component() {};

    T add(EntityID id) {
        T* t = new T();
        instances[id] = *t;
        return *t;
    };  
};

Then I use it like this:

struct UnitInfos {
    int owner_id;
    int health;
    float x, y;
};

class LogicComponent : public Component<UnitInfos> {};

The problem is that when it later retrieve data later on, like this:

comp.instance[id];

I get a breand new object with properties initialized at default values.

Is there something inherently wrong with this piece of code, or am I leaving out information about the problem?

---

As per @aaa suggestion, i change the code to

typedef std::map<EntityID, T> instances_map;
instances_map instances;
T& add(EntityID id) {
    instances[id] = T();
    return instances[id];
};

but when I access it

UnitInfos &info = logic_c.instances[id];

the value of info.x is still 0. Any pointers?

---

The problem was how I stored the reference to LogicComponent in another class. using LogicComponent logic_c; instead of LogicComponent& logic_c;. It now works, but I'm storing pointers in the map (instead of @aaa's suggestion). Is this a bad idea?

Answer 1

Clarify the operations you want to perform on LogicComponent. Assuming you are trying to achieve something like this:

Step 1: Add a new entry to the map:

LogicComponent comp; 
EntityID id = 99;
UnitInfos info = comp.add(id);

Step 2: Initialize the info:

info.x = 10.0;
info.y = 11.0
// etc

Step 3: Get the info object again:

UnitInfos info2 = comp.instances[id]; // this is uninitialized.

Then, a few code comments are in order:

The info object returned by comp.add is a COPY of the object you added to the map. By modifying it, you are not modifying what is in the map.

The simplest fix is to create a map of pointers to the object instead of the object itself.

typedef std::map<EntityID, T*> pinstances_map;

T * add(EntityID id) {
    T* t = new T();
    instances[id] = t;
    return t;
};  

// initialize as 
UnitInfo *info = comp.add(id);
info->x = 10.0;
info->y = 11.0;

// retrieve as 
UnitInfos *info = comp.instances[id];

Also, do use an accessor method to get the mapped value, instead of exposing the map object as public. Make the instances variable protected, and add a public get() method.

Edit: This code works fine for me:

#include <map>
#include <iostream>
using namespace std;

template<typename T>
class Component
{
public:
        typedef map<long, T*> pinstances_map;
        pinstances_map instances;

        T * add(long id)
        {
                T *t = new T();
                instances[id] = t;
                return t;
        }
};

struct UnitInfo 
{
        float x, y;
};

class LogicComponent: public Component<UnitInfo> {};

int main()
{
        LogicComponent comp;
        UnitInfo *info = comp.add(99);
        info->x = 10.0;
        info->y = 11.0;

        UnitInfo *info2 = comp.instances[99];
        cout << info2->x << " " << info2->y;

        return 0;
}

Answer 2

It sounds like you defined your indexing operator as:

template <typename T>
T& Component::operator[]( EntityID id )
{
    return instances[id];
}

Or something like that.

The likely unexpected effect of this is that it will automatically insert default-constructed instance of T into the map and then return it for non-exising entries. This is done in std::map so natural assignment syntax like instances[10] = t; works.

The key point here is constness. Define it exactly as above except returning by value and with a const attribute:

template <typename T>
T Component::operator[]( EntityID id ) const
{
    return instances[id];
}

This way you will get an exception when you try retrieving by non-existing key. Better yet, just typedef it like bellow and be done with it:

typedef std::map<EntityID,UnitInfos> EntityUnitMap;

Others already mentioned that you don't need to dynamically allocate an object - you store a copy in the container anyway - and that you leak memory when you do that.

Answer 3

might be that

T add(EntityID id) {
    T* t = new T();
    instances[id] = *t;
    return *t;  // return value and map instance are not the same anymore
};  

should be

T& add(EntityID id) {
    instances[id] = T();
    return instances[id];
};