Skip to content

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

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

Merged
merged 3 commits into from
Sep 13, 2017
Merged

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
87c456c
fix cc.GLProgramState.apply
cruzdanilo Sep 2, 2017
444c237
fix cc.GLProgram uniform cache
cruzdanilo Sep 3, 2017
d67a8b3
optimize uniform cache performance
cruzdanilo Sep 13, 2017
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
12 changes: 1 addition & 11 deletions cocos2d/shaders/CCGLProgramState.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ cc.UniformValue = function (uniform, glprogram) {
this._glprogram = glprogram;

this._value = null;
this._currentBoundValue = null;
this._type = -1;
};

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

apply: function apply() {
if (this._currentBoundValue === this._value
Copy link
Contributor

@DavidDeSimone DavidDeSimone Sep 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
         }
     },

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

&& this._type !== types.GL_CALLBACK) {
return;
}

this._currentBoundValue = this._value;
switch (this._type) {
case types.GL_INT:
this._glprogram.setUniformLocationWith1i(this._uniform.location, this._value);
Expand Down Expand Up @@ -180,10 +173,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();
}
},

Expand Down