-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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.
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
Handle uniform precision mismatches #2984
Changes from 3 commits
3b06e3c
e26d737
7c08b66
c9e9c64
53d8e48
2af0301
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ define([ | |
'../Core/DeveloperError', | ||
'../Core/RuntimeError', | ||
'./AutomaticUniforms', | ||
'./ContextLimits', | ||
'./createUniform', | ||
'./createUniformArray' | ||
], function( | ||
|
@@ -17,6 +18,7 @@ define([ | |
DeveloperError, | ||
RuntimeError, | ||
AutomaticUniforms, | ||
ContextLimits, | ||
createUniform, | ||
createUniformArray) { | ||
"use strict"; | ||
|
@@ -27,7 +29,51 @@ define([ | |
/** | ||
* @private | ||
*/ | ||
|
||
function extractUniforms(shaderText) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't use variable hoisting; however, we tolerate it for functions so we don't have to have a bunch of utility functions at the top of the file where we really want the constructor function. So I suggest moving this below the constructor function, or even below the |
||
// TODO: regex doesn't work for inline structs | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We generally don't merge code with FYI, I don't think we use inline structs anywhere. For example, here are the built-in structs. |
||
var uniformNames = []; | ||
var uniformLines = shaderText.match(/uniform[\w\s]*(?=[=\[;])/g); | ||
if (uniformLines) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Used if (defined(uniformLines)) { |
||
for (var i = 0; i < uniformLines.length; i++) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if it matters anymore, but I would write this as: var length = uniformLines.length;
for (var i = 0; i < length; i++) { There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it still matters. The length property will be accessed each iteration and not all browsers optimize the case where it doesn't change. |
||
var line = uniformLines[i].trim(); | ||
var name = line.slice(line.lastIndexOf(' ') + 1); | ||
uniformNames.push(name); | ||
} | ||
} | ||
return uniformNames; | ||
} | ||
|
||
var ShaderProgram = function(options) { | ||
// If a uniform exists in both the vertex and fragment shader but with different precision qualifiers, | ||
// give the fragment shader uniform a different name. This fixes shader compilation errors on devices | ||
// that only support mediump in the fragment shader. | ||
var vertexShaderText = options.vertexShaderText; | ||
var fragmentShaderText = options.fragmentShaderText; | ||
var duplicateUniformNames = {}; | ||
|
||
if (!ContextLimits.highpSupported) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We generally want all the code in a function to be at the same level of abstraction. I would probably break this logic into its own function that returns an object with two properties: |
||
var vertexShaderUniforms = extractUniforms(vertexShaderText); | ||
var fragmentShaderUniforms = extractUniforms(fragmentShaderText); | ||
|
||
// Find uniforms used in both | ||
var i, j; | ||
var uniformName; | ||
var duplicateName; | ||
for (i = 0; i < vertexShaderUniforms.length; i++) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same |
||
for (j = 0; j < fragmentShaderUniforms.length; j++) { | ||
if (vertexShaderUniforms[i] === fragmentShaderUniforms[j]) { | ||
uniformName = vertexShaderUniforms[i]; | ||
duplicateName = uniformName + "_f"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This, of course, could create a shader that will not compile if a uniform with the same name already exists. We could test for this and handle it, but, instead, when renaming existing functions and uniforms, we prefix them with Also, just FYI, GLSL identifiers are limited to 255 characters in WebGL 1; this is not the case in OpenGL. |
||
// Update fragmentShaderText with renamed uniforms | ||
var re = new RegExp(uniformName + "\\b", "g"); | ||
fragmentShaderText = fragmentShaderText.replace(re, duplicateName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't matter here, but whenever we add/remove new lines in GLSL, we use |
||
duplicateUniformNames[duplicateName] = uniformName; | ||
} | ||
} | ||
} | ||
} | ||
|
||
this._gl = options.gl; | ||
this._logShaderCompilation = options.logShaderCompilation; | ||
this._debugShaders = options.debugShaders; | ||
|
@@ -40,6 +86,7 @@ define([ | |
this._uniforms = undefined; | ||
this._automaticUniforms = undefined; | ||
this._manualUniforms = undefined; | ||
this._duplicateUniformNames = duplicateUniformNames; | ||
this._cachedShader = undefined; // Used by ShaderCache | ||
|
||
/** | ||
|
@@ -48,9 +95,9 @@ define([ | |
this.maximumTextureUnitIndex = undefined; | ||
|
||
this._vertexShaderSource = options.vertexShaderSource; | ||
this._vertexShaderText = options.vertexShaderText; | ||
this._vertexShaderText = vertexShaderText; | ||
this._fragmentShaderSource = options.fragmentShaderSource; | ||
this._fragmentShaderText = options.fragmentShaderText; | ||
this._fragmentShaderText = fragmentShaderText; | ||
|
||
/** | ||
* @private | ||
|
@@ -346,20 +393,28 @@ define([ | |
}; | ||
} | ||
|
||
function partitionUniforms(uniforms) { | ||
function partitionUniforms(shader, uniforms) { | ||
var automaticUniforms = []; | ||
var manualUniforms = []; | ||
|
||
for ( var uniform in uniforms) { | ||
for (var uniform in uniforms) { | ||
if (uniforms.hasOwnProperty(uniform)) { | ||
var automaticUniform = AutomaticUniforms[uniform]; | ||
var uniformObject = uniforms[uniform]; | ||
var uniformName = uniform; | ||
// if it's a duplicate uniform, use its original name so it is updated correctly | ||
var duplicateUniform = shader._duplicateUniformNames[uniformName]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, I'm glad we can do the remapping here instead of in |
||
if (duplicateUniform) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
uniformObject.name = duplicateUniform; | ||
uniformName = duplicateUniform; | ||
} | ||
var automaticUniform = AutomaticUniforms[uniformName]; | ||
if (automaticUniform) { | ||
automaticUniforms.push({ | ||
uniform : uniforms[uniform], | ||
uniform : uniformObject, | ||
automaticUniform : automaticUniform | ||
}); | ||
} else { | ||
manualUniforms.push(uniforms[uniform]); | ||
manualUniforms.push(uniformObject); | ||
} | ||
} | ||
} | ||
|
@@ -393,7 +448,7 @@ define([ | |
var program = createAndLinkProgram(gl, shader, shader._debugShaders); | ||
var numberOfVertexAttributes = gl.getProgramParameter(program, gl.ACTIVE_ATTRIBUTES); | ||
var uniforms = findUniforms(gl, program); | ||
var partitionedUniforms = partitionUniforms(uniforms.uniformsByName); | ||
var partitionedUniforms = partitionUniforms(shader, uniforms.uniformsByName); | ||
|
||
shader._program = program; | ||
shader._numberOfVertexAttributes = numberOfVertexAttributes; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,9 @@ define([ | |
"use strict"; | ||
|
||
function removeComments(source) { | ||
// remove inline comments | ||
source = source.replace(/\/\/.*/g, ''); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! |
||
// remove multiline comment block | ||
return source.replace(/\/\*\*[\s\S]*?\*\//gm, function(match) { | ||
// preserve the number of lines in the comment block so the line numbers will be correct when debugging shaders | ||
var numberOfLines = match.match(/\n/gm).length; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ defineSuite([ | |
'Renderer/Buffer', | ||
'Renderer/BufferUsage', | ||
'Renderer/ClearCommand', | ||
'Renderer/ContextLimits', | ||
'Renderer/DrawCommand', | ||
'Renderer/ShaderSource', | ||
'Renderer/VertexArray', | ||
|
@@ -29,6 +30,7 @@ defineSuite([ | |
Buffer, | ||
BufferUsage, | ||
ClearCommand, | ||
ContextLimits, | ||
DrawCommand, | ||
ShaderSource, | ||
VertexArray, | ||
|
@@ -77,7 +79,7 @@ defineSuite([ | |
} | ||
}); | ||
|
||
function renderFragment(context, shaderProgram) { | ||
function renderFragment(context, shaderProgram, uniformMap) { | ||
va = new VertexArray({ | ||
context : context, | ||
attributes : [{ | ||
|
@@ -97,7 +99,8 @@ defineSuite([ | |
var command = new DrawCommand({ | ||
primitiveType : PrimitiveType.POINTS, | ||
shaderProgram : shaderProgram, | ||
vertexArray : va | ||
vertexArray : va, | ||
uniformMap : uniformMap | ||
}); | ||
command.execute(context); | ||
|
||
|
@@ -355,6 +358,27 @@ defineSuite([ | |
expect(renderFragment(context, sp)).toEqual([255, 255, 255, 255]); | ||
}); | ||
|
||
it('creates duplicate uniforms if precision of uniforms in vertex and fragment shader do not match', function() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's make a similar test using a built-in uniform like czm_sunDirectionWC. This is the uniform that caused the original issue (used in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the test I'm using czm_viewport. Is there a reason to use czm_sunDirectionWC besides being the first issue? Also the czm_sunDirectionWC bug won't actually happen anymore. It was added as a dependency in ShaderSource despite being in a comment. But now inline comments are removed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see now. No, one test that covers both manual and automatic uniforms is fine (especially now that I looked at your implementation). However, my comment about the fragile test still stands. |
||
var highpSupported = ContextLimits.highpSupported; | ||
ContextLimits._highpSupported = false; | ||
var vs = 'attribute vec4 position; uniform float u_value; varying float v_value; void main() { gl_PointSize = 1.0; v_value = u_value + czm_viewport.z*0.2; gl_Position = position; }'; | ||
var fs = 'uniform float u_value; varying float v_value; void main() { gl_FragColor = vec4(u_value + v_value + czm_viewport.z*0.2); }'; | ||
sp = ShaderProgram.fromCache({ | ||
context : context, | ||
vertexShaderSource : vs, | ||
fragmentShaderSource : fs | ||
}); | ||
var uniformMap = { | ||
u_value : function() { | ||
return 0.2; | ||
} | ||
}; | ||
expect(sp.allUniforms.u_value).toBeDefined(); | ||
expect(sp.allUniforms.u_value_f).toBeDefined(); | ||
expect(renderFragment(context, sp, uniformMap)).toEqual([204, 204, 204, 204]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is most likely a fragile test, where it will pass on some systems, but not others. Can we rework the shader to be more coarse-grained, e.g., black is failing, non-black is passing. |
||
ContextLimits._highpSupported = highpSupported; | ||
}); | ||
|
||
it('1 level function dependency', function() { | ||
var vs = 'attribute vec4 position; void main() { gl_PointSize = 1.0; gl_Position = position; }'; | ||
var fs = | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a closer look at this code example. We need to account for
HIGH_INT
, not justHIGH_FLOAT
, even though it isn't a problem yet. Let's add explicit getters for highp float and int, and then just check to see if either isn't support inShaderProgram
.