[Piglit] [PATCH] Add tests for gl_ClipDistance and gl_ClipVertex.

[Paul Berry]

On 16 August 2011 13:45, Ian Romanick <idr at freedesktop.org> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 08/09/2011 02:55 PM, Paul Berry wrote:
>> This patch is a preparation for work I'm about to do to add
>> gl_ClipDistance support to Mesa, and future work that I hope to do to
>> fix bugs in gl_ClipVertex in Mesa.  It adds a thorough set of tests
>> for the following behaviors of gl_ClipDistance and gl_ClipVertex:
>>
>> For gl_ClipVertex:
>> - It behaves properly when equal to gl_Position.
>> - It behaves properly when not equal to gl_Position.
>> - It falls back to gl_Position when it is not set.
>> - Its homogeneous coordinate is respected.
>> - The homogeneous coordinate is respected when falling back to gl_Position.
>>
>> For gl_ClipDistance:
>> - It works when all 6 clip planes are enabled.
>> - Its behavior is properly affected by clip plane enable flags.
>> - It can be explicitly or implicitly sized in the vertex shader.
>> - It can be sized up to gl_MaxVaryingComponents but no higher.
>> - In the fragment shader, it contains properly interpolated values.
>
> There a couple of "dumb" tests missing:
>
>  - Write a constant value to gl_ClipVertex that will never be clipped.
> Verify that the whole primitive is drawn.
>
>  - Write a constant value to gl_ClipVertex that will always be clipped.
>  Verify that nothing is drawn.
>
>  - Write a constant values to gl_ClipDistance that will never be
> clipped.  Verify that the whole primitive is drawn.
>
>  - Write a constant values to gl_ClipDistance that will always be
> clipped.  Verify that nothing is drawn.
>
> Silly tests like these are useful for bring-up.

Hmm, my intuition is that the existing tests are going to be adequate
for bring-up, but the tests you suggest are easy enough to write.
I'll put 'em in and maybe I will learn something :)

