On 8 September 2011 23:07, Eric Anholt <<a href="mailto:eric@anholt.net" target="_blank">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"> --- tests/all.tests | 15 ++ tests/spec/gl-2.0/CMakeLists.gl.txt | 16 ++ tests/spec/gl-2.0/CMakeLists.txt | 1 + tests/spec/gl-2.0/vertex-program-two-side.c | 235 +++++++++++++++++++++++++++ 4 files changed, 267 insertions(+), 0 deletions(-) create mode 100644 tests/spec/gl-2.0/CMakeLists.gl.txt create mode 100644 tests/spec/gl-2.0/vertex-program-two-side.c diff --git a/tests/all.tests b/tests/all.tests index fc66ff6..cbd40d0 100644 --- a/tests/all.tests +++ b/tests/all.tests @@ -729,6 +729,21 @@ add_texwrap_test3(gl20, '2D', 'npot', 'proj') add_texwrap_test2(gl20, '3D', 'npot') add_texwrap_test3(gl20, '3D', 'npot', 'proj') add_plain_test(gl20, 'getattriblocation-conventional') +gl20['vertex-program-two-side '] = concurrent_test('vertex-program-two-side') +gl20['vertex-program-two-side front'] = concurrent_test('vertex-program-two-side front') +gl20['vertex-program-two-side back'] = concurrent_test('vertex-program-two-side back') +gl20['vertex-program-two-side primary'] = concurrent_test('vertex-program-two-side primary') +gl20['vertex-program-two-side primary front'] = concurrent_test('vertex-program-two-side primary front') +gl20['vertex-program-two-side primary back'] = concurrent_test('vertex-program-two-side primary back') +gl20['vertex-program-two-side secondary'] = concurrent_test('vertex-program-two-side secondary') +gl20['vertex-program-two-side secondary front'] = concurrent_test('vertex-program-two-side secondary front') +gl20['vertex-program-two-side secondary back'] = concurrent_test('vertex-program-two-side secondary back') +gl20['vertex-program-two-side disabled'] = concurrent_test('vertex-program-two-side disabled') +gl20['vertex-program-two-side disabled front'] = concurrent_test('vertex-program-two-side disabled front') +gl20['vertex-program-two-side disabled primary'] = concurrent_test('vertex-program-two-side disabled primary') +gl20['vertex-program-two-side disabled primary front'] = concurrent_test('vertex-program-two-side disabled primary front') +gl20['vertex-program-two-side disabled secondary'] = concurrent_test('vertex-program-two-side disabled secondary') +gl20['vertex-program-two-side disabled secondary front'] = concurrent_test('vertex-program-two-side disabled secondary front') </blockquote><div> I found the meaning of the arguments to be hard to follow: - "front" sets the global variable "back" to false, which means the VS doesn't set gl_BackColor or gl_BackSecondaryColor. - "back" sets "front" to false, which means the VS doesn't set gl_FrontColor or gl_FrontSecondaryColor. - "primary" sets "secondary" to false, which means the VS doesn't set gl_FrontSecondaryColor or gl_BackSecondaryColor. - "secondary" sets "primary" to false, which means the VS doesn't set gl_BackSecondaryColor or gl_FrontSecondaryColor. So, for example, concurrent_test('vertex-program-two-side') causes the VS to set all four color outputs, where a naive reader might have expected none of them to be set. Also, concurrent_test('vertex-program-two-side front back primary') would cause nothing to be tested, where a naive reader would probably have expected it to test both front and back. It would be way easier to reason about this code if the globals "front", "back", "primary", and "secondary" all started out false, and the presence of arguments set them to true. Also, a few cases aren't tested: - VS sets all colors except gl_BackSecondaryColor. - VS sets all colors except gl_FrontSecondaryColor. - VS sets all colors except gl_BackColor. - VS sets all colors except gl_FrontColor. - VS sets only gl_FrontColor and gl_BackSecondaryColor. - VS sets only gl_BackColor and gl_FrontSecondaryColor. Granted, the standard doesn't specify the meaning of these cases, but it doesn't specify that their behavior is undefined either, so I think it's reasonable to verify that the undefined behavior is confined to the variable that wasn't set. For instance, in the case where the VS sets all colors except gl_BackSecondaryColor, we should test that primary color works properly, and that secondary color works properly when you look at the front side of a triangle. Considering some of the primary/secondary color code I ran across in my recent refactor of i965 VUE setup on Mesa, I would not be surprised if in some of these cases, the "undefined" behavior bleeds over between primary and secondary colors, or between front and back. It might even cause mesa to abort with an assertion. Considering the standard's vagueness on what exactly is undefined in these cases, I think it's reasonable to err on the side of testing more cases rather than fewer. (Note: of course, to test these additional cases we would have to further change the meaning of the command-line options. You might consider just having one flag per VS output variable, plus of course the enable/disable flag for VERTEX_PROGRAM_TWO_SIDE). By the same token, I see that we aren't testing the cases where no well-defined color is expected at all (e.g. VERTEX_COLOR_TWO_SIDE is disabled, and the VS only sets gl_BackColor and/or gl_BackSecondaryColor). Although we can't check the colors of the rectangles in this case (since they are undefined), I think we still should try drawing the rectangles just to make sure there isn't a crash. </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> # Group spec/glsl-1.00 spec['glsl-1.00'] = Group() diff --git a/tests/spec/gl-2.0/CMakeLists.gl.txt b/tests/spec/gl-2.0/CMakeLists.gl.txt new file mode 100644 index 0000000..bc771e7 --- /dev/null +++ b/tests/spec/gl-2.0/CMakeLists.gl.txt @@ -0,0 +1,16 @@ +include_directories( + ${GLEXT_INCLUDE_DIR} + ${OPENGL_INCLUDE_PATH} + ${GLUT_INCLUDE_DIR} + ${piglit_SOURCE_DIR}/tests/mesa/util + ${piglit_SOURCE_DIR}/tests/util +) + +link_libraries ( + piglitutil + ${OPENGL_gl_LIBRARY} + ${OPENGL_glu_LIBRARY} + ${GLUT_glut_LIBRARY} +) + +add_executable (vertex-program-two-side vertex-program-two-side.c) diff --git a/tests/spec/gl-2.0/CMakeLists.txt b/tests/spec/gl-2.0/CMakeLists.txt index 9a13702..6903d88 100644 --- a/tests/spec/gl-2.0/CMakeLists.txt +++ b/tests/spec/gl-2.0/CMakeLists.txt @@ -1 +1,2 @@ add_subdirectory (api) +piglit_include_target_api() \ No newline at end of file diff --git a/tests/spec/gl-2.0/vertex-program-two-side.c b/tests/spec/gl-2.0/vertex-program-two-side.c new file mode 100644 index 0000000..b2038e9 --- /dev/null +++ b/tests/spec/gl-2.0/vertex-program-two-side.c @@ -0,0 +1,235 @@ +/* + * Copyright © 2011 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + */ + +/** @file vertex-program-two-side.c + * + * Tests two-sided lighting behavior. + * + * From the GL 2.1 spec, page 63 (page 77 of the PDF): + * + * "Additionally, vertex shaders can operate in two-sided color + * mode. When a vertex shader is active, front and back colors + * can be computed by the vertex shader and written to the + * gl_FrontColor, gl_BackColor, gl_FrontSecondaryColor and + * gl_BackSecondaryColor outputs. If VERTEX PROGRAM TWO SIDE is + * enabled, the GL chooses between front and back colors, as + * described below. Otherwise, the front color output is always + * selected. Two-sided color mode is enabled and disabled by + * calling Enable or Disable with the symbolic value VERTEX + * PROGRAM TWO SIDE." + * + * This appears to override the text in the GLSL 1.10 spec which + * implies that two-sided behavior always occurs. + */ + +#define _GNU_SOURCE +#include "piglit-util.h" + +int piglit_width = 100, piglit_height = 100; +int piglit_window_mode = GLUT_RGB | GLUT_ALPHA | GLUT_DOUBLE; + +static GLint prog; + +static bool primary = true; +static bool secondary = true; +static bool enabled = true; +static bool front = true; +static bool back = true; +static float frontcolor[4] = {0.0, 0.5, 0.0, 0.0}; +static float backcolor[4] = {0.0, 0.0, 0.5, 0.0}; +static float secondary_frontcolor[4] = {0.0, 0.25, 0.0, 0.0}; +static float secondary_backcolor[4] = {0.0, 0.0, 0.25, 0.0}; + +static const char *fs_source_primary = + "void main()\n" + "{\n" + " gl_FragColor = gl_Color;\n" + "}\n"; + +static const char *fs_source_secondary = + "void main()\n" + "{\n" + " gl_FragColor = gl_SecondaryColor;\n" + "}\n"; + +static const char *fs_source_both = + "void main()\n" + "{\n" + " gl_FragColor = gl_Color + gl_SecondaryColor;\n" + "}\n"; </blockquote><div> Rather than add the colors in this case, consider having a uniform variable to tell the FS whether to use gl_Color or gl_SecondaryColor, and having the program draw four rectangles instead of two. This would make it easier to inspect the test results manually (since it's diffucult to mentally add RGB color values when viewing on screen), and it would have the advantage allowing some of the partially-undefined cases to be tested (e.g. where the VS sets only gl_FrontColor and gl_BackSecondaryColor). </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> + +void +add(float *values, float *a) +{ + int i; + + for (i = 0; i < 4; i++) + values[i] += a[i]; +} + +void +get_expected(float *values, bool drew_front) +{ + int i; + + for (i = 0; i < 4; i++) + values[i] = 0.0; + + if (drew_front || !enabled) { + if (primary) + add(values, frontcolor); + if (secondary) + add(values, secondary_frontcolor); + } + + if (!drew_front && enabled) { + if (primary) + add(values, backcolor); + if (secondary) + add(values, secondary_backcolor); + } </blockquote><div> Minor nit: it's not immediately obvious on first reading that: (a) exactly one or the other of these if tests will always execute, and (b) the condition indicates whether we expect to see front colors or back colors. Consider changing this to something like: bool front_colors_expected = enabled ? draw_front : true; ... if (front_colors_expected) { ... } else { ... } </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204, 204, 204);padding-left:1ex"> +} + +enum piglit_result +piglit_display(void) +{ + int front_x = 10; + int front_y = 10; + int front_w = piglit_width / 2 - 20; + int front_h = piglit_height - 20; + int back_x = piglit_width - 10; + int back_y = 10; + int back_w = -(front_w); </blockquote><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204, 204, 204);padding-left:1ex"> + int back_h = piglit_height - 20; </blockquote><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204, 204, 204);padding-left:1ex"> + bool pass = true; + float expected[4]; + + piglit_ortho_projection(piglit_width, piglit_height, false); + + glClearColor(0.5, 0.5, 0.5, 0.5); + glClear(GL_COLOR_BUFFER_BIT); + + if (front) { + piglit_draw_rect(front_x, front_y, front_w, front_h); + get_expected(expected, true); + pass = pass && piglit_probe_rect_rgba(front_x, front_y, + front_w, front_h, + expected); + } + + if (back || !enabled) { + piglit_draw_rect(back_x, back_y, back_w, back_h); + get_expected(expected, false); + pass = pass && piglit_probe_rect_rgba(back_x + back_w, back_y, + -back_w, back_h, + expected); + } </blockquote><div> Another minor nit: usually the one-letter abbreviations x, y, w, and h are intuitively clear, but the fact that we're cleverly negating w in order to get a peek at the back side of the rectangle makes the second call to piglit_probe_rect_rgba() hard to follow. Consider doing something more explicit. Also a comment would be nice to explain the cleverness of negating the width to see the back side of the rectangle. For example, perhaps something like this: int back_x_right = piglit_width - 10; int back_w = front_w; int back_x = back_x_right - back_w; ... if (back || !enabled) { /* Draw a reversed rectangle so we see its back side */ piglit_draw_rect(back_x_right, back_y, -back_w, back_h); get_expected(expected, false); pass = pass && piglit_probe_rect_rgba(back_x, back_y, back_w, back_h, expected); } </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204, 204, 204);padding-left:1ex"> + + piglit_present_results(); + + return pass ? PIGLIT_PASS : PIGLIT_FAIL; +} + +static void +setup_output(char **out, const char *name, float *values) +{ + asprintf(out, + " %s = vec4(%f, %f, %f, %f);\n", + name, + values[0], + values[1], + values[2], + values[3]); +} + +void +piglit_init(int argc, char **argv) +{ + GLint vs, fs; + char *vs_outputs[4] = {"", "", "", ""}; + char *vs_source; + const char *fs_source = fs_source_both; + int i; + + piglit_require_GLSL(); + + if (!GLEW_VERSION_2_0) { + printf("Requires OpenGL 2.0\n"); + piglit_report_result(PIGLIT_SKIP); + } + + for (i = 1; i < argc; i++) { + if (strcmp(argv[i], "disabled") == 0) { + enabled = false; + } else if (strcmp(argv[i], "front") == 0) { + back = false; + } else if (strcmp(argv[i], "back") == 0) { + front = false; + } else if (strcmp(argv[i], "primary") == 0) { + secondary = false; + fs_source = fs_source_primary; + } else if (strcmp(argv[i], "secondary") == 0) { + primary = false; + fs_source = fs_source_secondary; + } else { + fprintf(stderr, "unknown argument %s\n", argv[i]); + } + } + + assert(enabled || front); + + if (front && primary) + setup_output(&vs_outputs[0], "gl_FrontColor", frontcolor); + if (back && primary) + setup_output(&vs_outputs[1], "gl_BackColor", backcolor); + if (front && secondary) + setup_output(&vs_outputs[2], "gl_FrontSecondaryColor", secondary_frontcolor); + if (back && secondary) + setup_output(&vs_outputs[3], "gl_BackSecondaryColor", secondary_backcolor); + + asprintf(&vs_source, + "void main()\n" + "{\n" + " gl_Position = ftransform();\n" + "%s%s%s%s" + "}\n", + vs_outputs[0], + vs_outputs[1], + vs_outputs[2], + vs_outputs[3]); + + vs = piglit_compile_shader_text(GL_VERTEX_SHADER, vs_source); + fs = piglit_compile_shader_text(GL_FRAGMENT_SHADER, fs_source); + prog = piglit_link_simple_program(vs, fs); + + if (!prog || !vs || !fs) { + printf("VS source:\n%s", vs_source); + printf("FS source:\n%s", fs_source); + piglit_report_result(PIGLIT_FAIL); + } + + glUseProgram(prog); + + if (enabled) + glEnable(GL_VERTEX_PROGRAM_TWO_SIDE); +} -- 1.7.5.4 _______________________________________________ Piglit mailing list <a href="mailto:Piglit@lists.freedesktop.org" target="_blank">Piglit@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/piglit" target="_blank">http://lists.freedesktop.org/mailman/listinfo/piglit</a> </blockquote></div>