On Tuesday 29 October 2013, Eric Anholt wrote: > Sorry we all dropped this on the floor. I'm taking a look at your > series now. > > Fredrik Höglund <fred...@kde.org> writes: > > diff --git a/tests/spec/arb_vertex_attrib_binding/errors.c > > b/tests/spec/arb_vertex_attrib_binding/errors.c > > new file mode 100644 > > index 0000000..adac098 > > --- /dev/null > > +++ b/tests/spec/arb_vertex_attrib_binding/errors.c > > > +#define MIN(a, b) (a < b ? a : b) > > There's already a MIN2 macro you can use.
This test doesn't actually use the macro. I guess it's a left over from the test I based it on. > > + /* uncreached */ > > "unreached" > > > +void > > +piglit_init(int argc, char **argv) > > +{ > > + GLint maxBindings, maxAttribs, maxRelativeOffset; > > + GLuint vbo, vao; > > + int i; > > + > > + piglit_require_extension("GL_ARB_vertex_attrib_binding"); > > + piglit_require_extension("GL_ARB_vertex_array_bgra"); > > + piglit_require_extension("GL_ARB_vertex_type_2_10_10_10_rev"); > > + piglit_require_extension("GL_ARB_ES2_compatibility"); > > Would be nice to see the extra extensions moved into > piglit_is_extension_supported() checks for executing those tests. > Though I suppose since a lot of the code for them is expecting > INVALID_ENUM, you can execute that with or without the extension > providing them :) Yeah, I've been meaning to do that, but I wanted to hear what others had to say about the ARB_vertex_attrib_binding dependencies first. > > + > > + /* Create and bind a vertex array object */ > > + glGenVertexArrays(1, &vao); > > + glBindVertexArray(vao); > > + > > + glGenBuffers(1, &vbo); > > + glBindBuffer(GL_ARRAY_BUFFER, vbo); > > + > > + > > + /* Query limits */ > > + glGetIntegerv(GL_MAX_VERTEX_ATTRIBS, &maxAttribs); > > + glGetIntegerv(GL_MAX_VERTEX_ATTRIB_BINDINGS, &maxBindings); > > + glGetIntegerv(GL_MAX_VERTEX_ATTRIB_RELATIVE_OFFSET, &maxRelativeOffset); > > + > > + /* Clear the error state */ > > + while (glGetError() != GL_NO_ERROR) {} > > What error state are you clearing here? If you really do expect to have > something you're ignoring, piglit_reset_gl_error() is the helper for > this. There shouldn't be any errors at this point, it's just make sure we start with a clean slate. Other piglit tests seem to do this as well. > > + /* index >= GL_MAX_VERTEX_ATTRIB_BINDINGS */ > > + glBindVertexBuffer(maxBindings, vbo, 0, 0); > > + if (!piglit_check_gl_error(GL_INVALID_VALUE)) > > + piglit_report_result(PIGLIT_FAIL); > > Instead of piglit_report_result(PIGLIT_FAIL) immediately, please set > pass = false, and only at the end of the test do > piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL); > > That way somebody writing a new implementation gets to see what's still > failing as they incrementally make things work. Good point. > Once these comments are fixed, this test is: > > Reviewed-by: Eric Anholt <e...@anholt.net> > > Errors I don't see covered: > > "An INVALID_OPERATION error is generated if no vertex array object is > bound." (When in core profile for BindVertexbuffer, VertexAttribBinding, > VertexBindingDivisor) > > " [Core profile only:] > An INVALID_OPERATION error is generated if buffer is not zero or a > name returned from a previous call to GenBuffers, or if such a name > has since been deleted with DeleteBuffers." Good catch. The specification actually didn't say anything about non-gen buffer names when I started on the implementation. > So it seems like we might want a core profile-only extra errors test. Yeah. > A non-error I noticed not explicitly touched in this series: "If > <buffer> is zero, any buffer object attached to this bindpoint is > detached." I'll add that in get.c, since it already has tests for binding buffers. Fredrik _______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit