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 arrays of arrays so you could enable it to accept even more
> columns.
Well, but in this case we don't have a use for it, so I will let anyone
needing it proposing it in the future.
>
> Thanks,
> Tim
Thanks to you, for your patch.
>
>>> +
>>> + 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
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/piglit
_______________________________________________
Piglit mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/piglit
