Original Post
removed [Edited by - JoshWalters on December 20, 2007 2:56:38 AM]
🔒 Locked
Removed
Advertisement
class CObj{ CObj( const std::string &fileName ); ...}CObj::CObj( const std::string &fileName ) : enableTexture( false ) , enableNormal( false ){ string line, buf; Vertex tempVertices, tempNormals; TextureCoord tempTextureCoord; Face tempFace; ifstream fin( fileName.c_str() ); if(!fin.is_open()) { throw std::exception("Error: Could not open "); } ....Some people dislike exceptions. But since this isn't time-critical code-path, RAII design is better.
Also, what happens in your code if you do:
CObj o;o.Load("filename.txt");o.Load("filename.txt");o.Load("filename.txt");o.Load("filename.txt");o.Load("filename.txt");
removed
[Edited by - JoshWalters on December 20, 2007 2:31:50 AM]
[Edited by - JoshWalters on December 20, 2007 2:31:50 AM]
While the code itself seems ok (I didn't test it), there are two functional aspects you'd want to consider.
First is the OO-ification of the design. This isn't needed, but if you're using C++, it doesn't hurt to think about it. This is just my personal view, and isn't needed, since it doesn't really change much in functionality (apart from full RAII), it merely makes code more modular, and minimizes code duplication.
Apart from RAII (which prevents both, double loading and CObj containing an invalid class), you might also want to consider providing a more functional interface. One that's not only self-documenting, but also much simpler.
An example:
What does this gain you?
- Each data structure defines how to read itself from the file. Since we know the structure of Obj file, it's easy to determine that we have 1 definition per line, consisting of tokens. So it makes sense to centralize this.
- RAII - if anything goes wrong while reading the file, the object will not be created, exception will be thrown, everything will be cleaned up automatically. Here is where the design advantage of exceptions becomes clear.
- Extensible - if you choose to add another type, simply define its structure, and add a check to your parseContents method. Much better than rewriting the whole 1000 line block of code.
However, the last thing that's not addressed here, is faces. You're currently providing a indexes. This is somewhat of a problem, since Face itself can't do anything with them, and passing it into wrong context would cause problems. Also, as your code example demonstrates it, you need to write *a lot* of code just to find proper vertices, textures and other objects.
A more OO aproach would be, to resolve these indexes during loading. You'd end up with something like this:
And while loading, you'd then lookup the indexes, and create the faces that already point to proper structures, or, if these don't exist, find that out during loading.
After that, your rendering code becomes much less cluttered, and also more straightforward:
You could also use references instead of pointers in Face class, since you know that Face must be fully defined, and you also know that dependent classes will exist long enough.
First is the OO-ification of the design. This isn't needed, but if you're using C++, it doesn't hurt to think about it. This is just my personal view, and isn't needed, since it doesn't really change much in functionality (apart from full RAII), it merely makes code more modular, and minimizes code duplication.
Apart from RAII (which prevents both, double loading and CObj containing an invalid class), you might also want to consider providing a more functional interface. One that's not only self-documenting, but also much simpler.
An example:
typedef std::vector< std::string > TokenizedLine;// all public, make it a structstruct Vertex{ // RAII again Vertex( const TokenizedLine &tokens ) { if(tokens.size() < 4) throw std::exception("Error reading vertex"); x = atof(tokens[1].c_str()); y = atof(tokens[2].c_str()); z = atof(tokens[3].c_str()); } float x, y, z;};struct TextureCoord{ TextureCoord( const TokenizedLine &tokens ) { if(tokens.size() < 3) throw std::exception("Error creating texture, need 2 values"); u = atof(tokens[1].c_str()); v = atof(tokens[2].c_str()); } float u, v;};class CObj{public: CObj( const std::string &filename );private: void scanForFlags( std::ifstream &file ); void parseContents( std::ifstream &file );}CObj::CObj( const std::string &filename ){ std::ifstream f( filename ); scanForFlags( f ); f.seekp(0, std::ios_base::beg); parseContents( f ); f.close();}CObj::parseContents( std::ifstream &file ){ std::string line; while(!file.eof()) { getline(fin,line); stringstream ss(line); TokenizedLine tokens; while(ss >> buf) tokens.push_back(buf.c_str()); if(line.compare(0,2,"v ",0,2) == 0) { Vertices.push_back( Vertex( tokens) ); } else if(line.compare(0,2,"vt",0,2) == 0) { TextureCoords.push_back( TextureCoords( tokens ) ); } else ... }}What does this gain you?
- Each data structure defines how to read itself from the file. Since we know the structure of Obj file, it's easy to determine that we have 1 definition per line, consisting of tokens. So it makes sense to centralize this.
- RAII - if anything goes wrong while reading the file, the object will not be created, exception will be thrown, everything will be cleaned up automatically. Here is where the design advantage of exceptions becomes clear.
- Extensible - if you choose to add another type, simply define its structure, and add a check to your parseContents method. Much better than rewriting the whole 1000 line block of code.
However, the last thing that's not addressed here, is faces. You're currently providing a indexes. This is somewhat of a problem, since Face itself can't do anything with them, and passing it into wrong context would cause problems. Also, as your code example demonstrates it, you need to write *a lot* of code just to find proper vertices, textures and other objects.
A more OO aproach would be, to resolve these indexes during loading. You'd end up with something like this:
class Face{ Vertex *v1, *v2, *v3; Normal *n1, *n2, *n3; TextureCoord *t1, *t2, *t3;};And while loading, you'd then lookup the indexes, and create the faces that already point to proper structures, or, if these don't exist, find that out during loading.
After that, your rendering code becomes much less cluttered, and also more straightforward:
void renderFace( Face &f ){ if(model->enableTexture ) glTexCoord2f( f.t1->u, f.t1->v); if(model->enableNormal ) glNormal3f( f.n1->x, f.n1->y, f.n1->z); glVertex3f( f.v1->x, f.v1->y, f.v1->z); if(model->enableTexture ) glTexCoord2f( f.t2->u, f.t2->v); if(model->enableNormal ) glNormal3f( f.n2->x, f.n2->y, f.n2->z); glVertex3f( f.v2->x, f.v2->y, f.v2->z); if(model->enableTexture ) glTexCoord2f( f.t3->u, f.t3->v ); if(model->enableNormal ) glNormal3f( f.n3->x, f.n3->y, f.n3->z); glVertex3f( f.v3->x, f.v3->y, f.v3->z); }}void renderFaces(){ glBegin(GL_TRIANGLES); for(size_t size = 0; size < model->Faces.size(); size+=1) { renderFace( model->Faces[size] ); } glEnd();}You could also use references instead of pointers in Face class, since you know that Face must be fully defined, and you also know that dependent classes will exist long enough.
Topic Locked
This topic has been locked by a moderator. New replies are not allowed.
Sign in to reply to this topic.
Advertisement