[Piglit] [PATCH 3/4] vertex-program-two-side: New test for VERTEX_PROGRAM_TWO_SIDE.
Paul Berry
stereotype441 at gmail.com
Mon Sep 12 13:56:28 PDT 2011
On 8 September 2011 23:07, Eric Anholt <eric at anholt.net> wrote:
> ---
> 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')
>
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.
>
> # 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";
>
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).
> +
> +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);
> + }
>
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 {
...
}
> +}
> +
> +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);
>
+ int back_h = piglit_height - 20;
>
+ 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);
> + }
>
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);
}
> +
> + 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
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20110912/d03f5868/attachment-0001.htm>
More information about the Piglit
mailing list