>
> There are comments about a couple specific tests below...
>
>> diff --git a/tests/spec/glsl-1.30/execution/clipping/vs-clip-distance-implicitly-sized.shader_test b/tests/spec/glsl-1.30/execution/clipping/vs-clip-distance-implicitly-sized.shader_test
>> new file mode 100644
>> index 0000000..185fb22
>> --- /dev/null
>> +++ b/tests/spec/glsl-1.30/execution/clipping/vs-clip-distance-implicitly-sized.shader_test
>> @@ -0,0 +1,52 @@
>> +# From the GLSL 1.30 spec section 7.1 (Vertex Shader Special
>> +# Variables):
>> +#
>> +#   The gl_ClipDistance array is predeclared as unsized and must be
>> +#   sized by the shader either redeclaring it with a size or indexing
>> +#   it only with integral constant expressions. This needs to size the
>> +#   array to include all the clip planes that are enabled via the
>> +#   OpenGL API; if the size does not include all enabled planes,
>> +#   results are undefined. The size can be at most
>> +#   gl_MaxClipDistances. The number of varying components (see
>> +#   gl_MaxVaryingComponents) consumed by gl_ClipDistance will match
>> +#   the size of the array, no matter how many planes are enabled. The
>> +#   shader must also set all values in gl_ClipDistance that have been
>> +#   enabled via the OpenGL API, or results are undefined. Values
>> +#   written into gl_ClipDistance for planes that are not enabled have
>> +#   no effect.
>> +#
>> +# This test checks that the GLSL compiler respects the size of
>> +# gl_ClipDistance when it is implicitly sized in the vertex shader by
>> +# indexing into it with integral constant expressions.
>> +#
>> +# The spec isn't clear about whether it's necessary for the GLSL
>> +# compiler to size gl_ClipDistance to the minimum size possible.  In
>> +# other words, if only gl_ClipDistance[2] is accessed, must the
>> +# compiler infer that the size of gl_ClipDistance is 3, or is any size
>> +# greater than or equal to 3 acceptable?  This test assumes any size
>> +# greater than or equal to 3 is acceptable.
>> +
>> +[require]
>> +GLSL >= 1.30
>> +
>> +[vertex shader]
>> +#version 130
>> +
>> +void main()
>> +{
>> +  gl_Position = gl_Vertex;
>> +  gl_FrontColor = (gl_ClipDistance.length() >= 3) ? vec4(0.0, 1.0, 0.0, 1.0)
>> +                                                  : vec4(1.0, 0.0, 0.0, 1.0);
>
> This should actually generate an error.  Page 26 (page 32 of the PDF) of
> the GLSL 1.30 spec says:
>
>    "The length method cannot be called on an array that has not been
>    explicitly sized."

Yes, you're right.  I discovered this when starting to work on Mesa's
gl_ClipDistance implementation, sadly not until after committing this
patch.  Unfortunately, since you can't find out the size of an array
using .length(), I can't think of a way to verify using Piglit that
implicit array sizing of gl_ClipDistance works correctly, other than
the indirect approach of using gl_ClipDistance in a shader (without
explicitly sizing it) and verifying that clipping takes effect.  Can
you think of a more direct test?  Since we already have a test that
uses gl_ClipDistance without sizing it, and verifies that clipping
takes effect, I think the best thing to do is to just delete this
test.  But I'm open to alternatives if you have a better idea.

>
> In this case gl_ClipDistance is implicitly sized after the call to
> length.  We /should/ have similar tests for gl_TexCoord.  It behaves in
> a similar manner.
>
>> +  gl_ClipDistance[2] = 1.0;
>> +}
>> +
>> +[fragment shader]
>> +#version 130
>> +void main()
>> +{
>> +  gl_FragColor = gl_Color;
>> +}
>> +
>> +[test]
>> +draw rect -1 -1 2 2
>> +probe all rgba 0.0 1.0 0.0 1.0
>> diff --git a/tests/spec/glsl-1.30/execution/clipping/vs-clip-distance-sizeable-to-max.shader_test b/tests/spec/glsl-1.30/execution/clipping/vs-clip-distance-sizeable-to-max.shader_test
>> new file mode 100644
>> index 0000000..accd639
>> --- /dev/null
>> +++ b/tests/spec/glsl-1.30/execution/clipping/vs-clip-distance-sizeable-to-max.shader_test
>> @@ -0,0 +1,45 @@
>> +# From the GLSL 1.30 spec section 7.1 (Vertex Shader Special
>> +# Variables):
>> +#
>> +#   The gl_ClipDistance array is predeclared as unsized and must be
>> +#   sized by the shader either redeclaring it with a size or indexing
>> +#   it only with integral constant expressions. This needs to size the
>> +#   array to include all the clip planes that are enabled via the
>> +#   OpenGL API; if the size does not include all enabled planes,
>> +#   results are undefined. The size can be at most
>> +#   gl_MaxClipDistances. The number of varying components (see
>> +#   gl_MaxVaryingComponents) consumed by gl_ClipDistance will match
>> +#   the size of the array, no matter how many planes are enabled. The
>> +#   shader must also set all values in gl_ClipDistance that have been
>> +#   enabled via the OpenGL API, or results are undefined. Values
>> +#   written into gl_ClipDistance for planes that are not enabled have
>> +#   no effect.
>> +#
>> +# This test checks that the size of gl_ClipDistance can be set to
>> +# gl_MaxClipDistances without error, and that this actually causes the
>> +# size of the array to be set properly.
>> +
>> +[require]
>> +GLSL >= 1.30
>> +
>> +[vertex shader]
>> +#version 130
>> +out float gl_ClipDistance[gl_MaxClipDistances];
>
> We should also test that
>
> out float gl_ClipDistance[gl_MaxClipDistances + 1];
>
> generates an error.  I see there's a similar test for accesses beyond
> gl_MaxClipDistances when gl_ClipDistance is implicitly sized.

Actually the test that I think you're referring to
(clip-distance-not-sizeable-above-max.c) checks that an error occurs
when there is an access beyond gl_MaxClipDistances and gl_ClipDistance
is _explicitly_ sized to gl_MaxClipDistances + 1.  But your point is
valid: we should have two tests, one which verifies that
gl_ClipDistance can't be explicitly sized larger than
gl_MaxClipDistances, and one which verifies that it can't be
implicitly sized larger than gl_MaxClipDistances (by accessing an
element beyond gl_MaxClipDistances).  I'll make that change.

So far in the process of implementing gl_ClipDistance in Mesa, I've
discovered two other flaws in this patch:

1. fs-clip-distance-interpolated.shader_test needs to explicitly size
gl_ClipDistance[] in the shader, since it accessed it using a
non-constant index
2. clip-distance-not-sizeable-above-max.c needs to pass regardless of
whether the error occurs at compile time or link time, because some
implementations (e.g. nVidia) produce the error at link time, while
others (Mesa, with the code that I'm in the process of writing)
produce it at compile time.

I wouldn't be surprised to find more flaws as I continue working on
gl_ClipDistance in Mesa.  If it's all the same to you, I'd prefer to
reduce code churn by buffering up all of these Piglit fixes and
sending them to the list as a lump around the time that I have a first
cut at actual gl_ClipDistance functionality in Mesa.