Basic OpenGL application that renders multiple cubes on a textured plane, setup for a 3D game

Question

I'm learning OpenGL and started work on a small 3D game (like Sokoban) for practice. I want to know if the design of my OpenGL code reflects a good understanding of the technology. For example, one problem I ran into was how to handle drawing multiple objects with different shaders, which I fixed thus far by using a separate VAO for each type of object. I'm not sure if that's an optimal/scalable solution, hence why I'm here.

Here's a screenshot of what the final application looks like (also includes controls to spherically orbit the camera around the scene):

enter image description here

Currently, my code is organized into 3 parts: main.cpp, game.cpp, and world.cpp. main.cpp only contains the main function, which just creates the Game singleton and calls its core procedures. game.cpp contains the code for initializing the app (using GLFW), loading shaders, and running the game loop, while world.cpp does all of the actual work. The total LOC is about 500, but I'll do my best to abbreviate that. Without further ado, the code:

main.cpp

#include "game.hpp"
int main()
{
 Game& game = Game::Instance();
 game.Initialize();
 game.Run();
 game.Quit();
 return 0;
}

That's it for main.cpp, not much to say here. Game is implemented as a Meyer's singleton, which I mention because I would additionally like guidance/opinion about its use here:

game.hpp

struct Game
{
 static Game& Instance()
 {
 static Game game;
 return game;
 }
 Game(const Game&) = delete;
 Game& operator =(const Game&) = delete;
 GLFWwindow* window;
 std::unordered_map<std::string, unsigned int> shaderFileToId {};
 
 World world {};
 
 void Initialize();
 void Run();
 void Quit();
private:
 void InitializeGLFW();
 void InitializeShaders();
 void CreateShader(std::string filename);
 void ReadShaderFromFile(std::string& shader, std::string file);
 
 void Start();
 
 void DeinitializeGLFW();
 Game() { };
 ~Game() { };
};

The singleton implementation starts at the top and also includes the private constructor and destructor at the bottom. I use it because generally when I make games in C++ I make a couple of system-level classes (like Game and World) that contain data used throughout the program. I've been advised to perhaps just use a namespace and have thought about static classes, but this solution seems nice to me. Thoughts?

Anyway, the implementation of the header is mostly unimportant, so I'll omit the .cpp file, but will mention the shaderFileToId member. I use this map to access shader program IDs by the root name of the shader files. The main problem I find with this method is that the code (as you'll see below) gets pretty long-winded combined with the singleton calls. Any suggestions there would also be appreciated. Next is the world header:

world.hpp // World struct

struct World
{
 GLuint VAOfloor;
 GLuint VAOcube;
 
 glm::mat4 projection;
 glm::mat4 view;
 glm::vec3 eyePolar;
 Floor floor {};
 std::vector<Cube> cubes;
 
 void Initialize();
 void Update();
 void RenderUpdate();
};

The main part of world.hpp is the World struct, which contains the VAO containers for a Floor object and all of the Cube objects that will be placed. So I'm using one VAO for each type of object I'm drawing, and that's because they use different shaders. Before I used multiple cubes, I did have just one VAO, but to make it work I needed to make the layout locations for all attributes between the two shaders unique, otherwise one would overwrite the other. But because both needed a layout for the positions of each vertex, I would have rather used the same location for the position attribute for each shader. That's mostly what motivated me to use separate VAOs, but I'm still not sure of what the proper way to handle the multiple shader/VAO situation is.

The rest of the class isn't super relevant, some variables to control the world transform and camera, containers for the objects, and some basic procedures. However, world.hpp also contains the Floor and Cube structs:

world.hpp // Floor & Cube structs

struct Floor
{
 float cornerBuffer[30] = {
 // x y z s t
 -10.0f, 0.0f, -10.0f, 0.0f, 0.0f,
 10.0f, 0.0f, 10.0f, 10.0f, 10.0f,
 10.0f, 0.0f, -10.0f, 10.0f, 0.0f, // triangle 1
 -10.0f, 0.0f, -10.0f, 0.0f, 0.0f,
 -10.0f, 0.0f, 10.0f, 0.0f, 10.0f,
 10.0f, 0.0f, 10.0f, 10.0f, 10.0f, // triangle 2
 };
 GLuint VAO;
 GLuint buffer;
 GLuint texture;
 glm::mat4 model;
 GLint transformLocation;
 GLint textureLocation;
 void Initialize(GLuint VAOfloors);
 void Render(glm::mat4 renderTransform);
};
struct Cube
{
 float cornerPositions[24] = {
 // x y z
 -1.0f, -1.0f, -1.0f,
 -1.0f, -1.0f, 1.0f,
 1.0f, -1.0f, 1.0f,
 1.0f, -1.0f, -1.0f, // bottom 4 corners
 -1.0f, 1.0f, -1.0f,
 -1.0f, 1.0f, 1.0f,
 1.0f, 1.0f, 1.0f,
 1.0f, 1.0f, -1.0f // top 4 corners
 };
 GLuint indexBuffer[36] = {
 0, 1, 2, 0, 2, 3, // bottom
 4, 5, 6, 4, 6, 7, // top
 0, 1, 5, 0, 5, 4, // left
 3, 2, 6, 3, 6, 7, // right
 1, 5, 6, 1, 6, 2 // front 
 0, 4, 7, 0, 7, 3, // back
 
 };
 GLuint VAO;
 GLuint buffers[2];
 glm::mat4 model;
 GLint transformLocation;
 void Initialize(GLuint VAOcubes);
 void Place(glm::vec3 position);
 
