Closed Bug 1048745 Opened 10 years ago Closed 8 years ago

WebGL2 - Implement Uniforms and Attributes

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: u480271, Assigned: u480271)

References

Details

(Keywords: dev-doc-complete)

Attachments

(8 files)

Blocks: webgl2
Depends on: 899206
Attachment #8493661 - Flags: review?(bjacob)
Attachment #8493662 - Flags: review?(bjacob)
Keywords: leave-open
Attachment #8493660 - Flags: review?(bjacob) → review+
Attachment #8493661 - Flags: review?(bjacob) → review+
Attachment #8493662 - Flags: review?(bjacob) → review+
Attachment #8524365 - Flags: review?(jgilbert)
Attachment #8524365 - Flags: review?(jgilbert) → review+
Attachment #8527391 - Flags: review?(jgilbert)
Attachment #8527429 - Flags: review?(jgilbert)
Attachment #8527429 - Flags: review?(jgilbert) → review+
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.
Attachment #8527391 - Flags: review?(jgilbert) → review+
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.
Attachment #8579163 - Flags: review?(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.
Attachment #8579163 - Flags: review?(jgilbert) → review+
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.
Attachment #8579163 - Flags: review?(bugs)
I'm not seeing void uniform1uiv(WebGLUniformLocation? location, Uint32Array value); etc in the spec.
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/
Attachment #8579163 - Flags: review?(bugs) → review-
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)
Attachment #8579163 - Flags: review- → review+
(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.
Attachment #8593720 - Flags: review?(jgilbert)
Attachment #8593720 - Flags: review?(jgilbert) → review+
Is there anything left that needs to be done here?
Flags: needinfo?(dglastonbury)
Just ran the conformance tests and I don't think so. I'll remove the leave-open and close.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: needinfo?(dglastonbury)
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.