Read and write triangulated surfaces

Question

Recently, I wrote a class which can load an .obj file into vector array, and save the vector array as an .obj file. The .obj file looks something like this:

v 0.041159 -0.342570 1.029412 //vertices position : x, y,z
v 0.015579 -0.341674 1.077751
v 0.014664 -0.341184 1.031222
v 0.041923 -0.343100 1.073583
v 0.064655 -0.344713 1.068008
vt 0.516780 0.631480 // vertices' texture coordinate
vt 0.491540 0.621580
vt 0.495720 0.621740
vt 0.500000 0.621740
vt 0.504300 0.624760
vt 0.508540 0.628060
vt 0.512810 0.631590
vt 0.487450 0.621300
f 6497/6542 6321/6366 6319/6364 // triangles compounded by three vertices
f 6498/6543 6495/6540 6496/6541 // the number is index of vertices
f 6498/6543 6496/6541 6497/6542
f 6499/6544 6494/6539 6495/6540
f 6499/6544 6495/6540 6498/6543
f 6500/6545 6492/6537 6494/6539
f 6500/6545 6494/6539 6499/6544
f 6501/6546 6497/6542 6319/6364
f 6501/6546 6319/6364 6316/6361
f 6502/6547 6498/6543 6497/6542
f 6502/6547 6497/6542 6501/6546
f 6503/6548 6499/6544 6498/6543
f 6503/6548 6498/6543 6502/6547
f 6504/6549 6501/6546 6316/6361

I use double type for vertices and vertices' texture coordinates, unsigned int for faces.

Here is my header and implementation. The code can be compiled and run successfully. Are there any lurking errors here? How can I improve it?

trimesh.h

#ifndef TRIMESH_H
#define TRIMESH_H
#include <fstream>
#include <iostream>
#include <string>
#include <vector>

using namespace std;
class trimesh {
    public:
    std::vector<std::vector<double > > vertices;   
    std::vector<std::vector<double > > tex_coords;   
    std::vector<std::vector<double > > vert_normal;  
    std::vector<std::vector<double > > vert_color;  
    std::vector<std::vector<unsigned int> >  faces;
    std::vector<std::vector<unsigned int> >  faces_tc;  
    void load(const std::string filename);// load .obj file from given path
    void save(const std::string save_path, const bool with_color = true, const bool with_vt = true); // save the current mesh to the given save_path
};
#endif

trimesh.cpp

#include "trimesh.h"
#include <fstream>
#include <iostream> 
#include <string>
#include <sstream>
#include <vector>

using namespace std;
using std::ofstream;

void trimesh::load(const std::string filename)
{
    std::ifstream file(filename);
  
    if (file.is_open()) 
    {
        std::string line;
        while (std::getline(file, line)) 
        {
            // using printf() in all tests for consistency
            //printf("%s", line.c_str());
          
            istringstream in(line);
            vector<string> vec_line;
            string line_s;
            while (in >> line_s)
            {
                vec_line.push_back(line_s);
            }

            //std::cout << "vec_line:" << vec_line[0] << std::endl;
            std::vector<double> double_line;
            std::vector<double> double_line_color; 
            std::vector<unsigned int> unint_line;
            std::vector<unsigned int> unint_line_tc;
            if (vec_line[0]=="v")
            {
                double v0 = std::stod(vec_line[1]);
                double v1 = std::stod(vec_line[2]);
                double v2 = std::stod(vec_line[3]); 
                double_line.push_back(v0);
                double_line.push_back(v1);
                double_line.push_back(v2);
                vertices.push_back(double_line);
                double_line.clear();
            
                if (vec_line.size()==7)
                {
                    double c1 = std::stod(vec_line[4]);
                    double c2 = std::stod(vec_line[5]);
                    double c3 = std::stod(vec_line[6]);
                    double_line_color.push_back(c1);
                    double_line_color.push_back(c2);
                    double_line_color.push_back(c3);
                    vert_color.push_back(double_line_color);
                    double_line_color.clear();
                }
            }
            else if (vec_line[0]=="vt")
            {
                double vt0 = std::stod(vec_line[1]);
                double vt1 = std::stod(vec_line[2]);
                double_line.push_back(vt0);
                double_line.push_back(vt1);
                tex_coords.push_back(double_line);
                double_line.clear();
            }
            else if (vec_line[0]=="f")
            {
                size_t idx1 = vec_line[1].find("/");
                unsigned int v_idx_f_1 = std::stod(vec_line[1].substr(0, idx1)); 
                size_t idx2 = vec_line[1].find("/", idx1 + 1);
                unsigned int v_idx_ftc_1 = std::stod(vec_line[1].substr(idx1 + 1, idx2));

                size_t idx3 = vec_line[2].find("/");
                unsigned int v_idx_f_2 = std::stod(vec_line[2].substr(0, idx3)); 
                size_t idx4 = vec_line[2].find("/", idx3 + 1);
                unsigned int v_idx_ftc_2 = std::stod(vec_line[2].substr(idx3 + 1, idx4));

                size_t idx5 = vec_line[3].find("/");
                unsigned int v_idx_f_3 = std::stod(vec_line[3].substr(0, idx5)); 
                size_t idx6 = vec_line[3].find("/", idx5 + 1);
                unsigned int v_idx_ftc_3 = std::stod(vec_line[3].substr(idx5 + 1, idx6));

                unint_line.push_back(v_idx_f_1);
                unint_line.push_back(v_idx_f_2);
                unint_line.push_back(v_idx_f_3);
                faces.push_back(unint_line);

                unint_line_tc.push_back(v_idx_ftc_1);
                unint_line_tc.push_back(v_idx_ftc_2);
                unint_line_tc.push_back(v_idx_ftc_3);
                faces_tc.push_back(unint_line_tc);
            }
        }
        file.close();
    }
    else
    {
        printf ("loading obj: %s doesn't exists!\n", filename.c_str());
        throw;
    }
}

