patterncppMinor
Simple CubeMap opengl wrapper class
Viewed 0 times
simplewrappercubemapopenglclass
Problem
I recently added this
CubeMap.h:
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
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:
-
-
This
-
Since you are storing all those
-
The correct signature of the assignment operator is:
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
-
That
-
That 31 is very hardcoded in `assert(unit >= 0 && unit
-
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.