This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100392/

graphics/mesh.h (Diff revision 1)
namespace GluonGraphics
71	/**	73	VertexBuffer* vertexBuffer() const;

Please add a comment about what this method does.

graphics/mesh.cpp (Diff revision 1)

126	glGenBuffers( 1, &d->buffer );	107	return d->buffer->isLoaded();

Why does buffer have an isLoaded method? Buffer should not track that state, it should be indifferent to how vertices have been added.

graphics/vertexattribute.h (Diff revision 1)
		namespace GluonGraphics
		10	class VertexAttribute

Comments!!!

graphics/vertexattribute.h (Diff revision 1)
		namespace GluonGraphics
		18	void insert( float data );

insert? insert where? if you mean append, please use append as method name. :)

graphics/vertexattribute.h (Diff revision 1)
		namespace GluonGraphics
		22	const char* name() const;

Hm, I tend to prefer to use QString as much as possible.

graphics/vertexattribute.h (Diff revision 1)
		namespace GluonGraphics
		23	int size() const;

What about using size() for the size in bytes and itemCount or something similar for the number of items?

graphics/vertexattribute.cpp (Diff revision 1)
		VertexAttribute::VertexAttribute( const VertexAttribute& other )
		42	d->data = other.d->data;

Just copying these pointers seems rather dangerous to me. Then again, you're not cleaning them so I guess we just leak instead of crashing....

graphics/vertexattribute.cpp (Diff revision 1)
		VertexAttribute::VertexAttribute( const VertexAttribute& other )
		106	d->data = other.d->data;

See the comment @ copy ctor. (By the way, it is usually easier to either write *this = other in the copy ctor so you use the assignment operator and save some additional lines of code)

graphics/vertexbuffer.h (Diff revision 1)
		namespace GluonGraphics
		24	void create();

create does not really seem the best name for this method, maybe use initialize() or initializeHardwareBuffer even?

graphics/vertexbuffer.h (Diff revision 1)
		namespace GluonGraphics
		34	bool isLoaded() const;

See my comment on mesh, buffer should not track loading state. If it means "created has been called" then that should be the name of the method.

- Arjen

On January 14th, 2011, 9:54 p.m., Giulio Camuffo wrote:

Review request for Gluon.

By Giulio Camuffo.

Updated Jan. 14, 2011, 9:54 p.m.

Description

This patch adds the VertexAttribute and VertexBuffer classes, moving the rendering out of Mesh. It allows a Mesh subclass to use more vertex attributes adding them to the buffer provided by the method Mesh::vertexBuffer.

There is the singleDataSize method in VertexAttribute which name is basically a placeholder because i could not find a better one. It returns the number of fields for each vertex. E.g 3 for the vertex position (x,y,z), 4 for the color (r,g,b,a), etc. Suggestions welcome!

Testing

I tested it with a sprite component and my TerrainMesh. It works right.

Diffs

graphics/CMakeLists.txt (984fe25)
graphics/mesh.h (95d1230)
graphics/mesh.cpp (d367c62)
graphics/vertexattribute.h (PRE-CREATION)
graphics/vertexattribute.cpp (PRE-CREATION)
graphics/vertexbuffer.h (PRE-CREATION)
graphics/vertexbuffer.cpp (PRE-CREATION)

View Diff