void trimesh::save(const std::string save_path, const bool with_color, const bool with_vt)
{
    ofstream outdata;
    outdata.open(save_path);
    if( !outdata ) 
    { // file couldn't be opened
        std::cout << "Error: file could not be opened" << std::endl;
        exit(1);
    }

  if (with_color && (vertices.size()==vert_color.size())) //write the vertices and colors
  {
      for (size_t i=0; i<vertices.size(); i++)
      {
          outdata << "v ";
          outdata << vertices[i][0] << " " << vertices[i][1] << " " << vertices[i][2] << " " << vert_color[i][0] << " " << vert_color[i][1] << " " << vert_color[i][2]  <<endl;
      }  
  }
  else // only write the vertices
  {
      for (size_t i = 0; i < vertices.size(); i++)
      {
          outdata << "v ";
          outdata << vertices[i][0] << " " << vertices[i][1] << " " << vertices[i][2]<<endl;
      }
  }

  if (with_vt)
  {
      for (size_t j=0; j<tex_coords.size(); j++)
      {
          outdata << "vt ";
          outdata << tex_coords[j][0] << " " << tex_coords[j][1] << endl;
      }
  }

  if (faces.size() == faces_tc.size())
  {
      for (size_t k = 0; k < faces.size(); k++)
      {
          outdata << "f ";
          outdata << faces[k][0] << "/" << faces_tc[k][0] << " " << faces[k][1] << "/" << faces[k][1] << " " << faces[k][2] << "/" << faces_tc[k][2]  <<endl;
      }  
  }
  else
  {
      for (size_t k = 0; k < faces.size(); k++)
      {
          outdata << "f ";
          outdata << faces[k][0]  << " " << faces[k][1] << " " << faces[k][2] << endl;
      }  
  }

  outdata.close();
}

Answer 1

Header

The header file includes a couple of headers that it doesn't need:

#include <fstream>
#include <iostream>

That said, I would consider including <iosfwd> and declaring functions that work on streams (probably using >> and <<) rather than files, to make unit-testing possible.

It also has using namespace std;, which is harmful - especially so in headers, where that affects every translation unit that includes the header. It's much better to qualify names as you use them, or to import just the names you need into a much smaller scope (e.g. at the function level).

A class with all its data public is usually written struct. Usually, we then make the functions that operate on it not be members.

I don't like vectors of vectors - it's a good idea to at least give a name to the inner vector. For example:

using vertex = std::vector<double>;
std::vector<vertex> vertices;

However, I'm not sure a vertex needs to be resizable - if they always have three members, then

using vertex = std::array<double,3>;

save() shouldn't modify the object, so declare it as a const member function.

The filenames should be passed as references to constant strings, rather than copying the string arguments. And the bool arguments shouldn't be declared const here, as that makes no difference to the caller (we can, and should, still use const where we define the functions):

void load(const std::string& filename);
void save(const std::string& save_path,
          bool with_color = true,
          bool with_vt = true) const;

