Closed Bug 1170455 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: djg, Assigned: djg)

References

()

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files)

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.
So the correct typed array can be returned from GetVertexAttrib.
Attachment #8636334 - Flags: review?(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
Attachment #8636330 - Flags: review?(jgilbert) → review+
Attachment #8636331 - Flags: review?(jgilbert) → review+
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 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.
Attachment #8636333 - Flags: review?(jgilbert) → review+
(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?
(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> Gotta love the "I[u]iv" letter soup.

It's like Superbowl numbering!
Assignee: nobody → dglastonbury
Status: NEW → ASSIGNED
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)`.
Attachment #8636334 - Flags: review?(jgilbert) → review+
(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.
You need to log in before you can comment on or make changes to this bug.