Improving Chapter 4 Maintainability

This post takes a break from rewriting various modules from the book Game and Graphics Programming for iOS and Android with OpenGL ES 2.0 to focus on some minor style issues which can impact maintainability.  I'm taking the time to cover these issues here partly because Chapter 4 doesn't introduce any new modules and partly because these particular issues first come up in this chapter.

In previous posts I've ranted about the use of "magic numbers", i.e., numeric constants which don't state explicitly for the reader what their meanings are.  Such as using zero, and one for boolean values, or using integer constants as array indices without explaining to the reader what a particular array element represents.  In Tables 4-1, and 4-2 the author introduces his conventions for assigning various texture map types to OpenGL texture units, and how various vertex attributes are assigned locations.  I like that the code uses a consistent standard throughout the examples starting in Chapter 4.  What I don't like is that if someone were to pick up these modules and reuse them in a work environment that uses a different standard for assigning texture maps to texture units, and/or assignment of vertex attributes to locations that the process of converting the code to use a new set of assignments could become error prone.

In an earlier post I mentioned that I added three new enumeration types to the SDK/common/obj.h file.  I talked about the AttributeOffset enumeration type but deferred discussion of the TextureMap, and VertexAttribute types until now.  The material_draw_callback() function (using my changes to the PROGRAM data type) would look something like

    void material_draw_callback(void *ptr)

    {

        OBJMATERIAL *objmaterial = (OBJMATERIAL *)ptr;

        PROGRAM *program = objmaterial->program;

        for (auto it=program->uniform_map.begin();

             it!=program->uniform_map.end(); ++it) {

            auto    &name = it->first;

            auto    &uniform = it->second;

            if (name == "DIFFUSE") {

                glUniform1(uniform.location, 1);

            } else if (name == "MODELVIEWPROJECTIONMATRIX") {

                .

                .

                .

            }

        }

    }

Looking at Table 4-1 from the book you'll see that it specifies that the diffuse texture map gets assigned to texture unit 1, and that, indeed, when the code above is processing the "DIFFUSE" uniform variable for the shader program the code specifies that the code will use texture unit 1 for the diffuse texture map.  Here is where I think using enumerated types can make the code more easily maintainable.  The type TextureMap is declared as

    enum TextureMap {

        TM_map_Ka       = 0,
        TM_map_Kd       = 1,
        TM_map_Ks       = 2,
        TM_map_Disp     = 3,
        TM_map_Bump     = 4,
        TM_map_Tr       = 5,
        TM_Unused       = 7,

        TM_Ambient      = TM_map_Ka,
        TM_Diffuse      = TM_map_Kd,
        TM_Specular     = TM_map_Ks,
        TM_Displacement = TM_map_Disp,
        TM_Bump         = TM_map_Bump,
        TM_Transparency = TM_map_Tr,
        };

Note that I've declared two different names for the values zero through five.  I don't intend that a program, should use both names for a particular value.  Instead, I define both names because different people will prefer different conventions.  I personally prefer the second set of names for the values zero through five because they're not tied to a particular file type (Wavefront), and so I think they're more generic/descriptive than the names which come out of the Wavefront files.  Also, note that I defined the second set of names for values zero through five using the first set of definitions.  This way changing the first set of definitions automatically changes the corresponding name in the second set of definitions.  I expect that, in practice, an individual programmer, or a development team would pick one set of definitions, and delete the others, if either (i.e., the programmer or the team) were to adopt my changes to the code.

Using these values the code

    glUniform1(uniform.location, 1);

becomes

    glUniform1(uniform.location, TM_Diffuse);

Now, if someone comes along and decides that since most, or all, of their models will have diffuse texture maps, but none of their models will have ambient texture maps, it's easy to redefine TM_map_Kd/TM_Diffuse to use texture unit 0.  Of course, the corresponding change needs to be made in the methods for the OBJ class, or whatever other class might be used to load the graphics data.

While I'm changing this routine there is a second problem.  The code searches for the diffuse texture unit location by comparing against the string literal "DIFFUSE".  The compiler can only detect whether the code provides a syntactically valid string literal; it can't do any checking that the string is valid in the context of the program.  So, the compiler would be just as happy to accept the string literals "DIFFUES", and/or "DEFUSE".  To leverage the compiler to help find typos I've also defined a set of strings with symbolic names which parallel the values declared in the TextureMap enumeration type.  Using the symbolic names changes the test to:

    if (name == TM_Diffuse_String) {
        .
        .
        .
    }

In Xcode the programmer might not even need to type out the full symbolic name; the editor will often, helpfully, suggest symbols which match the first few letters of the name, and the programmer can pick from one of the suggestions.  Even if the editor doesn't suggest various symbolic names, and the programmer misspells the name, it's likely that the misspelling won't be a valid symbol and the compiler will detect, and report the error.

In the case that the programmer simply uses the wrong string symbol the code might end up looking something like:

    if (name == TM_Ambient_String) {
        glUniform1(uniform.location, TM_Diffuse);
    } else if (name == "MODELVIEWPROJECTIONMATRIX") {
        .
        .
        .
    }

