Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for the OES_vertex_array_object extension #206

Merged
merged 4 commits into from
Nov 25, 2020

Conversation

erikmchut
Copy link

@erikmchut erikmchut commented Nov 13, 2020

Closes #204.

Overview

This PR adds support for the OES_vertex_array_object extension. Vertex Array Objects encapsulate state from vertex buffers and vertex attributes.

Extension information

https://www.khronos.org/registry/webgl/extensions/OES_vertex_array_object/
https://www.khronos.org/opengl/wiki/Vertex_Specification#Vertex_Array_Object

Rationale

This is incredibly helpful for developers that target WebGL, WebGL2, and OpenGL3+ development in the same codebase, since the latter two require VertexArrayObjects and using them requires different buffer management.

Implementation

To implement this, I had to split the state for WebGLVertexAttributes into 'Object' data and 'Global' data. Object is the vertex data that is stored in VAOs, whereas Global data is the data that is not stored in VAOs. With this split, I implemented the extension to according to the spec, changing the WebGLRenderingContext to use the different state objects and allowing the extension to swap the state when a VAO buffer is bound.

Testing

All WebGL compliance tests continue pass with this change, including extensions_oes-vertex-array-object and extensions_oes-vertex-array-object-bufferData which comprehensively test the feature.

Copy link
Member

@dhritzkiv dhritzkiv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR! Looks like a great contribution. I'm going to have to do some more reading of the code + consider the changes surrounding WebGLVertexAttribute.

For reference, the inclusion of this functionality has increased the number of tests run + passed from 16137 to 16210.

src/native/procs.h Outdated Show resolved Hide resolved
@erikmchut
Copy link
Author

Also, here is a table showing the types of information in global scope vs. vertex array object scope:
https://www.khronos.org/registry/OpenGL/specs/gl/glspec46.core.pdf#page=607&zoom=100,0,173

(newer opengl spec, but a good way to visualize).

TL;DR:

Global data:

  • All the glVertexAttrib*v style methods set global data
  • The bind state for an ARRAY_BUFFER buffer

Object data (in a vertex array object):

  • The bind state for an ELEMENT_ARRAY_BUFFER
  • The value of the bound ARRAY_BUFFER at the time of a call to vertexAttribPointer
  • All of the data set in vertexAttribPointer
  • The glEnableVertexAttribArray setting

@dhritzkiv
Copy link
Member

Thanks for continuing to add more commits! I likely won't get to this until the weekend, so feel free to make more improvements where necessary

@erikmchut
Copy link
Author

No worries – I'm done making changes. Let me know if you have any questions about the structure once you get the chance to review.

@dhritzkiv
Copy link
Member

@robertleeplummerjr hey, could I get another pair of eyes on this? I figured your GPU projects might benefit from / be affected by these changes, and would love your thumbs up.

I've yet to sit down to look at this fully myself (probably later this weekend)

@erikmchut
Copy link
Author

Hi @dhritzkiv, friendly ping regarding the above. Let me know if I can give any additional information to help with the review.

@dhritzkiv dhritzkiv merged commit ab824e9 into stackgl:master Nov 25, 2020
@dhritzkiv
Copy link
Member

Released as v4.9.0!

Sorry for the delay - I had to review it a few times as there were some bigger changes made than previous extension additions (but they were necessary). I've also never use this extension before, so it was new to me.

Very nice code. Thank you for the contribution to the library.

@erikmchut
Copy link
Author

Hi Daniel, that's excellent – thank you! I'll be using the new version right away. Happy to make more contributions in the future if I (or the team at 8th Wall) runs into any issues or missing features.

@dhritzkiv
Copy link
Member

Please do!

@dhritzkiv
Copy link
Member

Btw, I forgot to publish to npm last night. It is up now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing support for WebGL OES_vertex_array_object
2 participants