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

Adds support for combined Depth+Stencil Texture #9368

Merged
merged 8 commits into from Jul 27, 2016
Merged

Adds support for combined Depth+Stencil Texture #9368

merged 8 commits into from Jul 27, 2016

Conversation

AtiX
Copy link
Contributor

@AtiX AtiX commented Jul 21, 2016

Extends DepthTexture to not only support the depth component, but also depth+stencil component.

Two new constants (THREE.DepthStencilFormat and THREE.UnsignedInt248Type) are introduced.

The WebGLRenderTarget is extended to support setting the depthTexture in the options instead of currently setting it manually afterwards:

target = new THREE.WebGLRenderTarget( width, height, {
   ...
   depthTexture: myDepthTexture
})

Create a depth + stencil texture the following way:

depthTexture = new THREE.DepthTexture(
    texWidth
    texHeight
    THREE.UnsignedInt248Type
    mapping
    wrapS
    wrapT
    magFilter
    minFilter
    anisotropy
    THREE.DepthStencilFormat
  )

(see also: https://www.khronos.org/registry/webgl/extensions/WEBGL_depth_texture/)

@mrdoob
Copy link
Owner

mrdoob commented Jul 21, 2016

@mattdesl looks good?

@AtiX
Copy link
Contributor Author

AtiX commented Jul 22, 2016

I resolved all merge conflicts.

Ping @mattdesl ;)

@AtiX
Copy link
Contributor Author

AtiX commented Jul 25, 2016

Merge conflicts resolved again.

(And yeah, ping @mattdesl )

@Bryce-Summers
Copy link

When using depth only attachments, the paradigm I have observed in THREE.js is that the user creates a depth texture with an empty constructor as follows:

depthTexture = new THREE.DepthTexture()

then the depth texture is set to default values and later updated with framebuffer size information in the function setupDepthTexture()

// upload an empty depth texture with framebuffer size
if ( !properties.get( renderTarget.depthTexture ).__webglTexture ||
    renderTarget.depthTexture.image.width !== renderTarget.width ||
    renderTarget.depthTexture.image.height !== renderTarget.height ) {
    renderTarget.depthTexture.image.width = renderTarget.width;
    renderTarget.depthTexture.image.height = renderTarget.height;
    renderTarget.depthTexture.needsUpdate = true;
}

Since the texture is set to be a depth only texture by default, it naturally needs to be called using the paradigm of a complete constructor as in your usage comment at the start of this discussion:

depthTexture = new THREE.DepthTexture(
    texWidth
    texHeight
    THREE.UnsignedInt248Type
    mapping
    wrapS
    wrapT
    magFilter
    minFilter
    anisotropy
    THREE.DepthStencilFormat
  )

I think that it would be more consistent if THREE.js constructed a texture with a depth_stencil attachment once it sees that its target buffer has the stencil flag set to true or better yet new in addition to THREE.DepthTexture(), we could also have a THREE.DepthStencilTexture() class to preserve the THREE.js paradigm of allocating textures using default constructors and allowing the rendering targets to fill them in as needed.

@AtiX
Copy link
Contributor Author

AtiX commented Jul 26, 2016

I personally dislike the idea of automatically deciding which texture type to use based on the stencil flag, since this means changing/overriding other values like the pixel format, which could have been set by users.

Using a separate class is probably feasible, but creates duplicate code.

I think a clean approach would be to change the constructor to an options-object styled constructor (since missing parameters are filled in with defaults anyways), this would then result in

new THREE.DepthTexture()

and

new THREE.DepthTexture({
  format: THREE.DepthStencilFormat
  type: THREE.UnsignedInt248Type
})

@mrdoob , @Bryce-Summers what do you think?

@mrdoob
Copy link
Owner

mrdoob commented Jul 27, 2016

Looks good!

@mrdoob mrdoob merged commit 7250c5c into mrdoob:dev Jul 27, 2016
@mrdoob
Copy link
Owner

mrdoob commented Jul 27, 2016

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Jul 27, 2016

Any chance you could also provide an example for this? That way we'll make sure that we don't break it in the future.

@mrdoob
Copy link
Owner

mrdoob commented Jul 27, 2016

FYI 9c8d18d 63f412e

@AtiX
Copy link
Contributor Author

AtiX commented Jul 27, 2016

Thanks!

I hope to find time on the weekend, created issue #9417 for that. (Assign to me pls :)?)

@Bryce-Summers
Copy link

Bryce-Summers commented Jul 28, 2016

@AtiX I agree with you. By the way, I integrated your PR into an older version of three.js and it works great for image space CSG!

aardgoose pushed a commit to aardgoose/three.js that referenced this pull request Oct 7, 2016
* Adds THREE.UnsignedInt248Type and THREE.DepthStencilFormat constants

* Extends DepthTexture to allow for either THREE.DepthFormat or THREE.DepthStencilFormat

* Extends depthTexture initialization to support both DepthTextureFormat and DepthStencilFormat

* Fixes typos

* Ensures correct internal format for DepthStencil textures

* Allows to set depthTexture via options.depthTexture in constructor
@makc
Copy link
Contributor

makc commented Oct 27, 2019

I integrated your PR into an older version of three.js and it works great for image space CSG

@Bryce-Summers do you have a demo up anywhere ?

@Bryce-Summers
Copy link

I wish I could point you to my demo code, but at that time I was working with a team with a closed source policy.

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.

None yet

4 participants