and when the code doesn't work as expected I think it's easier for the programmer to see that TM_Ambient_String and TM_Diffuse probably don't belong together, and, hopefully, this makes debugging the issue easier.

The enumeration type VertexAttribute serves a similar purpose as the TextureMap enumeration type, but instead of mapping the location of shader uniform variables the VertexAttribute enumeration type is used to map vertex attributes (duh!).  The old version of the function program_bind_attrib_location() looked something like:

    void program_bind_attrib_location(void *ptr) {
        PROGRAM *program = (PROGRAM *)ptr;
        glBindAttribLocation(program->pid, 0, "POSITION");
        glBindAttribLocation(program->pid, 2, "TEXCOORD0");
    }

Using the symbolic names the code now looks like

    void program_bind_attrib_location(void *ptr) {
        PROGRAM *program = (PROGRAM *)ptr;
        glBindAttribLocation(program->pid, VA_Position,
                             VA_Position_String);
        glBindAttribLocation(program->pid, VA_TexCoord0,
                             VA_TexCoord0_String);
    }

Also, have a look at SDK/common/obj.cpp and you'll see that I've used the VertexAttribute enumeration type values to, IMHO, make the OBJMESH::set_attributes() method less cryptic.

If you look at sample program chapter4-5 you'll see that I have also added a set of symbolic names for strings related to the material properties which all have the prefix "MP_"

I've changed the code to use these symbolic names for all of the sample code starting in Chapter 4 through the end of the book.  You can pull a copy of this version of the code from GitHub using the tag chapter4.

Notice that all of the string literals I've been replacing refer to items in the modules provided by the author, and also to variable names which are used in the shader programs.  You might have noticed that I didn't replace the literal string "MODELVIEWPROJECTIONMATRIX" even though, it too, is a uniform variable in the shader programs.  Perhaps I should have but remember that one of the reasons for moving away from string literals and magic numbers is to leverage the compiler to find errors in the code.  The literal string "MODELVIEWPROJECTIONMATRIX" in the sample programs doesn't refer to anything in any of the author's modules; it only refers to a uniform variable in the shader programs.  The compiler doesn't (and can't) ensure consistency between the C++ code and the GLSL (aka OpenGL Shader Language) code so there is less benefit gained from replacing this literal string with a symbolic name.  If you think that this change is beneficial to you, please feel free to make the change in your copy of the code; I said there is less benefit, not that there is no benefit.  A programmer could implement a strategy to iterate over all the symbolic names for strings and try to look up the strings' addresses in the shader program.  If the corresponding variable is missing from the program the code could signal an error.  The programmer could go so far as to ensure that the variables are also of the correct type, either uniform, or attribute.  This wouldn't tell the programmer that there is a problem at compile time, only at run time, but it does still provide some level of protection against programming errors.  If I were going to implement this strategy I would probably bracket the code so that it is only included when the code is compiled in debug mode using something like:

    #ifdef  DEBUG
        // Loop over strings to see if they have a match in
        // the shader program
        .
        .
        .
    #endif  /* DEBUG */

One last thing to consider before wrapping up this post.  Both the VertexAttribute and AttributeOffset enumeration types define symbols representing the same things:  the five vertex attribute values position, normal, fnormal, texture coordinate 0, and tangent 0.  Since the code now uses symbolic names there isn't really any reason to keep both; I could easily remove (and have removed) the AttributeOffset enumeration type.  That the two  enumeration types number the attributes in a different order is of no consequence.  It might be if, instead of deleting the definition of AttributeOffset, I deleted the definition of VertexAttribute.

Next there are no new classes to be created for Chapters 5 and 6 so next time I'll talk about cleaning up some of the utility routines.

Making the TEXTURE Class

This post continues a series of posts in which I discuss how to rewrite the various modules presented in Game and Graphics Programming for iOS and Android with OpenGL ES 2.0.  The code is well organized but, as presented in the book, does very little to leverage the advantages of using the C++ language.  The code, as described in this post, can be downloaded from GitHub using the tag chapter3.  While rewriting the TEXTURE data type into a proper C++ class I made many of the same changes as were discussed in previous posts.  The changes to the TEXTURE class include:

• TEXTURE_init() becomes a private TEXTURE::init() method, and one of the constructors,
• TEXTURE_create() becomes the second constructor,
• TEXTURE_free() becomes the destructor,
• where appropriate, change while loops into for loops, and
• where appropriate, change the data type of variables, and function arguments to bool (or const bool).

In the post describing implementation of the OBJ class I mentioned that the function OBJ_build_texture() needs to become the TEXTURE::build() method member of the TEXTURE class.  If you examine the file SDK/common/texture.cpp you'll see that I've moved, and renamed the function from the file SDK/common/obj.cpp.

