[Bf-codereview] GPU buffers refactor (issue4631052)

[NicholasBishop at gmail.com]

Thanks for the review psy-fi.

> Better indeed, though in my opinion a bigger refactor should be done
on
> cdderivedmesh. The code should ideally setup the vbo's once (on
editmode exit)
> and the draw functions should be as simple as glVertexPointer,
gl*Pointer,
> glDrawArrays. Not 100% sure if this is possible but it would be ideal
if it is.

Regarding refactoring in cdderivedmesh -- I agree that this is needed,
and in fact a more cohesive refactoring of drawing across all the
DerivedMesh code is my eventual goal. (For example, CCGSubsurf doesn't
even use VBOs, except in combination with PBVH drawing.)

The problem is, there's a whole lot of drawing code already, and it
handles a lot of settings and combinations successfully. I don't want to
break things while refactoring, so I'm trying to clean up existing code
first; then we can get a better handle on larger redesign.


http://codereview.appspot.com/4631052/diff/1/source/blender/gpu/intern/gpu_buffers.c#newcode104
> source/blender/gpu/intern/gpu_buffers.c:104: useVBOs =
> (GL_ARB_vertex_buffer_object ? 1 : 0);
> shouldn't GL_ARB_vertex_buffer_object be
GLEW_ARB_vertex_buffer_object?

Not sure about this; the original code had GL_ARB_vertex_buffer_object,
so I kept it the same. Don't know enough about GLEW to say if it's wrong
though.



http://codereview.appspot.com/4631052/diff/1/source/blender/gpu/intern/gpu_buffers.c#newcode399
> source/blender/gpu/intern/gpu_buffers.c:399: int
points_per_mat[MAX_MATERIALS];
> one of the tragedies in the code is allocating MAX_MATERIALS(which is
the
> maximum short if memory serves right?) integers on the stack. It does
work
> apparently, though. I guess some other dynamic structure such as a
Linked List
> would serve better maybe. Likewise for every other function that uses
this
> scheme.

We could definitely replace these with e.g. a hash table from blenlib.
I'm not sure how much it matters in practice though; each of these
arrays is 64kb (MAX_MATERIALS=16384) and they're really only "allocated"
once. So we could definitely save a bunch of space (at least in the
common case where the number of active materials is way lower than
MAX_MATERIALS), but I doubt if there would be much impact (unless
MAX_MATERIALS becomes 2^32 in the future or something.)

Thanks,
-Nicholas

http://codereview.appspot.com/4631052/