Skip to content

fix: preserve user-set uSampler uniform for custom shaders#8869

Merged
perminder-17 merged 6 commits into
processing:dev-2.0from
BHARATH0153:fix/usampler-user-shader-override
Jun 5, 2026
Merged

fix: preserve user-set uSampler uniform for custom shaders#8869
perminder-17 merged 6 commits into
processing:dev-2.0from
BHARATH0153:fix/usampler-user-shader-override

Conversation

@BHARATH0153
Copy link
Copy Markdown

@BHARATH0153 BHARATH0153 commented Jun 2, 2026

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:

  • src/core/p5.Renderer3D.js: skip uSampler override when user set it
  • src/webgl/p5.Shader.js: add _userSetUniforms tracking, check _isInternalSetUniform flag
  • test/unit/webgl/p5.RendererGL.js: add test verifying user-set uSampler is preserved

PR Checklist

npm run lint passes
Inline reference is included / updated
Unit tests are included / updated

@welcome
Copy link
Copy Markdown

welcome Bot commented Jun 2, 2026

🎉 Thanks for opening this pull request! For guidance on contributing, check out our contributor guidelines and other resources for contributors!
🤔 Please ensure that your PR links to an issue, which has been approved for work by a maintainer; otherwise, there might already be someone working on it, or still ongoing discussion about implementation. You are welcome to join the discussion in an Issue if you're not sure!
🌸 Once your PR is merged, be sure to add yourself to the list of contributors on the readme page !

Thank You!

@BHARATH0153 BHARATH0153 force-pushed the fix/usampler-user-shader-override branch from 7edf49d to 65440c1 Compare June 2, 2026 16:22
Copy link
Copy Markdown
Collaborator

@perminder-17 perminder-17 left a comment

Choose a reason for hiding this comment

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

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:

  1. When the active shader is a built-in shader (not a user-provided one), i.e. it is neither userFillShader nor userImageShader.
  2. When a texture is actually active via texture(), i.e. this.states._tex is 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?

@BHARATH0153 BHARATH0153 force-pushed the fix/usampler-user-shader-override branch 2 times, most recently from 2c61f4f to ab90d41 Compare June 3, 2026 10:59
@BHARATH0153 BHARATH0153 requested a review from perminder-17 June 3, 2026 11:00
@BHARATH0153
Copy link
Copy Markdown
Author

BHARATH0153 commented Jun 3, 2026

@perminder-17 Great suggestion thanks! I've updated the PR with this approach
Removed _userSetUniforms Set, _isInternalSetUniform flag, and per-frame clearing
Now only overrides uSampler when this.states._tex is active or the shader is built-in
test passes

Copy link
Copy Markdown
Collaborator

@perminder-17 perminder-17 left a comment

Choose a reason for hiding this comment

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

Looks good, do you think we could add one visual test as well as per @davepagurek sketch?

@p5-bot
Copy link
Copy Markdown

p5-bot Bot commented Jun 3, 2026

@BHARATH0153 BHARATH0153 force-pushed the fix/usampler-user-shader-override branch from 8c64504 to 382e3ef Compare June 3, 2026 12:03
@BHARATH0153 BHARATH0153 requested a review from perminder-17 June 3, 2026 12:11
@BHARATH0153
Copy link
Copy Markdown
Author

@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().
@BHARATH0153 BHARATH0153 force-pushed the fix/usampler-user-shader-override branch 2 times, most recently from 8a4d161 to d6f8635 Compare June 3, 2026 14:46
@BHARATH0153 BHARATH0153 force-pushed the fix/usampler-user-shader-override branch from 155581f to 355a04b Compare June 3, 2026 14:49
@BHARATH0153 BHARATH0153 force-pushed the fix/usampler-user-shader-override branch from 59e5570 to d444280 Compare June 3, 2026 14:51
@BHARATH0153
Copy link
Copy Markdown
Author

@davepagurek @perminder-17 please rerun the checks thanks!

Copy link
Copy Markdown
Collaborator

@perminder-17 perminder-17 left a comment

Choose a reason for hiding this comment

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

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.

Comment thread src/webgl/p5.Shader.js
@BHARATH0153 BHARATH0153 force-pushed the fix/usampler-user-shader-override branch from b7ac07b to fa694aa Compare June 4, 2026 00:41
@BHARATH0153 BHARATH0153 requested a review from perminder-17 June 4, 2026 00:50
@BHARATH0153
Copy link
Copy Markdown
Author

BHARATH0153 commented Jun 4, 2026

@davepagurek @perminder-17 once please rerun the checks

@BHARATH0153
Copy link
Copy Markdown
Author

@perminder-17 @davepagurek all checks have been passed please once review thanks!

Copy link
Copy Markdown
Collaborator

@perminder-17 perminder-17 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@perminder-17 perminder-17 merged commit 549dd20 into processing:dev-2.0 Jun 5, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants