Loading and drawing an image

Question

I have some functions and structs for loading and drawing an image with OpenGL. The two main places I would like feedback are:

1. I've tried to separate the OpenGL-specific code into the implementation so the caller doesn't have to know what rendering API I'm using. Can I do a better job of separating the OpenGL-specific code from the code necessary to specify the image?

2. I'm not happy with my convert_pixels_to_render_coordinates function. It works, it's just klunky. Passing 7 floats is just begging for mis-use. Is there any way I can simplify things, or make it easier to use? I show my usage at the end.

image.h

#ifndef IMAGE_H
#define IMAGE_H

struct rectangle
{
    float x, y, width, height;
};

void rectangle_vertices(const struct rectangle *rectangle,
                        float *vertices);


/*
 * Calculate the y_start and height of an image in 
 * render coordinates by "glueing" two points on
 * the image to two points on the coordinate system
 */
void convert_pixels_to_render_coordinates(
    float image_height,
    float px_0, float px_1,
    float coord_0, float coord_1,
    float *y_offset, 
    float *height);

struct image
{
    struct rectangle rect;
    void *render_data;
};

void set_position(struct image *image, 
                  float new_x, float new_y);

void load_image(struct image *image,
                const char *pathname);

void render_image(struct image *image);

#endif

image.c

include "image.h"
#include "gl_header.h"

#define STB_IMAGE_IMPLEMENTATION
#include "stb_image.h"

#include <stdbool.h>

void rectangle_vertices(const struct rectangle *rectangle,
                        float *vertices)
{
    vertices[0] = rectangle->x; 
    vertices[1] = rectangle->y;

    vertices[2] = rectangle->x + rectangle->width; 
    vertices[3] = rectangle->y;

    vertices[4] = rectangle->x + rectangle->width; 
    vertices[5] = rectangle->y - rectangle->height;

    vertices[6] = rectangle->x;                    
    vertices[7] = rectangle->y - rectangle->height;
}

void convert_pixels_to_render_coordinates(
    float image_dim,
    float px_0, float px_1,
    float coord_0, float coord_1,
    float *offset, 
    float *length)
{
    const float px_delta = px_1 - px_0;
    const float coord_delta = coord_0 - coord_1;

    /* Get the size of the image in render coordinates by solving the equation

        px_delta           *length
       -----------  =   -------------
        image_dim        coord_delta
    */
    *length = image_dim * coord_delta / px_delta;

    /* Solving this equation gives us the offset to our first point:

       image_dim        offset
      ----------- =    --------
         px_0           *length

       And then we need to apply the coord offset
    */
    *offset = coord_0 + (px_0 * (*length) / image_dim);
}

static const struct rectangle FULL_UV_COORDS = {0.f, 0.f, 1.f, 1.f};

struct image_impl
{
    GLuint texture;
    GLuint buffers[2];
};

#define VERTEX_BUFFER buffers[0]
#define UV_BUFFER buffers[1]

void load_image(struct image *image, const char *pathname)
{
    stbi_set_flip_vertically_on_load(true);
    image->render_data = malloc(sizeof(struct image_impl));
    if (!image->render_data)
    {
        fprintf(stderr, "Out of memory\n");
        exit(1);
    }

    struct image_impl *impl = (struct image_impl *)image->render_data;
    glGenBuffers(2, impl->buffers);

    int width_px, height_px, channels;
    unsigned char *pixels = stbi_load(pathname, &width_px, &height_px, &channels, 0);

    set_position(image, image->rect.x, image->rect.y);

    const struct rectangle *texture_rect = &FULL_UV_COORDS;
    GLfloat note_uv[8] = {};
    rectangle_vertices(texture_rect, note_uv);
    glBindBuffer(GL_ARRAY_BUFFER, impl->UV_BUFFER);
    glBufferData(GL_ARRAY_BUFFER, sizeof(note_uv), note_uv, GL_STATIC_DRAW);

    glGenTextures(1, &impl->texture);
    glActiveTexture(GL_TEXTURE0);
    glBindTexture(GL_TEXTURE_2D, impl->texture);
    glPixelStorei(GL_UNPACK_ALIGNMENT, 1);

    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);

    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, width_px, height_px,
                 0, GL_RGBA, GL_UNSIGNED_BYTE, pixels);
    glGenerateMipmap(GL_TEXTURE_2D);

    stbi_image_free(pixels);
}

void render_image(struct image *image)
{
    struct image_impl *impl = (struct image_impl *)image->render_data;
    glActiveTexture(GL_TEXTURE0);
    glBindTexture(GL_TEXTURE_2D, impl->texture);

    glEnableVertexAttribArray(0);
    glBindBuffer(GL_ARRAY_BUFFER, impl->VERTEX_BUFFER);
    glVertexAttribPointer(0, 2, GL_FLOAT, GL_FALSE, 0, 0);

    glEnableVertexAttribArray(1);
    glBindBuffer(GL_ARRAY_BUFFER, impl->UV_BUFFER);
    glVertexAttribPointer(1, 2, GL_FLOAT, GL_FALSE, 0, 0);

    glDrawArrays(GL_TRIANGLE_FAN, 0, 4);

    glDisableVertexAttribArray(0);
    glDisableVertexAttribArray(1);
}

