Mesh Class Design

Started by
7 comments, last by TTT_Dutch 12 years, 6 months ago
Hello all,

Currently my Mesh class looks something like this:

#include <vector>

#include <Tech3D/Math/Vector3D.h>
#include <Tech3D/Graphics/OpenGL.h>
#include <Tech3D/Graphics/AABBox.h>
#include <Tech3D/Graphics/Program.h>
#include <Tech3D/Graphics/Image.h>

using namespace std;

/**
* Stores face properties and data.
* @see Mesh
*/
struct Face
{
Uint32 m_index[3];
};

/**
* Mesh used to store meshes.
*/
class Mesh
{
public:
/**
* Constructor for Mesh
*/
Mesh();

/**
* Destructor for Mesh
*/
~Mesh();

/**
* @return Number of vertices.
*/
int NumVertices()
{
return m_vertices.size();
}

/**
* @return Number of faces.
*/
int NumFaces()
{
return m_faces.size();
}

/**
* @return Number of texture coordinates.
*/
int NumTexCoords()
{
return m_textureCoords.size();
}
/**
* @return Number of normals.
*/
int NumNormals()
{
return m_normals.size();
}

/**
* Generates a bounding box around the mesh.
* @param boundingBox which should be generated.
*/
void GenerateAABB(AABBox *boundingBox);

/**
* Used to find out if the vertex exist.
* @return Index to a similar vertex, or to a new vertex.
*/
int VertexExist(const Vector3D &vertex);

/**
* Generates a buffer for the mesh.
*/
void GenerateBuffers();

/**
* Render this geometry with texture.
* @param textures Points to one texture which overwrite the existing.
*/
void Render(Matrix44 *clipMatrix, Program *glslProgram, Image *texture);

vector<Vector3D> m_vertices;///< Vertices of the geometry.
vector<Vector3D> m_normals;///< Normals of the geometry.
vector<UV> m_textureCoords;///< Texture coordinates.
vector<Face> m_faces;///< @see Face

int m_nFaces;///< Number of faces.
short m_dataType;///< Specifies how indices are in memory. GL_UNSIGNED_SHORT or GL_UNSIGNED_INT.
Uint32 m_buffers[3];///< Index of vertex object buffers.
};



First off, can anyone tell me if there are any problems with the way I am doing things? Second, I have two questions:

1. First, if I were to drop all the vector member variables and just generate the buffers by having the user pass arrays, vectors, or something with data in it as parameters to GenerateBuffers() then could I still access the data even though it would be stored only on the GPU via various VBOs? So that I could still call functions like:

void GenerateAABBox()
{
float *array;
array = GetVboDataFromGL(VboID); //Returns a pointer to data or something of those sorts.


for(int i = 0;i < NumVertices();i++)
{
boundingBox->Add(array);
}
}



2. Secondly, is there anyway I could slim down this class so that it doesn't need to have a GLSL program, Clip Matrix, or Texture passed to it in the Render Function (I guess I could create an Image member variable and pass it in when I send the other data)? Because right now we are using those things like so:


void Mesh::Render(Matrix44 *clipMatrix, Program *glslProgram, Image *texture)
{
glslProgram->Bind();

glBindVertexArray(m_buffers[2]);

glActiveTexture(GL_TEXTURE0);

int texID = -1;

if(texture)
{
texture->Bind();
texID = 0;
}

string texture_name = "texture", matrix_name = "projModelMatrix";

int matrixLocation = glslProgram->GetUniformLocation(texture_name), textureLocation = glslProgram->GetUniformLocation(matrix_name);

glUniformMatrix4fv(matrixLocation, 1, GL_FALSE, &clipMatrix->data[0]);
glUniform1f(textureLocation, texID);

glDrawElements(GL_TRIANGLES, m_nFaces * 3, m_dataType, BUFFER_OFFSET(0));
glslProgram->UnBind();
}


THANKS,
Brent
Advertisement
Replaced your comments with some notes (keep the doxygen commenting though obviously!)

#include <vector>
#include <Tech3D/Math/Vector3D.h>
#include <Tech3D/Graphics/OpenGL.h>
#include <Tech3D/Graphics/AABBox.h>
#include <Tech3D/Graphics/Program.h>
#include <Tech3D/Graphics/Image.h>

using namespace std; // Don't do this in a header. It's *VERY* bad form....

struct Face
{
Uint32 m_index[3];
};