There is only one change to the code which is new, and it's fairly minor.  While making the TEXTURE::load_pvr() method from the old TEXTURE_load_pvr() function I noticed that the logic to confirm that the file being operated on is, indeed, a PVR file was, IMHO, more complicated than it needs to be.  The old code loads a 4 byte integer, performs a series of bit shift and bit mask operations against the integer value and then compares the results of the shifting and masking against the various ASCII characters which are supposed to be in the tag of a valid PVR header.  I simplified the code by creating a "char *" which points to the tag area of the PVR header, and then I simply used strncmp(3) to do the comparison.  Is my code faster or slower than the original code?  I don't know; I didn't do any benchmarking but I do think it's a lot easier to read, and the TEXTURE::load_pvr() method is only used in the beginning of the application to initialize the needed data structures.  That is, the animation loop isn't continuously calling this method so, IMHO, maintainability is more important than speed.

Just as there was very little new to talk about regarding the changes to the TEXTURE class there is little to change in the sample program chapter3‑4 to use the new, class oriented implementation of TEXTURE.  The call to the old TEXTURE_create() function gets replaced with "new TEXTURE(…)".  Finally, the call to the old TEXTURE_free() function gets replaced with "delete texture".

In the next post I'll continue with the sample code from Chapter 4 and since Chapter 4 doesn't introduce any new modules I'll spend some time talking about various other code clean-ups which, IMHO, should be made.

Using the OBJ Class

I've already discussed how to use the PROGRAM class methods in my previous posts regarding the sample programs from Chapter 2 of Game and Graphics Programming for iOS and Android with OpenGL ES 2.0 so I'll skip that discussion here while I discuss the first three sample programs from Chapter 3.

Remember that the code described in this post can be retrieved from GitHub using the tag OBJ.

Near the top of the file SDK/chapter3‑1/templateApp.cpp I changed DEBUG_SHADERS to be defined as true rather than 1 (one).  This has been discussed before.  The value is being used as a logical value rather than as a numeric value when it's passed to the PROGRAM constructor, and, IMHO, true reflects that usage better than using 1 (one) as a "magic number".

The rest of the changes are pretty straight forward.  To create an instance of OBJ I changed

    obj = OBJ_load(OBJ_FILE, 1);

to

    obj = new OBJ(OBJ_FILE, true);

At the other end of the OBJ life cycle in templateAppExit() I changed

    OBJ_free(obj);

to

    delete obj;

Before going to the next change needed to use the updated OBJ data types I want to point out that the book has a bug in its description of how to calculate the size parameter.  The book tells the reader to use

    size = objmesh->n_objvertexdata * sizeof(vec3) *
           sizeof(vec3);

The correct computation of size is

    size = objmesh->n_objvertexdata *
           (sizeof(vec3) + sizeof(vec3));

Of course, both of these statements assume that the code is still using the old definition of the OBJMESH data type.  The new OBJMESH data type no longer has the n_objvertexdata member; the new code needs to substitute objvertexdata.size() so the statement becomes

    size = objmesh->objvertexdata.size() *
           (sizeof(vec3) + sizeof(vec3));

Likewise, the loop in templateAppInit() which creates the vertex attribute buffer (which eventually gets passed to glBufferData()) also needs to replace objmesh‑>n_objvertexdata with objmesh‑>objvertexdata.size().

This covers most of the changes which need to be made to the sample programs chapter3‑1, chapter3‑2, and chapter3‑3.  Sample program chapter3‑4 will be covered after I've discussed rewriting the TEXTURE module using classes.  The TEXTURE module will be the topic of my next post.

The last changes I need to talk about are to the loops which iterate over the program‑>uniform_map and objmesh‑>objvertexdata entries.

If you look at the code for sample program chapter3‑1 you'll see that beneath the loop in templateAppInit() there is another version of the same loop which has been commented out.  The version of the loop which is commented out is called a range for loop.  Per the C++ book I'm reading range for loops are part of the (relatively) new C++ 11 standard.  As the comment explains I have found that the current version of Xcode (5.x.x) recognizes range for loops even when the developer hasn't enabled the C++ 11 syntax.  I don't know if the Android developer tools support C++ 11 yet so I have opted (in the short term) to avoid using range for loops; the author of the book went to a lot of trouble to write code which would run on both iOS, and Android, and (up to a point) I want to preserve that feature.  As a software developer I think being able to cover a broader market is a good thing.  Still, some will find them useful so I thought I should point out the option.

There is middle ground I could have used to iterate over the values in objmesh‑>objvertexdata; I could have used C++ iterators explicitly instead of implicitly as a range for loop does.  In that case the loop might look something like:

    for (auto objvertexdata=objmesh->objvertexdata.begin();
         objvertexdata!=objmesh->objvertexdata.end();
         ++objvertexdata) {
        index = objvertexdata->vertex_index;
        // The code from here to the end of the loop is the
        // same as the other implementations.
        .
        .
        .
    }

Using an iterator here, IMHO, more clearly defines for the reader what it is that the loop is really processing.  It's not processing a sequence of integers; it's processing a sequence of OBJVERTEXDATA objects.  Using a range for statement accomplishes the same thing, and is, IMHO, even better because the expression of the loop is more compact than explicitly using the std::vector::begin() and std::vector::end() methods, and, again IMHO, easier to read, too.

