fix: preserve user-set uSampler uniform for custom shaders#8869
Conversation
|
🎉 Thanks for opening this pull request! For guidance on contributing, check out our contributor guidelines and other resources for contributors! Thank You! |
perminder-17
left a comment
There was a problem hiding this comment.
Hi, Thanks for your work on this @BHARATH0153 , I think I'm unable to understand your solution?
The core requirement is that p5.js should not override the uSampler uniform when a user has set it manually on their own shader.
We only need to set uSampler internally in two cases:
- When the active shader is a built-in shader (not a user-provided one), i.e. it is neither
userFillShadernoruserImageShader. - When a texture is actually active via
texture(), i.e.this.states._texis truthy.
So the condition becomes:
const isUserFillShader =
fillShader === this.states.userFillShader ||
fillShader === this.states.userImageShader;
if (this.states._tex || !isUserFillShader) {
fillShader.setUniform("uSampler", this.states._tex || empty);
}Does this feels right to you?
|
@perminder-17 Great suggestion thanks! I've updated the PR with this approach |
perminder-17
left a comment
There was a problem hiding this comment.
Looks good, do you think we could add one visual test as well as per @davepagurek sketch?
|
@perminder-17 Thanks again for the review I've simplified the approach as you suggested now only overrides uSampler when the shader is built-in or a texture is active. Also added a visual test based on @davepagurek's sketch. |
Apply review feedback: instead of tracking _userSetUniforms with _isInternalSetUniform flag, only override uSampler when shader is built-in or texture is active. Removes _userSetUniforms Set, _isInternalSetUniform flag, and per-frame clearing in _update().
|
@davepagurek @perminder-17 please rerun the checks thanks! |
perminder-17
left a comment
There was a problem hiding this comment.
Hi, thanks for adding the test. I am unable to see the metaData and the screenshot which was generated from that test which were present in this commit : f75f227
I think, you need to run npm test locally to generate those and needs to commit them again in your latest commit.
|
@davepagurek @perminder-17 once please rerun the checks |
|
@perminder-17 @davepagurek all checks have been passed please once review thanks! |
Resolves #8200
overview
p5.js overrides the uSampler uniform on user shaders after the user has already set a value via setUniform(). This happens because _setFillUniforms always resets uSampler to the current p5.js texture state.
fixes Track which uniforms have been explicitly set by the user via setUniform(). In _setFillUniforms, skip overriding uSampler if the user has already set it. The tracking is reset each frame in _update() so per-frame setUniform calls are honored.
Changes:
PR Checklist
npm run lint passes
Inline reference is included / updated
Unit tests are included / updated