[Piglit] [PATCH] util: Add new headed matrix_index header to [vertex data]

[Alejandro Piñeiro]

While searching how to specify matrices on shader runner I found this
unreviewed patch. I guess that a late review is better than nothing.

On 10/12/15 06:25, Timothy Arceri wrote:
> This allows data to be set for matrix attributes in shader runner.
>
> For example to set colunm 2 of mat2x3:
> attname/float/3/1
> ---
>  tests/util/piglit-vbo.cpp | 43 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/tests/util/piglit-vbo.cpp b/tests/util/piglit-vbo.cpp
> index 6453d40..07a5ee6 100644
> --- a/tests/util/piglit-vbo.cpp
> +++ b/tests/util/piglit-vbo.cpp
> @@ -151,6 +151,11 @@ public:
>  	size_t count;
>  
>  	/**
> +	 * Index of the matrix column for this attribute.
> +	 */
> +	size_t matrix_index;

Isn't size_t for the index somewhat an overkill. I guess that GLint is
enough. But this is just a nitpick, you can forget it if you prefer.

> +
> +	/**
>  	 * Index of this vertex attribute in the linked program.
>  	 */
>  	GLuint index;
> @@ -171,7 +176,8 @@ vertex_attrib_description::vertex_attrib_description(GLuint prog,
>  	/* Split the column header into name/type/count fields */
>  	const char *first_slash = strchr(text, '/');
>  	if (first_slash == NULL) {
> -		printf("Column headers must be in the form name/type/count.  "
> +		printf("Column headers must be in the form name/type/count/matrix_column.\n"
> +		       "Note: matrix_column is optional.\n"
>  		       "Got: %s\n",
>  		       text);
>  		piglit_report_result(PIGLIT_FAIL);
> @@ -179,7 +185,7 @@ vertex_attrib_description::vertex_attrib_description(GLuint prog,
>  	std::string name(text, first_slash);
>  	const char *second_slash = strchr(first_slash + 1, '/');
>  	if (second_slash == NULL) {
> -		printf("Column headers must be in the form name/type/count.  "
> +		printf("Column headers must be in the form name/type/count/matrix_column.\n"
>  		       "Got: %s\n",
>  		       text);
>  		piglit_report_result(PIGLIT_FAIL);
> @@ -188,11 +194,28 @@ vertex_attrib_description::vertex_attrib_description(GLuint prog,
>  	this->data_type = decode_type(type_str.c_str());
>  	char *endptr;
>  	this->count = strtoul(second_slash + 1, &endptr, 10);
> +
>  	if (*endptr != '\0') {
> -		printf("Column headers must be in the form name/type/count.  "
> -		       "Got: %s\n",
> -		       text);
> -		piglit_report_result(PIGLIT_FAIL);
> +		const char *third_slash = strchr(second_slash + 1, '/');
> +		this->matrix_index = strtoul(third_slash + 1, &endptr, 10);
> +
> +		if (this->matrix_index < 0 || this->matrix_index > 3) {
> +			printf("Matrix column index must be between 0 and 3.  Got: %lu\n",
> +				(unsigned long) this->matrix_index);
> +			piglit_report_result(PIGLIT_FAIL);
> +		}
> +		/* Turn the column into an offset */
> +		this->matrix_index -= 1;

This leads to need to specify the column number starting from 1. The
commit message seems to suggest that it would start from 0.

Or in other words, with the patch as it is, specifying a matrix  like this:
position/float/3  matrix/float/3/0 matrix/float/3/1 matrix/float/3/2
-1.0 -1.0 0.0   0 0 0           1 1 1           2 2 2

Leads to an error, because matrix_index -1 for the first column is used.

In my opinion that line is not needed, unless you wanted to specify
columns starting to count from 1.

Additionally, piglit-vbo.cpp has a header explaining how to specify
vertex data. Probably it would be good to add some info about how to use
matrices.

> +
> +		if (*endptr != '\0') {
> +			printf("Column headers must be in the form name/type/matrix_column.\n"
> +			       "Note: matrix_column is optional.\n"
> +			       "Got: %s\n",
> +			       text);
> +			piglit_report_result(PIGLIT_FAIL);
> +		}
> +	} else {
> +		this->matrix_index = 0;
>  	}
>  
>  	GLint attrib_location = glGetAttribLocation(prog, name.c_str());
> @@ -293,7 +316,7 @@ vertex_attrib_description::setup(size_t *offset, size_t stride) const
>  	int attribute_size = ATTRIBUTE_SIZE;
>  	switch (this->data_type) {
>  	case GL_FLOAT:
> -		glVertexAttribPointer(this->index, this->count,
> +		glVertexAttribPointer(this->index + this->matrix_index, this->count,
>  				      this->data_type, GL_FALSE, stride,
>  				      (void *) *offset);
>  		break;
> @@ -302,7 +325,7 @@ vertex_attrib_description::setup(size_t *offset, size_t stride) const
>  			fprintf(stderr,"vertex_attrib_description fail. no 64-bit float support\n");
>  			return;
>  		}
> -		glVertexAttribLPointer(this->index, this->count,
> +		glVertexAttribLPointer(this->index + this->matrix_index, this->count,
>  				      this->data_type, stride,
>  				      (void *) *offset);
>  		attribute_size *= 2;
> @@ -312,12 +335,12 @@ vertex_attrib_description::setup(size_t *offset, size_t stride) const
>  			fprintf(stderr,"vertex_attrib_description fail. no int support\n");
>  			return;
>  		}
> -		glVertexAttribIPointer(this->index, this->count,
> +		glVertexAttribIPointer(this->index + this->matrix_index, this->count,
>  				       this->data_type, stride,
>  				       (void *) *offset);
>  		break;
>  	}
> -	glEnableVertexAttribArray(index);
> +	glEnableVertexAttribArray(index + this->matrix_index);
>  	*offset += attribute_size * this->count;
>  }
>