Consider using std::filesystem::path instead of std::string, for clarity. Note also that adding bool arguments to a function like that can make the call sites hard to read (because it's not clear what each argument does). Consider using a "flags" enum instead:

enum save_option {
     color = 1;
     vt = 2;
     // then 4, 8, etc
};

Decide how errors are to be reported. If load() and save() are to return void, then we should document which exceptions may be thrown.

Implementation: load()

Again, it's a bad idea to using namespace std; at file scope.

Perhaps load() should clear its members before reading from the file? I'd write two member functions here:

void append(…);

void load(…) { clear(); append(…); }

In load() the test for file opening would be easier to read if inverted:

if (!file.is_open()) {
    printf("loading obj: %s doesn't exists!\n", filename.c_str());
    throw;
}
/// no need for "else" here
std::string line;
while (std::getline(file, line)) {

The content of the failure case needs some attention:

• printf isn't defined (std::printf is in <cstdio>).
• Error messages should go to the standard error stream (std::cerr if we use C++ streams rather than C-style I/O).
• throw without an object is only usable when we have a current exception - i.e. if we're within a catch - which is not the case here.
• The open could fail for reasons other than the file not existing - perhaps it exists, but is a directory, or doesn't have sufficient permission for us. We could use std::strerror(errno) to create a more informative message.
• We probably don't want to be printing a message at all here - pass the information to the caller (which might be a GUI program, for example) and let it choose how to deal with it.

---

It's good to read a line at a time and then parse it:

        std::istringstream in(line);
        std::vector<std::string> vec_line;
        std::string line_s;
        while (in >> line_s) {
            vec_line.push_back(line_s);
        }

We could eliminate the loop, if we include <algorithm> and <iterator>:

        std::istringstream in(line);
        std::vector<std::string> vec_line;
        std::copy(std::istream_iterator<std::string>{in},
                  std::istream_iterator<std::string>{},
                  std::back_inserter(vec_line));

Either way, we'll want to check that in isn't in error after reading it all, and that vec_line has at least one element before we attempt to use vec_line[0].

---

When parsing each line, we're breaking it up into a std::vector<std::string>, and then we parse some of those strings. But we can do better. If we read just the first word, we can then use that to determine how to read the rest of the line:

        std::istringstream in(line);
        std::string linetype;
        in >> linetype;
        if (in.eof()) {
            continue; // blank line - ignore
        }
        if (!in) {
            throw parse_failed();
        }

        if (linetype=="v") {
            vertex v;
            in >> v.x >> v.y >> v.z;
            if (!in) {
                throw parse_failed();
            }
            color c;
            if (in >> c.red >> c.green >> c.blue) {
                // we have a color; store it in the vertex
            }
            vertices.push_back(vertex);
        } else if (linetype=="vt") {
            point vt;
            in >> vt.x >> vt.y;
            if (!in) {
                throw parse_failed();
            }
            tex_coords.push_back(vt);
        } else if (linetype=="f") {
            face f;
            char a, b, c;   // separators
            in >> f.f1 >> a >> f.ftc1
               >> f.f2 >> b >> f.ftc2
               >> f.f3 >> c >> f.ftc3;
            if (!in || a != '/' || b != '/' || c != '/') {
                throw parse_failed();
            }
            faces.push_back(f);
        }

This actually gets quite a bit simpler if we make vertex, point and face into object types with their own >> operators:

        if (linetype=="v") {
            vertex v;
            in >> v;
            vertices.push_back(vertex);
        } else if (linetype=="vt") {
            point vt;
            in >> vt;
            tex_coords.push_back(vt);
        } else if (linetype=="f") {
            face f;
            in >> f;
            faces.push_back(f);
        }

We might choose to set our stream to throw exceptions - e.g. in.exceptions(std::istream::failbit|std::istream::badbit);. We could improve reporting by keeping count of the number of lines we have read, so we can be more informative in our parse_failed exception.

Implementation: save()

The save() function is simpler than load(), but I spotted the following:

• Don't use std::exit() - it terminates the process immediately, without running destructors. We really need to be able to report errors back rather than abandoning the program anyway (it would be really annoying for a user to lose all their work when trying to save to a directory where they don't have permission, without being given a chance to select an alternative location!).

• The two alternatives for the with_color test are very similar. We can reduce the duplication there:

    if (vertices.size() != vert_color.size()) {
        // something's wrong - we can't write the colors
        with_color = false;
    }
    for (std::size_t i=0; i<vertices.size(); i++) {
        outdata << "v " << vertices[i][0] << " " << vertices[i][1] << " " << vertices[i][2];
        if (with_color) {
            out << " " << vert_color[i][0] << " " << vert_color[i][1] << " " << vert_color[i][2];
        }
        out << '\n';
    }
  

• Don't use std::endl when all you need is a newline. Flushing the output buffer every line unnecessarily slows the program.

• There's no point calling ofstream::close() unless we are going to check the state of the stream before it goes out of scope.

---

Summary

• Create and use specific types, and enable each to read and write itself.
• Improve the error checking and reporting.

Answer 2

Don't overuse std::vector

You are using std::vector for many things that shouldn't be vectors. For example, storing a single vertex should be done in a struct that holds the x, y and z coordinates:

struct dvec3 {
    double x;
    double y;
    double z;
};

I named it dvec3 because it is a vector (in the mathematical sense) of 3 doubles, and it follows the naming convention of GLSL. Inside class trimesh, you can then store the list of vertices like so:

std::vector<dvec3> vertices;

And when reading a vertex:

dvec3 vertex;
vertex.x = std::stod(vec_line[1]);
vertex.y = std::stod(vec_line[2]);
vertex.z = std::stod(vec_line[3]);
vertices.push_back(vertex);

Everything that has a fixed number of elements should probably not be a std::vector. For texture coordinates you should create a struct dvec2, for the vertex indices you should create a struct ivec3.

I strongly recommend you use the OpenGL Mathematics library, as it provides all these types for you.

Finally, splitting the string into the vector vec_line is also not necessary. Consider that you can rewrite the parsing code like so:

std::istringstream in(line);

char type;
in >> type;

switch (type) {
case 'v':
{
    dvec3 vertex;
    in >> vertex.x >> vertex.y >> vertex.z;
    vertices.push_back(vertex);
    break;
}
case 't':
    ...

Missing error checking

You check if opening the file was succesful, but you never check if it was completely read or written without errors. You can check if a file was completely read by checking that file.eof() is true after reading in all the lines. When writing files, you can check whether file.bad() is false.

Use range-for where appropriate

Since C++11 you can iterate over containers much easier using range-for notation. For example, when writing out all the vertices:

for (auto &vertex: vertices)
    outdata << "v " << vertex.x << ' ' << vertex.y << ' ' << vertex.z << '\n';

Use \n instead of std::endl

Prefer using \n instead of std::endl. The latter is equivalent to the former, except it also forces the output to be flushed, which is usually not necessary and is bad for performance.

Pass string by reference when appropriate

Strings can be large, and if you pass them by value to a function, a copy has to be made. You can avoid that by passing them as const references:

void trimesh::load(const std::string &filename)
{
    ...

How to handle errors

If opening the file did not succeed, your code does:

printf ("loading obj: %s doesn't exists!\n", filename.c_str());
throw;

This has several issues, the least of them is using the C printf() function when you should just use the C++ way if printing in C++ code. Second, always print error messages to the standard error output, so:

std::cerr << "Loading obj file failed: " << filename << " does not exist!\n";

But worst is throw. This doesn't even throw an exception, it is meant to rethrow an exception. If there is no active exception, this will just immediately terminate the process. There is no way for the caller to catch this.

The proper way is to throw an exception object that is one of the standard exception types, or a custom type that inherits from one of the standard ones. Almost always, you can just use std::runtime_error, like so:

throw std::runtime_error("Could not load obj file");

This allows the caller to catch the exception and handle it gracefully if so desired.

Alternatively, you could have the load() and save() member functions return a bool or another type that can indicate success or failure.

Pass std::istream and std::ostream instead of filenames

Your load() and save() functions expect to be given a filename, and those functions will take care of opening the file. Consider however passing a reference to a std::istream or std::ostream object instead. This means these functions no longer have the responsibility of opening the file, but also that they are much more flexible; they could read or write from any stream, not just files. For example, you can rewrite the save() function like so:

void trimesh::save(std::ostream &outdata, const bool with_color, const bool with_vt)
{
    if (with_color && vertices.size() == vert_color.size())
    {
        ...

And then the caller can either take care of opening the file:

trimesh mesh;
...
ofstream outdata("path/to/data.obj");
mesh.save(outdata, true, true);

But if it wanted to print the mesh to the terminal instead, it can just do:

mesh.save(std::cout, true, true);