Thanks for catching this! After #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.

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

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:

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

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:

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

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?

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.

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

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).

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.

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

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.

If unbinding is necessary only at the level of framebuffer then we could call setUniforms only when framebuffers are involved. I would love to take this on.

  unbindTextures() {
    for (const uniform of this.samplers) {
      // Check if the uniform's texture is a framebuffer
      if ((uniform.texture instanceof p5.Framebuffer) {
        // Call setUniform if it's a framebuffer
        this.setUniform(uniform.name, this._renderer._getEmptyTexture());
      }
    }
  }

@Forchapeatl I've tested your change with the original sketch in Firefox and Chrome and it seems to solve the issue! (there's just an extra parenthesis in the condition)

unbindTextures() {
  for (const uniform of this.samplers) {
    // Check if the uniform's texture is a framebuffer
    if (uniform.texture instanceof p5.Framebuffer) {
      // Call setUniform if it's a framebuffer
      this.setUniform(uniform.name, this._renderer._getEmptyTexture());
    }
  }
}

While it does solve the issue here, it also would mean that while some textures remain bound, others do not, which could lead to confusion for users. To keep consistency between texture types, I think we'd still need to record what used to be bound before we unbind it so that it can be re-bound.

I think we'd still need to record what used to be bound before we unbind it so that it can be re-bound.

@davepagurek , please for an example of this scenario . Why would we re-bound after unbinding ?

Hi , @davepagurek . Please, I am still struggling to understand this logic

I think we'd still need to record what used to be bound before we unbind it so that it can be re-bound.

  bindTextures() {
    const gl = this._renderer.GL;

    for (const uniform of this.samplers) {
      let tex = uniform.texture;
      if (tex === undefined) {
        // user hasn't yet supplied a texture for this slot.
        // (or there may not be one--maybe just lighting),
        // so we supply a default texture instead.
        tex = this._renderer._getEmptyTexture();
      }
      gl.activeTexture(gl.TEXTURE0 + uniform.samplerIndex);
      tex.bindTexture();
      console.log(tex);
      tex.update();
      gl.uniform1i(uniform.location, uniform.samplerIndex);
    }
  }

Forgive me if I sound silly or ignorant but , it seems like every texture gets bounded here. From my logs i can see that textures get bounded. My question is , If textures are bounded already there is no need to unbind them ( the code works when unbindTextures function is empty ) , why do we have to track them ?

I think there are two parts here, first being why we unbind things at all. This issue #5921 happens if you leave certain framebuffers bound. Essentially: if you have a framebuffer bound as an input (as a uniform to a shader), many WebGL implementations prevent you from also using it as an output (drawing contents to it.) In that issue, the code isn't actually trying to both read from and draw to the framebuffer at the same time, it only reads from the framebuffer within the push/pop where it uses the shader, but if p5 leaves the uniforms bound, then the browser doesn't know we won't read from it while we're writing. So by unbinding after we're finished drawing something that reads from it, we avoid that issue.

The problem that comes up, then, is if you try to draw two different things in a row with a texture bound. Because we unbind after a draw, this code will no longer work as expected:

shader(myShader)
myShader.setUniform('someTexture', myFramebuffer)

// This one works, the framebuffer is still bound
model(modelA)
// After the draw, it gets unbound immediately.

// Since it got unbound, the framebuffer still won't be set here, even though it looks
// like it should be present.
model(modelB)

This isn't the only possible solution, but my suggestion was to separate the concepts of (1) what textures are currently bound, and (2) what the user last passed into setUniform. The idea would be, setUniform just records what the user wants to be set. When we draw an object, we first bind that recorded texture, then draw, then unbind it. That would update the flow for that code:

shader(myShader)
myShader.setUniform('someTexture', myFramebuffer)
// Here we have recorded that we are filling the someTexture slot with myFramebuffer,
// but it is not yet bound.

// Before draw, we bind myFramebuffer
model(modelA)
// After the draw, it gets unbound, but we still know that that's what should be in someTexture

// Before draw, we bind myFramebuffer again
model(modelB)
// After the draw, we unbind again, and still keep it recorded

// Now we finally update it to something else when setUniform is called again:
myShader.setUniform('someTexture', myFramebuffer2)

Hopefully that example makes sense, let me know if I can clarify anything!

I'm trying to test a system to store and reapply the previous texture in shaders. Currently, I have the following code in the p5.Shader:

unbindTextures() {
  for (const uniform of this.samplers) {
    const tex = uniform.texture;
    if (tex) {
      // Store the texture as the previous texture
      this._prevTexture = this._prevTexture !== tex ? tex : this._prevTexture;
    }
    this.setUniform(uniform.name, this._renderer._getEmptyTexture());
  }
}

I've added the _prevTexture property to the `p5.Shader class:

constructor(rederer, vertSrc, fragSrc) {
  // other properties
  this._bound = false;
  this._prevTexture = null;
  this.samplers = [];
}

However, I'm unsure about when and how to reapply _prevTexture. I've tried assigning the texture in p5.RendererGL.js after bindShader like this:

fillShader.bindShader();
// Reapply the last texture that was bound if not null
if (fillShader._prevTexture != null) {
  console.log('Applying previous fill textures: ');
  this._tex = fillShader._prevTexture;
}

@davepagurek

1. Should this be implemented inside bindShader instead?
2. Or should this logic only be applied to _setFillUniforms and _setStrokeUniforms?

Any guidance is appreciated

Thanks for taking a look! I think maybe the places that would make sense are bindShader and unbindTextures. I'm imagining:

• In bindShader, you could loop through this.samplers and re-apply uniform.texture for each one
• In unbindTextures, you can set them all back to empty textures, but update the way it does it: right now I think it does it in a way that will overwrite uniform.texture, so we'd want to make it do everything it currently does except that bit.

I'm suggesting continuing to store it as uniform.texture because you could have multiple of these per shader, not just one, and we already have something that works close to what we want, so it might be a smaller change to adapt that existing system.

Finally solved the issue. It tracks previous boundedTextures / previousBindings.

p5.Shader = class {
    ...
    this.previousBindings = new Map();  // Map to store previous bindings
    this.boundedTextures = new Set();
  }

bindTextures() {
    const gl = this._renderer.GL;
    this.previousBindings = new Map();  // Map to store previous bindings

    for (const uniform of this.samplers) {
        let tex = uniform.texture;
        if (tex === undefined) {
            tex = this._renderer._getEmptyTexture();
        }

        const textureUnit = gl.TEXTURE0 + uniform.samplerIndex;
        gl.activeTexture(textureUnit);

        // Store the previous binding for this texture unit
        const previousTexture = gl.getParameter(gl.TEXTURE_BINDING_2D);
        this.previousBindings.set(textureUnit, previousTexture);

        tex.bindTexture();
        tex.update();
        this.boundedTextures.add(tex);

        gl.uniform1i(uniform.location, uniform.samplerIndex);
    }
}

unbindTextures() {

    const gl = this._renderer.GL;
    for (const uniform of this.samplers) {
        const textureUnit = gl.TEXTURE0 + uniform.samplerIndex;

        if (this.previousBindings.has(textureUnit) === false){
        this.setUniform(uniform.name, this._renderer._getEmptyTexture());
       }
    }
}

The complete code
May I get started on the PR ?

@Forchapeatl Hi, I tested your code and it's working fine with the OP sketch.
But, for me it's not clear when the boundedTextures Set is used:

    this.boundedTextures = new Set();

Maybe just the previousBindingsMap manages all the logic to solve this bug?

I've added code to the 2.0 branch that should resolve this issue, so I'm going to mark this as closed. Expect a fix in the 2.0 release early next year!