HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Simple CubeMap opengl wrapper class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
simplewrappercubemapopenglclass

Problem

I recently added this CubeMap class to my engine to make handling skyboxes easier. I would like to ask if my OpenGL coding semantics are good, and if I could improve this class in any way.

CubeMap.h:

/*
 * CubeMap.h
 *
 *  Created on: Aug 9, 2015
 *      Author: mattmatt
 */

#pragma once

#include 
#include 

static const GLenum GL_TEXTURE_CUBE_MAP_TYPES[6] =
                    {
                            GL_TEXTURE_CUBE_MAP_POSITIVE_X,
                            GL_TEXTURE_CUBE_MAP_NEGATIVE_X,
                            GL_TEXTURE_CUBE_MAP_POSITIVE_Y,
                            GL_TEXTURE_CUBE_MAP_NEGATIVE_Y,
                            GL_TEXTURE_CUBE_MAP_POSITIVE_Z,
                            GL_TEXTURE_CUBE_MAP_NEGATIVE_Z,
                    };

class CubeMap {
public:
    CubeMap(
                const char* posXFile,
                const char* negXFile,
                const char* posYFile,
                const char* negYFile,
                const char* posZFile,
                const char* negZFile
            );
    ~CubeMap();

    void Bind(GLenum unit = 0);

    void operator=(const CubeMap& other) = delete;
    CubeMap(const CubeMap& other) = delete;

private:
    GLuint m_texture;
    std::string m_fileNames[6];
};


CubeMap.cpp:

```
/*
* CubeMap.cpp
*
* Created on: Aug 9, 2015
* Author: mattmatt
*/

#include
#include "alpha/libs/stb_image.h"
#include "alpha/LogManager.h"
#include

CubeMap::CubeMap (
const char* posXFile,
const char* negXFile,
const char* posYFile,
const char* negYFile,
const char* posZFile,
const char* negZFile
)
: m_texture(0)
{

m_fileNames[0] = posXFile;
m_fileNames[1] = negXFile;
m_fileNames[2] = posYFile;
m_fileNames[3] = negYFile;
m_fileNames[4] = posZFile;
m_fileNames[5] = negZFile;

glGenTextur

Solution

It looks fine. Simple but gets the job done. A few things to look into:

-
GL_TEXTURE_CUBE_MAP_TYPES constant array could be moved to the source file, to hide that implementation detail, assuming it is not meaningful outside the class. I would also use a different name. It looks too much like an OpenGL constant, so it might be misleading. ALL_UPPERCASE is also usually associated with macro names. Not the case here.

-
This #include , it should probably be between quotes, since it is a local header file of your project.

-
Since you are storing all those pos...File parameters of the constructor into std::strings, you might as well take an array of strings, or even a vector/array to be more C++-strict. Or at least replace the char*s by strings.

-
The correct signature of the assignment operator is:

CubeMap& operator = (const CubeMap& other) = delete;
^^^^^^^^


Returning a reference to the class type. Even though deleted, declare it properly to make sure the compiler recognises it as the default assignment op.

-
To avoid having to write the lengthy and error prone (sizeof(GL_TEXTURE_CUBE_MAP_TYPES) / sizeof(GL_TEXTURE_CUBE_MAP_TYPES[0])) you can use std::array (C++11), which has a size() method, or define a template function to make it less verbose:

template
constexpr std::size_t arrayLength(const T (&)[N])
{
    return N;
}


-
That assert(0) in the error case after stbi_load seems a bit haphazard. If the image fails to load on "release", you'll pass a null pointer to glTexImage2D. That will still allocate a texture image with undefined contents. That's not really helpful to the user. The error is already being logged, so you could either set a default placeholder image to visually signify the error, or simpler, just throw an exception and let the caller decide what to do.

-
That 31 is very hardcoded in `assert(unit >= 0 && unit

Code Snippets

CubeMap& operator = (const CubeMap& other) = delete;
^^^^^^^^
template<class T, std::size_t N>
constexpr std::size_t arrayLength(const T (&)[N])
{
    return N;
}

Context

StackExchange Code Review Q#100972, answer score: 4

Revisions (0)

No revisions yet.