class Mesh
{
public:

Mesh();
~Mesh();

// method should be const
int NumVertices() const
{
return m_vertices.size();
}

// method should be const
int NumFaces() const
{
return m_faces.size();
}

// Surely this is the same number as the number of vertices? (or zero if not use I guess?)
// method should be const
int NumTexCoords() const
{
return m_textureCoords.size();
}

// same again. This should always match the num verts no?
// method should be const
int NumNormals()
{
return m_normals.size();
}

// ok, although you could pass by reference
// method should be const
void GenerateAABB(AABBox *boundingBox) const;

// method should be const
int VertexExist(const Vector3D &vertex) const;

void GenerateBuffers();

// again. You could pass by reference
// method should be const!
void Render(Matrix44 *clipMatrix, Program *glslProgram, Image *texture);


// Why is all this stuff public?

vector<Vector3D> m_vertices;///< Vertices of the geometry.
vector<Vector3D> m_normals;///< Normals of the geometry.
vector<UV> m_textureCoords;///< Texture coordinates.
vector<Face> m_faces;///< @see Face


// isn't this identical to m_faces.size() ????
int m_nFaces;
short m_dataType;
Uint32 m_buffers[3];
};


1. First, if I were to drop all the vector member variables and just generate the buffers by having the user pass arrays, vectors, or something with data in it as parameters to GenerateBuffers() then could I still access the data even though it would be stored only on the GPU via various VBOs? [/quote]

You can get a pointer to the data you've loaded up.... see glMapBuffer. Generally though, this incurs a bit of a performance penalty so it's not something you want to be doing frequently.

2. Secondly, is there anyway I could slim down this class so that it doesn't need to have a GLSL program, Clip Matrix, or Texture passed to it in the Render Function (I guess I could create an Image member variable and pass it in when I send the other data)? Because right now we are using those things like so:[/quote]

Make the program decide how to bind the mesh instead?

First, thank you for commenting up all that code. I am going to change it now :).

You can get a pointer to the data you've loaded up.... see glMapBuffer. Generally though, this incurs a bit of a performance penalty so it's not something you want to be doing frequently. [/quote]

With glMapBuffer. If all I wanted to do was get the size could I just call something like:

GLvoid * vbo_buffer = glMapBuffer(GL_ARRAY_BUFFER, GL_READ_ONLY);
sizeof(*vbo_buffer);



Also, if I wanted to use read the data would I have to do this?


GLvoid * vbo_buffer = glMapBuffer(GL_ARRAY_BUFFER, GL_READ_ONLY);
memcpy(vbo_buffer, vertices, sizeof(*vbo_buffer));

OR

GLvoid * vbo_buffer = glMapBuffer(GL_ARRAY_BUFFER, GL_READ_ONLY);
cout << vbo_buffer[0] << endl; // I want to be able to access all the data as an array so that I can generate a bounding box for each mesh.


Make the program decide how to bind the mesh instead? [/quote]

Well ya kinda. I guess I am asking, is there anyway I can send the clip matrix and texture to the GLSL program without having to pass them as params. I would be okay with just passing a glsl Program. Like: Mesh.Render(Program * glslProgram). How would I accomplish this though? I have been told by my fellow programmer that you need to pass the clipMatrix because its a combination of the object's modelview matrix and the camera's program matrix. Could I just send the modelview matrix and projection matrix seperately? Also I think I could just make it so that I would pass the image in when I generate the buffers and hold it as a member variable.


Thanks,
Brent

