New shader enhancements #1102
New shader enhancements #1102
Conversation
…es. ShaderProgram.js now only concatenates builtins that are referenced from the particular shader being compiled.
…es. ShaderProgram.js now only concatenates builtins that are referenced from the particular shader being compiled.
…structs/constants to include and the order to include them in
…ependency determination
|
@pjcozzi Looks like I had a rebase issue somewhere in here. One of your changes got in here somehow. Is this a problem? |
|
JSHint is failing. It is run automatically when we push to a branch. See https://travis-ci.org/AnalyticalGraphicsInc/cesium/builds/10815857 |
Generally we like to clean these up. In this case, it is trivial enough that I don't think it is a problem. However 4762e8c isn't even my commit IIRC. If @shunter has an easy solution, we'll do it. |
|
@gbeatty it would be fun to see if this affects compile/link times. We can fine-grained test the compile/link code, but we should also test the entire |
|
Update CHANGES.md. If the time differences are insignificant, we can just say something like "Improved runtime generation of GLSL shaders." |
| UniformDatatype) { | ||
|
|
||
| "use strict"; | ||
| /*global console*/ |
There was a problem hiding this comment.
We don't need this in this file.
|
Why did we rename all the GLSL files to have a |
| } | ||
|
|
||
| function generateDependencies(currentNode, dependencyNodes, shaderBuiltinDictionary) { | ||
|
|
There was a problem hiding this comment.
Sorry, I have a bunch of whitespace comments coming. We like to keep the Cesium code consistent.
Remove whitespace here.
|
Can you introduce new built-in |
| @@ -34,10 +36,36 @@ defineSuite([ | |||
|
|
|||
| beforeAll(function() { | |||
| context = createContext(); | |||
|
|
|||
| CzmBuiltins.czm_circularDependency1 = 'void czm_circularDependency1() { czm_circularDependency2(); }'; | |||
There was a problem hiding this comment.
I know it's indirectly tested elsewhere, but we should have a test here that includes comments.
|
Debug and release look pretty good. I think we are close. Make sure to update CHANGES.md with your next round. Let me know when it is ready. |
|
Ready for another review |
|
Cool. Will look at this shortly. Did you do any timings yet? |
| @@ -6,6 +6,7 @@ Beta Releases | |||
|
|
|||
| ### b21 - 2013-10-01 | |||
| * Added `CorridorOutlineGeometry`. | |||
| * Improved runtime generation of GLSL shaders. | |||
There was a problem hiding this comment.
We also added new czm_ functions, right?
| }; | ||
|
|
||
| // combine automatic uniforms and Cesium built-ins | ||
| var _czmBuiltinsAndUniforms; |
There was a problem hiding this comment.
This is declared here, then used above so it is using JavaScript's hoisting feature. We avoid this. Declare it above.
Do what you want, but I probably wouldn't do it this way at all; it makes the tests harder to follow. Instead, why not put _czmBuiltinsAndUniforms on ShaderProgram (again, it is private for testing) and just let the tests modify it as they did before? It feels cleaner and more direct.
|
Just those comments, then this is ready to merge. |
|
ready to go |
|
Tests are good in Chrome and Firefox. Code looks good (ShaderProgramSpec.js no longer needs Merging. Thanks @gbeatty |
|
CC #1031 |
Extracted glsl constants, structs, and functions into individual glsl files.
Build now builds constants.js structs.js and functions.js for use in ShaderProgram.js.
ShaderProgram.js builds a dependency graph for all functions used in the shader being compiled and determines the order the functions need to be defined in.
Only functions, structs, constants that are referenced are included in the final shader.