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.

Sign up for GitHub

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

Jump to bottom

Introduce GL state tracking #5739

Merged
merged 9 commits into from
Nov 30, 2017
Merged

Introduce GL state tracking #5739

merged 9 commits into from
Nov 30, 2017

Conversation

lbud
Copy link
Contributor

@lbud lbud commented Nov 22, 2017 •
edited
Loading

This PR introduces GL state tracking (similar to gl-native behavior) as a first step in #145.

Updated benchmark results: link
image

Remaining

  • unit tests
  • re-run benchmarks once everything is ready

jfirebaugh
jfirebaugh reviewed Nov 22, 2017
View reviewed changes
Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

👏 This looks great.

src/gl/index_buffer.js
@@ -29,30 +31,34 @@ class IndexBuffer {
// The bound index buffer is part of vertex array object state. We don't want to
// modify whatever VAO happens to be currently bound, so make sure the default
// vertex array provided by the context is bound instead.
if (this.gl.extVertexArrayObject === undefined) {
(this.gl: any).extVertexArrayObject = this.gl.getExtension("OES_vertex_array_object");
Copy link
Contributor

Choose a reason for hiding this comment

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

Glad to eliminate this any.

src/gl/index_buffer.js Outdated
const gl = this.context.gl;
gl.bindBuffer(gl.ELEMENT_ARRAY_BUFFER, this.buffer);
// this.context.bindElementBuffer.set(this.buffer);
// TODO not sure why this doesn't work, but using bindElementBuffer throws:
Copy link
Contributor

Choose a reason for hiding this comment

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

My hunch would be that a necessary call to gl.bindBuffer() is getting incorrectly deduped away somehow.

src/gl/state.js Outdated
}

set(t: T) {
if (this.current !== t) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have to be careful about using the !== operator here. It will always use object identity when T is an object or array type like BlendFuncType or StencilFuncType. That probably isn't what we want. We have a couple deep equal implementations that might work, or we could add an equal(a: T, b: T): boolean method to the Value interface.

src/gl/types.js Outdated

export type BlendFuncType = [BlendFuncConstant, BlendFuncConstant];

export type ColorType = [number, number, number, number];
Copy link
Contributor

Choose a reason for hiding this comment

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

Follow #5573 and use Color instead.

yarn.lock Outdated
@@ -188,11 +188,7 @@
version "3.0.0"
resolved "https://registry.yarnpkg.com/@mapbox/whoots-js/-/whoots-js-3.0.0.tgz#c1de4293081424da3ac30c23afa850af1019bb54"

"@types/node@*":
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there actual dependency updates in here or is this an accidental change (different version of yarn maybe?)

@lbud lbud force-pushed the state-only branch 2 times, most recently from 53bae79 to b979169 Compare November 29, 2017 23:50
@lbud
Copy link
Contributor Author

lbud commented Nov 29, 2017

I rolled back use of context.bindTexture after debugging a few failing render tests because the bound texture state is also dependent on the active texture unit — I'm going to address this in my next PR (focusing on framebuffer + renderbuffer + texture state).

@lbud lbud requested a review from jfirebaugh November 30, 2017 00:13
@lbud lbud changed the title [not ready] Introduce GL state tracking Introduce GL state tracking Nov 30, 2017
@lbud lbud merged commit 98b5213 into master Nov 30, 2017
@lbud lbud deleted the state-only branch November 30, 2017 19:41
@lbud lbud mentioned this pull request Nov 30, 2017
Merged
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfirebaugh jfirebaugh jfirebaugh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lbud @jfirebaugh