On 24 October 2011 14:56, Eric Anholt <<a href="mailto:eric@anholt.net">eric@anholt.net</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> <div class="im">On Thu, 20 Oct 2011 14:12:24 -0700, Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> wrote: > This patch adds the ability for shader_runner tests to include a > "[vertex data]" section containing data in columnar format, for example: > > [vertex data] > vertex/float/3 foo/uint/1 bar/int/2 > 0.0 0.0 0.0 0xe0000000 0 0 > 0.0 1.0 0.0 0x70000000 1 1 > 1.0 1.0 0.0 0x00000000 0 1 > > Each column header is of the form ATTRNAME/TYPE/COUNT, where ATTRNAME > is the name of the vertex attribute to be bound to this column, TYPE > is the type of data that follows ("float", "int", or "uint"), and > COUNT is the vector length of the data (e.g. "3" for vec3 data). > > To send vertex data to the shader, use the new shader_runner command > "draw arrays". The parameters are the same as for the glDrawArrays() > command, so for example to draw triangle primitives using 3 elements > from the vertex data array starting at element 0, use the command: > > draw arrays GL_TRIANGLES 0 3 > > More detailed examples can be found in the tests/shaders/vbo > directory. > > The implementation is largely in a new file, piglit-vbo.cpp, so that > it can be re-used by other piglit tests if necessary. </div>For the other commits, Reviewed-by: Eric Anholt <<a href="mailto:eric@anholt.net">eric@anholt.net</a>> For this one, comments inline. <div><div></div><div class="h5"> > + } else if (sscanf(line, "draw arrays %31s %d %d", s, &x, &y)) { > + GLenum mode = decode_mode(s); > + int first = x; > + size_t count = (size_t) y; > + if (first < 0) { > + printf("draw arrays 'first' must be >= 0\n"); > + piglit_report_result(PIGLIT_FAIL); > + } else if ((size_t) first >= num_vbo_rows) { > + printf("draw arrays 'first' must be < %lu\n", > + num_vbo_rows); > + piglit_report_result(PIGLIT_FAIL); > + } > + if (count <= 0) { > + printf("draw arrays 'count' must be > 0\n"); > + piglit_report_result(PIGLIT_FAIL); > + } else if (count > num_vbo_rows - (size_t) first) { > + printf("draw arrays cannot draw beyond %lu\n", > + num_vbo_rows); > + piglit_report_result(PIGLIT_FAIL); > + } > + /* TODO: wrapper? */ > + glDrawArrays(mode, first, count); > } else if (string_match("disable", line)) { > do_enable_disable(line + 7, false); > } else if (string_match("enable", line)) { </div></div>Not sure what the TODO is about here. </blockquote><div> Whoops, that was a reminder to myself to see if I needed to call a piglit_DrawArrays() wrapper function instead of calling glDrawArrays() directly. I'm pretty sure I don't, since this function is part of OpenGL 2.1 and every implementation that's worth testing these days supports OpenGL 2.1. I'll remove the comment. </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> <div class="im"> > @@ -1523,4 +1600,7 @@ piglit_init(int argc, char **argv) > > process_test_script(argv[1]); > link_and_use_shaders(); > + if (vertex_data_start != NULL) > + num_vbo_rows = setup_vbo_from_text(prog, vertex_data_start, > + vertex_data_end); > } > diff --git a/tests/shaders/vbo/vbo-generic-float.shader_test b/tests/shaders/vbo/vbo-generic-float.shader_test > new file mode 100644 > index 0000000..8982211 > --- /dev/null > +++ b/tests/shaders/vbo/vbo-generic-float.shader_test </div>The 3 new testcases in here surprised me -- I expected that patch 6/6 was going to be the new testcases, though I suspect you wrote these for testing this shader_runner code. I would think that vbo-generic-uint would be part of tests/spec/glsl-1.30/ and the others part of tests/spec/glsl-1.10/? But given the other interpolation tests, it seems like generic-uint is the only interesting one here. </blockquote><div> Yes, you're right--the other tests don't really verify any interesting functionality of the GL implementation. I only wrote them to help me verify that the new shader_runner functionality worked correctly. I was considering dropping them entirely, but they seemed like a nice source of extra documentation for how this new shader_runner feature works. That was my primary motivation for putting them in tests/shaders/vbo--so they would be nearer to the shader_runner.c code they were illustrating. But perhaps I'm overthinking things. I'd be just as happy moving vbo-generic-uint to tests/spec/glsl-1.30/ and dropping the others. </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> <div class="im"> > diff --git a/tests/util/piglit-vbo.cpp b/tests/util/piglit-vbo.cpp > new file mode 100644 > index 0000000..fdba6b1 > --- /dev/null > +++ b/tests/util/piglit-vbo.cpp </div><div><div></div><div class="h5">> +/** > + * \file piglit-vbo.cpp > + * > + * This file adds the facility for specifying vertex data to piglit > + * tests using a columnar text format, for example: > + * > + * \verbatim > + * vertex/float/3 foo/uint/1 bar/int/2 > + * 0.0 0.0 0.0 10 0 0 # comment > + * 0.0 1.0 0.0 5 1 1 > + * 1.0 1.0 0.0 0 0 1 > + * \endverbatim > + * > + * The format consists of a row of column headers followed by any > + * number of rows of data. Each column header has the form > + * "ATTRNAME/TYPE/COUNT", where ATTRNAME is the name of the vertex > + * attribute to be bound to this column, TYPE is the type of data that > + * follows ("float", "int", or "uint"), and COUNT is the vector length > + * of the data (e.g. "3" for vec3 data). > + * > + * The data follows the column headers in space-separated form. "#" > + * can be used for comments, as in shell scripts. > + * > + * To process textual vertex data, call the function > + * setup_vbo_from_text(), passing the int identifying the linked > + * program, and the string containing the vertex data. The return > + * value is the number of rows of vertex data found. > + * > + * If an error occurs, setup_vbo_from_text() will print out a > + * description of the error and exit with PIGLIT_FAIL. > + * > + * For the example above, the call to setup_vbo_from_text() is roughly > + * equivalent to the following GL operations: > + * > + * \code > + * struct vertex_attributes { > + * GLfloat vertex[3]; > + * GLuint foo; > + * GLint bar[2]; > + * } vertex_data[] = { > + * { { 0.0, 0.0, 0.0 }, 10, { 0, 0 } }, > + * { { 0.0, 1.0, 0.0 }, 5, { 1, 1 } }, > + * { { 1.0, 1.0, 0.0 }, 0, { 0, 1 } } > + * }; > + * size_t stride = sizeof(vertex_data[0]); > + * GLint vertex_index = glGetAttribLocation(prog, "vertex"); > + * GLint foo_index = glGetAttribLocation(prog, "foo"); > + * GLint bar_index = glGetAttribLocation(prog, "bar"); > + * GLuint buffer_handle; > + * glGenBuffers(1, &buffer_handle); > + * glBindBuffer(GL_ARRAY_BUFFER, buffer_handle); > + * glBufferData(GL_ARRAY_BUFFER, sizeof(vertex_data), &vertex_data, > + * GL_STATIC_DRAW); > + * glVertexAttribPointer(vertex_index, 3, GL_FLOAT, GL_FALSE, stride, > + * (void *) offsetof(vertex_attributes, vertex)); > + * glVertexAttribIPointer(foo_index, 3, GL_UNSIGNED_INT, stride, > + * (void *) offsetof(vertex_attributes, foo)); > + * glVertexAttribIPointer(bar_index, 3, GL_INT, stride, > + * (void *) offsetof(vertex_attributes, bar)); > + * glEnableVertexAttribArray(vertex_index); > + * glEnableVertexAttribArray(foo_index); > + * glEnableVertexAttribArray(bar_index); > + * \endcode > + */ </div></div>Don't we need to do something to ensure that our vertex data ends up as attribute 0? Or that one of them does? Or is this something that is guaranteed by the API? </blockquote><div> I don't think it matters whether our vertex data (or any of the vertex attributes) ends up as attribute 0. The only things that are special about attribute 0 are that (a) it aliases gl_Vertex, and (b) when using the begin/end paradigm, it is the attribute that triggers the vertex shader to execute. (a) shouldn't matter because this feature is intended to be used with shaders that don't use gl_Vertex (perhaps I should document that more clearly). (b) shouldn't matter because this feature doesn't use the begin/end paradigm. </div></div>