Hashmap of objects or references?

[Noise]

This is an efficiency question. I am going to have say 100 to 200 3D models, each an instance of a Model class. How should I store them, as a map of the actual instances or as a map of pointers to the models, where each one has been allocated it's own space on the heap?
So either:

map<int, Model> models;

OR

map<int, Model*> models;

If the map contains the actual models, then when I create them it will need to copy each model into the data structure after creating, possibly with a temporary in between which might take a long time. Is it bad to have them scattered all over memory in different places?

[phantomotap]

*sigh*

I can't wait for "r-value references".

Is it bad to have them scattered all over memory in different places?

You are using a 'std::map' which virtually has to be implemented using a tree structure to match the requirements. (Other possibilities exist, I just don't find them very often.)

In other words, whether you use a pointer to 'Model' or just 'Model' the memory representing the instances is almost certainly "scattered all over memory in different places".

You can, if you think it may give a noticeable performance boost, allocate an array of 'Model' instances and store pointers to those instances in your 'std::map'. Don't guess though, try out a few tests and see for yourself. (It really depends on a lot of factors.)

Soma

[Noise]

Thanks phantomotap, I will use a map of Model pointers - to avoid the copying when loading.

I don't think having them contiguous in memory would help anyway, I'm not going to be iterating over them.

[iMalc]

Copying an instance of your Model class into a map<int, Model> doesn't really have to be inefficient at all. You can:

1. Instead of setting it up in a temporary variable and then inserting that, you can first insert a default constructed object, and then set it up through a reference to that.
2. Or you can set it up using a temporary variable, but insert it into the map by first inserting a default-constructed instance, and then using a specialisation of std::swap to put it into the map.

Either way, by storing objects rather than pointers you can forget the possibility of leaks, and the syntax to access an item is nicer too.

I'd go for the first option, but perhaps implement a no-throwing std::swap specialisation as well.
It might help in discussing this if you post the definition of your class.

[Noise]

Ok I don't fully understand what you mean with your suggestions there. Here is my Model class (yes I know I am using structs with classes but I enjoy their simplicity):

using namespace std;

struct modelFace {
    int v[3];
    int vt[3];
    int vn[3];
    int matIndex;
    
};

struct modelVertex {
    GLfloat x;
    GLfloat y;
    GLfloat z;
};

struct modelMaterial {
    string name;
    
    GLfloat specExponent;
    GLfloat optDensity;
    GLfloat ambRefl[4];
    GLfloat difRefl[4];
    GLfloat specRefl[4];
    GLfloat transFilter[4];
    GLuint difMap;
    GLuint specMap;
    GLuint bumpMap;
    bool bump;
};

class Model{
public:
    Model();
    Model(const string &filename);
    Model(const string &filename, int id);
    ~Model();
    void draw();
    bool load(const string &filename);
    static bool load_tex(const string &filename, GLuint *texture, bool buildMipmaps);
    
    GLuint getDisplayList();
    GLint getShaderProgram();
    
    void setShaderProgram(GLint sp);
    
private:
    bool load_mtl(const string &filename);
    
    void drawFace(const modelFace &face);
    bool isFacePoint(const string &s);
    void getFacePoint(const string &s, int &vertexIndex, int &textureIndex, int &normalIndex);
    
    int getMaterial(const string &name);
    
    void useMaterial(int i);
    
    //Members
    GLuint displayList;
    
    GLint shaderProgram;
    
    modelFace *faces;
    modelVertex *vertices;
    modelVertex *texels;
    modelVertex *normals;
    modelMaterial *materials;
    
    int numFaces;
    int totalVertices;
    int totalTexels;
    int totalNormals;
    int numMaterials;
    int numTextures;
};

Memory leaks aren't going to be a big deal, because the instances are needed throughout the whole running of the program and get deleted by the "ResourceManager's" destructor.

[phantomotap]

I don't intend to put words in his mouth, but presumably, he means using something like the following example.

The fancy 'Model' constructor call followed by a chained call to the 'swap' method constructs a 'Model' object in all its glory, swaps the data pointer owned by the just constructed 'Model' instance with the data pointer of the 'Model' instance owned by the 'std::map', and finally calls the destructor for the local object.

The default constructor sets the data pointer to null. The destructor simply calls 'delete' on the pointer. The 'swap' method swaps the data pointers.

An exception safe 'swap' method is just an elegant means of having two instances exchange ownership of the objects they represent.

In this case, the availability of the option means that 'std::map' can cheaply construct the 'Model' instances it needs and you can construct the instances you want without needing to worry about the fiddly bits--like possibly expensive temporaries and complex assignment operators.

Soma

struct modelFace {
    int v[3];
    int vt[3];
    int vn[3];
    int matIndex;
    
};

struct modelVertex {
    GLfloat x;
    GLfloat y;
    GLfloat z;
};

struct modelMaterial {
    string name;
    
    GLfloat specExponent;
    GLfloat optDensity;
    GLfloat ambRefl[4];
    GLfloat difRefl[4];
    GLfloat specRefl[4];
    GLfloat transFilter[4];
    GLuint difMap;
    GLuint specMap;
    GLuint bumpMap;
    bool bump;
};

struct ModelMembers
{
    GLuint displayList;
    
    GLint shaderProgram;
    
    modelFace *faces;
    modelVertex *vertices;
    modelVertex *texels;
    modelVertex *normals;
    modelMaterial *materials;
    
    int numFaces;
    int totalVertices;
    int totalTexels;
    int totalNormals;
    int numMaterials;
    int numTextures;
};

class Model{
public:
    Model();
    Model(const string &filename);
    Model(const string &filename, int id);
    ~Model();
    void draw();
    bool load(const string &filename);
    static bool load_tex(const string &filename, GLuint *texture, bool buildMipmaps);
    
    GLuint getDisplayList();
    GLint getShaderProgram();
    
    void setShaderProgram(GLint sp);
    
private:
    bool load_mtl(const string &filename);
    
    void drawFace(const modelFace &face);
    bool isFacePoint(const string &s);
    void getFacePoint(const string &s, int &vertexIndex, int &textureIndex, int &normalIndex);
    
    int getMaterial(const string &name);
    
    void useMaterial(int i);

public:
    
    void swap
    (
        Model & other_f
    )
    {
        using std::swap;
        swap(data_m, other_f.data_m);
    }

private:    
    //Members
    ModelMembers * data_m;
    
};

void swap
(
    Model & m1_f,
    Model & m2_f
)
{
    m1_f.swap(m2_f);
}

int main()
{
    std::map<int, Model> my_map;
    // ...
    Model("FileName", ID /*, Whatever, Other, Parameters */).swap(my_map[GIANT_BUG]);
    // ...
}

[iMalc]

Here is an example of option 2 I described:

namespace std {
    template<> inline void swap(Model &a, Model &b) throw() {
        std::swap(a.displayList, b.displayList);
        std::swap(a.shaderProgram, b.shaderProgram);
        std::swap(a.faces, b.faces);
        std::swap(a.vertices, b.vertices);
        std::swap(a.texels, b.texels);
        std::swap(a.normals, b.normals);
        std::swap(a.numFaces, b.numFaces);
        std::swap(a.totalVertices, b.totalVertices);
        std::swap(a.totalTexels, b.totalTexels);
        std::swap(a.totalNormals, b.totalNormals);
        std::swap(a.numMaterials, b.numMaterials);
        std::swap(a.numTextures, b.numTextures);
    }
};

// ...

map<int, Model> models;

Model myModel("Monster.mdl");
std::swap(models[42], myModel);

phantomotap shows another very good way to do it.

Note that you should still also make sure you follow the rule of three here. You needed to implement the destructor, so you also need to either implement the copy-constructor and assignment operator, OR you should declare them but make them private and leave them unimplemented, to prevent them from ever being used.