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

Depthtextures should not need to be explicitly constructed #18860

Closed
khlorghaal opened this issue Mar 10, 2020 · 11 comments
Closed

Depthtextures should not need to be explicitly constructed #18860

khlorghaal opened this issue Mar 10, 2020 · 11 comments
Labels

Comments

@khlorghaal
Copy link

Depthtextures explicitly require construction and assignment

target.depthTexture = new THREE.DepthTexture();
which is bizarrely inconsistent with how the color texture is constructed automatically.

It would make sense only if #15440 were on master, however attempting to assign a depthtexture to multiple rendertargets instead results in silent undefined behavior.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 11, 2020

Depthtextures explicitly require construction and assignment

It was implemented like this on purpose since the respective depth texture extension is not supported by all devices. The default of generating depth textures is still RGBA depth packing.

@khlorghaal
Copy link
Author

khlorghaal commented Mar 11, 2020

The default of generating depth textures is still RGBA depth packing

wait, so a Three DepthTexture is not meant for use as a framebuffer depth attachment?
My understanding of the spec is that any depth attachments must be a special depth or depth-stencil internalformat? Or is that only required for shadow samplers?

@khlorghaal
Copy link
Author

khlorghaal commented Mar 11, 2020

opinion- my difficulty figuring out how to use three's render targets has far exceeded the amount of effort of using raw webgl framebuffers
granted this is my perspective as a desktop GL engine developer

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 12, 2020

Yes, you use DepthTexture so save depth information when rendering to a render target. In other words, an instance of DepthTexture represents the depth attachment of a custom framebuffer. However, there are some issue with WebGL 1 and the respective WEBGL_depth_texture extension: Not all devices support it and the floating point precision (at least in WebGL 1) is limited.

Hence, the mentioned RGBA depth packing is more reliable although this approach requires a separate render pass. Which is not always necessary e.g. when rendering a DoF effect. With DepthTexture, you can produce the beauty and depth pass with a single render call.

Because of this circumstance, the depth texture configuration has to be done by the developer.

@khlorghaal
Copy link
Author

khlorghaal commented Mar 12, 2020

Is it against Three's standard to alter its behavior based on available extensions? Would an auto-configuring depth setup be candidate for three's 'examples'?

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 12, 2020

I don't think there is "defined standard" but other features like WebGL 2 are also not automatically enabled. In this case, devs have to create the WebGL 2 rendering context by themselves and pass it into the renderer.

@khlorghaal
Copy link
Author

khlorghaal commented Mar 12, 2020

I see. A higher level library would handle this all automatically, but that would require pipeline modifications and hidden behavior, which don't fit with three's mid-level role.

But I would still like to see a syntax improvement here, since having to mutate a field for an object I just constructed, instead of the initial constructor doing so, feels unwieldy. And I would have never figured out how to do this if not for the depth texture example code.

I suggest this be done using an opt for the render target constructor, to avoid mutation.
such as boolean "depthRenderTexture" default=false which determines whether to use a depth render texture or render buffer. If false, RenderTarget.depthTexture is null, otherwise it is constructed with the RenderTarget.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 12, 2020

I suggest this be done using an opt for the render target constructor, to avoid mutation.
such as boolean "depthRenderTexture" default=false which determines whether to use a depth render texture or render buffer. If false, RenderTarget.depthTexture is null, otherwise it is constructed with the RenderTarget.

The problem is that you might still want to configure the depth texture, for example its type. In three.js, it's typical that users compose complex objects out of more basic building blocks. E.g. you create a material and a geometry in order to create a mesh. The current depth texture approach just follows this style. And at least from my point of view, I see no need for a change right now. The example code together with the documentation should be sufficient to figure out how develop with this API.

Let's see how others evaluate your suggestion.

@khlorghaal
Copy link
Author

khlorghaal commented Mar 12, 2020

Is that not inconsistent with how the color buffer is configured?
Ie If the developer needs WEBGL_draw_buffers then they need to use raw webgl.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 12, 2020

Yes, WEBGL_draw_buffers is not yet supported.

@DusanBosnjakKodiak

This comment has been minimized.

@Mugen87 Mugen87 closed this as completed Mar 3, 2021
Repository owner locked as resolved and limited conversation to collaborators Mar 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants