Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Handle uniform precision mismatches #2984

Merged
merged 6 commits into from
Sep 1, 2015
Merged

Handle uniform precision mismatches #2984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3b06e3c
Handle uniform precision mismatches
lilleyse Aug 28, 2015
e26d737
Tweak doc
pjcozzi Aug 31, 2015
7c08b66
Tweak comment
pjcozzi Aug 31, 2015
c9e9c64
Improvements to uniform mediump checking
lilleyse Aug 31, 2015
53d8e48
Tweak local naming and whitespace
pjcozzi Aug 31, 2015
2af0301
Fix rename
pjcozzi Aug 31, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Source/Renderer/Context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,11 @@ define([
ContextLimits._maximumViewportWidth = maximumViewportDimensions[0];
ContextLimits._maximumViewportHeight = maximumViewportDimensions[1];

var highpFloat = gl.getShaderPrecisionFormat(gl.FRAGMENT_SHADER, gl.HIGH_FLOAT);
ContextLimits._highpFloatSupported = highpFloat.precision !== 0;
var highpInt = gl.getShaderPrecisionFormat(gl.FRAGMENT_SHADER, gl.HIGH_INT);
ContextLimits._highpIntSupported = highpInt.rangeMax !== 0;

this._antialias = gl.getContextAttributes().antialias;

// Query and initialize extensions
Expand Down
27 changes: 26 additions & 1 deletion Source/Renderer/ContextLimits.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ define([
_maximumViewportHeight : 0,
_maximumTextureFilterAnisotropy : 0,
_maximumDrawBuffers : 0,
_maximumColorAttachments : 0
_maximumColorAttachments : 0,
_highpFloatSupported: false,
_highpIntSupported: false
};

defineProperties(ContextLimits, {
Expand Down Expand Up @@ -264,7 +266,30 @@ define([
get: function () {
return ContextLimits._maximumColorAttachments;
}
},

/**
* High precision float supported (<code>highp</code>) in fragment shaders.
* @memberof ContextLimits
* @type {Boolean}
*/
highpFloatSupported : {
get: function () {
return ContextLimits._highpFloatSupported;
}
},

/**
* High precision int supported (<code>highp</code>) in fragment shaders.
* @memberof ContextLimits
* @type {Boolean}
*/
highpIntSupported : {
get: function () {
return ContextLimits._highpIntSupported;
}
}

});

return ContextLimits;
Expand Down
80 changes: 72 additions & 8 deletions Source/Renderer/ShaderProgram.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,25 @@
define([
'../Core/defaultValue',
'../Core/defined',
'../Core/definedNotNull',
'../Core/defineProperties',
'../Core/destroyObject',
'../Core/DeveloperError',
'../Core/RuntimeError',
'./AutomaticUniforms',
'./ContextLimits',
'./createUniform',
'./createUniformArray'
], function(
defaultValue,
defined,
definedNotNull,
defineProperties,
destroyObject,
DeveloperError,
RuntimeError,
AutomaticUniforms,
ContextLimits,
createUniform,
createUniformArray) {
"use strict";
Expand All @@ -28,6 +32,8 @@ define([
* @private
*/
var ShaderProgram = function(options) {
var modifiedFS = handleUniformPrecisionMismatches(options.vertexShaderText, options.fragmentShaderText);

this._gl = options.gl;
this._logShaderCompilation = options.logShaderCompilation;
this._debugShaders = options.debugShaders;
Expand All @@ -40,6 +46,7 @@ define([
this._uniforms = undefined;
this._automaticUniforms = undefined;
this._manualUniforms = undefined;
this._duplicateUniformNames = modifiedFS.duplicateUniformNames;
this._cachedShader = undefined; // Used by ShaderCache

/**
Expand All @@ -50,7 +57,7 @@ define([
this._vertexShaderSource = options.vertexShaderSource;
this._vertexShaderText = options.vertexShaderText;
this._fragmentShaderSource = options.fragmentShaderSource;
this._fragmentShaderText = options.fragmentShaderText;
this._fragmentShaderText = modifiedFS.fragmentShaderText;

/**
* @private
Expand Down Expand Up @@ -127,6 +134,55 @@ define([
}
});

function extractUniforms(shaderText) {
var uniformNames = [];
var uniformLines = shaderText.match(/uniform.*?(?![^{]*})(?=[=\[;])/g);
if (definedNotNull(uniformLines)) {
var len = uniformLines.length;
for (var i = 0; i < len; i++) {
var line = uniformLines[i].trim();
var name = line.slice(line.lastIndexOf(' ') + 1);
uniformNames.push(name);
}
}
return uniformNames;
}

function handleUniformPrecisionMismatches(vertexShaderText, fragmentShaderText) {
// 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 duplicateUniformNames = {};

if (!ContextLimits.highpFloatSupported || !ContextLimits.highpIntSupported) {
var i, j;
var uniformName;
var duplicateName;
var vertexShaderUniforms = extractUniforms(vertexShaderText);
var fragmentShaderUniforms = extractUniforms(fragmentShaderText);
var vertexUniformsCount = vertexShaderUniforms.length;
var fragmentUniformsCount = fragmentShaderUniforms.length;

for (i = 0; i < vertexUniformsCount; i++) {
for (j = 0; j < fragmentUniformsCount; j++) {
if (vertexShaderUniforms[i] === fragmentShaderUniforms[j]) {
uniformName = vertexShaderUniforms[i];
duplicateName = "czm_mediump_" + uniformName;
// Update fragmentShaderText with renamed uniforms
var re = new RegExp(uniformName + "\\b", "g");
fragmentShaderText = fragmentShaderText.replace(re, duplicateName);
duplicateUniformNames[duplicateName] = uniformName;
}
}
}
}

return {
fragmentShaderText : fragmentShaderText,
duplicateUniformNames : duplicateUniformNames
};
}

var consolePrefix = '[Cesium WebGL] ';

function createAndLinkProgram(gl, shader) {
Expand Down Expand Up @@ -346,20 +402,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];
if (automaticUniform) {
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];
Copy link
Contributor

Choose a reason for hiding this comment

The 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 _setUniforms.

if (defined(duplicateUniform)) {
uniformObject.name = duplicateUniform;
uniformName = duplicateUniform;
}
var automaticUniform = AutomaticUniforms[uniformName];
if (defined(automaticUniform)) {
automaticUniforms.push({
uniform : uniforms[uniform],
uniform : uniformObject,
automaticUniform : automaticUniform
});
} else {
manualUniforms.push(uniforms[uniform]);
manualUniforms.push(uniformObject);
}
}
}
Expand Down Expand Up @@ -393,7 +457,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;
Expand Down
3 changes: 3 additions & 0 deletions Source/Renderer/ShaderSource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ define([
"use strict";

function removeComments(source) {
// remove inline comments
source = source.replace(/\/\/.*/g, '');
Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Expand Down
28 changes: 26 additions & 2 deletions Specs/Renderer/ShaderProgramSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ defineSuite([
'Renderer/Buffer',
'Renderer/BufferUsage',
'Renderer/ClearCommand',
'Renderer/ContextLimits',
'Renderer/DrawCommand',
'Renderer/ShaderSource',
'Renderer/VertexArray',
Expand All @@ -29,6 +30,7 @@ defineSuite([
Buffer,
BufferUsage,
ClearCommand,
ContextLimits,
DrawCommand,
ShaderSource,
VertexArray,
Expand Down Expand Up @@ -77,7 +79,7 @@ defineSuite([
}
});

function renderFragment(context, shaderProgram) {
function renderFragment(context, shaderProgram, uniformMap) {
va = new VertexArray({
context : context,
attributes : [{
Expand All @@ -97,7 +99,8 @@ defineSuite([
var command = new DrawCommand({
primitiveType : PrimitiveType.POINTS,
shaderProgram : shaderProgram,
vertexArray : va
vertexArray : va,
uniformMap : uniformMap
});
command.execute(context);

Expand Down Expand Up @@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 SkyAtmosphere vertex and fragment shaders).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 highpFloatSupported = ContextLimits.highpFloatSupported;
ContextLimits._highpFloatSupported = 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; 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); }';
sp = ShaderProgram.fromCache({
context : context,
vertexShaderSource : vs,
fragmentShaderSource : fs
});
var uniformMap = {
u_value : function() {
return 1.0;
}
};
expect(sp.allUniforms.u_value).toBeDefined();
expect(sp.allUniforms.czm_mediump_u_value).toBeDefined();
expect(renderFragment(context, sp, uniformMap)).not.toEqual([0, 0, 0, 0]);
ContextLimits._highpFloatSupported = highpFloatSupported;
});

it('1 level function dependency', function() {
var vs = 'attribute vec4 position; void main() { gl_PointSize = 1.0; gl_Position = position; }';
var fs =
Expand Down