WebGLRenderer: Fix active texture setting in copyTextureToTexture().

[geekuillaume]

Fixed #25618.

Description

In some cases, calling gl.copyTextureToTexture can target a different texture than the one actually passed as parameter. This happens if:

• the target texture is already in slot 0
• the active slot is not slot 0

In this case, when copyTextureToTexture calls textures.setTexture2D, state.bindTexture is called but because this texture was already in slot 0, state.bindTexture is a noop. It means that the active slot is not changed and if it's not 0, then the target of copyTextureToTexture is invalid.

The fix in this PR solves the problem for copyTextureToTexture specifically but maybe we should fix state.bindTexture instead. The current behavior of state.bindTexture can create problems because it only set the active slot if the texture was not already bound to this slot.

Here's what it currently does:

if ( boundTexture.type !== webglType || boundTexture.texture !== webglTexture ) {

	if ( currentTextureSlot !== webglSlot ) {

		gl.activeTexture( webglSlot );
		currentTextureSlot = webglSlot;

	}

	gl.bindTexture( webglType, webglTexture || emptyTextures[ webglType ] );

	boundTexture.type = webglType;
	boundTexture.texture = webglTexture;

}

But instead we could do:

if ( currentTextureSlot !== webglSlot ) {

	gl.activeTexture( webglSlot );
	currentTextureSlot = webglSlot;

}

if ( boundTexture.type !== webglType || boundTexture.texture !== webglTexture ) {

	gl.bindTexture( webglType, webglTexture || emptyTextures[ webglType ] );

	boundTexture.type = webglType;
	boundTexture.texture = webglTexture;

}

I'm not sure if this will cause some other side-effect. Let me know what you think!

[github-actions]

📦 Bundle size

Full ESM build, minified and gzipped.

	Before	After	Diff
WebGL	359.27 85.31	359.3 85.32	+30 B +10 B
WebGPU	630.15 174.95	630.15 174.95	+0 B +0 B
WebGPU Nodes	628.73 174.7	628.73 174.7	+0 B +0 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

	Before	After	Diff
WebGL	491.47 119.97	491.5 119.98	+30 B +13 B
WebGPU	703.37 189.9	703.37 189.9	+0 B +0 B
WebGPU Nodes	652.59 177.29	652.59 177.29	+0 B +0 B

[Mugen87]

Can you please reproduce the issue with a live example?

Use https://jsfiddle.net/3mrkqyea/ as a starter template.

We must be able to reproduce the issue so we have a chance to verify if the PR actually fixes it.

[geekuillaume]

Here is the reproduction case: https://jsfiddle.net/xko5s6zf/5/

You should see a green sphere on top and a blue sphere, but actually you see a blue sphere on top of a red one. Try commenting the renderer.copyTextureToTexture call. It will change the color of the top sphere even if this call doesn't impact the renderTarget used to texture the sphere.

[Mugen87]

While studying the solution I think it's indeed better to fix the issue in WebGLState as you mentioned in #33153 (comment). The fix in copyTextureToTexture() looks more like a bandage that "hides" the real root cause.

WebGLState.bindTexture() should not only update the active texture slot when a gl.bindTexture() happens. Even without this it must check if the texture is registered in the correct slot.

Would you please update the PR and add your proposed fix to WebGLState instead? Can you also update the WebGL backend of WebGPURenderer? It requires the same fix, see:

three.js/src/renderers/webgl-fallback/utils/WebGLState.js

Lines 1248 to 1253 in bf65eaf

	if ( currentTextureSlot !== webglSlot ) {
	gl.activeTexture( webglSlot );
	this.currentTextureSlot = webglSlot;
	}

[geekuillaume]

Done! I also changed the title of the PR to reflect the actual change and not mention copyTextureToTexture directly

[Mugen87]

I did a performance review and it turns out this change introduces unnecessary gl.activeTexture() calls. I then let Gemini review the PR with regards on performance and it does not recommend to make this change.

I'm sorry for the back and forth but could you restore the previous version and just modify copyTextureToTexture()? If we stumble across this issue elsewhere, we can think of a different approach in WebGLState. But for now, let's fix this locally and don't risk performance regressions.

To further explain the design issue: If a texture is just used for rendering, a simple bind is sufficient. But when mutating the texture (like copyTextureToTexture() does), gl.activeTexture() is required so commands like gl.texImage2D() and gl.texParameteri() target the correct texture. state.bindTexture() does currently do both which is not necessarily correct.

[geekuillaume]

No problem, I force pushed the first version. I'll let you edit the title of the PR however you see fit.

[Mugen87]

Can we move this below the else block so it also works for 3d and array textures?

[Mugen87]

Edit: I think WebGPURenderer does not need the change because the WebGL backend does not explicitly set 0 for the texture unit.

[Mugen87]

Indeed, WebGPURenderer works fine for both backends: https://jsfiddle.net/wenck4aq/

[Mugen87]

I've made a small update and also added a comment to this PR so it's more clear why the line has been added.

[geekuillaume]

Okay, thanks for taking care of this!