3
\$\begingroup\$

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);
}
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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 \$\endgroup\$ – MattMatt Aug 15 '15 at 12:36

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.