void set_position(struct image *image,
                  float new_x, float new_y)
{
    image->rect.x = new_x;
    image->rect.y = new_y;

    struct image_impl *impl = (struct image_impl *)image->render_data;
    GLfloat note_vertices[8] = {};
    rectangle_vertices(&image->rect, note_vertices);
    glBindBuffer(GL_ARRAY_BUFFER, impl->VERTEX_BUFFER);
    glBufferData(GL_ARRAY_BUFFER, sizeof(note_vertices), note_vertices, GL_STATIC_DRAW);
}

#undef VERTEX_BUFFER
#undef UV_BUFFER

Usage:

convert_pixels_to_render_coordinates(
            256,
            0.f, 74.f,
            .5f, .45f,
            &state->bass_clef.rect.y,
            &state->bass_clef.rect.height);
state->bass_clef.rect.x = -1.f;
state->bass_clef.rect.width = .1f;

load_image(&state->bass_clef,
           "./res/bass-clef.png");

And to try to explain where all these numbers are coming from, here's a (marked) image that I'm trying to load with this:

The two red dots are the y-coordinates (0.f, 74.f) (manually extracted by looking at it in GIMP) where the clef needs to intersect lines on the staff. (See the below image for how these points need to line up in context)

The .5f and .45f are the y-values in OpenGL coordinates [-1..1] of the lines on the staff those dots need to touch. From this we calculate the y offset and height of the image rectangle for the clef.

Answer 1

Kudos on using modern OpenGL for this! We see a lot of OpenGL code around here relying on deprecated functionality, so this is a breath of fresh air.

Naming

I think your naming needs some work. The name convert_pixels_to_render_coordinates is highly confusing. When I think of pixels, I generally think of an array of RGB(A) values (or possibly in some other color space). While I have seen OpenGL code that stuffs x,y,z coordinates into an image, this code doesn't appear to be doing that. So the name just seems wrong. It's not converting pixels to anything. It appears to be converting some sort of length from one coordinate system to another.

Next are the arguments to the function. The header calls the first parameter image_height. The implementation calls it image_dim. I'm guessing that since the other parameters have names like y_offset and height that you expect it to be used for calculating something related to height. The comment that goes with the function doesn't illuminate much for me as a reader who has never seen this code. What are "render coordinates"? Are these world coordinates? Normalized device coordinates? Object coordinates? I can't tell from your usage example what these various coordinates and dimensions are supposed to be. You pass in 256 for the image_height, but you haven't loaded the image yet. How do you know it's 256 pixels high? Then you pass 0 and 74 for the px_0 and px_1 values. Is that the width? Given the ratios you calculate, I can't tell at all what this is doing. Are you just trying to calculate normalized texture coordinates from the size of the image? Is this getting something out of a texture atlas? Suffice to say, I can't tell what's going on from the name of the function or the arguments.

In the struct image, the render_data pointer has a meaningless name. In general, you should avoid using words like data, info, record, object, etc. in the names of types because those words describe all types. That leaves "render" which is equally meaningless because all of this code is related to rendering. What you're passing here are vertex attributes, so I recommend calling it vertex_attributes or something like that.

The function name rectangle_vertices() is also not very descriptive. I suggest something along the lines of image_coordinates_to_vertex_coordinates(), or object_coordinates_to_vertex_coordinates(). (Note that the use of object here is not in a type definition, so the above rule doesn't apply. "Object Space" is a term commonly used to refer to a rendered object's local coordinate space.)

Constants

It's nice to see you using named constants for some things rather than magic numbers. I recommend you change these macros to be just indexes, though:

#define VERTEX_BUFFER buffers[0]
#define UV_BUFFER buffers[1]

Rather than making them macros, I'd define them as an enum, like this:

typedef enum {
    VERTEX_BUFFER = 0,
    UV_BUFFER = 1
};

And then you can use them like this:

glBindBuffer(GL_ARRAY_BUFFER, impl->buffers [ VERTEX_BUFFER ]);

That's more idiomatic C.

Your Questions

To answer your questions, I think you've done fine separating the interface from the implementation. I just think your interface could be better. I can't comment more on convert_pixels_to_render_coordinates because I don't know what it's doing.

Answer 2

typedef is your friend

You're using C99, so typedef struct { ... } rectangle; instead of the older style.

Additional structs

If you want to tighten up some of your code, one potential way is to make a coordinate struct. This would halve the number of members of rectangle, for instance. Such nesting would not have a performance impact. It would also be used instead of new_x / new_y (for instance).

Failure handling

The old-school C behaviour for malloc failure is to not only return null, but to set errno. This should not be ignored. Have a read through http://pubs.opengroup.org/onlinepubs/009695399/functions/perror.html