Bug 99854 – [WebGL] getUniformLocation fails for uniform array name without array brackets

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Max Vujovic]

For example:

Given the GLSL:
uniform float array[10];

In JS, the following fails:
gl.getUniformLocation(program, "array");

The following succeeds:
gl.getUniformLocation(program, "array[0]");
gl.getUniformLocation(program, "array[1]");
...
gl.getUniformLocation(program, "array[9]");

According to the glGetUniformLocation man page [1], the former should succeed:
"""
Except if the last part of name indicates a uniform variable array, the location of the first element of an array can be retrieved by using the name of the array, or by using the name appended by "[0]".
"""

[1]: http://www.khronos.org/opengles/sdk/docs/man/xhtml/glGetUniformLocation.xml

[Max Vujovic]

Created attachment 169650 [details]
Patch

For this patch's test, I've made to following change to Khronos WebGL conformance test suite: https://github.com/KhronosGroup/WebGL/commit/6451bfd138b2f603b667cbf7839885f665607c7a

Do we need a WebKit test for this or is the WebGL conformance test sufficient?

[Roger Fong]

Hmm based on this patch it looks like just using the array name with no subscripts was probably broken before I landed r131105. Sorry I missed that :/ but at least I didn't break it! woo?

Also I think that the single size array does work properly, if it's size 1 then it'll just hit the 'else' path which behaves properly (although that probably wasn't the clearest way of doing things...). 

Hmm, o wait, I guess that in order to fix the first problem, the distinction between 'array' and 'not array' has to be made regardless.

Anyways, looks good to me, although I don't have review so we'll have to wait for dino.

Also hi!

[Roger Fong]

(In reply to comment #1)
> Do we need a WebKit test for this or is the WebGL conformance test sufficient?

I wanna say just the conformance test, but Dean will know better.

[Max Vujovic]

(In reply to comment #2)
> Hmm based on this patch it looks like just using the array name with no subscripts was probably broken before I landed r131105. Sorry I missed that :/ but at least I didn't break it! woo?

No worries- this was broken before you touched it :)

> 
> Also I think that the single size array does work properly, if it's size 1 then it'll just hit the 'else' path which behaves properly (although that probably wasn't the clearest way of doing things...). 

Yup, that worked properly before. I think it's just more explicit now.

> 
> Hmm, o wait, I guess that in order to fix the first problem, the distinction between 'array' and 'not array' has to be made regardless.
> 
> Anyways, looks good to me, although I don't have review so we'll have to wait for dino.

Thanks for looking over it. I'll ask Dino for a review.

> 
> Also hi!

Hi! Nice to see you working on WebKit :)

(In reply to comment #3)
> (In reply to comment #1)
> > Do we need a WebKit test for this or is the WebGL conformance test sufficient?
> 
> I wanna say just the conformance test, but Dean will know better.

Ok, will do.

[Dean Jackson]

(In reply to comment #1)
> Created an attachment (id=169650) [details]
> Patch
> 
> For this patch's test, I've made to following change to Khronos WebGL conformance test suite: https://github.com/KhronosGroup/WebGL/commit/6451bfd138b2f603b667cbf7839885f665607c7a
> 
> Do we need a WebKit test for this or is the WebGL conformance test sufficient?

I have been submitting patches to WebKit that reference the Khronos suite. There is an open bug to merge that into WebKit. Once that's done we'll be in a great position for testing regressions.

[Max Vujovic]

Thanks for the review!

(In reply to comment #5)
> (In reply to comment #1)
> > Created an attachment (id=169650) [details] [details]
> > Patch
> > 
> > For this patch's test, I've made to following change to Khronos WebGL conformance test suite: https://github.com/KhronosGroup/WebGL/commit/6451bfd138b2f603b667cbf7839885f665607c7a
> > 
> > Do we need a WebKit test for this or is the WebGL conformance test sufficient?
> 
> I have been submitting patches to WebKit that reference the Khronos suite. There is an open bug to merge that into WebKit. Once that's done we'll be in a great position for testing regressions.

Ok- I'm looking forward to it!

[WebKit Review Bot]

Comment on attachment 169650 [details]
Patch

Clearing flags on attachment: 169650

Committed r131952: <http://trac.webkit.org/changeset/131952>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.