[Piglit] [PATCH] Add tests for gl_ClipDistance and gl_ClipVertex.
Ian Romanick
idr at freedesktop.org
Tue Aug 16 16:37:35 PDT 2011
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 08/16/2011 03:00 PM, Paul Berry wrote:
> 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.
Right. The only methods that you have available to probe the size of an
implicitly sized array will affect the size of that array.
However, since there are things that you cannot do with an implicitly
sized array (e.g., use .length() or a non-constant index), we should
have negative compile tests for those cases.
>> 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
I missed that on my first review. If we're going to hit most of the
combinations, we should hit all of them. In particular, we should
expect compilation errors when:
- explicit size <= gl_MaxClipDistances, access >= explicit size
- explicit size > gl_MaxClipDistances
- implicit size > gl_MaxClipDistances
I thought the various C code tests hit the first two.
> 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
Right. I missed that on my first review.
> 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.
Oh bother. I really *HATE* that the GLSL spec doesn't say when errors
are supposed to be generated. The particular issue is that an array may
be "implicitly" sized in one compilation unit and explicitly sized in
another. The error in these two vertex shaders can only generated at
link time:
#version 120
float a[];
float foo(float x) { return a[37] * x; }
#version 120
float foo(float);
float a[5];
void main() { gl_Position = gl_Vertex * foo(3.14159); }
Again, we have the same issue with gl_TexCoord. It might be worth
cleaning up the few gl_TexCoord tests
(test/shaders/glsl-link-varyings-*.shader_tests,
tests/shaders/glsl-link-varying-TexCoord.shader_test,
tests/shaders/glsl-texcoord-array-2.shader_test, and
tests/glslparsertest/glsl2/unsized-array-non-const-index.vert) to be
more like these tests. *shrug*
> 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.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAk5K/z4ACgkQX1gOwKyEAw87VgCgjgbc/pr6p5P9EcwFAcuC76pt
zAUAnj1HAq/OMeta/vm/6EOSsPgHp63H
=fDyh
-----END PGP SIGNATURE-----
More information about the Piglit
mailing list