 void Render(glm::mat4 renderTransform);
};

The important parts here are the GL data they contain: they each hold a reference to the VAO that are used to create their renders, as well as the required Buffer Objects and buffer data. One thing to note is that the VAOs aren't created within the struct, but is created by World and then passed to their Initialize methods as arguments. It will become more clear how they are used, but I would also like to know if storing the GL objects within the structures is a good way of storing the data.

Okay, lastly, and the most important part, the implementation of how the objects are created and rendered:

world.cpp // Initialize methods

void World::Initialize()
{
 glGenVertexArrays(1, &VAOfloor);
 floor.Initialize(VAOfloor);
 glGenVertexArrays(1, &VAOcube);
 for (int i = -2; i < 3; i++)
 {
 Cube cube;
 cube.Initialize(VAOcube);
 cubes.emplace_back(cube);
 }
 // ...
}
void Floor::Initialize(GLuint VAOfloors)
{
 VAO = VAOfloors;
 
 glBindVertexArray(VAO);
 int width, height, channelCount;
 unsigned char* data = stbi_load("Assets/Tile.png", &width, &height, &channelCount, 0);
 if (data)
 {
 glGenTextures(1, &texture);
 // ...
 }
 stbi_image_free(data);
 glBindVertexArray(0);
 
 transformLocation = glGetUniformLocation(Game::Instance().shaderFileToId.at("floor"), "transform");
 textureLocation = glGetUniformLocation(Game::Instance().shaderFileToId.at("floor"), "textureSampler");
}
void Cube::Initialize(GLuint VAOcubes)
{
 VAO = VAOcubes;
 
 glBindVertexArray(VAO);
 glGenBuffers(2, buffers);
// ...
 glBindVertexArray(0);
 
 transformLocation = glGetUniformLocation(Game::Instance().shaderFileToId.at("cube"), "transform");
}

The Initialize methods do basically the same thing, although Floor has the added texture details, where they bind the VAO passed from World, fill in the VAO using the buffer data (omitted), and then I get the uniform locations of the shaders for later purposes. I honestly don't know if/how there could be a better way of doing this procedure, although you can see here the verbose nature of retrieving the shader program (might I just accept the wordiness?).

I'd also show the rendering code, but it's super straightforward (bind VAO and program, draw transformed object, unbind) and I think there's enough code here.

Conclusion

That's the gist of the whole program. To summarize, here are some of the concerns I have with the code:

1. Am I correctly/sufficiently making use of multiple VAOs/shaders and is my reason for using them (multiple layouts with the same index) valid?
2. Is it a good idea to contain the VAOs in a larger struct (World) and pass the particular ones to the objects that need them?
3. Is my implementation of the Meyer's Singleton (useful/unnecessary), and could there be a better way of accessing my shader program?
4. More generally, is the organization and design of the program as a whole reasonable?

Answer 1

You don't need a copy of the vertex and index data in every cube object. Only one copy is necessary. So make the cornerPositions, indexBuffer and cornerBuffer static.

At the same time you can share VAO and VBOs between instances of Cubes. In fact the only thing you need to keep per cube is the transform. The only thing that you need to keep per floor is texture (if you want different textures per for each floor you have) and model.

this turns the Floor and Cube into:

