Full shader transpilation #1342
Conversation
There was a problem hiding this comment.
Looks good, but one question:
Before this change, we would force transpile shaderModule's code to actual shader version. Now with the change, we optionally force all the shader code to 100, but if this option , transpileShaders is not set and actual shader and shader module code are different version, its going fail compilation? If so make this option mandatory or make it backward compatible ?
| @@ -40,10 +39,10 @@ export default class ShaderModule { | |||
| let moduleSource; | |||
| switch (type) { | |||
| case VERTEX_SHADER: | |||
| moduleSource = transpileShader(this.vs || '', targetGLSLVersion, true); | |||
| moduleSource = this.vs || ''; | |||
There was a problem hiding this comment.
Remove targetGLSLVersion argument ?
That's handled by the full transpilation here: https://github.com/uber/luma.gl/pull/1342/files#diff-939a698d001db05bb4762bf253428143R144 If |
| * `vs` - (VertexShader|*string*) - A vertex shader object, or source as a string. | ||
| * `fs` - (FragmentShader|*string*) - A fragment shader object, or source as a string. | ||
| * `varyings` (WebGL 2) - An array of vertex shader output variables, that needs to be recorded (used in TransformFeedback flow). | ||
| * `bufferMode` (WebGL 2) - Mode to be used when recording vertex shader outputs (used in TransformFeedback flow). Default value is `gl.SEPARATE_ATTRIBS`. | ||
| * `modules` - shader modules to be applied. | ||
| * `program` - pre created program to use, when provided, vs, ps and modules are not used. | ||
| * `shaderCache` - (ShaderCache) - Compiled shader (Vertex and Fragment) are cached in this object very first time they got compiled and then retrieved when same shader is used. When using multiple Model objects with duplicate shaders, use the same shaderCache object for better performance. | ||
| * `programManager` - `ProgramManager` to use for program creation and caching. | ||
| * `transpileShaders` - Transpile vertex and fragment shaders to GLSL 1.0. |
There was a problem hiding this comment.
I'd call it transpileToGLSL1: true or targetVersion: 100 to remove any ambiguity.
| const outputName = outputMatch[1]; | ||
| source = source | ||
| .replace(FS_OUTPUT_REGEX, '') | ||
| .replace(new RegExp(`\\b${outputName}\\b`, 'g'), 'gl_FragColor'); |
There was a problem hiding this comment.
Any reason to not use the inline regex syntax?
There was a problem hiding this comment.
The interpolation of outputName. Is there a way to do that with a literal?
| @@ -28,6 +29,117 @@ void main(void) { | |||
| } | |||
| `; | |||
|
|
|||
| const VS_GLSL_300_2 = `\ | |||
There was a problem hiding this comment.
Add a test case for in/out qualifier in function params? e.g. https://github.com/uber/deck.gl/blob/master/modules/core/src/shaderlib/project32/project32.js#L25
| .replace(/texture\(/g, 'texture2D('); | ||
| .replace(/^#version\s+300\s+es/, '#version 100') | ||
| .replace(/^[ \t]*in[ \t]+/gm, 'attribute ') | ||
| .replace(/^[ \t]*out[ \t]+/gm, 'varying ') |
There was a problem hiding this comment.
No handling for layout yet?
There was a problem hiding this comment.
Saving it for a later iteration.
assembleShaders.Model,ProgramManagerandassembleShadersto force transpilation to GLSL 1.0.Tested with a simplified version of the deck.gl GLSL 3.0 mesh layer shaders.