[Piglit] [PATCH 3/4] vertex-program-two-side: New test for VERTEX_PROGRAM_TWO_SIDE.

[Eric Anholt]

On Mon, 12 Sep 2011 13:56:28 -0700, Paul Berry <stereotype441 at gmail.com> wrote:
Non-text part: multipart/alternative
> 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.

I was trying to make things a bit more concise so that the test name
for the likely most relevant test wasn't "vertex-program-two-side
gl_FrontColor gl_BackColor gl_FrontSecondaryColor
gl_BackSecondaryColor".  I think what you're saying makes more sense,
so the no-arguments version ends up undefined.

> 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).

> 
> 
> >
> >  # 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).

Fair enough.

> > +
> > +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 {
>     ...
> }

Agreed.

> > +}
> > +
> > +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);
> }

OK.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20110919/eb2cae5b/attachment.pgp>