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.

• MattMatt: 1) eclipse reorganises my file architecture so it uses <>, because headers are set to the include path of my program : looks like : -include/alpha set to include path, and src/... is compiled