On 19 September 2011 10:49, Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 <div class="im"> On 09/13/2011 02:54 PM, Paul Berry wrote: > The clipping tests previously in Piglit only tested the first 6 > clipping planes, since prior to GLSL 1.30, a conforming > implementation was allowed to only support 6 clipping planes. > However, in GLSL 1.30, the minimum value of gl_MaxClipDistances and > gl_MaxClipPlanes was increased to 8. > > This patch adds tests to verify that if the implementation reports > a GLSL version of at least 1.30, gl_MaxClipDistances and > gl_MaxClipPlanes are both at least 8. It also modifies > vs-clip-distance-all-planes-enabled.shader_test to exercise all 8 > clipping planes. > > The previous implementation of > vs-clip-distance-all-planes-enabled.shader_test used to also > verify, as a side effect, that gl_ClipDistance could be implicitly > sized. The new version does not (it requires gl_ClipDistance to be > explicitly sized, since it indexes into it using a loop variable). > So I've added a new test, > vs-clip-distance-implicitly-sized.shader_test, to verify this. > > Tested using the nVidia proprietary Linux driver. </div>I made some comments about max-clip-distances.shader_test (which also apply to max-clip-planes.shader_test) below. The changes to shader_runner and the other tests look fine. You can consider them Reviewed-by: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>> if you want to split them out and push them sooner rather than later. > --- tests/shaders/shader_runner.c | 2 + <div><div></div><div class="h5">> .../clipping/max-clip-distances.shader_test | 54 > ++++++++++++++ .../execution/clipping/max-clip-planes.shader_test | > 54 ++++++++++++++ > ...vs-clip-distance-all-planes-enabled.shader_test | 76 > ++++++++++---------- > .../vs-clip-distance-implicitly-sized.shader_test | 67 > +++++++++++++++++ tests/util/glew.h > | 2 + 6 files changed, 218 insertions(+), 37 deletions(-) create > mode 100644 > tests/spec/glsl-1.30/execution/clipping/max-clip-distances.shader_test > > create mode 100644 tests/spec/glsl-1.30/execution/clipping/max-clip-planes.shader_test > create mode 100644 > tests/spec/glsl-1.30/execution/clipping/vs-clip-distance-implicitly-sized.shader_test > > diff --git a/tests/shaders/shader_runner.c > b/tests/shaders/shader_runner.c index cde5ae3..a46ea4d 100644 --- > a/tests/shaders/shader_runner.c +++ > b/tests/shaders/shader_runner.c @@ -819,6 +819,8 @@ struct > enable_table { { "GL_CLIP_PLANE3", GL_CLIP_PLANE3 }, { > "GL_CLIP_PLANE4", GL_CLIP_PLANE4 }, { "GL_CLIP_PLANE5", > GL_CLIP_PLANE5 }, + { "GL_CLIP_PLANE6", GL_CLIP_PLANE6 }, + { > "GL_CLIP_PLANE7", GL_CLIP_PLANE7 }, { NULL, 0 } }; > > diff --git > a/tests/spec/glsl-1.30/execution/clipping/max-clip-distances.shader_test > b/tests/spec/glsl-1.30/execution/clipping/max-clip-distances.shader_test > > new file mode 100644 > index 0000000..d593fbf --- /dev/null +++ > b/tests/spec/glsl-1.30/execution/clipping/max-clip-distances.shader_test > > @@ -0,0 +1,54 @@ > +# [description] +# Verify correct behavior of > gl_MaxClipDistances. +# +# From section the GLSL 1.30 spec, section > 7.4 (Built-In Constants): +# +# "The following built-in constants > are provided to vertex and +# fragment shaders. The actual values > used are implementation +# dependent, but must be at least the > value shown. +# +# ... +# +# const int gl_MaxClipDistances = > 8;" +# +# This test verifies that gl_MaxClipDistances has the same > value in +# both the vertex and fragment shader, and that this > value is at least +# 8. + +[require] +GLSL >= 1.30 + +[vertex > shader] +#version 130 + +out float maxClipDistances; + +void > main() +{ + gl_Position = gl_Vertex; + + maxClipDistances = > gl_MaxClipDistances; +} + +[fragment shader] +#version 130 + +in > float maxClipDistances; + +void main() +{ + if (maxClipDistances != > gl_MaxClipDistances) { </div></div>This is valid, but probably not what we actually want. Shouldn't we pass maxClipDistances as an int? It means that the 'flat' interpolation qualifier will have to be used, and that may cause the test to fail on Mesa now even though we set gl_MaxClipDistances correctly. </blockquote><div> Hmm, yeah, using ints and the 'flat' qualifier would be a better way to do this. You're right that this causes the tests to fail right now on Mesa. I assume this is because passing 'int' data between VS and FS doesn't work properly yet? </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> Also, this test is really checking that gl_MaxClipDistances is set the same in the VS and FS. I'm not sure how useful that is. It seems like we want to query GL_MAX_CLIP_DISTANCES from the API and pass that as a uniform. Maybe that's an additional test? </blockquote><div> You're right--this would be more useful. Ok, I'll commit the tests that got your Reviewed-by, and I'll submit a follow-up patch to test that GL_MAX_CLIP_DISTANCES matches the value of gl_MaxClipDistances in both the VS and FS. Paul </div></div>