On 08/26/2013 11:38 PM, Paul Berry wrote:
Verified using the NVIDIA proprietary driver for linux.
---
tests/all.tests | 9 +
.../glsl-1.50/execution/geometry/CMakeLists.gl.txt | 1 +
.../execution/geometry/primitive-id-restart.c | 265 +++++++++++++++++++++
3 files changed, 275 insertions(+)
create mode 100644
tests/spec/glsl-1.50/execution/geometry/primitive-id-restart.c
+#define LONGEST_INPUT_SEQUENCE 12
Did you choose 12 to give gl_PrimitiveIDIn an opportunity to increment
for GL_TRIANGLES_ADJANCENCY? If so, comment please. If not, then comment
double-please, because I don't see the reason.
+#define NUM_INPUT_ELEMENTS \
+ (LONGEST_INPUT_SEQUENCE * (LONGEST_INPUT_SEQUENCE + 3) / 2)
+#define MAX_TOTAL_PRIMS \
+ (LONGEST_INPUT_SEQUENCE * (LONGEST_INPUT_SEQUENCE + 1) / 2)
The term 'INPUT' in the var name NUM_INPUT_ELEMENTS confused me. So went my
mind... "The ELEMENTS
are qualified as INPUT ELEMENTS, which distinguishes them from... OUTPUT
ELEMENTS?
SOMETHING_ELSE ELEMENTS? What type of elements are these, if there be INPUT and
OTHER_TYPES of elements? I do not know these ELEMENTS." Eventually, I saw that
they
were just vanilla ELEMENTS. Oh well.
The expression for MAX_TOTAL_PRIMS may look mysterious to the uninitiated.
Please
comment with
/* Sum of 1, 2, ..., LONGEST_INPUT_SEQUENCE. */
I found the expression for NUM_INPUT_ELEMENTS mysterious, and eventually
discovered
its round-about derivation. The following comment, or something similar, is
strongly needed:
/* Sum of 2, 3, ..., LONGEST_INPUT_SEQUENCE + 1. */
+static const char *vs_text =
+ "#version 150\n"
+ "\n"
+ "void main()\n"
+ "{\n"
+ "}\n";
+
+static const char *gs_template =
+ "#version 150\n"
+ "#define INPUT_LAYOUT %s\n"
+ "layout(INPUT_LAYOUT) in;\n"
This shader preamble feels weird. I expected the macro INPUT_LAYOUT to
be used elsewhere, but it isn't. I think
#version 150
layout(%s) in;
is easier to read. Anyways... just ignore me here if you want to call bikeshed.
+ "layout(points, max_vertices = 1) out;\n"
+ "\n"
+ "out int primitive_id;\n"
+ "\n"
+ "void main()\n"
+ "{\n"
+ " primitive_id = gl_PrimitiveIDIn;\n"
+ " EmitVertex();\n"
+ "}\n";
+ /* Find out how many times the GS got invoked so we'll know
+ * how many transform feedback outputs to examine.
+ */
+ glGetQueryObjectuiv(generated_query, GL_QUERY_RESULT,
+ &primitives_generated);
+ if (primitives_generated > MAX_TOTAL_PRIMS) {
+ printf("Expected no more than %d primitives, got %d\n",
+ MAX_TOTAL_PRIMS, primitives_generated);
+ pass = false;
+
+ /* Set primitives_generated to MAX_TOTAL_PRIMS so that
+ * we don't overflow the buffer in the loop below.
+ */
The test neglects to clamp primitives_generated like the comment says.
+ }
+
+ /* Check transform feedback outputs. */
+ readback = glMapBuffer(GL_TRANSFORM_FEEDBACK_BUFFER, GL_READ_ONLY);
+ for (i = 0; i < primitives_generated; i++) {
+ if (readback[i] != i) {
+ printf("Expected primitive %d to have gl_PrimitiveIDIn"
+ " = %d, got %d instead\n", i, i, readback[i]);
+ pass = false;
If gl_PrimitiveIDIn fails to increment at the first occurrence of the primitive
restart, then this errors gets emitted up to sum(i, {i, 2, 12}) = 77 times.
I'd prefer that this error message not spam the console and instead be emitted
only once. But, if you disagree, then I defer.
+ }
+ }
+ glUnmapBuffer(GL_TRANSFORM_FEEDBACK_BUFFER);
+
+ piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL);
+}
Other than the requests for clarification, and the clamping issue,
the test's behavior looks correct to me.
_______________________________________________
Piglit mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/piglit
