processing / p5.js

p5.js is a client-side JS platform that empowers artists, designers, students, and anyone to learn to code and express themselves creatively on the web. It is based on the core principles of Processing. http://twitter.com/p5xjs —
http://p5js.org/
GNU Lesser General Public License v2.1
21.1k stars 3.22k forks source link

Image uniforms get reset after each draw using a shader #7030

Open nakednous opened 1 month ago

nakednous commented 1 month ago

Most appropriate sub-area of p5.js?

p5.js version

1.6.0 onwards

Web browser and version

No response

Operating system

No response

Steps to reproduce this

I've implemented shadow mapping in p5.js v1.5.0 using p5.Graphics and the p5.treegl library. While this works seamlessly in v1.5.0, I'm facing challenges with versions from p5.js v1.6.0 onwards. I am planning to transition to using p5.Framebuffer once this issue is resolved.

Steps to Reproduce:

  1. Run the shadow mapping implementation in p5.js v1.5.0 here.
  2. Attempt the same using p5.js v1.9.3 here.

I've verified that the p5.treegl commands function identically in both versions. Notably, p5.js v1.6.0 introduced significant updates to shader-related features. I'm unsure if this issue qualifies as a bug or if I should report it or ask for help at the p5.js forum.

@davepagurek, could you possibly take a look and provide some guidance?

davepagurek commented 1 month ago

Thanks for catching this! After https://github.com/processing/p5.js/pull/5923, textures are unbound when the shader is unbound to prevent some other bugs (keeping textures bound when not using them causes a lot of other downstream bugs). However, we unbind the shader immediately after each draw, so only the first shape has the depth texture bound, and then subsequent shapes have an empty texture. Right now, it means I guess you'll have to re-apply the uniform before each shape you draw.

To fix this in p5, maybe we need to keep track of what textures shader had bound the last time it was used so that we can re-bind those values when you use the shader again. That way textures will work like other uniforms.

nakednous commented 1 month ago

Thanks for the suggestion! I can confirm that reapplying the depth uniform before rendering each shape serves as a workaround.

davepagurek commented 1 month ago

If anyone is interested in taking this on, I think what we would need to do is adjust setUniform to store the last set texture on the shader:

https://github.com/processing/p5.js/blob/bcf9134547389fd26cdb251d3055206866dc3498/src/webgl/p5.Shader.js#L957-L960

...and then re-apply the last used texture value, if it exists, when you bind the shader again in _setFillUniforms and _setStrokeUniforms:

https://github.com/processing/p5.js/blob/bcf9134547389fd26cdb251d3055206866dc3498/src/webgl/p5.RendererGL.js#L2071-L2072

One complication: since this gets called after each draw finishes, this call below would end up storing an empty image when we really just want it to unbind the current image:

https://github.com/processing/p5.js/blob/bcf9134547389fd26cdb251d3055206866dc3498/src/webgl/p5.Shader.js#L573-L577

So maybe unbindTextures should just do this bit instead of calling setUniform?

https://github.com/processing/p5.js/blob/bcf9134547389fd26cdb251d3055206866dc3498/src/webgl/p5.Shader.js#L959-L960

JordanSucher commented 1 month ago

I tried to recreate #5923 so I could understand the issues that arise if we don't unbind a texture, but I found that when I commented out the unbindTextures() call, it didn't seem to cause any problems - tried the sketch from that issue, fill renders as expected.

Is it possible we don't need to have that call anymore?

https://github.com/processing/p5.js/assets/9809109/d7763f5f-4336-4984-932e-5299a099d1de

davepagurek commented 1 month ago

I made a new sketch on the most recent p5 here: https://editor.p5js.org/davepagurek/sketches/pwIAXyF0X

With the implementation of unbindTextures empty, the fills still stop working for me and I get the same error about framebuffer feedback in the console.

JordanSucher commented 1 month ago

very strange - that sketch works fine for me across chrome, safari, arc

nakednous commented 1 month ago

I tested this on Arch Linux. It fills the texture in both Chromium and Falkon, but in Firefox it only does it the first frame (mousePress to reveal it).

davepagurek commented 1 month ago

It's a weird situation because it's left up to the driver developers to determine what's considered feedback or not, so on some platforms it works. Unfortunately we mostly have to program to the lowest common denominator, and for now that means cleaning up our textures when not using them.

nakednous commented 1 month ago

weirdly enough leaving unbindTextures empty, solves the shadow mapping issue here even in firefox.

davepagurek commented 1 month ago

That makes sense, there isn't any framebuffer feedback going on in that sketch so in that case there isn't a need to unbind anything. Unbinding is only required for a few cases where there would be a cycle between the bound texture and the texture being written to, which basically only happens if a framebuffer is involved.