Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebGLRenderer: Calling "copyTextureToTexture" on an unused WebGLRenderTarget does nothing #28282

Closed
gkjohnson opened this issue May 4, 2024 · 7 comments · Fixed by #28285
Closed

Comments

@gkjohnson
Copy link
Collaborator

gkjohnson commented May 4, 2024

Description

Found during #28281

If you call copyTextureToTexture to copy data into a RenderTarget that hasn't been rendered to, yet, then nothing occurs and an error is logged:

GL_INVALID_VALUE: Offset overflows texture dimensions.

Reproduction steps

  1. Create a new WebGLRenderTarget
  2. Create a new data texture
  3. Copy data texture data into the render target
  4. Render target is still empty and a warning is logged

Code

const target = new THREE.WebGLRenderTarget( 16, 16 );
const source = new THREE.DataTexture( new Uint8Array( 16 * 16 * 4 ).fill( 255 ), 16, 16 );

// uncomment these lines initialize the render target and prevent the error for logging
// renderer.setRenderTarget( target );
// renderer.render( scene, camera );
// renderer.setRenderTarget( null );

renderer.copyTextureToTexture( new THREE.Vector2( 0, 0 ), source, target.texture );

// logs warning

Live example

https://jsfiddle.net/2yakmh60/3/

Screenshots

No response

Version

dev

Device

Desktop

Browser

Chrome

OS

MacOS

@Spiri0
Copy link
Contributor

Spiri0 commented May 5, 2024

From what I understand, it can't work that way because your target.texture doesn't exist yet.
The srcTexture can only be copied into an existing dstTexture. Your renderTarget must have been rendered beforehand for this to work.
So you have already implemented the solution yourself, just commented it out.

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented May 5, 2024

So you have already implemented the solution yourself, just commented it out.

What I'm suggesting is that the render target initialization should happen in the copyTextureToTexture if it hasn't happened already. What I have currently is more of a workaround.

@Spiri0
Copy link
Contributor

Spiri0 commented May 5, 2024

To do this you would also have to pass the renderTarget, scene and camera to copyTextureToTexture and then the 3 lines are executed in copyTextureToTexture. I personally don't think that's a good solution.
I have worked a lot with copyTextureToTexture and for me it was always a matter of course to make sure that my dstTexture existed beforehand.
I don't see your initialization of your renderTarget as a workaround, but as exactly the right way.
But @Mugen87 can decide that. I'm just an enthusiastic user 😊

@Mugen87
Copy link
Collaborator

Mugen87 commented May 5, 2024

It's actually not necessary to render the scene. Just setting the render target once (and then remove it so it is not bound) is sufficient to make sure setupRenderTarget() has been executed once: https://jsfiddle.net/womfczx7/

In copyTextureToTexture() it would be necessary to have something like this:

const renderTargetProperties = properties.get( renderTarget );

if ( renderTargetProperties.__webglFramebuffer === undefined ) {

    textures.setupRenderTarget( renderTarget );

}

But since you pass in a texture to copyTextureToTexture(), you don't have access to the render target.

@Spiri0
Copy link
Contributor

Spiri0 commented May 5, 2024

Therefore the renderTarget should be able to be passed to copyTextureToTexture. You can do that and then check whether the renderTarget was also passed, but is that also wanted for a function called copyTextureToTexture? This is probably purely a matter of taste. It wouldn't bother me but I don't think it's necessary.

There is a certain irony in the fact that passing renderTargets to readRenderTargetPixels in webgl instead of textures in readRenderTargetPixelsAsync for webgpu makes the issue #22403 so complicated. Precisely because it is handled so elementary in the node system and only textures are passed, it was quickly solvable very clean even for me, who is not that deep into the system code.

@gkjohnson
Copy link
Collaborator Author

@Mugen87

Just setting the render target once (and then remove it so it is not bound)
...
But since you pass in a texture to copyTextureToTexture(), you don't have access to the render target.

I noticed this when looking through the code. In #28285 I've updated initTexture so it will initialize a WebGLRenderTarget which simplifies this use case (you no longer have to confusingly set and reset the active RT). And it updates the copy texture to texture functions to take and initialize a WebGLRenderTarget. Now the only case that will behave a bit erroneously is passing a RT texture when it has not been initialized, yet:

const target = new WebGLRenderTarget( 1, 1 );

// doesn't work because render target is uninitialized
renderer.copyTextureToTexture( box, position, srcTexture, target.texture );

// works because the render target can be initialized
renderer.copyTextureToTexture( box, position, srcTexture, target );

Maybe it's because my background is from Unity years ago (which had a RenderTexture API that extended from Texture) but I always found it odd that WebGLRenderTarget was considered separate and not usable in the same way as a three.js Texture and this might be a moment where thats surfacing. In my mind it makes sense to be able to use them interchangeably but I know there are larger architecture and implementations considerations that make this more difficult.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 6, 2024

As mentioned in #28285 (comment) three.js has the policy to not use render target as textures and I don't think we should change that (see #8417, #8658 and #8615).

copyTextureToTexture() can not be called with render targets and I do not vote to add support to it. Uninitialized render targets must be handled on app level. I suggest to add a new method WebGLRenderer.initRenderTarget() that allows devs access to the logic of setupRenderTarget(), see #28285 (comment).

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 a pull request may close this issue.

3 participants