Limit Scope of Default Uniforms #5280
Conversation
They should not be set if using a user-defined fill shader.
They should not be set if using a user-defined stroke shader.
as other stroke shader uniforms
as other stroke shader uniforms
| fillShader.setUniform('uMaterialColor', this.curFillColor); | ||
| fillShader.setUniform('isTexture', !!this._tex); | ||
| if (this._tex) { | ||
| fillShader.setUniform('uSampler', this._tex); |
There was a problem hiding this comment.
Wouldn't this prevent people from using the uSampler uniform? For instance, at the moment, you can use uSampler as a uniform in your shader and have p5 send you whatever texture was bound with the texture() function. I think we wouldn't want to take that functionality away.
There was a problem hiding this comment.
Just based off of the code, I think if a user has set texture(someTexture), then this._tex will be set, and the uSampler uniform will still get set for you here. A test to confirm that would be great though
| @@ -262,15 +260,6 @@ p5.Shader.prototype.unbindTextures = function() { | |||
|
|
|||
| p5.Shader.prototype._setMatrixUniforms = function() { | |||
| this.setUniform('uProjectionMatrix', this._renderer.uPMatrix.mat4); | |||
| if (this.isStrokeShader()) { | |||
There was a problem hiding this comment.
I think moving this to the set stroke uniforms makes sense!
|
Thanks @JetStarBlues for working on it and @aferriss for the review. I'm tagging this year's GSoC WebGL shader contributor @wong-justin and mentors @aferriss @aceslowman @sosunnyproject @davepagurek to take a look at this, so we can decide how to move forward with this PR. |
|
I think this is probably the right direction to move in. Currently it's pretty hard to tell whether p5 will decide to send uniforms to you or not. Unfortunately any changes we make to this might end up breaking someone's workflow, so I feel like if we want to settle on something we can confidently support going forward, we need a system for users to explicitly opt in to shader features via something like #6144 (ideally also by just replacing one "block" of a shader where possible so that most other p5 features continue to work.) Does that sound like the right direction to you all? |
Resolves #5279
Changes:
Only sets the default uniforms if the current shader is not user-defined. See linked issue for more details.