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

WebGLRenderTarget: clone texture image data when cloning render target #21719

Merged
merged 2 commits into from
May 7, 2021

Conversation

gkjohnson
Copy link
Collaborator

Related issue: #20328

Description

Clone the WebGLRenderTarget.texture image data object when cloning the render target. If it's not cloned then modifying the cloned render target will modify the original render targets image object causing it to be out of sync. See this case:

const x = new THREE.WebGLRenderTarget( 10, 10 );
const y = new THREE.WebGLRenderTarget( 20, 20 );
console.log( x.texture.image.width, x.texture.image.height, x.width, x.height ); // 10, 10, 10, 10

y.copy( x );
y.setSize( 100, 100 );

console.log( x.texture.image.width, x.texture.image.height, x.width, x.height ); // 100, 100, 10, 10

This shouldn't be a breaking change because I can't see how you'd be relying on this behavior.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 26, 2021

What about #20328 (comment)?

Well, cloning data textures and changing the size is probably no common operation (compared to render targets where the resize can happen in context of post-processing).

@gkjohnson
Copy link
Collaborator Author

What about #20328 (comment)?

While that case can be confusing right now the biggest issue is that when the image data is updated the cloned texture isn't automatically updated. I've thought about how to solve it but given that TextureLoader (and therefore Manager.getHandler) affords the pattern of returning a not-yet-fully-loaded texture object it's important that .clone retain the image data reference. I think think #17766 would be a better solution to that problem.

In the case of WebGLRenderTarget, though, not only do object variables become out of sync but the contents and GPU data size do to. Perhaps #17766 would be a fix for that, as well? Then you could have two render targets pointing to the same GPU data but I don't want to over complicate things. I'm hoping that PR is still on the table for getting merged.

@mrdoob mrdoob added this to the r129 milestone May 7, 2021
@mrdoob mrdoob merged commit 3060ed3 into mrdoob:dev May 7, 2021
@mrdoob
Copy link
Owner

mrdoob commented May 7, 2021

Thanks!

@gkjohnson gkjohnson deleted the fix-rendertarget-clone branch May 7, 2021 04:26
@@ -89,6 +89,7 @@ class WebGLRenderTarget extends EventDispatcher {
this.viewport.copy( source.viewport );

this.texture = source.texture.clone();
this.texture.image = { ...this.texture.image }; // See #20328.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't it be:

this.texture.image = { ...source.texture.image }; // See #20328.

Copy link
Collaborator

@Mugen87 Mugen87 Jan 14, 2022

Choose a reason for hiding this comment

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

Or better like so to avoid #22618:

this.texture.image = { 
    width: source.texture.image.width,
    height: source.texture.image.height,
    depth: source.texture.image.depth
};

Copy link
Owner

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this.texture.image = { ...source.texture.image }; // See #20328.

This is the same as { ...this.texture.image } since after source.texture.clone() the source values are the same object between the two instances.

I don't have an issue with changing it since it's the only location in core that's using spread. Though there are quite a few others it looks like in the examples files.

@Mugen87 suggestion looks good but Object.assign( {}, this.texture.image ) would work, as well

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.

3 participants