3
\$\begingroup\$

I have been coding in a certain style for some time now, where I try to avoid classes and stick to pure data structures whenever possible. Basically every module of my program has its own namespace in which all the data structures and functions are declared.

I started doing this because I wanted to focus on the actual data that gets processed and wanted to abstract data as little as possible. Performance is very important to me and I would like to develop a minimal, cache- and compiler-friendly style and I need someone to help me understand what I actually do by structuring my code like this.


So this is how I would structure the functionality of loading 3D meshes into my program for example. The actual code is very abbreviated because right now I would like to only focus on the structure with the namespace and the external declarations.

The header Mesh.h

#include "OpenGL.h" //a higher level namespace containing context information
namespace OpenGL{
    namespace Mesh{

struct Vertex{
    vec3 pos;
    vec3 normal;
    vec2 uv;
};

struct Material{
    vec3 amb;
    vec3 diff;
    vec3 spec;
    float shininess;
};
struct Mesh{
    unsigned int indexOffset;
    unsigned int indexCount;
    unsigned int materialIndex;
};
extern vector<Vertex> allVertices;
extern vector<unsigned int> allIndices;
extern vector<Material> allMaterials;
extern vector<Mesh> allMeshes;

extern string MESH_DIR;
extern vector<string> meshNames; //find mesh index by name


void setMeshDirectory(string pDirectory);

void loadAllMeshes();
Mesh loadMesh(string pFilename);
void addMesh(string pFileName);
    }
}

The code file

#include "Mesh.h"
#define DEFAULT_MESH_DIR "..//assets//meshes//"
//define all external variables
vector<Vertex> allVertices = {};
vector<unsigned int> allIndices = {};
vector<Material> allMaterials = {};
vector<Mesh> allMeshes = {};

string MESH_DIR = DEFAULT_MESH_DIR;
vector<string> meshNames = {};

void OpenGL::Mesh::setMeshDirectory(string pDirectory){
    MESH_DIR = pDirectory;
}

void OpenGL::Mesh::addMesh(string pFileName){
    auto it = find(meshNames.begin(), meshNames.end(), pFileName);
    if(it == meshNames.end()){
        meshNames.push_back(pFileName);
    }else{
    //error
    }
}

void OpenGL::Mesh::loadAllMeshes(){
    allMeshes.resize(meshNames.size()); //resize allMeshes array in advance
    for(unsigned int m = 0; 0 < meshNames.size(); ++m){
        allMeshes[m] = loadMesh(meshNames[m]);
    }
}

OpenGL::Mesh::Mesh OpenGL::Mesh::loadMesh(string pFilename){
    Mesh mesh;
    mesh.indexOffset = allIndices.size();
    unsigned int indexCount;
    unsigned int materialIndex;

    //use assimp to actually load the mesh from an .obj, .3DS, .fbx (etc) file
    //fill allMaterials, allVertices and allIndices with the data from this mesh

    mesh.indexCount = indexCount;
    mesh.materialIndex = materialIndex;
    return mesh;
}
\$\endgroup\$
  • \$\begingroup\$ Do not edit the code in your question after someone has answered. \$\endgroup\$ – Justin Jul 6 '17 at 0:00
  • \$\begingroup\$ Im sorry, but i said in the question that this was abbreviated code, which was meant to be understood as "pseudo code". The whole point of the question was the overall structure of the module, not the grammar. \$\endgroup\$ – stimulate Aug 10 '17 at 8:18
  • 2
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 Aug 10 '17 at 8:48
6
\$\begingroup\$

Your header file is missing include guards. Either put #pragma once at the start, or use the more portable:

#ifndef MESH_H_
#define MESH_H_

// ... rest of header

#endif

MESH_H_ has to be unique for the file.


namespace OpenGL{

If you don't own OpenGL, I'd recommend not naming your namespace after it. If they release OpenGL in a namespace, your code wouldn't be compatible. Try to use a namespace for your project. Similarly, I'd recommend not naming your header OpenGL.h, unless you put the header in a directory.


extern vector<Vertex> allVertices;
extern vector<unsigned int> allIndices;
extern vector<Material> allMaterials;
extern vector<Mesh> allMeshes;

extern string MESH_DIR;
extern vector<string> meshNames; //find mesh index by name

I'm assuming that in OpenGL.h, you have using namespace std;. Don't do this. It is especially bad in a header.

Also, if you find yourself using extern globals, you are almost certainly doing it wrong. These should either not exist at all (as in the calling code takes care of it), or they should be wrapped in a class that the calling code passes around.

These globals and the following point out that you really need to define a class. Maybe something like this:

class MeshManager { // this name is suspicious; "Manager" in a name is usually bad.
    std::vector<Vertex> vertices;
    std::vector<unsigned int> indices;
    std::vector<Material> materials;
    std::vector<Mesh> meshes;

    std::string meshDirectory;
    std::vector<std::string> meshNames;

public:
    void setMeshDirectory(std::string directory);
    void loadAllMeshes();

    Mesh loadMesh(std::string filename);
    void addMesh(std::string filename);
};

Note that this design isn't the best. You still need a constructor, and possibly accessors on the member variables. Also, setMeshDirectory and loadAllMeshes might be better as a constructor to MeshManager, or a static factory function if you prefer that style.

\$\endgroup\$
  • \$\begingroup\$ Are these variables global? I thought they are local to the namespace. Also, i dont want to use classes because then i would need an instance of it somewhere, which is unnecessary because i will only ever need one instance of this class. I could make all the variables in the class static, but then it is practically the same as a namespace, isn´t it? \$\endgroup\$ – stimulate Jul 5 '17 at 19:31
  • 1
    \$\begingroup\$ @stimulate Even though they aren't in the global namespace, they are globals. There is only 1 instance of each of them, and they are fully accessible outside. \$\endgroup\$ – Justin Jul 5 '17 at 19:32
  • 2
    \$\begingroup\$ @stimulate "[I don't] want to use classes because then [I] would need an instance of it somewhere". Whether or not you use the class, you still have an instance. Use the class. In your main function, or wherever you need to use this, you can write MeshManager manager; and now you have your instance \$\endgroup\$ – Justin Jul 5 '17 at 19:33
  • 3
    \$\begingroup\$ stimulate, to put them in a namespace you only mitigate name clashes but you do not prevent any of the others badness of global variables. Everyone will have free access to those variables, they're accessible to anyone without any protection and everyone using them is tightly coupled to all the others (just to mention few). Advantage of classes (isn't it the reason you picked C++ instead of C?) is that with classes you mitigate those problems (of course you can still design them badly and suffer of the same issues) \$\endgroup\$ – Adriano Repetti Jul 6 '17 at 10:53
  • 2
    \$\begingroup\$ Not to mention that sooner or later (sooner?!) you will need to test your code and to mock and test in isolation components coupled with global variables (or static fields!) is pretty a pain in... \$\endgroup\$ – Adriano Repetti Jul 6 '17 at 10:54
1
\$\begingroup\$

I find your approach of using a namespace instead of a class a good fit for for your purpose. I recently started to use it as an alternative to the standard singleton pattern.

Regarding the global vectors, I would declare them directly in the implementation file, making them accessible by the implementation only: actually, they are an implementation detail. Then you can expose them with proper functions: I'd provide a pair of iterators.

This way you can avoid the (massively documented) pitfalls of using globals, but you are still able to access your data: the fact you're the only one working on this code is not a valid reason to ignore best practices.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.