Created attachment 198369 [details] [review]
crate: Use indices

I think the problem is because the number of rectangles passed to cogl_get_rectangle_indices only indicates the number of indices to generate, not the actual number of indices to render. Therefore the primitive is still being drawn using the number of vertices in the primitive which was set in the primitive constructor to G_N_ELEMENTS (vertices), which is 6 * 4.

To make it work you just need to add this line:

  cogl_primitive_set_n_vertices (data.prim, 6 * 6);

I guess the confusion is because the primitive_new constructors conflate the number of vertices to store in the buffer with the number of vertices to render. This doesn't make as much sense when you are using indices.

I wonder if we should think of a new name for cogl_primitive_set_n_vertices. I guess GL would refer to this number as 'number of elements'. It's conceptually more like cogl_primitive_set_n_vertices_to_draw. We should at least clarify this in the documentation.

If you can still break API at this point, why not make cogl_primitive_set_indices take an index count? The renames you suggest also sound good.

yeah this is a good point about us muddling the n_vertices for what we upload with the n_vertices we want for drawing.

I like the suggestion of passing an n_indices as part of cogl_primitive_set_indices as you suggest.

Although renaming the set_n_vertices api seems good on the face of it to help clarify things, when thinking through it a bit further I doesn't seem so clear that it's worthwhile. The problem is that the other members of CoglPrimitive (first_vertex, mode and attributes) are also conceptually all parameters for drawing only and so it starts to seem inconsistent if they aren't also named something like _get/set_first_draw_vertex, get/set_draw_mode and get/set_draw_attributes. At that point the "_draw" infix looks redundant. If we had had the n_indices argument for _set_indices() probably there wouldn't have been this confusion. Also a bigger problem we currently have when it comes to avoiding confusion here is a total lack of documentation for this API :-)

I'm hopeful that things will be clear by just adding the n_indices arg, adding some documentation for this API and explicitly explaining how the n_vertices args passed to the _primitive_new constructors and the n_indices args passed to _the set_indices API both implicitly update the CoglPrimitive::n_vertices arg. I'd say if things still seem a bit unclear at that point then lets think again if a rename could help.

Additionally, what is the GL terminology on this? If there's any parameter named something like "n_verts", does it refer to the number of verts in the VBO or the number of verts drawn?

ok I've updated the api as we discussed above, adding an n_indices arg to cogl_primitive_set_indices() and i've tried to improve the documentation a bit too.

Jasper, in answer to your last question GL would refer to the vertices being drawn. VBOs for GL just have a size in bytes and you either mmap the buffer to set whatever data you like or you use glBufferData/glBufferSubData to copy data into the buffer passing a size in bytes for what to copy.

Please see:

commit 8cf76ee36b46b8601a15ac223cb27e75f85f70d4
Author: Robert Bragg <robert@linux.intel.com>
Date:   Tue Oct 25 22:34:59 2011 +0100

    primitive: Add n_indices arg to _set_indices function
    
    When associating indices with a CoglPrimitive you are now forced to
    specify the number of indices that should be read when drawing.
    
    It's easy to forget to call cogl_primitive_set_n_vertices() after
    associating indices with a primitive (and anyway you can see that someone
    could be led to believe Cogl can determine that implicitly somewhow) so
    this should avoid a lot of mistakes with using the API.
    
    We'd expect that setting indices and updating the n_vertices property
    would go hand in hand 99% of the time anyway so this change should
    be more convenient as well as less error prone.
    
    This patch adds some documentation for cogl_primitive_set_indices and
    cogl_primitive_get/set_n_vertices. It also tries to clarify how the
    CoglPrimitive:n_vertices property is updated and what that property
    means in relation to other functions too.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=661019
    
    Reviewed-by: Neil Roberts <neil@linux.intel.com>

and

commit a8fbde4710baa3b8b17ac16b4ffaef1ac472b71e
Author: Robert Bragg <robert@linux.intel.com>
Date:   Tue Oct 25 21:45:49 2011 +0100

    doc: Adds some documentation for CoglIndices
    
    This adds just some basic documentation to try and explain what
    CoglIndices are useful for.
    
    Reviewed-by: Neil Roberts <neil@linux.intel.com>

Jasper, I've rebased your patch and pushed it to master. I hope you don't mind I also added some comments in there instead of having a separate commit for that.

thanks for the patch.

commit 5f7cdfed69b97c896f740a03e447f8de907e3e96
Author: Jasper St. Pierre <jstpierre@mecheye.net>
Date:   Thu Sep 8 21:35:48 2011 -0400

    crate: Use indices
    
    https://bugzilla.gnome.org/show_bug.cgi?id=661019
    
    Reviewed-by: Robert Bragg <robert@linux.intel.com>