2
\$\begingroup\$

I'm working my way through the examples in an OpenGL website; I'm now on tutorial 2 ("the first triangle").

Among the (many) things I've changed from the sample include:

  • Replacing the hodge-podge of C and C++ with pure C
  • Adding more error handling output
  • Adding a makefile
  • Moving all of the pertinent code from their common folder into the main file for legibility of beginners
  • Renaming the shader files to end in .glsl
  • Other minor tweaks.

I'm open to constructive feedback of any kind. The complete source is on GitHub.

makefile

#!/usr/bin/make -f

cflags=-ggdb -Wall -std=c17

all: 02

# These are used instead of implicit rules, for clarity    
02: 02.o makefile
    gcc $(cflags) -o $@ $< $(shell pkg-config --libs glew glfw3)
02.o: 02.c makefile
    gcc $(cflags) -o $@ $< $(shell pkg-config --cflags glew glfw3) -c

simple-fragment.glsl

#version 330 core

// Output data
out vec3 color;

void main()
{
    // Output color = red 
    color = vec3(1,0,0);
}

simple-vertex.glsl

#version 330 core

// Input vertex data, different for all executions of this shader.
layout(location = 0) in vec3 vertexPosition_modelspace;

void main() {
    gl_Position.xyz = vertexPosition_modelspace;
    gl_Position.w = 1;
}

02.c

#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>

#include <GL/glew.h>
#include <GLFW/glfw3.h>


static void glfwCB(int error, const char *desc)
{
    fprintf(stderr, "GLFW error 0x%08X: %s\n", error, desc);
}

static GLuint loadShader(const char *fn, GLenum shaderType)
{
    printf("Compiling shader '%s'...\n", fn);

    GLuint shaderID = glCreateShader(shaderType);
    if (!shaderID) {
        fprintf(stderr, "Failed to create shader\n");
        exit(1);
    }

    FILE *f = fopen(fn, "r");
    if (!f) {
        perror("Failed to load shader file");
        exit(1);
    }
    if (fseek(f, 0, SEEK_END)) {
        perror("Failed to get file size");
        exit(1);
    }
    GLint size[1] = {ftell(f)};
    if (size[0] == -1) {
        perror("Failed to get file size");
        exit(1);
    }
    rewind(f);
    char *source = malloc(size[0]);
    if (!source) {
        perror("Failed to allocate source memory");
        exit(1);
    }
    if (fread(source, 1, size[0], f) != size[0]) {
        perror("Failed to read file");
        exit(1);
    }
    if (fclose(f))
        perror("Warning: failed to close source file");

    const GLchar *rosource = source;
    glShaderSource(shaderID, 1, &rosource, size);
    free(source);

    glCompileShader(shaderID);

    GLint logLength;
    glGetShaderiv(shaderID, GL_INFO_LOG_LENGTH, &logLength);
    if (logLength) {
        GLchar *log = malloc(logLength);
        if (!log) {
            perror("Couldn't allocate shader compile log");
            exit(1);
        }
        glGetShaderInfoLog(shaderID, logLength, NULL, log);
        printf("Shader compile message: %s\n", log);
        free(log);
    }

    GLint status;
    glGetShaderiv(shaderID, GL_COMPILE_STATUS, &status);
    if (!status)
        exit(1);

    return shaderID;
}

static GLuint loadShaders(const char *vertex_fn, const char *fragment_fn)
{
    // Compile the shaders
    GLuint vertexShaderID = loadShader(vertex_fn, GL_VERTEX_SHADER),
           fragmentShaderID = loadShader(fragment_fn, GL_FRAGMENT_SHADER);

    puts("Linking shader program...");

    GLuint programID = glCreateProgram();
    glAttachShader(programID, vertexShaderID);
    glAttachShader(programID, fragmentShaderID);
    glLinkProgram(programID);

    // Check the program
    GLint logLength;
    glGetProgramiv(programID, GL_INFO_LOG_LENGTH, &logLength);
    if (logLength > 0) {
        char *log = malloc(logLength);
        if (!log) {
            perror("Couldn't allocate shader compile log");
            exit(1);
        }
        glGetProgramInfoLog(programID, logLength, NULL, log);
        printf("Shader link message: %s\n", log);
        free(log);
    }

    GLint status;
    glGetProgramiv(programID, GL_LINK_STATUS, &status);
    if (!status)
        exit(1);

    glDetachShader(programID, vertexShaderID);
    glDetachShader(programID, fragmentShaderID);
    glDeleteShader(vertexShaderID);
    glDeleteShader(fragmentShaderID);

    return programID;
}