If you look at the program_draw_callback() function you'll see that I also used an iterator to process the elements of program‑>uniform_map.  This is a necessary change; the code can't use an integer counter to step through the contents of an std::map.  Using an iterator with an std::map is a little messier than using an iterator with std::vector because the iterator contains not just the value, but also the key which the code uses to look up the value in the map.  I try to clean this up a bit by creating references (aliases) for the key/name (first) and the value (second) portions of the iterator.

[Note:  Remember that the key for the uniform_map container is what used to be the name field of the original version of the UNIFORM data type.  Details can be found here.]

Like the loop to process objmesh‑>objvertexdata, this loop has below it a range for implemention for comparison which has been commented out.

Going forward you'll see that in most cases I've chosen to use iterators to loop over the contents of std::vector and std::map containers.  I think they are much more readable than using an integer counter (of course, integer counters can't be used with std::map containers anyway), and it makes the loops less wordy as I will demonstrate again when I blog about the Chapter 4 sample programs.

[Note:  I only included range for versions of the loops in comments for the chapter3‑1 sample program, not in any of the other sample programs.  Demonstrating their use once for each of std::vector and std::map should be sufficient for those who are interested.]

In the next post I'll talk about changes needed to implement the TEXTURE data type as a class.

Reading Wavefront Files

This post covers rewriting the OBJ data type to make it a full C++ object. The OBJ data type is central to the remainder of the book. In fact, starting in chapter 3 there are only two sample programs which don't use the OBJ data type. Those two sample programs introduce audio and don't draw any "objects" on the screen; they simply fill the screen with a solid color. OBJ objects are initialized by reading the contents of Wavefront files. These files usually have names which end in ".obj".

To get the version of the code as described in this post you can pull a copy of the code from the archive using the tag OBJ.

The writing, and (consequently) the reading of the text below is complicated by using the word "object" to mean different things. In some cases the word object is used to mean an instance of a C++ data class type; in other cases it refers to a thing to be drawn by the sample program. For example, in chapter 3 the sample programs draw the Momo object. I've tried to clarify whether I'm talking about the instantiation of a C++ class or a thing to be drawn but I may have missed places and I'm hoping this warning will help the reader to remember when "decoding" this post that "object" has more than one meaning.

The OBJ data type, unsurprisingly, is implemented in the SDK/common/obj.cpp and SDK/common/obj.h files. Starting with the header file note that the file defines the data types OBJMATERIAL, OBJTRIANGLEINDEX, OBJTRIANGLELIST, OBJVERTEXDATA, and OBJMESH, as well as the data type OBJ. I have converted all of these data types into classes. The types OBJTRIANGLEINDEX and OBJVERTEXDATA only have constructors; if needed, they depend on the compiler to create default destructors. The OBJ data type only has a constructor and a destructor. The remaining types all have at least one constructor, a destructor, a copy constructor, and an assignment operator.

Before beginning in earnest I want to point out that at the top of the header file I added a forward reference to the OBJ class by adding the line

    struct OBJ;

I'll explain why I needed this forward reference later in this post.

Every data member in the header file which is declared as unsigned char, except n_objtrianglelist (it's going to be deleted below, anyway), should be changed to bool. Also, the return type for OBJ_load_mtl() should be changed to bool. Lastly, unsigned char should be changed to bool every place the type appears in a function parameter list declaration. Correspondingly, I changed uses of 0 (zero), and 1 (one) to false, and true, respectively, when those values are used to initialize bool variables.

OBJTRIANGLEINDEX, and OBJVERTEXDATA are so simple that there probably isn't any need for comment. In fact, I almost didn't make them classes at all. The reason I did make them into classes is that OBJMESH, and OBJTRIANGLELIST manage sets of these data types as instances of std::vector. By making these two data types into classes it simplifies adding entries to the std::vectors; the code uses a constructor to create and initialize these data objects at the same time the code uses the std::vector::push_back() method to add the OBJTRIANGLEINDEX or OBJVERTEXDATA objects to the array. See the methods OBJMESH::add_vertex_data() and OBJ::OBJ().

The changes to OBJTRIANGLELIST are the next simplest case. As before, I changed its declaration from being a typedef to a named struct so we can use the name OBJTRIANGLELIST as a class name. If you look at the definition of OBJTRIANGLELIST in the header file you'll see a familiar pattern. The data members n_objtriangleindex and objtriangleindex are used to implement a dynamically sized/allocated array of OBJTRIANGLEINDEX entries. Likewise, n_indice_array and indice_array are used to implement a dynamically sized/allocated array of unsigned short integers. My first impulse was to reimplement data member pairs as:

    std::vector<OBJTRIANGLEINDEX>       objtriangleindex;

and

    std::vector<unsigned short>         indice_array;

respectively, deleting n_objtriangleindex, and n_indice_array. This works fine for objtriangleindex; it introduces a bug for indice_array. The bug isn't with indice_array itself; the bug is caused by using indice_array.size() as a substitute for n_indice_array. In some of the sample programs later in the book the original code frees (malloc(3)) the space allocated for indice_array but keeps the old value of n_indice_array. The code then passes n_indice_array as a parameter to an OpenGL call. In these cases when indice_array.size() is substituted for n_indice_array, because the contents of indice_array have been cleared, indice_array.size() evaluates to 0 (zero) and chaos ensues. The two obvious solutions are to keep the old code which uses malloc(3) to allocate/deallocate space or to keep around the variable n_indice_array but still implement indice_array using std::vector. I chose the latter and explained this choice with a comment in the header file.

[Note: This is an important tenant of writing software for me. I left n_indice_array in the code on purpose. Someone else coming along behind me to maintain the code may think that what I've done is an oversight , and change the code without understanding the impact. After all, none of the other cases of replacing a counter and a pointer with an instance of std::vector to dynamically allocate space need to retain the old counter. The comment in the header file helps prevent this.]

The constructor and destructor for OBJTRIANGLELIST are unremarkable implementations. The copy constructor and the assignment operator are a bit more interesting. These two methods do a "deep copy" of the data members, i.e., the code dynamically allocates space to hold the objtriangleindex array and indice_array in the OBJTRIANGLELIST which is being constructed or assigned to. The implementation sounds more complicated that it is; the copy constructor and the assignment operator use the methods built into the std::vector template; these methods hide all of the implementation details from the programmer.

Before discussing the specifics of rewriting the OBJMATERIAL, OBJMESH, and OBJ data types let me take a step back to discuss the relationship of these data types to each other. The OBJ data type is the top level container for the 3D (graphics) objects which will be drawn in the various sample programs. Each OBJ object can own several OBJMESH and OBJMATERIAL objects. The OBJMESH data object holds a geometric description (the "mesh") for a graphics object to be draw on the screen and it also has a pointer to the OBJMATERIAL which is currently being used to render (i.e., draw) its mesh. Every OBJMESH and OBJMATERIAL (data) object is "owned" by an OBJ "parent". In the original version of the code the OBJMESH and OBJMATERIAL data objects didn't need to know which OBJ data object was their parent. As I've rewritten the code the OBJMESH and OBJMATERIAL data objects do need to know which OBJ object is their parent. This is the reason for adding the forward reference to the OBJ class near the top of the header file: I have added a new data member "OBJ *parent" to both the OBJMESH and OBJMATERIAL data objects.

[Note: When I "push" the code for my next post you'll see that I've actually changed the type of this new member in both OBJMATERIAL, and OBJMESH to "const OBJ *parent". More on this below.]

Continuing with the changes to the OBJ data type the declarations

    std::vector<OBJMESH>        objmesh;
    std::vector<OBJMATERIAL>    objmaterial;
    std::vector<TEXTURE *>      texture;
    std::vector<PROGRAM *>      program;
    std::vector<vec3>           indexed_vertex;
    std::vector<vec3>           indexed_normal;
    std::vector<vec3>           indexed_fnormal;
    std::vector<vec3>           indexed_tangent;
    std::vector<vec2>           indexed_uv;

replace the original declarations of these data members and eliminate the need for the data members n_objmesh, n_objmaterial, n_texture, n_program, n_indexed_vertex, and n_indexed_uv. Details of how these changes are performed can be found in an earlier post.

Now that the changes to the data members of the OBJ class are complete I'll begin on the class methods. For the previous class types (SHADER and PROGRAM) I was able to use the THING_init() functions, and/or the THING_create() function as the basis of class constructors. The original code has no OBJ_init() or OBJ_create() functions. If you look at the SDK/chapter3‑1/templateApp.cpp file for the sample program chapter3‑1 you'll see that the call to OBJ_load() is used to allocate and initialize an OBJ object. Consequently, I used OBJ_load() as the basis for a class constructor. Remember that new allocates the space for us automatically so the call to calloc(3) needs to be removed from the body of the function.

Like previous modules OBJ_free() is used as the basis of the destructor. For each of the data members which have been redeclared using the std::vector type template the destructor uses the clear() member function to release the space. This probably isn't really needed but it mimics the operation of the original code. Before applying clear() to the std::vector objmesh, the destructor looks to see if the vao and vbo data members of each OBJMESH instance are initialized, and if they are, they are released using OpenGL calls. Also, the destructor looks through each entry of the objtrianglelist data member of the OBJMESH entries to see if the OBJTRIANGLELIST's vbo data member is initialized, and, if needed, releases it. Note that these values are created using OpenGL; it's likely that we can/should change their data types to be data types declared by OpenGL. For the vectors of "TEXTURE *" and "PROGRAM *" objects each of the objects must be released before releasing the space held by the vectors themselves; the std::vector::clear() method won't (and can't) do this for us since the template library doesn't know each application's data retention policies.

[Note: If we used C++ 11 shared_ptrs we might save ourselves some work and avoid bugs. Hopefully this will be covered in a future post.]

The following functions all become OBJ member functions:

• OBJ_get_mesh()
• OBJ_get_mesh_index()
• OBJ_get_program()
• OBJ_get_material()
• OBJ_get_texture()
• OBJ_load_mtl()
• OBJ_free_vertex_data()

As before, I stripped off the "OBJ_" prefix from the names and removed the first argument from the parameter lists because the first parameter will be passed into the method as this. You might have noticed that this list omits the functions:

• OBJ_build_texture()
• OBJ_build_program()
• OBJ_build_material()
• OBJ_set_draw_callback_material()
• OBJ_update_bound_mesh()
• OBJ_build_vbo_mesh()
• OBJ_set_attributes_mesh()
• OBJ_build_mesh()
• OBJ_build_mesh2()
• OBJ_optimize_mesh()
• OBJ_draw_material()
• OBJ_draw_mesh()
• OBJ_draw_mesh2()
• OBJ_draw_mesh3()
• OBJ_free_mesh_vertex_data()

This is deliberate. It's my opinion that these functions don't really operate on OBJ objects. The discussion of each of the functions will be postponed until I discuss the class type which I think is a more suitable home for that particular function.

[Note: I'll admit I'm torn about whether OBJ_draw_mesh2() and OBJ_draw_mesh3() shouldn't have been left as OBJ class methods. Perhaps there will be more on that in a later post but these don't appear to be used anywhere in the sample code from the book so sorting this out was a problem for me.]

The various "get" methods (OBJ::get_mesh(), OBJ::get_mesh_index(), OBJ::get_program(), OBJ::get_material(), and OBJ::get_texture()) can be implemented more optimally. As an example I'll discuss only the OBJ::get_mesh() method; the changes to the others all follow an identical pattern. To start, I changed the code from using a while loop to using an iterated for loop. The bool parameter exact_name is passed into the function and never changes (in fact, it should be declared in the parameter list as const bool and will be so in a future revision of the code). Its value is used inside the loop to determine which match criterion is used. Since exact_name never changes during the execution of the loop, each time the loop is executed the CPU will follow the same path through the loop for every iteration. This makes testing the value of exact_match inside the loop wasted effort. The code could test the value of exact_match once outside the loop. This changes the code

    for (int i=0; i != this->objmesh.size(); ++i) {

        if (exact_match) {

            if (!strcmp(this->objmesh[i].name, name)

                return i;

        } else {

            if (!strstr(this->objmesh[i].name, name)

                return i;

        }

    }

into

    if (exact_match) {

        for (int i=0; i != this->objmesh.size(); ++i) {

            if (!strcmp(this->objmesh[i].name, name)

                return i;

        }

    } else {

        for (int i=0; i != this->objmesh.size(); ++i) {

            if (!strstr(this->objmesh[i].name, name)

                return i;

        }

    }

Notice that the new version of the code is a little longer than the original code. This is a common trade off between code speed and code size. Pick your poison.

Continuing with the implementation of the OBJ class you may have noticed that there are four functions in the file that weren't defined in the header file:

• OBJ_get_texture_index(),
• OBJ_add_texture(),
• OBJ_get_program_index(), and
• OBJ_add_program().

In the header file I declared their class method analogs (OBJ::get_texture_index(), etc.) to be private. These routines are only ever used directly by other methods in this file.

The remainder of the implementation of the OBJ class consists of things which have been discussed before, and will be omitted here for brevity.

Moving on to OBJMATERIAL the changes in the header file to data members have been covered before, with the exception of adding a pointer to the OBJMATERIAL instance's parent. Before discussing that let me go back to the list of "orphan" functions which I asserted aren't really part of the OBJ class. I think it's easiest to make the case that the OBJ_draw_material() function is misnamed; I think it should have been named OBJMATERIAL_draw_material(). Ignoring the "OBJ_" prefix of its name, look at the parameter list:

    OBJ_draw_material( OBJMATERIAL *objmaterial );

and then look at the body of the function. Inside the function there isn't a single reference to an OBJ data object; all of the work inside this function is done using OBJMATERIAL's data members.

The argument that OBJ_set_draw_callback_material() is also really an OBJMATERIAL member function requires only a little more work. Start by looking at its argument list:

    void OBJ_set_draw_callback_material( OBJ *obj,

         unsigned int material_index,

         MATERIALDRAWCALLBACK *materialdrawcallback );

Even though the first argument is a pointer to an OBJ instance the second argument is used as an index into the objmaterial data member of the OBJ instance to select which OBJMATERIAL entry in the objmaterial array of the OBJ instance is operated on. Another way to state this is that these two arguments don't specify which OBJ the method should work on, but rather, which OBJMATERIAL it should work on. And when we examine the body of the function again we see that the code changes a data member of OBJMATERIAL; the data members of OBJ are referenced only so that the function can select the correct OBJMATERIAL as its operand.

[Note: This is where changing the type of parent to "const OBJ *" comes into play as mentioned above. By making parent a pointer to a constant OBJ the code emphasizes that this method is really a member function of the OBJMATERIAL class because the method can't change the contents of the OBJ via the parent pointer. This also applies below in the discussion of making OBJ_build_material() into an OBJMATERIAL member function.]

In the sample code calls to the old OBJ_set_draw_callback_material() which look like:

    OBJ_set_draw_callback_material( obj, i, material_draw );

are changed to look like:

    obj->objmaterial[i].set_draw_callback_material(material_draw);

The argument that OBJ_build_material() is really a class method of the OBJMATERIAL class continues this line of thought. Data members of OBJ are referenced so the function knows how to change the instance of OBJMATERIAL (aka this once the code has been changed to a class method) but none of the OBJ data members are changed; OBJ data members are only read, not modified. Note that in the original implementation the only OBJMATERIAL objects which could be changed by the function were children of the OBJ instance passed into the function. The version of the function rewritten as a class method of the OBJMATERIAL class needs to access the OBJ instance which is this (aka this) OBJMATERIAL's parent, hence the need for the new data member parent.

Also, note that the old OBJ_build_material() function uses the old OBJ_get_texture_index(), OBJ_add_texture(), OBJ_get_program_index(), and OBJ_add_program() functions. The new class method OBJMATERIAL::build_material() still needs access to the OBJ member functions. As mentioned above, I have rewritten these functions to be private OBJ methods since they are never referenced outside of this file. I still think the new version of the OBJ methods should be private, but, to allow OBJMATERIAL access to these private functions the OBJ class needs to declare that OBJMATERIAL is a friend class. This really shouldn't be considered out of the ordinary since the OBJMATERIAL object (aka this) is accessing the data members of the OBJ which is its parent.

Before continuing to other orphan functions I want to talk about the names of the class methods I've just discussed putting in the OBJMATERIAL class. Again, their names are:

• draw_material(),
• build_material(), and
• set_draw_callback_material()

Looking at how they're used in the modified sample code and you might see something like:

    obj->objmaterial[i].build_material(NULL);

Note that "material" shows up twice in this line of code, once in "objmaterial", and again in "build_material". When the old code named the function OBJ_build_material(), but no OBJMATERIAL was to be seen in the argument list it made sense for the code to call out that what the function really did was operate on an OBJMATERIAL (data) object. Now that the new code is used as a class method to explicitly operate on an instance of OBJMATERIAL I think having "_material" in the name of the method is redundant so I have removed it and the names are now:

• draw(),
• build(), and
• set_draw_callback()

The OBJMESH data class acquires most of the remaining functions which begin with the "OBJ_" prefix. The methods added to the OBJMESH class are:

• update_bounds() (I made this name plural),
• build_vbo(),
• set_attributes(),
• build(),
• build2(),
• optimize(),
• draw(),
• draw2(),
• draw3(), and
• free_vertex_data()

Like the methods moved into the OBJMATERIAL class I've stripped "_mesh" from the names of the methods to avoid redundancy.

This should leave only two of the original functions unaccounted for:

• OBJ_build_texture(), and
• OBJ_build_program()

As you might have guessed OBJ_build_program() becomes PROGRAM::build(). This was hinted at in a previous post.

If you guessed that OBJ_build_texture() will become TEXTURE::build() you were again correct but since I haven't yet discussed creating the TEXTURE data class I've postponed that change.

As mentioned above the OBJ class uses the std::vector template to create containers to hold OBJMATERIAL, OBJMESH, "PROGRAM *", and "TEXTURE *" data. Each of of these data types start off with a name field. In a previous post, I removed the name field from the UNIFORM data type and changed the PROGRAM object class to hold its UNIFORM data in a container created using the std::map template. I don't think the elements of the vectors objmaterial, objmesh, program, or texture have an inherent order. There are OBJ methods which assume an inherent order (OBJ::get_texture_index() and OBJ::get_program_index()) but the code could just as easily retrieve items from these containers using a name (like OBJ::get_mesh(), OBJ::get_program(), OBJ::get_material() and OBJ::get_texture()). The question then becomes "Why didn't I declare objmaterial, objmesh, program, and texture using the std::map template?" (Of course, if I had done this I would have had to modify any place in the code which uses OBJ::get_texture_index() or OBJ::get_program_index() but I think that's easily done.) The PROGRAM class type dereferences its name field in several of its class methods so we can't easily remove the name data member from the class definition. If we store the collection of PROGRAM pointers using std::map we have to store the name information twice, once as data member of the class, again as a key into the std::map to retrieve the PROGRAM pointer. There's a second problem: the name field of the PROGRAM class is passed into the utility function get_file_name() and this function can modify the value stored in name. If we use the PROGRAM name as the key with std::map the key must be treated as a read-only entry so name must be read-only, but clearly, because get_file_name() changes the contents of name, name is not read-only. The TEXTURE data type (to become the TEXTURE class in a future post) suffers from the same problems. It looks like the name fields for OBJMESH and OBJMATERIAL are treated as read-only and are never dereferenced in any of their respective member functions so we can probably store the OBJMESH and OBJMATERIAL instances in their OBJ parent using std::map. I started to re-implement objmaterial as

    std::map<std::string,OBJMATERIAL>   objmaterial;

but ran into problems when a function would return "OBJMATERIAL *" by doing a look-up on objmaterial. For some reason, the object pointed to by the "OBJMATERIAL *" return value would get corrupted; I suspect that the problem has to do with the inner workings of std::map but haven't had a chance to verify my hypothesis. I was able to make the code work correctly if I redefined objmaterial as:

    std::map<std::string,OBJMATERIAL *> objmaterial;

I have briefly tried to implement this same strategy for objmesh and ran into a problem. In one of the modules I haven't yet spoken about there is a function which passes in a pointer to an OBJ ("OBJ *") and an integer which is used by the function to index into the objmesh array of the OBJ. It may be possible to rewrite the interface to that function so that the address of the objmesh needed is passed in directly but that task will have to wait until I get to that particular module.

In the interim, I'm not committing the change to objmaterial to be an instance of std::map into the archive at GitHub until I've had more time to make sure that the change is stable.

The method OBJMESH::update_bounds() calculates:

• the Axially Aligned Bounding Box of the mesh (AABB),
• the midpoint of the AABB, and
• (the comment says that the method also calculates) the radius of the bounding sphere of the mesh.

The author has commented out what I would expect to be the correct method of computing the radius of the bounding sphere of the mesh from the dimensions of the AABB but instead
the author calculated the radius of the bounding sphere as one half of the AABB's maximum dimension. I think the description of what is being calculated is wrong but the author doesn't explain why he calculated the radius this way or how the quantity is really used so all I can say about the computation is that I don't think it calculates what the comment says it calculates. Using this method to calculate the bounding sphere radius it's entirely possible to have two meshes with their midpoints separated by a distance that exceeds the sum of the lengths of their respective bounding sphere radii yet have the two objects intersecting each other. This could lead to situations where a collision has happened on the display but the code hasn't yet detected the collision. You need to determine for your own applications which computation better describes what you need, or, possibly, you need to compute the bounding sphere radius using some other means.

One other comment regarding the OBJMESH::update_bounds() method. I changed the initial values for the vec3 data members named min and max of the OBJMESH pointed to by this. The new values which I've used have also been used to initialize min and max in another module which will be covered in a future post. Feel free to search for the changes in the other module if you want to look ahead. Anyway, I think the values I've assigned to min, and max are less likely to get tripped up by pathological data cases. One never knows what kinds of models the code might import.

Another bug lurking in the original version of the code is a bit more subtle but I'm still surprised that the compiler didn't print an error message. In the OBJ class the data member offset is declared to be an array of unsigned int. This actually leads to two bugs. The first is that offset[3] is tested to see if it holds the value -1, if it does then that means that the (graphics) object doesn't have texture coordinates or bi-normal/-tangent information which needs to be passed to the programmable shaders. Since the entries in offset are unsigned int they can only hold values that are non-negative, i.e., it's never possible for offset[3] to contain the value -1. The second bug is that in the original code space was allocated for the OBJ data object using calloc(3). One of the things calloc() does is zero out the all of the memory space it returns to the caller; after the OBJ data object is allocated the only other time offset[3] is set is inside OBJMESH::build_vbo() and then the only value it is ever given (besides its initial value of zero) is the offset of the texture coordinate vertex attribute. That is, even if offset[3] could hold the value -1 the code never even tries to set offset[3] to use the flag value -1. These two problems each require a solution. I solve the problem of offset[3] not being set to a flag value by initializing the entire offset array to have all of its bits set to one (in for a penny, in for a pound!); as written today we don't need a flag value in any of the other items in the offset array but while we're making the change we might as well future-proof the code. The second fix is that offset[3] is no longer tested against -1; it is now tested against ~0. And I need to add a symbol to the header file so that ~0 has a symbolic name. Remember, I hate "magic numbers" and ~0 is a magic number.

Speaking of hating magic numbers I've added several other things to the header file as well. In particular, I've added three enum types (TextureMap, VertexAttribute, and AttributeOffset), and #define statements which give a collection of string constants symbolic names. I'll discuss AttributeOffset now but postpone talking about the others until I cover the sample code from chapter 4.

Above I referred repeatedly to the OBJ data member offset[3]. The code doesn't have any comments to tell the reader what the significance of the offset[3] is. I had to reverse engineer that the offset[3] holds the OpenGL offset for the vertex attribute which the shader code uses to hold texture coordinates. I worked it out once and I can work it out again but by replacing 3 in the expression offset[3] with a symbolic name I don't have to remember what offset[3] represents; the symbolic name will tell me. I used the symbolic name AO_TexCoord0 (AO stands for "[OpenGL Vertex] Attribute Offset"). Likewise, I reverse engineered what the other entries in the offset array represent and added symbolic names for them too. I encapsulated the various symbolic names as values of the enum data type AttributeOffset. Lastly, I modified the various uses of offset[int] to use the new symbolic names for the various indices in place of the integers 0, 1, 2, 3, and 4.

Giving the array indices symbolic names isn't the only way to solve this problem. I also considered changing offset from being an array of unsigned int to be a struct with the data fields vertex, normal, fnormal, texcoord0, and tangent0. In some ways I think this is a more natural expression of the data which offset holds. Normally, I think of using an array (or in C++ a type created from the std::vector template) as a collection of data items which I need to iterate over sequentially. The code doesn't iterate over the elements of the array (to me, iterating implies a loop statement) but the elements of the array do form the members of a set and belong together logically, and, IMHO, using a struct expresses their relationship to each other better than using an array. But, again, it's just an opinion, and, in the end, I did leave the data field as an array.

That about wraps up this post. Next time I'll cover the changes to the first three sample programs in chapter 3.