Re: [Piglit] [PATCH] util: Add new headed matrix_index header to [vertex data]
On 03/04/16 03:45, Timothy Arceri wrote:
> On Fri, 2016-04-01 at 13:01 +0200, Alejandro Piñeiro wrote:
>> 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.
> Hi,
>
> Thanks for the review. Do you have a use for this patch? I think I may
> have made some small changes locally but I never pushed forward with
> this patch because my use case went away (was going to use it for
> testing component packing in ARB_enhanced_layouts but after filing a
> spec bug it was reveal matrices can't be packed).
We are working on ARB_vertex_attrib_64bit, and for example, right now
most of the things are working but matrices. In fact we are using your
parch locally in order to have tests with vertex attrib matrices.
> If you have a use for it and want to fix this up feel free and I'll
> gladly take a look but I don't think I will have any time to do so for
> a little bit.
Ok, I will do this next Monday. But fwiw, with the change I suggested
the patch is working fine.
> The other thing to consider is that the same functionality can be used
> for ar
Re: [Piglit] [PATCH] util: Add new headed matrix_index header to [vertex data]
On Fri, 2016-04-01 at 13:01 +0200, Alejandro Piñeiro wrote:
> 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.
Hi,
Thanks for the review. Do you have a use for this patch? I think I may
have made some small changes locally but I never pushed forward with
this patch because my use case went away (was going to use it for
testing component packing in ARB_enhanced_layouts but after filing a
spec bug it was reveal matrices can't be packed).
If you have a use for it and want to fix this up feel free and I'll
gladly take a look but I don't think I will have any time to do so for
a little bit.
The other thing to consider is that the same functionality can be used
for arrays of arrays so you could enable it to accept even more
columns.
Thanks,
Tim
>
> >
> > +
> > + 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",
> > +
Re: [Piglit] [PATCH] util: Add new headed matrix_index header to [vertex data]
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(s
[Piglit] [PATCH] util: Add new headed matrix_index header to [vertex data]
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;
+
+ /**
* 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;
+
+ 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(in
