depthTexture share source after RenderTarget.clone()

[TheBlek]

Description

Cloning a RenderTarget does not create a new Source for the new DepthTexture.

I've encountered this bug while using EffectComposer. If you pass a RenderTarget with depthTexture into EffectComposer, it will clone the depthTexture, but will not update its Source. This results in GL_INVALID_OPERATION: Feedback loop formed between Framebuffer and active Texture if you use passes that use depth texture.

I've found two very similar issues: one two

Reproduction steps

1. Create RenderTarget
2. Create DepthTexture for ithe RenderTarget
3. Clone RenderTarget
4. Compare sources of depth textures of new and old RenderTargets

Code

const fbo = new THREE.WebGLRenderTarget(640, 480);
fbo.texture.format = THREE.RGBFormat;
fbo.texture.generateMipmaps = false;
fbo.depthBuffer = true;
fbo.depthTexture = new THREE.DepthTexture(640, 480);
fbo.depthTexture.format = THREE.DepthFormat;
console.log(fbo.depthTexture.source);
console.log(fbo.depthTexture.clone().source);

See jsfiddle for the example where that breaks EffectComposer

Live example

https://jsfiddle.net/teu61qyj/3/
P.S. jsfiddle does not print the feedback error for me, but the rendering still breaks

Screenshots

No response

Version

r173

Device

Desktop

Browser

Chrome

OS

Windows

[TheBlek]

Also, shouldn't every texture in WebGLRenderTarget.textures receive a new Source?

[gkjohnson]

@Mugen87 can we revert #30570 and please give more than an hour between making a PR and merging it to give time for feedback and discussion in the future? What might be considered "correct" API behavior here is not obvious.

In the same way that two Meshes can share a material and geometry after clone (see #30340), conceptually there is no reason that two render targets or frame buffers should not be able to share and render to the same underlying textures. Users can assign and clone textures as-needed depending on their use case. I'd argue the only reason RenderTarget.texture and RenderTarget.texture are cloned (including sources) are due to historical reasons and desire to avoid breaking changes.

I'm curious what the conceptual line is here? Why should Mesh.material not be cloned when making a copy of Mesh but RenderTarget.textures should be cloned?

[Mugen87]

I have reverted the PR via #30572.

I've though the change is so obvious that no further discussion was required. The render target class already cloned the image/source but only for the default color attachment, not for all other attachments or depth. That is clearly inconsistent.

I would argue that render targets are different than 3D objects. The default should be a deep clone since normally framebuffers hold separate data. Sharing data seems more of an edge case to me that developers still can configure.

[TheBlek]

I wonder if it would be worthwhile to differentiate between a clone and a copy in the API?

In Rust, for example, Copy types are

Types whose values can be duplicated simply by copying bits.

So Copy is shallow and does not duplicate any resources. Whereas Clone is:

A common trait for the ability to explicitly duplicate an object.
Differs from Copy in that Copy is implicit and an inexpensive bit-wise copy, while Clone is always explicit and may or may not be expensive.

So Clone could in fact allocate new resources (and it generally does) and that would be a deep copy.

Users can assign and clone textures as-needed depending on their use case.

Code that clones RenderTarget currently clones depthTexture. This doesn't however fix the problem with them sharing a single source. So clone on the texture isn't enough either - you have to clone .source. I can't find a clean way to clone Source on a texture. Maybe that should be added? As well as comments in the documentation to make it obvious what users should do if they intent to create a new texture from the existing one?

Documentation wording on RenderTarget in not clear on whether that should be a deep or a shallow copy either:

Creates a copy of this render target.

Compare that to the wording on Texture's clone:

Make copy of the texture. Note this is not a "deep copy", the image is shared. Cloning the texture automatically marks it for texture upload.

[Mugen87]

I can't find a clean way to clone Source on a texture. Maybe that should be added?

That is normally not wanted since that would duplicate the texture source leading to redundant texture uploads. That was the reason why Source was added in the first place.

I would also not change the semantics of clone since cloning 3D objects is often done on app level and that should not be deep. Otherwise you end up with many duplicated geometries and materials.

The implementation of copy and clone methods depends on the use case. If I could start from scratch, I would only add copy/clone methods to the math classes.

There was one point where I actually suggested to remove copy() and clone() from render targets because there are pro and cons for different kind of implementations. Anyway, it's okay to keep them but they should work like suggested in #30572, imo.

There are use cases where you want to share e.g. the depth across render targets (like for OIT) but in most cases they refer to separate data storage. What you essentially copy/clone is the framebuffer setup but without sharing resources.

[gkjohnson]

I would also not change the semantics of clone since cloning 3D objects is often done on app level and that should not be deep. Otherwise you end up with many duplicated geometries and materials.

I agree I don't think we should change these semantics. Not to mention this would be a significant breaking change.

The implementation of copy and clone methods depends on the use case. If I could start from scratch, I would only add copy/clone methods to the math classes.

Yeah this is the fundamental issue - but it's still the case that at least the "clone" function get a lot of use. Copy is the function that poses conceptual issues in my opinion. The behavior on Object3D is really odd (meaning it adds children to those already there rather than copying fields onto the existing children which can get complicated for a lot of reasons - not that there's better behavior I can think of). A similar conceptual fault is present in RenderTarget, I feel. If you "copy" a render target then all original textures are overwritten (not modified) meaning references can be lost, leading to a memory leak.

All to say I think the clone functions are useful on these classes - though I don't think I've ever successfully used the "copy" functions for objects or render targets.

Anyway, it's okay to keep them but they should work like suggested in #30572, imo.
...
What you essentially copy/clone is the framebuffer setup but without sharing resources.

My only issue here is that clone in this case will create new assets that may not be needed and just be replaced by the original references (though I understand they won't have been initialized). Admittedly it is probably the less common use case to want the texture references to be retained on clone. When I look at the code, though, it looks like it could have been a deliberate choice. Adjusting all those textures to be cloned could be a breaking change for some apps, no?

[Mugen87]

If #30570 would be merged, it should definitely be noted in the migration guide. But I would rather risk a breakage if we end up with the more consistent setup. If apps are not happy with the default clone, they can implement a custom render target class and overwrite the clone() method. Same for DepthTexture.clone().

[gkjohnson]

Understood - okay lets merge #30570 but I think we should also make sure to update the docs to be clear that all MRT subtextures and depth texture are cloned, and that users can construct a new render target to share buffers.

Thank you for discussing it!