[Mesa-dev] [PATCH] ARB_Uniform_Buffer_Object: layout(std140) test

[Eric Anholt]

On Mon,  5 Dec 2011 17:06:00 +0100, Vincent Lejeune <vljn at ovi.com> wrote:

Nice start!  I love that this just takes a chunk of the spec and tests
it.  Just a few little comments.

One is that the all.tests line to get your new testcase executed is
missing.  Since this test doesn't do any drawing, it should be a
concurrent test.  Unfortunately our helper functions are pretty dumb so
far, so you'll want something like:

arb_uniform_buffer_object['standard_layout'] = concurrent_test('arb_uniform_buffer_object_standard_layout')

because if you just use add_concurrent_test(), it will end up in the
summary file as:

ARB_uniform_buffer_object/arb_uniform_buffer_object_standard_layout

which is quite ugly, though we have many examples of that sort of ugly
around.  The way we write entries in all.tests today is awful, and I
want something better.  I just haven't figured out what that is exactly.

> +#define Elements(x) (sizeof(x)/sizeof(*(x)))

There's the usual linux kernel-named ARRAY_SIZE macro already
available.

> +static const struct result {
> +   const char* name;
> +   GLint offset;
> +   GLint size;

tab indenting.

> +} expected_result[] =
> +	{
> +		{ "a", 0, 1 },
> +		{ "b", 8, 1 },
> +		{ "c", 16, 1 },
> +		{ "f.d", 32, 1},
> +		{ "f.e", 40, 1},
> +		{ "g", 48, 1},
> +		{ "h", 64, 2},
> +		{ "i", 96, 1},
> +		{ "o[0].j", 128,1},
> +		{ "o[0].k", 144, 1},
> +		{ "o[0].l", 160, 2},
> +		{ "o[0].m", 192, 1},
> +		{ "o[0].n", 208, 2},
> +		{ "o[1].j", 304,1},

whitespace

> +		{ "o[1].k", 320, 1},
> +		{ "o[1].l", 336, 2},
> +		{ "o[1].m", 368, 1},
> +		{ "o[1].n", 384, 2},
> +	};

> +static const char frag_shader_text[] =
> +	"#version 130\n"
> +	"#extension GL_ARB_uniform_buffer_object : enable \n"
> +	"layout(std140) uniform test_ubo { \n"
> +	"float a;\n"
> +	"vec2 b;\n"
> +	"vec3 c;\n"
> +	"struct {\n"
> +	"int d;\n"
> +	"bvec2 e;\n"
> +	"} f;\n"
> +	"float g;\n"
> +	"float h[2];\n"
> +	"mat2x3 i;\n"
> +	"struct {\n"
> +	"uvec3 j;\n"
> +	"vec2 k;\n"
> +	"float l[2];\n"
> +	"vec2 m;\n"
> +	"mat3 n[2];\n"
> +	"} o[2];"
> +	"};\n"
> +	"void main() {}";

Could we get some indenting in the shader code?

> +static void
> +init(void)
> +{
> +	GLuint fs;
> +
> +	piglit_require_GLSL_version(120);

I think you mean 130.

> +static bool
> +validate_offset_and_size()
> +{
> +	unsigned i,index;
> +	GLint offset,size;

s/,/, /

> +	for(i = 0; i < Elements(expected_result); i++) {
> +		const char* temp_name = expected_result[i].name;

s/char* /char */

> +		glGetUniformIndices(prog, 1, &temp_name, &index);
> +		if (index == GL_INVALID_INDEX) {
> +			printf("%s not found\n",temp_name);
> +			return 0;
> +		}
> +		glGetActiveUniformsiv(prog, 1, &index, GL_UNIFORM_OFFSET, &offset);
> +		if (offset != expected_result[i].offset) {
> +			printf("For %s : offset is %d, should be %d\n",temp_name,
> +				offset,expected_result[i].offset);
> +			return 0;
> +		}
> +		glGetActiveUniformsiv(prog, 1, &index, GL_UNIFORM_SIZE, &size);
> +		if (size != expected_result[i].size) {
> +			printf("For %s : size is %d, should be %d\n",temp_name,
> +				size,expected_result[i].size);
> +			return 0;
> +		}
> +	}
> +	return 1;
> +
> +}

Let's use true and false for bool return values :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20111205/66938fd1/attachment.pgp>