10
\$\begingroup\$

I'm working my way through some basic OpenGL tutorials and have decided to offload the shader loading/compiling/linking to a separate object that I'll use for the remainder of the tutorial material. Ideally, I'd also like it to be useful later, when I get on with working my own OpenGL projects. And, who knows, maybe someone else would find it useful.

That said, I'd like someone (everyone?) to offer their feedback on the robustness and correctness of this code. I'm shooting for C++11/14 modes and methods as well as useful errors on something going wrong.

One thing I know doesn't work correctly is that if the user tries to create an instance of a Shader object using the constructor that takes file names before they've created an OpenGL context, it simply segfaults. I'm not sure how I can stop OpenGL from doing that, though.

And, of course, if anyone sees any needless complexity, poor practices, unused includes, etc, I'd be happy to have those pointed out too. =)

Header:

#ifndef SHADER_H
#define SHADER_H

#include <cstring>
#include <iostream>
#include <fstream>
#include <streambuf>
#include <stdexcept>

using std::string;
using std::ifstream;
using std::cerr;
using std::endl;
using std::invalid_argument;
using std::runtime_error;
using std::ios;

#include<GL/glew.h>

class Shader
{
private:
  GLuint program = 0, vertex_shader = 0, fragment_shader = 0;
  GLchar *vertex_shader_source = nullptr, *fragment_shader_source = nullptr;
  GLint  vertex_shader_status = GL_FALSE, fragment_shader_status = GL_FALSE,
         shader_program_status = GL_FALSE;

  GLenum gl_error = GL_NO_ERROR;

  void load_shader(const std::string &filename, GLchar *&shader_source);
  void shader_compile_error(const GLuint &vertex_shader,
                            const string &vertex_source,
                            const GLuint &fragment_shader,
                            const string &fragment_source);
  void shader_link_error(const GLuint &shader_program);

public:
  Shader();
  Shader(const string &vertex_filename, const string &fragment_filename);
  ~Shader();

  void create_shader(const string &vertex_filename, const string &fragment_filename);
  void use_program();
};

#endif //SHADER_H

Class definition:

#include "Shader.h"

Shader::Shader(const string &vertex_filename, const string &fragment_filename)
{
  try { create_shader(vertex_filename, fragment_filename); }
  catch(...)
  {
    runtime_error ex("Failed to create shader");
    throw ex;
  }
}

Shader::Shader()
{
}

Shader::~Shader()
{
  if(vertex_shader_source != nullptr) delete[] vertex_shader_source;
  if(fragment_shader_source != nullptr) delete[] fragment_shader_source;

  glDeleteShader(vertex_shader);
  glDeleteShader(fragment_shader);

  glDeleteProgram(program);
}

