Skip to content

GL cleanup and optimizations. - #1972

Merged
kripken merged 11 commits into
emscripten-core:incomingfrom
juj:gl__opts
Jan 9, 2014
Merged

GL cleanup and optimizations.#1972
kripken merged 11 commits into
emscripten-core:incomingfrom
juj:gl__opts

Conversation

@juj

@juj juj commented Jan 3, 2014

Copy link
Copy Markdown
Collaborator

Optimizes GL emulation layer to avoid last detected duplicate redundant calls to GL. Cleans up GL library a bit by using 'GLImmediate' and 'GLctx' global names instead of routing though GL. and Module.

Still has a minor cleanup pending for used #ifdef macros, which needs #1969. A few places would like to use #if defined(FULL_ES2) || defined(LEGACY_GL_EMULATION) but the code had to duplicate that since the support for that isn't present.

@juj

juj commented Jan 3, 2014

Copy link
Copy Markdown
Collaborator Author

Running try builds now to check that no regressions are present..

@kripken

kripken commented Jan 4, 2014

Copy link
Copy Markdown
Member

Please comment here to ping me after tests passed.

@juj

juj commented Jan 4, 2014

Copy link
Copy Markdown
Collaborator Author

Overnight, OSX ran the default and other suites and passed, and Linux ran the browser suite and passed as well, so all looks good.

@kripken

kripken commented Jan 5, 2014

Copy link
Copy Markdown
Member

You removed quotes on Module.ctx['compressedTexImage2D'], however I think we need those because closure compiler mistakenly minified that name. Try to build a test with that call with --closure 1 to see if that changed. Ditto for Sub.

You can now remove GL.immediate = GLImmediate.

Why are matrix and matrixStack initialized to arrays of 16 zeros? Should be nulls since they will be references to objects (numbers tell the js engine to expect it to stay an array of numbers, which can confuse it), and why 16? Also, can't we initialize it to null in the JSON object definition, then set it to the right size in the initializer? Same with this.textureMatrixVersion = [ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 ];

I see you removed the unsafe check on // Potentially unsafe, since lastArrayBuffer might not reflect the true array buffer in code that mixes immediate/non-immediate - is that now guaranteed to be safe?

@kripken

kripken commented Jan 5, 2014

Copy link
Copy Markdown
Member

Please test this on BananaBread, with and without closure, to verify it does not regress anything.

@kripken

kripken commented Jan 5, 2014

Copy link
Copy Markdown
Member

One other thing missing here is var GLctx in the top level, unless I missed that. Without that, we pollute the global scope.

@juj

juj commented Jan 7, 2014

Copy link
Copy Markdown
Collaborator Author

Oh, I was puzzled about the Module.ctx['compressedTexImage2D'] and why only that function was closure-shielded, and thought that it was some leftover, so converted it to dot notation to be consistent.

I used [0,0,0, ..., 0] to avoid having to add dynamic initialization code (and defaulting on that number of texture coordinate attribute matrices). Agree on your points, I'll convert the initialization to use a dynamic init so that it's more consistent.

juj added 11 commits January 9, 2014 20:05
…as key to FFP matrix file. This allows using arrays instead of maps to store the file, and avoids unnecessary string operations.
…ase, currently bound buffers do not need to be tracked.
…tly using the global name GLImmediate over the GL.immediate form.
…ring global WebGL context in the variable GLctx in addition to Module.ctx.
…ith GLImmediate.matrix array that contains emulated modelview,projection and texture matrices. Restore compressedTexImage2D symbols to be closure-friendly.
…pace. Replace all 'this.xxx' with 'GLImmediate.xxx' when this==GLImmediate, to avoid throwing off Closure minification pass.
@juj

juj commented Jan 9, 2014

Copy link
Copy Markdown
Collaborator Author

Ok, updated the pull request. Works with Bananabread with closure on. How does it look now?

@kripken

kripken commented Jan 9, 2014

Copy link
Copy Markdown
Member

I don't have time to look at it right now, but let's merge it in, will take a look later. I'm sure it's fine though.

kripken added a commit that referenced this pull request Jan 9, 2014
GL cleanup and optimizations.
@kripken
kripken merged commit c230f7c into emscripten-core:incoming Jan 9, 2014
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.

2 participants