[Piglit] [PATCH] util: Add new headed matrix_index header to [vertex data]
Timothy Arceri
t_arceri at yahoo.com.au
Sun Apr 3 01:45:38 UTC 2016
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",
> > + 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;
> > }
> >
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/piglit
More information about the Piglit
mailing list