fix cc.GLProgram and cc.GLProgramState uniform handling #3511

cruzdanilo · 2017-09-02T07:21:43Z

this patch fixes a bug that happens when using multiple cc.GLProgramState with a single cc.GLProgram, because the uniforms don't get updated

DavidDeSimone · 2017-09-02T07:50:48Z

Hello @cruzdanilo. Can I get some more details about this bug? Maybe a minimal reproduction case? It sounds like you are setting a uniform value on a GLProgramState, but it's not getting updated?

cruzdanilo · 2017-09-02T17:02:31Z

hi @DavidDeSimone. i've written a simple reproduction case as follows:

const program = new cc.GLProgram();
program.initWithVertexShaderByteArray(
  cc.SHADER_POSITION_COLOR_LENGTH_TEXTURE_VERT,
  'uniform lowp vec4 u_color; void main() { gl_FragColor = u_color; }');
program.addAttribute(cc.ATTRIBUTE_NAME_POSITION, cc.VERTEX_ATTRIB_POSITION);
program.addAttribute(cc.ATTRIBUTE_NAME_TEX_COORD, cc.VERTEX_ATTRIB_TEX_COORDS);
program.addAttribute(cc.ATTRIBUTE_NAME_COLOR, cc.VERTEX_ATTRIB_COLOR);
program.link();
program.updateUniforms();
const position = cc.p(100, 100);
[cc.color.RED, cc.color.GREEN, cc.color.BLUE].forEach((color) => {
  const node = new cc.DrawNode();
  node.drawDot(cc.p(), 10);
  const state = new cc.GLProgramState(program);
  state.setUniformVec4('u_color', color.r, color.g, color.b, color.a);
  node.setGLProgramState(state);
  node.setPosition(position);
  position.x += 25;
  this.addChild(node);
});

without the patch, the 3 nodes will be blue, which is the last color applied to the program.

DavidDeSimone · 2017-09-02T19:39:10Z

@cruzdanilo Thank you, I was able to repro, good catch. I have left some feedback in the inline comments.

DavidDeSimone · 2017-09-02T19:41:57Z

cocos2d/shaders/CCGLProgramState.js

@@ -105,12 +104,6 @@ cc.UniformValue.prototype = {
    },

    apply: function apply() {
-        if (this._currentBoundValue === this._value


I think this logic should be:

if (this._glprogram._currentBoundValue === this.value // ...)

this._glprogram._currentBoundValue = this._value.

I think we still want the early out to avoid setting a uniform that is already bound to the correct value, however we were incorrectly setting that value on the GLProgramState itself instead of the GLProgram.

Applying this patch seems to also fix the issue:

diff --git a/cocos2d/shaders/CCGLProgramState.js b/cocos2d/shaders/CCGLProgramState.js index 0a9cef0..50aeeaf 100644 --- a/cocos2d/shaders/CCGLProgramState.js +++ b/cocos2d/shaders/CCGLProgramState.js @@ -105,12 +105,12 @@ cc.UniformValue.prototype = { }, apply: function apply() { - if (this._currentBoundValue === this._value + if (this._glprogram._currentBoundValue === this._value && this._type !== types.GL_CALLBACK) { return; } - this._currentBoundValue = this._value; + this._glprogram._currentBoundValue = this._value; switch (this._type) { case types.GL_INT: this._glprogram.setUniformLocationWith1i(this._uniform.location, this._value); @@ -180,10 +180,7 @@ cc.GLProgramState.prototype = { } for (var i = 0; i < this._uniforms.length; ++i) { - var uniform = this._uniforms[i]; - if (uniform._currentBoundValue !== uniform._value) { - uniform.apply(); - } + this._uniforms[i].apply(); } },

this is incorrect. the bound value is per uniform per program, not only per program. this patch actually breaks the rendering even more.
according to the api documentation, cc.GLProgram.setUniformLocationWith* should be able to know when the uniform has updates and don't call gl.uniform* unnecessarily. unfortunately, the uniform cache currently only works with string locations. i am fixing this behaviour right now, but maybe it's more appropriate to create a new pull request for this, as it's a different (although related) problem.

You're right, that's what I get for writing code without coffee

hi @DavidDeSimone, i submitted the cache fix in this same pr, and now the cache also works for matrices, which i think will improve performance for static nodes, and also other situations

cruzdanilo · 2017-09-03T00:55:03Z

as it turns out, it's not possible to use WebGLUniformLocation (returned by gl.getUniformLocation) as a key in a map/hash, like cocos2d-x does with GLint (returned by glGetUniformLocation). this answer has some info about it. this explains the less intuitive modifications i made.
for api compatibility sake, all cc.GLProgram.setUniformLocationWith* keep the option of receiving both WebGLUniformLocation and string as the location parameter. internal WebGLUniformLocation references are kept private and receive a private field _name for efficient uniform cache mapping, without unnecessary gl.getUniformLocation calls.

DavidDeSimone · 2017-09-06T22:24:49Z

This seems reasonable to me. I'd ask @pandamicro to look it over as well.

pandamicro · 2017-09-08T03:41:22Z

cocos2d/shaders/CCGLProgram.js

+        var updated = false;
+        var element = this._hashForUniforms[name];
+        var args = Array.isArray(arguments[1]) ? arguments[1] :
+            Array.prototype.slice.call(arguments, 1);


As this MDN document about arguments object stated, this implementation will cause v8 fail to optimize the whole function.
I'd suggest to use the following approach

_updateUniform: function (name, a1) { ... var args; if (Array.isArray(a1)) { args = a1; } else { args = new Array(arguments.length-1); for (var i = 1; i < arguments.length; i++) { args[i-1] = arguments[i]; } } }

pandamicro · 2017-09-08T03:46:00Z

cocos2d/shaders/CCGLProgram.js

+            cc.UNIFORM_TIME_S, cc.UNIFORM_SINTIME_S, cc.UNIFORM_COSTIME_S,
+            cc.UNIFORM_RANDOM01_S, cc.UNIFORM_SAMPLER_S].forEach(function(name) {
+            this._addUniformLocation(name);
+        }.bind(this));


I'd also prefer the former implementation instead of forEach, because forEach will create one array and one temporary function for each updateUniforms invocation, they all become garbages after.

pandamicro · 2017-09-08T05:51:40Z

cocos2d/shaders/CCGLProgram.js

+            Array.prototype.slice.call(arguments, 1);
+
+        if (!element || element.length !== args.length) {
+            this._hashForUniforms[name] = args.slice();


There is no need to do slice here

Sorry, I don't get it. Why is it necessary to copy the args array ? It's a local variable, and it's not used anymore after this line.

if (Array.isArray(arguments[1])) { args = arguments[1]; } else { ... }

when arguments[1] is an array, you need to copy it, otherwise external modifications to the array will change the cache. as (!element || element.length !== args.length) doesn't happen often, i don't think it needs another Array.isArray check, just a shallow copy.

I see, you are right, I missed this condition

pandamicro · 2017-09-08T05:59:32Z

cocos2d/shaders/CCGLProgramState.js

-            }
-        }
+
+        Object.values(this._uniforms).forEach(function(uniform) {


Same as previous comment, we prefer the engine logic generate as less garbage as possible, so Please keep using the for loop, just a little bit more code is not a problem

pandamicro · 2017-09-08T06:07:37Z

Thanks for the effort, guys, @cruzdanilo I have posted some comments

pandamicro · 2017-09-29T04:48:39Z

Fixed an issue about the implementation in

f63c49d

Maybe you guy need to know it, @cruzdanilo @DavidDeSimone

fix cc.GLProgramState.apply

87c456c

DavidDeSimone reviewed Sep 2, 2017

View reviewed changes

fix cc.GLProgram uniform cache

444c237

cruzdanilo changed the title ~~fix cc.GLProgramState.apply~~ fix cc.GLProgramState and cc.GLProgram uniform handling Sep 3, 2017

cruzdanilo changed the title ~~fix cc.GLProgramState and cc.GLProgram uniform handling~~ fix cc.GLProgram and cc.GLProgramState uniform handling Sep 3, 2017

pandamicro suggested changes Sep 8, 2017

View reviewed changes

optimize uniform cache performance

d67a8b3

pandamicro approved these changes Sep 13, 2017

View reviewed changes

pandamicro merged commit d7cefcc into cocos2d:develop Sep 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix cc.GLProgram and cc.GLProgramState uniform handling #3511

fix cc.GLProgram and cc.GLProgramState uniform handling #3511

cruzdanilo commented Sep 2, 2017

DavidDeSimone commented Sep 2, 2017

cruzdanilo commented Sep 2, 2017

DavidDeSimone commented Sep 2, 2017

DavidDeSimone Sep 2, 2017 •

edited

Loading

cruzdanilo Sep 2, 2017

DavidDeSimone Sep 2, 2017

cruzdanilo Sep 3, 2017

cruzdanilo commented Sep 3, 2017

DavidDeSimone commented Sep 6, 2017

pandamicro Sep 8, 2017

pandamicro Sep 8, 2017

pandamicro Sep 8, 2017

pandamicro Sep 13, 2017

cruzdanilo Sep 13, 2017

pandamicro Sep 13, 2017

pandamicro Sep 8, 2017

pandamicro commented Sep 8, 2017

pandamicro commented Sep 29, 2017

fix cc.GLProgram and cc.GLProgramState uniform handling #3511

fix cc.GLProgram and cc.GLProgramState uniform handling #3511

Conversation

cruzdanilo commented Sep 2, 2017

DavidDeSimone commented Sep 2, 2017

cruzdanilo commented Sep 2, 2017

DavidDeSimone commented Sep 2, 2017

DavidDeSimone Sep 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cruzdanilo commented Sep 3, 2017

DavidDeSimone commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pandamicro commented Sep 8, 2017

pandamicro commented Sep 29, 2017

DavidDeSimone Sep 2, 2017 •

edited

Loading