int main()
{
    // Set error callback to see more detailed failure info
    glfwSetErrorCallback(glfwCB);

    if (!glfwInit())
    {
        fprintf(stderr, "Failed to initialize GLFW\n");
        return -1;
    }

    glfwWindowHint(GLFW_SAMPLES, 4); // 4x antialiasing

    // To ensure compatiblity, check the output of this command:
    // $ glxinfo | grep 'Max core profile version'
    glfwWindowHint(GLFW_CONTEXT_VERSION_MAJOR, 3);
    glfwWindowHint(GLFW_CONTEXT_VERSION_MINOR, 3);

    // We don't want the old OpenGL 
    glfwWindowHint(GLFW_OPENGL_PROFILE, GLFW_OPENGL_CORE_PROFILE);

    // Open a window and create its OpenGL context
    GLFWwindow *window = glfwCreateWindow(1024, 768, "Tutorial 02 - Red triangle", NULL, NULL);
    if (!window)
    {
        fprintf(stderr, "Failed to open GLFW window. If you have an Intel GPU, "
                        "they are not 3.3 compatible. Try the 2.1 version of "
                        "the tutorials.\n");
        glfwTerminate();
        return -1;
    }
    glfwMakeContextCurrent(window);

    glewExperimental = true; // Needed in core profile
    if (glewInit() != GLEW_OK) {
        fprintf(stderr, "Failed to initialize GLEW\n");
        glfwTerminate();
        return -1;
    }

    // Ensure we can capture the escape key being pressed below
    glfwSetInputMode(window, GLFW_STICKY_KEYS, GL_TRUE);

    // Dark blue background
    glClearColor(0.0f, 0.0f, 0.4f, 0.0f);

    GLuint VertexArrayID;
    glGenVertexArrays(1, &VertexArrayID);
    glBindVertexArray(VertexArrayID);

    // Create and compile our GLSL program from the shaders
    GLuint programID = loadShaders("simple-vertex.glsl", "simple-fragment.glsl");

    const GLfloat g_vertex_buffer_data[] = { 
        -1.0f, -1.0f, 0.0f,
         1.0f, -1.0f, 0.0f,
         0.0f,  1.0f, 0.0f,
    };

    GLuint vertexbuffer;
    glGenBuffers(1, &vertexbuffer);
    glBindBuffer(GL_ARRAY_BUFFER, vertexbuffer);
    glBufferData(GL_ARRAY_BUFFER, sizeof(g_vertex_buffer_data),
                 g_vertex_buffer_data, GL_STATIC_DRAW);

    puts("Initialized.");

    do
    {
        // Clear the screen
        glClear(GL_COLOR_BUFFER_BIT);

        // Use our shader
        glUseProgram(programID);

        // 1st attribute buffer: vertices
        glEnableVertexAttribArray(0);
        glBindBuffer(GL_ARRAY_BUFFER, vertexbuffer);
        glVertexAttribPointer(
            0,         // attribute 0. No particular reason for 0, but must match the layout in the shader.
            3,         // size
            GL_FLOAT,  // type
            GL_FALSE,  // normalized?
            0,         // stride
            NULL       // array buffer offset
        );

        // Draw the triangle!
        glDrawArrays(GL_TRIANGLES, 0, 3); // 3 indices starting at 0 -> 1 triangle

        glDisableVertexAttribArray(0);

        // Swap buffers
        glfwSwapBuffers(window);
        glfwPollEvents();

        // Check if the ESC key was pressed or the window was closed
    } while (glfwGetKey(window, GLFW_KEY_ESCAPE) != GLFW_PRESS &&
             !glfwWindowShouldClose(window));

    // Cleanup VBO
    glDeleteBuffers(1, &vertexbuffer);
    glDeleteVertexArrays(1, &VertexArrayID);
    glDeleteProgram(programID);

    // Close OpenGL window and terminate GLFW
    glfwTerminate();

    return 0;
}
\$\endgroup\$
2
\$\begingroup\$

A few minor things after a quick look.