void Shader::create_shader(const string &vertex_filename,
                           const string &fragment_filename)
{
  try { load_shader(vertex_filename, vertex_shader_source); }
  catch(invalid_argument &ex)
  {
    delete[] vertex_shader_source;
    cerr << "Something went wrong loading the vertex shader "
         << "(" << ex.what() << ")" << endl;
    invalid_argument ex_next("Failed to load vertex shader");
    throw ex_next;
  }
  catch(runtime_error &ex)
  {
    delete[] vertex_shader_source;
    cerr << "Something went wrong loading the vertex shader "
         << "(" << ex.what() << ")" << endl;
    runtime_error ex_next("Failed to load vertex shader");
    throw ex_next;
  }
  catch(...)
  {
    delete[] vertex_shader_source;
    cerr << "Something went wrong loading the vertex shader!" << endl;
    runtime_error ex_next("Failed to load vertex shader");
    throw ex_next;
  }

  try { load_shader(fragment_filename, fragment_shader_source); }
  catch(invalid_argument &ex)
  {
    delete[] fragment_shader_source;
    cerr << "Something went wrong loading the fragment shader "
         << "(" << ex.what() << ")" << endl;
    invalid_argument ex_next("Failed to load vertex shader");
    throw ex_next;
  }
  catch(runtime_error &ex)
  {
    delete[] fragment_shader_source;
    cerr << "Something went wrong loading the fragment shader "
         << "(" << ex.what() << ")" << endl;
    runtime_error ex_next("Failed to load vertex shader");
    throw ex_next;
  }
  catch(...)
  {
    delete[] fragment_shader_source;
    cerr << "Something went wrong loading the fragment shader!" << endl;
    runtime_error ex_next("Failed to load vertex shader");
    throw ex_next;
  }

  vertex_shader   = glCreateShader(GL_VERTEX_SHADER);
  fragment_shader = glCreateShader(GL_FRAGMENT_SHADER);
  glShaderSource(vertex_shader, 1, &vertex_shader_source, nullptr);
  glShaderSource(fragment_shader, 1, &fragment_shader_source, nullptr);
  glCompileShader(vertex_shader);
  glCompileShader(fragment_shader);

  gl_error = glGetError();
  if(gl_error != GL_NO_ERROR)
  {
    cerr << " GLError - " << gluErrorString(gl_error) << " (" << gl_error << ")"
         << " on shader compile" << endl;
  }

  delete[] vertex_shader_source;
  delete[] fragment_shader_source;
  vertex_shader_source = fragment_shader_source = nullptr;

  glGetShaderiv(vertex_shader, GL_COMPILE_STATUS, &vertex_shader_status);
  glGetShaderiv(fragment_shader, GL_COMPILE_STATUS, &fragment_shader_status);

  if((vertex_shader_status == GL_FALSE) || (fragment_shader_status == GL_FALSE))
  {
    shader_compile_error(vertex_shader, vertex_filename,
                         fragment_shader, fragment_filename);

    glDeleteShader(vertex_shader);
    glDeleteShader(fragment_shader);

    runtime_error ex_next("Failed to compile shader");
    throw ex_next;
  }

  program = glCreateProgram();
  glAttachShader(program, vertex_shader);
  glAttachShader(program, fragment_shader);
  glLinkProgram(program);

  gl_error = glGetError();
  if(gl_error != GL_NO_ERROR)
  {
    cerr << " GLError - " << gluErrorString(gl_error) << " (" << gl_error << ")"
         << " on shader link" << endl;
  }

  glDeleteShader(vertex_shader);
  glDeleteShader(fragment_shader);
  glGetProgramiv(program, GL_LINK_STATUS, &shader_program_status);

  if(shader_program_status == GL_FALSE)
  {
    shader_link_error(program);

    glDeleteProgram(program);

    runtime_error ex_next("Failed to link shader");
    throw ex_next;
  }
}

void Shader::use_program()
{
  if(shader_program_status != GL_FALSE)
  {
    glUseProgram(this->program);
  }
  else
  {
    runtime_error ex("Shader program not ready!");
    throw ex;
  }
}

void Shader::load_shader(const std::string &filename, GLchar *&shader_source)
{
  if(filename.empty())
  {
    invalid_argument ex("Invalid shader filename");
    throw ex;
  }

  string shader_string;
  ifstream input_file(filename);

  if(input_file.good() != true)
  {
    runtime_error ex("Unable to open shader source");
    throw ex;
  }

  input_file.seekg(0, ios::end);
  shader_string.reserve(input_file.tellg());
  input_file.seekg(0, ios::beg);
  shader_string.assign((std::istreambuf_iterator<char>(input_file)),
                       std::istreambuf_iterator<char>());

  shader_source = new char[(shader_string.length() + 1)];

  strncpy(shader_source, shader_string.c_str(), (shader_string.length() + 1));
}

void Shader::shader_compile_error(const GLuint &vertex_shader,
                                  const string &vertex_source,
                                  const GLuint &fragment_shader,
                                  const string &fragment_source)
{
  GLint vertex_shader_log_length = 0;
  GLint fragment_shader_log_length = 0;

  cerr << "Something went wrong compiling the shaders!\n" << endl;

  glGetShaderiv(vertex_shader, GL_INFO_LOG_LENGTH, &vertex_shader_log_length);
  glGetShaderiv(fragment_shader, GL_INFO_LOG_LENGTH,
                &fragment_shader_log_length);

  if(vertex_shader_log_length > 1)
  {
    GLchar *shader_log = new GLchar[(vertex_shader_log_length + 1)];
    glGetShaderInfoLog(vertex_shader, vertex_shader_log_length, nullptr,
                       shader_log);
    cerr << vertex_source << ":\n" << shader_log << endl;
    delete[] shader_log;
  }

  if(fragment_shader_log_length > 1)
  {
    GLchar *shader_log = new GLchar[(fragment_shader_log_length + 1)];
    glGetShaderInfoLog(fragment_shader, fragment_shader_log_length, nullptr,
                       shader_log);
    cerr << fragment_source << ":\n" << shader_log << endl;
    delete[] shader_log;
  }
}

