1048745 - WebGL2 - Implement Uniforms and Attributes

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[u480271]

https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.7

Comment 1

[u480271]

Attachment: WebGL2 - GL symbols for uniformui & non-square uniform matrix.

Comment 2

[u480271]

Attachment: WebGL2 - GL symbols for VertexAttribI.

Comment 3

[u480271]

Attachment: WebGL2 - GL symbols for gpu_shader_4.

Comment 4

[u480271]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7f0c0d7fc44
https://hg.mozilla.org/integration/mozilla-inbound/rev/15a03bd9042f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff9851d1520d

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e7f0c0d7fc44
https://hg.mozilla.org/mozilla-central/rev/15a03bd9042f
https://hg.mozilla.org/mozilla-central/rev/ff9851d1520d

Comment 6

[u480271]

Attachment: [WebGL2] Extend WebGLUniformInfo with GLES 3 types

Comment 7

[u480271]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52f3f4a553a5

Comment 8

[u480271]

Attachment: [WebGL2] Integer vertex attributes

Comment 9

[u480271]

Attachment: [WebGL2] Extend UniformInfo with WebGL2/GLES3 types

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/52f3f4a553a5

Comment 11

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8527391 [details] [diff] [review]
[WebGL2] Integer vertex attributes

Review of attachment 8527391 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLContext.h
@@ +141,5 @@
>  class WebGLIntOrFloat {
>      enum {
>          Int,
> +        Float,
> +        Unsigned

Uint

::: dom/canvas/WebGLContextValidate.cpp
@@ +1634,5 @@
> +    GLsizei requiredAlignment = 0;
> +    if (!ValidateAttribPointerType(integerMode, type, &requiredAlignment, info))
> +        return false;
> +
> +    // requiredAlignment should always be a power of two.

Assert it.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d29f55692d81

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e5c404f95a9d

Comment 14

[u480271]

Attachment: WebGL2 - Uniform with GLuint and non-square UniformMatrix. r=jgilbert

Comment 15

[u480271]

Comment on attachment 8579163 [details] [diff] [review]
WebGL2 - Uniform with GLuint and non-square UniformMatrix. r=jgilbert

I cleaned this up and made it like the WebGL1 functions. Sorry it turned out to be more changes than I expected.

Comment 16

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8579163 [details] [diff] [review]
WebGL2 - Uniform with GLuint and non-square UniformMatrix. r=jgilbert

Review of attachment 8579163 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGL1ContextUniforms.cpp
@@ +41,5 @@
> +        ErrorInvalidValue("%s: transpose must be FALSE as per the "
> +                          "OpenGL ES 2.0 spec", info);
> +    }
> +
> +    return !transpose;

It's more clear if you keep `return false` in the branch, and leave `return true` outside of it.

::: dom/canvas/WebGLUniformLocation.cpp
@@ +49,5 @@
>  
>  static bool
>  IsUniformSetterTypeValid(GLenum setterType, GLenum uniformType)
>  {
> +    // The order of the values in this switch match table 2.10 from

"the order [...] matches" :)

@@ +98,5 @@
> +    case LOCAL_GL_SAMPLER_2D_SHADOW:
> +    case LOCAL_GL_SAMPLER_2D_ARRAY:
> +    case LOCAL_GL_SAMPLER_2D_ARRAY_SHADOW:
> +    case LOCAL_GL_SAMPLER_CUBE_SHADOW:
> +        /**/

I don't think we need these comments as spacers. Just a blank line would be fine.

Comment 17

[u480271]

Comment on attachment 8579163 [details] [diff] [review]
WebGL2 - Uniform with GLuint and non-square UniformMatrix. r=jgilbert

Olli,

I added TypedArray variants to the .webidl to match the WebGL 2 spec.  These were missing from the spec when I first implemented webidl for WebGL 2.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

I'm not seeing void uniform1uiv(WebGLUniformLocation? location, Uint32Array value); etc in the spec.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8579163 [details] [diff] [review]
WebGL2 - Uniform with GLuint and non-square UniformMatrix. r=jgilbert

(I would of course prefer to see Mozilla coding style being used in C++ but perhaps all this code needs to be converted to the right style in a different bug).



But r- until it is explain why we add these methods if those aren't in the spec.
Or am I looking at some old version of the spec?
https://www.khronos.org/registry/webgl/specs/latest/2.0/

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8579163 [details] [diff] [review]
WebGL2 - Uniform with GLuint and non-square UniformMatrix. r=jgilbert

Or, perhaps conditional r+ is fine.
So, please explain why those methods are added and why they aren't in the spec yet. (I assume they will be in the spec)

Comment 21

[u480271]

(In reply to Olli Pettay [:smaug] from comment #20)
> Comment on attachment 8579163 [details] [diff] [review]
> WebGL2 - Uniform with GLuint and non-square UniformMatrix. r=jgilbert
> 
> Or, perhaps conditional r+ is fine.
> So, please explain why those methods are added and why they aren't in the
> spec yet. (I assume they will be in the spec)

I've asked for the spec to be updated to accept TypedArrays.  Until that makes it to the latest draft, I'll split this patch to land the changes that don't rely upon webidl changes.

Comment 22

[u480271]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfe2490b2b2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dae2d1f59ff

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dfe2490b2b2b
https://hg.mozilla.org/mozilla-central/rev/4dae2d1f59ff

Comment 24

[u480271]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddf60eb38236

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ddf60eb38236

Comment 26

[u480271]

Attachment: Query GL for glGetUniformuiv function.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2799372e82b

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b2799372e82b

Comment 29

[Jeff Muizelaar [:jrmuizel]]

Is there anything left that needs to be done here?

Comment 30

[u480271]

Just ran the conformance tests and I don't think so. I'll remove the leave-open and close.

Comment 31

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext#Uniforms_and_attributes