EDIT: Could I not just use GetBufferSubData instead of glMapBuffers? Which is better? Also would [color=#1C2837][size=2]glGetBufferParameteriv(GL_BUFFER_SIZE) work for getting size?
string texture_name = "texture", matrix_name = "projModelMatrix";
This overhead is unnecessary. Either make the variables static so they are only created once or just use the raw string values. Feel free to wrap the string literals behind macros in order to ensure the same strings are used exactly each time (avoids misspellings).


L. Spiro

I restore Nintendo 64 video-game OST’s into HD! https://www.youtube.com/channel/UCCtX_wedtZ5BoyQBXEhnVZw/playlists?view=1&sort=lad&flow=grid


string texture_name = "texture", matrix_name = "projModelMatrix";
This overhead is unnecessary. Either make the variables static so they are only created once or just use the raw string values. Feel free to wrap the string literals behind macros in order to ensure the same strings are used exactly each time (avoids misspellings).


L. Spiro


Okay well I would just pass them in to the function but g++ throws:
error: no matching function for call to ‘Program::GetUniformLocation(const char [8])
note: candidates are: int Program::GetUniformLocation(std::string&)

Okay well I would just pass them in to the function but g++ throws:
error: no matching function for call to ‘Program::GetUniformLocation(const char [8])
note: candidates are: int Program::GetUniformLocation(std::string&)

GetUniformLocation should take a const std::string&.
Who wrote this?
Anyway make those strings static. As it is now you are allocating memory, copying characters over, and releasing that memory every time you call that function.


L. Spiro

I restore Nintendo 64 video-game OST’s into HD! https://www.youtube.com/channel/UCCtX_wedtZ5BoyQBXEhnVZw/playlists?view=1&sort=lad&flow=grid


You can get a pointer to the data you've loaded up.... see glMapBuffer. Generally though, this incurs a bit of a performance penalty so it's not something you want to be doing frequently.


With glMapBuffer. If all I wanted to do was get the size could I just call something like:

GLvoid * vbo_buffer = glMapBuffer(GL_ARRAY_BUFFER, GL_READ_ONLY);
sizeof(*vbo_buffer);
[/quote]

You should already know the size before hand! Failing that, you can use [font="Arial"]glGetBufferParameteriv, i.e.[/font]
[font="Arial"] [/font]
[font="Arial"]
GLint size;
glGetBufferParameteriv(GL_ARRAY_BUFFER, GL_BUFFER_SIZE, &size)
[/font]


Also, if I wanted to use read the data would I have to do this?



Either memcpy....

GLvoid * vbo_buffer = glMapBuffer(GL_ARRAY_BUFFER, GL_READ_ONLY);
memcpy(vbo_buffer, vertices, size);


or, cast to a pointer of the correct type to extract...

GLint numVec3s = size/(sizeof(float)*3);
Vector3* vec = (*Vector3*)glMapBuffer(GL_ARRAY_BUFFER, GL_READ_ONLY);
for(int i=0; i!=numVec3s; ++i)
{
std::cout << vec << std::endl;
}


Make the program decide how to bind the mesh instead? [/quote]

Well ya kinda. I guess I am asking, is there anyway I can send the clip matrix and texture to the GLSL program without having to pass them as params.
No, they gotta be params (unless you are using legacy GL, which isn't a great idea imho). The compiled and assembled program should be able to tell you if it uses one of those matrices or not (or takes a combined matrix) by simply querying the uniform locations. -1 means it's not used.... (and can be ignored).



EDIT: Could I not just use GetBufferSubData instead of glMapBuffers? Which is better?
[/quote]

buffers sub data can in theory be better for performance, but map buffers may be a little bit more convenient (no memory allocation required)

GetUniformLocation should take a const std::string&.


Okay thanks, I will change that now :).


Who wrote this?
Anyway make those strings static. As it is now you are allocating memory, copying characters over, and releasing that memory every time you call that function.


L. Spiro


Well the guy I am making this game with wrote the class originally, but I noticed stuff that was wrong but I didn't know how to solve it so that is why I consulted you guys.


You should already know the size before hand! Failing that, you can use [font="Arial"]glGetBufferParameteriv, i.e.[/font]

[font="Arial"]
GLint size;
glGetBufferParameteriv(GL_ARRAY_BUFFER, GL_BUFFER_SIZE, &size)
[/font]

[font="Arial"]Okay well I will either get the size as a parameter or figure it out when the data is passed :)
[/font]


Either memcpy....

GLvoid * vbo_buffer = glMapBuffer(GL_ARRAY_BUFFER, GL_READ_ONLY);
memcpy(vbo_buffer, vertices, size);


or, cast to a pointer of the correct type to extract...

GLint numVec3s = size/(sizeof(float)*3);
Vector3* vec = (*Vector3*)glMapBuffer(GL_ARRAY_BUFFER, GL_READ_ONLY);
for(int i=0; i!=numVec3s; ++i)
{
std::cout << vec << std::endl;
}
[/quote]

Okay thanks I will implement the latter now :D.

No, they gotta be params (unless you are using legacy GL, which isn't a great idea imho). The compiled and assembled program should be able to tell you if it uses one of those matrices or not (or takes a combined matrix) by simply querying the uniform locations. -1 means it's not used.... (and can be ignored). [/quote]

Alright I kinda understand. Hopefully I will figure it out. Thanks!


buffers sub data can in theory be better for performance, but map buffers may be a little bit more convenient (no memory allocation required)
[/quote]

Alright I will choose one of them.

Thanks for everyone's help,
Brent

This topic is closed to new replies.

Advertisement