1170455 - WebGL 2 - getVertexAttrib(..., gl.CURRENT_VERTEX_ATTRIB) doesn't have integer types.

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[u480271]

getVertexAttrib needs to track integer types properly.

FAIL gl.getVertexAttrib(0, gl.CURRENT_VERTEX_ATTRIB) is not an instance of Int32Array
FAIL gl.getVertexAttrib(0, gl.CURRENT_VERTEX_ATTRIB)[0] should be -1. Was NaN.
PASS gl.getVertexAttrib(0, gl.CURRENT_VERTEX_ATTRIB)[1] is 0
FAIL gl.getVertexAttrib(0, gl.CURRENT_VERTEX_ATTRIB)[2] should be 1. Was 1.401298464324817e-45.
FAIL gl.getVertexAttrib(0, gl.CURRENT_VERTEX_ATTRIB)[3] should be 2. Was 2.802596928649634e-45.
FAIL gl.getVertexAttrib(0, gl.CURRENT_VERTEX_ATTRIB) is not an instance of Uint32Array
PASS gl.getVertexAttrib(0, gl.CURRENT_VERTEX_ATTRIB)[0] is 0
FAIL gl.getVertexAttrib(0, gl.CURRENT_VERTEX_ATTRIB)[1] should be 1. Was 1.401298464324817e-45.
FAIL gl.getVertexAttrib(0, gl.CURRENT_VERTEX_ATTRIB)[2] should be 2. Was 2.802596928649634e-45.
FAIL gl.getVertexAttrib(0, gl.CURRENT_VERTEX_ATTRIB)[3] should be 3. Was 4.203895392974451e-45.

Comment 1

[u480271]

Attachment: Part 1: Reformat GetVertexAttrib function.

Comment 2

[u480271]

Attachment: Part 2: Split vertex attribute functions into separate file.

Comment 3

[u480271]

Attachment: Part 3: Wrangle GetVertexAttribI symbols.

Comment 4

[u480271]

Attachment: Part 4: Track the type of the generic vertex attribute.

So the correct typed array can be returned from GetVertexAttrib.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8636330 [details] [diff] [review]
Part 1: Reformat GetVertexAttrib function.

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

::: dom/canvas/WebGLContextVertices.cpp
@@ +282,2 @@
>          {
> +            return JS::Int32Value(mBoundVertexArray->mAttribs[index].divisor);

One-line return should either omit `{` or they should go on the same line as the one-line conditional.

I see this was already here, but while we're cleaning up weird braces, let's clean this up too.

@@ +286,3 @@
>  
> +    case LOCAL_GL_CURRENT_VERTEX_ATTRIB: {
> +        GLfloat vec[4] = {0, 0, 0, 1};

This should be indented again. Also, I think the `{` should go on the next line, properly indented for normal case branch contents.

We can't do:
switch (foo) {
case BAR: {
  // ...
}
} // <- what?

So we have been treating case scopes like anonymous scopes:

switch (foo) {
case BAR:
  // base indent level
  {
    // ...
  }
} // <- normal

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8636331 [details] [diff] [review]
Part 2: Split vertex attribute functions into separate file.

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

I would call the new file WebGL2ContextVertexAttribs. It's not about vertices per se.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8636333 [details] [diff] [review]
Part 3: Wrangle GetVertexAttribI symbols.

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

::: gfx/gl/GLContext.h
@@ +2385,5 @@
> +    }
> +
> +    void fGetVertexAttribIuiv(GLuint index, GLenum pname, GLuint* params)
> +    {
> +        ASSERT_SYMBOL_PRESENT(fGetVertexAttribIuiv);

Gotta love the "I[u]iv" letter soup.

Comment 8

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> I would call the new file WebGL2ContextVertexAttribs. It's not about
> vertices per se.

The equivalent functions for WebGL1 are in WebGLContextVertices.cpp. I kept that pattern. Is that a good enough reason not to rename to WebGL2ContextVertexAttribs?

Comment 9

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> Gotta love the "I[u]iv" letter soup.

It's like Superbowl numbering!

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8636334 [details] [diff] [review]
Part 4: Track the type of the generic vertex attribute.

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

::: dom/canvas/WebGLContextVertices.cpp
@@ +24,5 @@
> +GLint PuntToInt(GLfloat f)
> +{
> +  fi_t tmp;
> +  tmp.f = f;
> +  return tmp.i;

Consider using MFBT's `ToT mozilla::BitwiseCast<ToT, FromT>(FromT)`.

Comment 11

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #10)
> Consider using MFBT's `ToT mozilla::BitwiseCast<ToT, FromT>(FromT)`.

Oh, cool. Didn't know about that.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce1426e69a5c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f03847cf62c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c815e17ac8a
https://hg.mozilla.org/integration/mozilla-inbound/rev/bef0545626b5

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ce1426e69a5c
https://hg.mozilla.org/mozilla-central/rev/f03847cf62c2
https://hg.mozilla.org/mozilla-central/rev/5c815e17ac8a
https://hg.mozilla.org/mozilla-central/rev/bef0545626b5