Refactor shader source classes, in order to work around a Firefox bug. #2214
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The Firefox bug prevents shaders from compiling with the CORDIC algorithm, so we detect Firefox and fall back to the original algorithm. Issue #2197. The overall goal of the refactoring is to allow `#defines` to affect built-in functions, which was not possible before. In order to do this, we need the `#defines` to appear at the top of the source, so I've merged the procedural shader generation previously done in `ShaderProgram` with the generation done in `createShaderSource`. * `createShaderSource` (private) is removed and replaced by `ShaderSource`. `ShaderSource` now manages an array of `#defines`, concating source shaders, and automatically including built-in functions, keeping everything in the correct order. * `ShaderCache` (and by extension `Context.createShaderProgram` and `Context.replaceShaderProgram`) now accept either `ShaderSource` objects, or a single string, for vertex and fragment shaders. * `ShaderProgram` constructor (private) now takes an options object. It also exposes the vertex and fragment shaders as `ShaderSource` objects instead of strings. External code can still clone the `ShaderSources` and then screw around with the implementation to make new shaders procedurally, as `OIT` does.
| * @param {Boolean} isFragmentShader True if this shader will be a fragment shader. | ||
| * @returns {String} The combined shader string. | ||
| */ | ||
| ShaderSource.prototype.getCombinedShader = function(isFragmentShader) { |
There was a problem hiding this comment.
This is a bit hacky, right? I had no idea what this parameter did when I looked at code calling this function. Why not two separate functions: one for vertex shaders and another for fragment shaders?
There was a problem hiding this comment.
I split into two functions.
|
Thanks @shunter. Just those comments. FF 33 on Windows looks good. There's a few test failures (below) but I would not worry given this version of ANGLE. |
OK |
|
I think I've addressed your comments. I merged in master as well. I believe the remaining Firefox test failures are all long-standing issues. |
|
Very nice, thanks. Can you update CHANGES.md for the FF fix in master and reply to the forum? https://groups.google.com/forum/#!msg/cesium-dev/-NRjGK1rcdU/ozcB3Ux5CJgJ |
pjcozzi
added a commit
that referenced
this pull request
Oct 23, 2014
…Crash Refactor shader source classes, in order to work around a Firefox bug.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Firefox bug prevents shaders from compiling with the CORDIC algorithm, so we detect Firefox and fall back to the original algorithm. Issue #2197.
The overall goal of the refactoring is to allow
#definesto affect built-in functions, which was not possible before. In order to do this, we need the#definesto appear at the top of the source, so I've merged the procedural shader generation previously done inShaderProgramwith the generation done increateShaderSource.createShaderSource(private) is removed and replaced byShaderSource.ShaderSourcenow manages an array of#defines, concating source shaders, and automatically including built-in functions, keeping everything in the correct order.ShaderCache(and by extensionContext.createShaderProgramandContext.replaceShaderProgram) now accept eitherShaderSourceobjects, or a single string, for vertex and fragment shaders.ShaderProgramconstructor (private) now takes an options object. It also exposes the vertex and fragment shaders asShaderSourceobjects instead of strings. External code can still clone theShaderSourcesand then screw around with the implementation to make new shaders procedurally, asOITdoes.