3
\$\begingroup\$

The function's job is to load a vertex attribute buffer or an element buffer from a specified path, and upload it to OpenGL. Also, optionally, return the number of vertices/indices loaded through an output parameter.

Signature:

GLuint loadBufferFromFile(std::string const& bufferPath, unsigned char vertexElementCount, GLenum vertexFormat, GLenum bufferType=GL_ARRAY_BUFFER, GLuint *vertexCount=NULL, GLenum usageHint=GL_STATIC_DRAW);

Body:

GLuint loadBufferFromFile(std::string const& bufferPath, unsigned char vertexElementCount, GLenum vertexFormat, GLenum bufferType, GLuint *vertexCount, GLenum usageHint)
{
    const std::unordered_set<GLenum> VALID_VERTEX_FORMATS = {GL_FLOAT, GL_UNSIGNED_INT, GL_UNSIGNED_BYTE};
    //bufferType and usageHint will be validated by OpenGL

    if(vertexElementCount < 1 || vertexElementCount > 4)
    {
        throw std::invalid_argument("Vertex element count must be in range <1,4>");
    }

    if(bufferType == GL_ELEMENT_ARRAY_BUFFER && vertexElementCount != 1)
    {
        throw std::invalid_argument("Buffer of type GL_ELEMENT_ARRAY_BUFFER must have element count of 1");
    }

    if(VALID_VERTEX_FORMATS.find(vertexFormat) == VALID_VERTEX_FORMATS.end())
    {
        throw std::invalid_argument("Invalid vertex format");
    }

    std::ifstream file;
    file.open(bufferPath, std::ios::binary | std::ios::in);

    if(!file.is_open())
    {
        throw std::runtime_error(std::string("Buffer file ") + bufferPath + " could not be opened");
    }

    file.seekg(0, file.end);
    unsigned int length = file.tellg();
    file.seekg(0, file.beg);

    if(vertexCount != NULL)
    {
        std::size_t vertexSize;

        switch(vertexFormat)
        {
            case GL_FLOAT:
                vertexSize = sizeof(GLfloat);
                break;
            case GL_UNSIGNED_INT:
                vertexSize = sizeof(GLuint);
                break;
            case GL_UNSIGNED_BYTE:
                vertexSize = sizeof(GLubyte);
                break;
        }

        vertexSize *= vertexElementCount;
        *vertexCount = length/vertexSize;
    }

    char *buffer = new char [length];
    file.read(buffer, length);
    file.close();

    GLuint bufferID;
    glGenBuffers(1, &bufferID);

    glBindBuffer(bufferType, bufferID);
    glBufferData(bufferType, length, buffer, usageHint);

    delete[] buffer;
    return bufferID;
}

Specific questions:

  • Is it appropriate to fine-tune data types when it is not strictly necessary, like I did with vertexElementCount? int would have also worked just fine.
  • Is this level of error handling/validation appropriate?
  • Is it good to use a const collection for validation in this way? Are there runtime costs beyond the act of validation itself?
  • If yes, unordered_set or vector? On one hand, a set seems a logical choice for this, on the other, wisdom of the ancients says "Use vector for everything unless you absolutely can't!"
  • Are those appropriate exceptions to use?
  • Even though in this case I can be certain it will not lead to a memory leak (nothing in between new and delete throws exceptions), directly using dynamic allocation rubs me the wrong way. Can I avoid it in a reasonable way?
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

As for your questions:

  1. Just use int, unless this is critical code where benchmarks have shown that integers are noticeably slower. Furthermore: your char will support 255 vertices which isn't a lot, I've loaded models with millions of vertices.
  2. This level of error checking is possibly an indication that your code smells. You have a function that accepts many parameters, perhaps you can split it into multiple functions or a class type of structure.
  3. Myself, I make everything const unless it cannot be const.
  4. If the size is known at compile time, how about std::array instead of std::vector? As for std::unordered_set it seems too fancy for the job (e.g., a self balancing tree with 3 items, whoop! whoop!), but I have no strong arguments.
  5. No opinion. Myself, I never use C++ exceptions - due to my lack of understanding / education. In other languages I never directly use the build-in Exception types, I always subclass my own - if anything it helps the library user understand if they are library errors or system errors.
  6. You can place your new'ed memory in a std::unique_ptr or use the resize features of std::vector or std::string

Couple more random things:

  1. file.close(); is optional, the std::ifstream will also close the resource (if it was open).
  2. You're not testing for OpenGL related errors.
  3. I don't like 'out' arguments, such as vertexCount. Just return a std::tuple with multiple arguments, or a struct.
  4. Does your file contain binary data? You're reading it from disk, and directly upload it to the GPU without parsing/validation? If anything, make sure you've actually read the amount of bytes that tellg claims are available.
  5. std::ifstream has a constructor, use it.
\$\endgroup\$
2
  • \$\begingroup\$ To address some concerns: vertexElementCount is per vertex, not total, so it'll be at most 4. Perhaps I should make that clearer. The files are binary and contain raw data compatible with OpenGL. OpenGL errors are logged via an ARB_debug_output callback. The rest sounds great, although I'm not sure how this function could be split and how that would help with the error checking. \$\endgroup\$
    – user75008
    Jun 5, 2015 at 22:57
  • \$\begingroup\$ You can move the file reading logic to a separate function. You can let OpenGL handle the error checking, you'll only need to throw an exception if OpenGL yields an error. \$\endgroup\$
    – Gerard
    Jun 5, 2015 at 23:29
0
\$\begingroup\$