void Shader::shader_link_error(const GLuint &shader_program)
{
  GLint linker_log_length = 0;

  cerr << "Something went wrong linking the shaders!\n" << endl;

  glGetProgramiv(shader_program, GL_INFO_LOG_LENGTH, &linker_log_length);

  if(linker_log_length > 1)
  {
    GLchar *linker_log = new GLchar[(linker_log_length + 1)];
    glGetProgramInfoLog(shader_program, linker_log_length, nullptr, linker_log);
    cerr << "Linker Log:\n" << linker_log << endl;
    delete[] linker_log;
  }
}
\$\endgroup\$
8
\$\begingroup\$

A few general comments

Overall, your code has a very "90's" look-n-feel to it. I say that because old codebases and your code tend to have two things in common:

  1. Heavy use of raw pointers and manual memory management (new/delete).
  2. (ab)use of try/catch blocks.

Number one is a no-brainer to fix. Get familiar with smart pointers and modern resource management in C++. The overall concept behind this is called RAII - Resource Acquisition Is Initialization. Resource lifetime management should be automated, as much as possible.

The second issue gets partly solved by RAII, the need to delete those pointers will be gone, and so will half of your try/catch blocks. But also to get a better grasp of exception usage and handling in C++, I recommend taking some time to watch this presentation when you have the chance: “The Unexceptional Exceptions".

Specifics

This is bad, specially in a header file:

using std::string;
using std::ifstream;
using std::cerr;
using std::endl;
using std::invalid_argument;
using std::runtime_error;
using std::ios;

You don't want to force users of your code to import all those names into the global namespace. Say I have a class named string in my own code, if I include your header file, havoc will ensue. std:: qualify your uses of the Standard library instead, it will make your code more robust. Related: Why is “using namespace std;” considered bad practice?


Easy on the statefull objects. The less transient state you keep around inside your classes, the better. Not only it will save memory, but also make your code easier to reason about. Mutable state is a huge source of bugs, so force yourself to pass stuff around as function parameters as much as you can and slap const in front of the rest.

It's very unlikely that keeping the gl_error around will have any use once the error is printed. Same for the shader compile statuses. You can infer if the shaders are valid by simply checking that the handle is nonzero. I would also try to avoid keeping copies of the shader source code around. My GL shader classes are usually comprised of just the program handle. The shader handles themselves are pretty useless once attached to a program, so you can even dispose of them after shader compilation.


One thing I know doesn't work correctly is that if the user tries to create an instance of a Shader object using the constructor that takes file names before they've created an OpenGL context, it simply segfaults.

Easy fix by making dependencies explicit. Define a GLContext class that users have to instantiate somehow (probably by passing it some window handle that can only be acquired after creating a hardware context).

Shaders must then receive a GLContext instance upon creation, which you can then assert that it is valid.


Other minutiae:

  • Empty constructor could be removed or set to = default; in the declaration.

  • You can make your exception throwing code one-liners: throw ExceptionType("message");. No need for the local exs all around.

  • You should probably delete the copy constructor and assignment operator, but your class could be movable. Read about the Rule of 3/5/0.

\$\endgroup\$
1
\$\begingroup\$
  • it's useless to pass ref to GLuint as sizeof(GLuint&) >= sizeof(GUuint)
  • it's safer to change load_shader to std::string load_shader(const std::string& filename) - so no possible leaks and manual memory management. pls note that move constructor will be used.
  • as we are talking about low level realtime graphics - it's better not to use exceptions.
  • make logging optional and add information about source object
  • consider to add functionality for other shader types
  • consider to add functionality to setup shader program parameters, obtain shader program handle
  • create simple application that uses your class to test it usefulness
\$\endgroup\$

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.