<div dir="ltr">On 12 June 2013 02:29, Fabian Bieler <span dir="ltr"><<a href="mailto:fabianbieler@fastmail.fm" target="_blank">fabianbieler@fastmail.fm</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>On 2013-06-12 01:50, Paul Berry wrote:<br>
> This patch adds piglit tests to ensure that gl_ClipDistance works<br>
> properly as a geometry shader input.<br>
</div>[...]<br>
<div>> +in gl_PerVertex {<br>
> + float gl_ClipDistance[2];<br>
> +} gl_in[];<br>
</div>I'm not sure if it's ok to leave out members when redeclaring built-in interface blocks (gl_Position and gl_PointSize in this case). I didn't find anything on this in the spec but a quick google search indicates it's common practice so it should probably be ok.<br>
[...]<br></blockquote><div><br></div><div>My observation of the nVidia proprietary driver is that it doesn't care whether you leave out members when redeclaring built-in interface blocks, provided that you don't try to use the members that you leave out. Jordan, you've looked at interface blocks more recently than I have. Do you have an opinion on this?<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>> diff --git a/tests/spec/glsl-1.50/execution/geometry/clip-distance-bulk-copy.shader_test b/tests/spec/glsl-1.50/execution/geometry/clip-distance-bulk-copy.shader_test<br>
> new file mode 100644<br>
> index 0000000..fbdee46<br>
> --- /dev/null<br>
> +++ b/tests/spec/glsl-1.50/execution/geometry/clip-distance-bulk-copy.shader_test<br>
> @@ -0,0 +1,93 @@<br>
> +# This test checks that the geometry shader can perform a bulk copy of<br>
> +# the entire gl_ClipDistance array from input to output.<br>
> +<br>
> +[require]<br>
> +GL >= 2.0<br>
</div>Up to version 3.0 OpenGL only mandates the support of up to 6 user defined clip planes/distances. The version requirement should probably be bumped to 3.1 or only 6 clip distances should be used. This also applies to the same test in the next patch of the series.<br>
</blockquote><div><br></div><div>Yes, but GLSL 1.30 onward require that gl_MaxClipDistances be at least 8. Since this test requires GLSL 1.50 (see line below) there shouldn't be a problem.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>> +GLSL >= 1.50<br>
> +<br>
> +[vertex shader]<br>
> +#version 150<br>
> +<br>
> +in vec4 vertex;<br>
> +in float offset;<br>
> +out float offset_to_gs;<br>
> +out float gl_ClipDistance[8];<br>
</div>Shouldn't gl_ClipDistance be redeclared inside the gl_PerVertex interface block? I know the spec allows redeclaring gl_TexCoord at global scope in the compatibility profile but it explicitly states:<br>
"This treatment is a special case for gl_TexCoord[], not a general method for redeclaring members of blocks." (glsl spec v1.5 page 79 (page 85 of the PDF)).<br></blockquote><div><br></div><div>Good point. I'll fix this.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div>> +<br>
> +void main()<br>
> +{<br>
> + gl_Position = vertex;<br>
> + offset_to_gs = offset;<br>
> + for (int i = 0; i < 8; i++) {<br>
> + gl_ClipDistance[i] = offset + float(i);<br>
> + }<br>
> +}<br>
> +<br>
> +[geometry shader]<br>
> +#version 150<br>
> +<br>
> +layout(triangles) in;<br>
> +layout(triangle_strip, max_vertices = 3) out;<br>
> +<br>
> +in float offset_to_gs[3];<br>
> +in gl_PerVertex {<br>
> + vec4 gl_Position;<br>
> + float gl_ClipDistance[8];<br>
> +} gl_in[];<br>
> +out float offset_to_fs;<br>
> +out gl_PerVertex {<br>
> + vec4 gl_Position;<br>
> + float gl_ClipDistance[8];<br>
> +};<br>
> +<br>
> +void main()<br>
> +{<br>
> + bool ok = true;<br>
> + for (int i = 0; i < 3; i++) {<br>
> + gl_Position = gl_in[i].gl_Position;<br>
> + gl_ClipDistance = gl_in[i].gl_ClipDistance;<br>
> + offset_to_fs = offset_to_gs[i];<br>
> + EmitVertex();<br>
> + }<br>
> +}<br>
> +<br>
> +[fragment shader]<br>
> +#version 150<br>
> +<br>
> +in float gl_ClipDistance[8];<br>
> +in float offset_to_fs;<br>
> +<br>
> +void main()<br>
> +{<br>
> + bool ok = true;<br>
> + for (int i = 0; i < 8; i++) {<br>
> + if (distance(gl_ClipDistance[i], offset_to_fs + float(i)) > 1e-6)<br>
> + ok = false;<br>
> + }<br>
> + if (ok)<br>
> + gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0);<br>
> + else<br>
> + gl_FragColor = vec4(1.0, 0.0, 0.0, 1.0);<br>
> +}<br>
> +<br>
> +[vertex data]<br>
> +vertex/float/2 offset/float/1<br>
> +-1.0 -1.0 1.0<br>
> + 1.0 -1.0 2.0<br>
> + 1.0 1.0 3.0<br>
> +-1.0 1.0 4.0<br>
> +<br>
> +[test]<br>
> +# Since the fragment shader's gl_ClipDistance array is only defined<br>
> +# for elements that have clipping enabled, we need to enable all 8<br>
> +# clip planes. Fortunately the values we use for gl_ClipDistance are<br>
> +# always positive, so no pixels are actually clipped.<br>
> +enable GL_CLIP_PLANE0<br>
> +enable GL_CLIP_PLANE1<br>
> +enable GL_CLIP_PLANE2<br>
> +enable GL_CLIP_PLANE3<br>
> +enable GL_CLIP_PLANE4<br>
> +enable GL_CLIP_PLANE5<br>
> +enable GL_CLIP_PLANE6<br>
> +enable GL_CLIP_PLANE7<br>
> +draw arrays GL_TRIANGLE_FAN 0 4<br>
> +probe all rgba 0.0 1.0 0.0 1.0<br>
</div></div>[...]<br>
<br>
Otherwise, the series looks good to me.<br>
<span><font color="#888888"><br>
Fabian<br>
<br>
</font></span></blockquote></div><br></div></div>