Inconsistent use of spacing. The placement of the opening { is sometimes on the same line as the statement it belongs with, other times it is on a line by itself (which makes an almost blank-line gap in the code). Pick one style and stick with it.

What is glfwCB? That just seems like a bunch of random characters thrown together for a function name. Pick something more meaningful, even if it came directly from the tutorial.

Inconsistent passing arrays to glShaderSource. For the 'length' array you pass in a one element array, while for the array of strings you pass in the address of a variable as a one element array. Both ways are valid, but the inconsistency makes it harder to understand what exactly is going on there. Think about what you'll have to do when you alter this later to use two (or more) shaders.

Run on variable declarations. With vertexShaderID and fragmentShaderID, using two statements and repeating the type makes the code easier to read. As it is you need to look carefully to see if it is two declarations or one declaration that spans two lines (see, for example, the while near the end of main, which has a very similar appearance but is one statement).

g_vertex_buffer_data can be made static.

\$\endgroup\$
  • \$\begingroup\$ glfwCB I'll rename to glfwErrorCallback. I moved all of the open-braces to the same line. "Run-on variable declarations" I disagree with; it's quite clear what it's doing based on indentation. What advantage is there to setting g_vertex_buffer_data to be static? \$\endgroup\$ – Reinderien Dec 27 '18 at 17:10
  • 1
    \$\begingroup\$ @Reinderien You don't change any of the data in g_vertex_buffer_data, and to initialize the local array the compiler will have to store that data into it, either with some store instructions or (more likely here) copy the data from an internal static buffer. Making the array static will allow it to be initialized at compile time, saving some code space and execution time. We'll just have to disagree on the variable declaration, as my impression seeing it is different from yours (which should be reason enough to be explicit with what's going on). \$\endgroup\$ – 1201ProgramAlarm Dec 28 '18 at 1:39
2
\$\begingroup\$

3rd Party Libraries

I have a hard time with most OpenGL tutorials, and this one is no different. They often have a bunch of non-OpenGL code that mysteriously handles things without explaining how under the guise of "keeping things simple and cross platform." In my opinion it does more harm than good. In particular, the use of support libraries like GLEW, GLFW, GLU, GLUT, etc. leaves beginners confused about which parts are OpenGL and which parts are not. It ends up impairing their use of the tools and it hides very important details like OpenGL contexts. A better tutorial would simply supply code for creating a context on each platform that the tutorial supports. It's more work to write and to make clean and understandable to someone new to OpenGL, but it would be worth it in my opinion.

In fact, looking through the code, is GLEW even used? It's initialized, and glewExperimental is set to true, but I don't see any other calls to it. Does it affect GLFW code? If so, that's pretty confusing to a learner. In fact, having the GLEW and GLFW code intermixed, it's really hard to tell which is which at a glance.

Error Handling

You have some reasonably good error handling for reading in the shaders. You handle a bunch of errors around file handling, allocating memory for shaders and logs, etc. Then you completely ignore every possible OpenGL error in your program. There should be some calls to glGetError() somewhere in the code to help the reader understand that things can go wrong with OpenGL.

Also message about Intel GPUs not supporting OpenGL 3.3 is out-of-date. It can be removed. It's also probably not the only reason that not having a window can happen, so it's just confusing to a user that doesn't have an Intel GPU.

Order of Operations

Because OpenGL is a big state machine, it often doesn't matter what order you perform actions in, so long as they are all performed before the next draw call. But for a tutorial, it can be useful to have things grouped together when they affect state that's related. For that reason, I'd put the calls that generate and bind the vertex array near the calls that generate, bind, and fill the vertex buffers. They're all related to getting the geometry up to the GPU, so it would be nice to have them together.

Also, you don't need to re-bind the vertex buffer in the loop in main() as it's already current. Binding things has a performance impact and is very confusing to new learners of OpenGL in my experience. It's one of those things that leads to a lot of superstition when learning OpenGL. ("Last time I had this problem it was because something wasn't bound properly. I'll just bind everything again and hope that fixes it!")

Use Functions

The main() function has way too much going on. I'd break out setting up the window and context into its own function. I'd probably break out setting up all the buffers, too. And then I'd make the main loop its own function as well. This will help someone new to the library understand which bits of code are related to which parts of the functionality of the tutorial. (And that can be a good things with OpenGL which is quite opaque!)

\$\endgroup\$
  • \$\begingroup\$ At first glance it seems as if glew isn't doing a lot. Unfortunately, as soon as it's removed, using OpenGL (especially on bad operating systems - such as OSX - that I have to support) becomes a nightmare. It has a weird and broken way of handling "frameworks". glew fixes this. \$\endgroup\$ – Reinderien Dec 28 '18 at 3:08
  • \$\begingroup\$ Whereas it may be confusing to a learner, the alternative (writing a long, hacky makefile with multiple conditional blocks, and similar precompiler conditional blocks in the source itself) is worse. \$\endgroup\$ – Reinderien Dec 28 '18 at 3:09
  • \$\begingroup\$ "you completely ignore every possible OpenGL error in your program" - Well no, not really. I have an error callback. All errors go to stderr. If it breaks and it's the fault of glfw, the reason is seen on the console. \$\endgroup\$ – Reinderien Dec 28 '18 at 3:10
  • \$\begingroup\$ "message about Intel GPUs not supporting OpenGL 3.3 is out-of-date. It can be removed" - will do \$\endgroup\$ – Reinderien Dec 28 '18 at 3:11
  • \$\begingroup\$ "The main() function has way too much going on" agreed, and I've already pulled a pile of code out into an init function. \$\endgroup\$ – Reinderien Dec 28 '18 at 3:13
1
\$\begingroup\$

The render loop does a bunch of stuff over and over that only has to happen once. The vertex attribute array (VAA) does not need to be declared every loop. That gets stored in the VAO. The same program is being used every loop as well.

The variable names for the VBO and the VAO weren't very good. It's a VAO. It should be called a VAO.

Every time something is called <thing>ID, it isn't useful to include "ID". It makes it easier and conceptually equivalent to just say <thing>. This applies to the shaders, the shader program, and a couple other things.

A shader should be named according to what it does. The vertex shader just copies, so I named it "copy.vert". The fragment shader turns everything red, so I called it "red.frag". (The file extensions are not a standard, but my choice of .vert and .frag is pretty common. See for example the doom3 source code.)

Here's my version:

#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>

#include <GL/glew.h>
#include <GLFW/glfw3.h>


static void glfwCB(int error, const char *desc)
{
    fprintf(stderr, "GLFW error 0x%08X: %s\n", error, desc);
}

static GLuint loadShader(const char *fn, GLenum shaderType)
{
    printf("Compiling shader '%s'...\n", fn);

    GLuint shader = glCreateShader(shaderType);
    if (!shader) {
        fprintf(stderr, "Failed to create shader\n");
        exit(1);
    }

    FILE *f = fopen(fn, "r");
    if (!f) {
        perror("Failed to load shader file");
        exit(1);
    }
    if (fseek(f, 0, SEEK_END)) {
        perror("Failed to get file size");
        exit(1);
    }
    GLint size[1] = {ftell(f)};
    if (size[0] == -1) {
        perror("Failed to get file size");
        exit(1);
    }
    rewind(f);
    char *source = malloc(size[0]);
    if (!source) {
        perror("Failed to allocate source memory");
        exit(1);
    }
    if (fread(source, 1, size[0], f) != size[0]) {
        perror("Failed to read file");
        exit(1);
    }
    if (fclose(f))
        perror("Warning: failed to close source file");

    const GLchar *rosource = source;
    glShaderSource(shader, 1, &rosource, size);
    free(source);

    glCompileShader(shader);

    GLint logLength;
    glGetShaderiv(shader, GL_INFO_LOG_LENGTH, &logLength);
    if (logLength) {
        GLchar *log = malloc(logLength);
        if (!log) {
            perror("Couldn't allocate shader compile log");
            exit(1);
        }
        glGetShaderInfoLog(shader, logLength, NULL, log);
        printf("Shader compile message: %s\n", log);
        free(log);
    }

    GLint status;
    glGetShaderiv(shader, GL_COMPILE_STATUS, &status);
    if (!status)
        exit(1);

    return shader;
}

static GLuint loadShaders(const char *vertex_fn, const char *fragment_fn)
{
    // Compile the shaders
    GLuint vertexShader = loadShader(vertex_fn, GL_VERTEX_SHADER),
           fragmentShader = loadShader(fragment_fn, GL_FRAGMENT_SHADER);

    puts("Linking shader program...");

    GLuint program = glCreateProgram();
    glAttachShader(program, vertexShader);
    glAttachShader(program, fragmentShader);
    glLinkProgram(program);

    // Check the program
    GLint logLength;
    glGetProgramiv(program, GL_INFO_LOG_LENGTH, &logLength);
    if (logLength > 0) {
        char *log = malloc(logLength);
        if (!log) {
            perror("Couldn't allocate shader compile log");
            exit(1);
        }
        glGetProgramInfoLog(program, logLength, NULL, log);
        printf("Shader link message: %s\n", log);
        free(log);
    }

    GLint status;
    glGetProgramiv(program, GL_LINK_STATUS, &status);
    if (!status)
        exit(1);

    glDetachShader(program, vertexShader);
    glDetachShader(program, fragmentShader);
    glDeleteShader(vertexShader);
    glDeleteShader(fragmentShader);

    return program;
}

int main()
{
    // Set error callback to see more detailed failure info
    glfwSetErrorCallback(glfwCB);

    if (!glfwInit())
    {
        fprintf(stderr, "Failed to initialize GLFW\n");
        return -1;
    }

    glfwWindowHint(GLFW_SAMPLES, 4); // 4x antialiasing

    // To ensure compatiblity, check the output of this command:
    // $ glxinfo | grep 'Max core profile version'
    glfwWindowHint(GLFW_CONTEXT_VERSION_MAJOR, 3);
    glfwWindowHint(GLFW_CONTEXT_VERSION_MINOR, 3);

    // We don't want the old OpenGL
    glfwWindowHint(GLFW_OPENGL_PROFILE, GLFW_OPENGL_CORE_PROFILE);

    // Open a window and create its OpenGL context
    GLFWwindow *window = glfwCreateWindow(800, 600,
            "Tutorial 02 - Red triangle", NULL, NULL);
    if (!window)
    {
        fprintf(stderr, "Failed to open GLFW window. If you have an Intel "
                "GPU, are not 3.3 compatible. Try the 2.1 version of the "
                "tutorials.\n");
        glfwTerminate();
        return -1;
    }
    glfwMakeContextCurrent(window);

    glewExperimental = true; // Needed in core profile
    if (glewInit() != GLEW_OK) {
        fprintf(stderr, "Failed to initialize GLEW\n");
        glfwTerminate();
        return -1;
    }

    // Ensure we can capture the escape key being pressed below
    glfwSetInputMode(window, GLFW_STICKY_KEYS, GL_TRUE);

    // Dark blue background
    glClearColor(0.0f, 0.0f, 0.4f, 0.0f);

    // Make the VAO.
    GLuint vao;
    glGenVertexArrays(1, &vao);
    glBindVertexArray(vao);

    // Make the VBO and add it to the VAO.
    GLuint vbo;
    glGenBuffers(1, &vbo);
    glBindBuffer(GL_ARRAY_BUFFER, vbo);

    const GLfloat vertex_buffer_data[] = {
        -1.0f, -1.0f, 0.0f,
         1.0f, -1.0f, 0.0f,
         0.0f,  1.0f, 0.0f,
    };
    glBufferData(GL_ARRAY_BUFFER, sizeof(vertex_buffer_data),
                 vertex_buffer_data, GL_STATIC_DRAW);

    // vertex attribute array 0: vertices. No particular reason for 0, but must
    // match the layout in the shader.
    GLuint vaa = 0;
    glEnableVertexAttribArray(vaa);
    glVertexAttribPointer(
        vaa,
        3,         // number of numbers per vertex
        GL_FLOAT,  // type
        GL_FALSE,  // normalized?
        0,         // stride
        0          // array buffer offset
    );
    // The VAO is ready.


    // Create and compile our GLSL program from the shaders.
    GLuint program = loadShaders("copy.vert", "red.frag");
    // Use our shader.
    glUseProgram(program);

    puts("Initialized.");

    do
    {
        // Clear the screen
        glClear(GL_COLOR_BUFFER_BIT);

        // Draw the triangle! 3 indices starting at 0 -> 1 triangle.
        glDrawArrays(GL_TRIANGLES, 0, 3);

        // Swap buffers
        glfwSwapBuffers(window);
        glfwPollEvents();

        // Check if the ESC key was pressed or the window was closed
    } while (glfwGetKey(window, GLFW_KEY_ESCAPE) != GLFW_PRESS &&
             !glfwWindowShouldClose(window));

    // Cleanup VBO
    glDeleteBuffers(1, &vbo);
    glDeleteVertexArrays(1, &vao);
    glDeleteProgram(program);

    // Close OpenGL window and terminate GLFW
    glfwTerminate();

    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Pulling out all of that stuff from the main draw loop into the init section is a massive help. \$\endgroup\$ – Reinderien Dec 28 '18 at 22:23

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.