Simple CubeMap opengl wrapper class

Question

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 <GL/glew.h>
#include <string>

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 <alpha/CubeMap.h>
#include "alpha/libs/stb_image.h"
#include "alpha/LogManager.h"
#include <cassert>

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;


    glGenTextures(1, &m_texture);
    glBindTexture(GL_TEXTURE_CUBE_MAP, m_texture);

    for (unsigned i = 0; i < (sizeof(GL_TEXTURE_CUBE_MAP_TYPES) / sizeof(GL_TEXTURE_CUBE_MAP_TYPES[0])); ++i){
        int w, h, bytesPerPixel;
        unsigned char*  data = stbi_load((m_fileNames[i].c_str()), &w, &h, &bytesPerPixel, 4);

        if(data == nullptr){
            LOG_ERROR_INFO("CubeMap(Texture)", "Failed to load Texture from file", m_fileNames[i]);
                assert(0);
        }
        glTexImage2D(GL_TEXTURE_CUBE_MAP_TYPES[i], 0, GL_RGB, w, h, 0, GL_RGBA, GL_UNSIGNED_BYTE, data);
        stbi_image_free(data);
    }

    glTexParameteri(GL_TEXTURE_CUBE_MAP, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
    glTexParameteri(GL_TEXTURE_CUBE_MAP, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
    glTexParameteri(GL_TEXTURE_CUBE_MAP, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
    glTexParameteri(GL_TEXTURE_CUBE_MAP, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
    glTexParameteri(GL_TEXTURE_CUBE_MAP, GL_TEXTURE_WRAP_R, GL_CLAMP_TO_EDGE);

    LOG_INFO("CubeMap(Texture)",  ": Has init successfully : ID is", m_texture );
}

CubeMap::~CubeMap() {
    if(m_texture != 0){
        glDeleteTextures(1, &m_texture);
        LOG_INFO("CubeMap(Texture)",  ": Delete texture object successfully : ID was", m_texture );
    }
}


void CubeMap::Bind(GLenum unit){
    assert(unit >= 0 && unit <= 31);
    glActiveTexture(unit);
    glBindTexture(GL_TEXTURE_CUBE_MAP, m_texture);
}

Answer 1

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 <alpha/CubeMap.h>, 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<class T, std::size_t N>
  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 <= 31). There's actually a maximum value you can query with glGetInteger, which I don't recall now, but it is quite large, so you can indeed be lazy and use a constant instead if you want. But it would still be better to define a MaxTextureUnits somewhere to make the value self-explanatory for the non-OpenGL versed.