struct Floor
{
 static float cornerBuffer[30] = {
 // x y z s t
 -10.0f, 0.0f, -10.0f, 0.0f, 0.0f,
 10.0f, 0.0f, 10.0f, 10.0f, 10.0f,
 10.0f, 0.0f, -10.0f, 10.0f, 0.0f, // triangle 1
 -10.0f, 0.0f, -10.0f, 0.0f, 0.0f,
 -10.0f, 0.0f, 10.0f, 0.0f, 10.0f,
 10.0f, 0.0f, 10.0f, 10.0f, 10.0f, // triangle 2
 };
 static GLuint VAO;
 static GLuint buffer;
 static GLuint texture;
 static GLint transformLocation;
 static GLint textureLocation;
 static void Initialize_static(GLuint VAOfloors);
 glm::mat4 model;
 void Initialize();
 void Render(glm::mat4 renderTransform);
};
struct Cube
{
 static float cornerPositions[24] = {
 // x y z
 -1.0f, -1.0f, -1.0f,
 -1.0f, -1.0f, 1.0f,
 1.0f, -1.0f, 1.0f,
 1.0f, -1.0f, -1.0f, // bottom 4 corners
 -1.0f, 1.0f, -1.0f,
 -1.0f, 1.0f, 1.0f,
 1.0f, 1.0f, 1.0f,
 1.0f, 1.0f, -1.0f // top 4 corners
 };
 static GLuint indexBuffer[36] = {
 0, 1, 2, 0, 2, 3, // bottom
 4, 5, 6, 4, 6, 7, // top
 0, 1, 5, 0, 5, 4, // left
 3, 2, 6, 3, 6, 7, // right
 1, 5, 6, 1, 6, 2 // front 
 0, 4, 7, 0, 7, 3, // back
 
 };
 static GLuint VAO;
 static GLuint buffers[2];
 
 static GLint transformLocation;
 static void Initialize_static(GLuint VAOcubes);
 glm::mat4 model;
 void Initialize();
 
 void Place(glm::vec3 position);
 
 void Render(glm::mat4 renderTransform);
};

The static variables can be allocated to another struct if you really hate static members.

This also means that creating the VBO and uploading data to it only needs to happens once, not once per cube. Hence why I added the Initialize_static.

• GM997: "You don't need a copy of the vertex and index data in every cube object." - That was an egregious mistake I should have noticed. The VAO and general GL sharing between instances though I didn't think about. I'll extract the data from the structs and post a follow up answer with the modified code. +1

Answer 2

Game

• There's no benefit to making Game a singleton. If we want only a single instance of the object, we should just create one object.

• Having Initialize() and Quit() methods is an anti-pattern. Initialization and clean-up are exactly what the constructor and destructor are for.

So the main() function be simply:

int main()
{
 auto game = Game();
 game.Run();
}

The constructor does the initialization, and the destructor does cleanup when the game object is destroyed at the end of the function.

You can pass a reference to Game into places that need it (or better, pass a reference to the specific thing they need, e.g. the list of shaders).

---

World

• Again, use a constructor for initialization.

• It looks like you're unnecessarily duplicating the VAO ids in World. This can easily lead to mistakes (e.g. updating one value and not the other).

• Consider separating the idea of a "model" from the idea of an object instance in your game. Right now you only need a single floor (or textured quad) model, and a single cube model, that you can render several times at various locations. Don't do this by making data static (as per the other answer). Models are assets, and should be grouped with the other assets (i.e. shaders). So:

std::unordered_map<std::string, GLuint> shaders;
FloorModel floor;
CubeModel cube;

Later you might want to decide on a generic model format and load it from file, so you'd replace the two variables above with a second map:

std::unordered_map<std::string, Model> models;

In any case, our World just needs to store transforms for each instance we want to render:

glm::mat4 floorTransform;
std::vector<glm::mat4> cubeTransforms;

---

Floor and Cube

• Again, initialization should be done in a constructor, not a separate Initialize() function.

• cornerBuffer, cornerPositions and indexBuffer don't need to exist as member variables (even static member variables). Variables should have as small a scope as possible, and these can all be local variables in the class constructors.

---

Q/A:

1. See above (basically it's fine, but you only need one Cube model).
2. No! Store them in one place only (with the model).
3. No. The singleton is unnecessary. It's fine to pass references to data as parameters.
4. It's not terrible. Hard to say without seeing more complete code.

---

Some other things:

• A Game::GetShader(std::string const& name) helper function might be nicer than doing the raw map lookup.

• It might be worth writing some simple OpenGL object wrapper classes that you can add to as necessary (e.g. Shader, VAO, VBO etc. - just wrap what you need right now rather than aiming for completeness). That way you don't have as much code duplication, and can cut down on some of the raw OpenGL calls.

• Similarly, loading an image into an OpenGL texture could definitely be a separate function, instead of being hidden in Floor::Initialize().

• GM997: Yeah, storing the buffer data inside the instances is a big mistake I didn't even think about. Couple things I'm confused about though: (1) How would/should I pass the Game reference around? The way I'm imagining it now is just putting *this into whatever functions need it, but that feels fragile. (2) How are the VAOs in world getting duplicated currently? I'll likely take your suggestion of adding a GL wrapper or some way of extracting the GL code from the rest of the game code. +1, I'll probably post an answer with updated code. Thanks!