You say that "the files are binary and contain raw data compatible with OpenGL." On what platform? Does your code need to work properly on both big endian and little endian platforms? If so, then any float values (likely all vertices' coordinates) are not compatible until you've arranged their bytes correctly. But that may be beyond the scope of your project. I thought I'd mention it just in case.

One suggestion to clean things up just a little: Why not make all the validation at the beginning into a separate function? Something like validateArguments() or something similar?

void validateArguments(const unsigned char vertexElementCount, 
    const GLenum vertexFormat, const GLenum bufferType)
{
    const std::unordered_set<GLenum> VALID_VERTEX_FORMATS = {GL_FLOAT, GL_UNSIGNED_INT, GL_UNSIGNED_BYTE};
    //bufferType and usageHint will be validated by OpenGL

    if(vertexElementCount < 1 || vertexElementCount > 4)
    {
        throw std::invalid_argument("Vertex element count must be in range <1,4>");
    }

    if(bufferType == GL_ELEMENT_ARRAY_BUFFER && vertexElementCount != 1)
    {
        throw std::invalid_argument("Buffer of type GL_ELEMENT_ARRAY_BUFFER must have element count of 1");
    }

    if(VALID_VERTEX_FORMATS.find(vertexFormat) == VALID_VERTEX_FORMATS.end())
    {
        throw std::invalid_argument("Invalid vertex format");
    }
}

Next, I'd put the memory allocation and file reading into a function by itself:

char* allocateAndReadBufferData (std::string const& bufferPath, const unsigned char vertexElementCount, GLuint *vertexCount=NULL)
{
    std::ifstream file;
    file.open(bufferPath, std::ios::binary | std::ios::in);

    if(!file.is_open())
    {
        throw std::runtime_error(std::string("Buffer file ") + bufferPath + " could not be opened");
    }

    file.seekg(0, file.end);
    const unsigned int length = file.tellg();
    file.seekg(0, file.beg);

    if(vertexCount != NULL)
    {
        std::size_t vertexSize;

        switch(vertexFormat)
        {
            case GL_FLOAT:
                vertexSize = sizeof(GLfloat);
                break;
            case GL_UNSIGNED_INT:
                vertexSize = sizeof(GLuint);
                break;
            case GL_UNSIGNED_BYTE:
                vertexSize = sizeof(GLubyte);
                break;
        }

        vertexSize *= vertexElementCount;
        *vertexCount = length/vertexSize;
    }

    char* buffer = new char [length];
    file.read(buffer, length);
    file.close();
    return buffer;
} 

And finally, the OpenGL stuff where you actually allocate and upload the data could be a separate function as well:

GLuint uploadBuffer(char*& buffer, const GLenum bufferType, 
    const int length, const GLenum usageHint)
{
    GLuint bufferID;
    glGenBuffers(1, &bufferID);

    glBindBuffer(bufferType, bufferID);
    glBufferData(bufferType, length, buffer, usageHint);

    return bufferID;
}

And then your function looks like this:

GLuint loadBufferFromFile(std::string const& bufferPath, 
    const unsigned char vertexElementCount, const GLenum vertexFormat, 
    const GLenum bufferType, GLuint *vertexCount, const GLenum usageHint)
{
    validateArguments(vertexElementCount, vertexFormat, bufferType);

    char* buffer = allocateAndReadBufferData(bufferPath, vertexElementCount, vertexCount);

    GLuint bufferID = uploadBuffer(buffer, bufferType, length, usageHint);

    // Clean up
    delete [] buffer;

    return bufferID;
}

To answer your questions, I think I mostly agree with @Gerard above on all of them. Having said that, if you want to avoid the dynamic allocation, you could use a std::vector and call reserve() once you know the number of elements you'll need. Something like this:

std::vector<GLfloat> buffer; // NOTE: Assumes float data
allocateAndReadBufferData(bufferPath, vertexElementCount, buffer, vertexCount); // Note: buffer is now a parameter

and then in allocateAndReadBufferData() at the end, do this:

buffer.resize(*vertexCount); // Note you'll have to calculate vertex count even if variable is NULL in this case, but I think you get the idea
file.read(&buffer[0], length);
\$\endgroup\$
6
  • \$\begingroup\$ 1. The binary files are generated by an another part of the same program (the program parses .obj files into binary data and optionally saves it to disk as a cache to speed up next startup; this is the loader of that cache) so it portability isn't a problem. 2. Unsigned int is used for element data (vertex indices) and unsigned bytes for color data for directly colored objects. Both are currently in use, not a case of YAGNI. Other than that, thank you, great advice. \$\endgroup\$
    – user75008
    Jun 6, 2015 at 11:18
  • \$\begingroup\$ In general, I feel that the code was made worse with this approach, some syntax is illegal C++ and will generate run time errors. \$\endgroup\$
    – Gerard
    Jun 6, 2015 at 11:31
  • \$\begingroup\$ @Gerard, an interesting "historical" piece, shall we say, about inlining code. In here particularly, I'll have to agree with the proposed solution of breaking down the function. You can more easily understand the whole process just by taking a look at the main function, without having to worry about the details of each helper functions. \$\endgroup\$
    – glampert
    Jun 7, 2015 at 1:02
  • \$\begingroup\$ @Gerard please let us know what's wrong so we can fix it and help the community. I don't want to leave bad code sitting around. I fixed a syntax error where I had a non-default function argument after one that had a default value. Stupid mistake on my part, and good catch on your part! If you see more, please let me know so I can fix it up! \$\endgroup\$ Jun 7, 2015 at 2:00
  • \$\begingroup\$ @user1118321 reserve should be resize. You have split new and delete over two functions (who is responsible for the memory?). loadBufferFromFile misses a return type. readBufferData may as well return something instead of writing out data by reference. If you split logic like this, make sure your functions are private, i.e., you don't want someone else calling uploadBuffer since it basically does a uncontrolled memory dump to your GPU. \$\endgroup\$
    – Gerard
    Jun 7, 2015 at 8:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.