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

DepthTexture: Update depth + stencil textures to work with multi sample RTT, make formats consistent #28460

Merged
merged 12 commits into from
May 22, 2024

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented May 22, 2024

Related issue: --

Description

I expect this hasn't been touched much since WebGL has been removed but there were a few issues I noticed with DepthTextures when I was experimenting with them for #28423:

  • A stencil attachment could not be used with a 32F depth type.
  • A stencil attachment could not be used when a multi sample render target is used.
  • Using a stencil format didn't work at all unless using UnsignedInt248 type

You can see in the current depth texture example that using a stencil buffer breaks the page. With this PR:

  • DepthTexture.type is allowed to be set to FloatType, UnsignedIntType, and UnsignedShortType. If a stencil attachment is required then the appropriate internal type is selected. Stencil is unsupported for UnsignedShortType. The UnsignedInt248Type remains for backwards compatibility and does the same as UnsignedIntType.
  • The internal depth attachment for the multi sample textures is now created with the same format as the assigned depth texture.
  • Code redundancy has been reduced in WebGLTextures.
  • The depth texture demo has been modified to test all the new options including render target samples option.

@gkjohnson gkjohnson added this to the r165 milestone May 22, 2024
@gkjohnson gkjohnson requested a review from Mugen87 May 22, 2024 11:21
Copy link

github-actions bot commented May 22, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
679.8 kB (168.5 kB) 679.6 kB (168.5 kB) -203 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
457.7 kB (110.5 kB) 457.5 kB (110.6 kB) -189 B


let glInternalFormat = _gl.DEPTH_COMPONENT24;

if ( isMultisample || useMultisampledRTT( renderTarget ) ) {
Copy link
Collaborator

@Mugen87 Mugen87 May 22, 2024

Choose a reason for hiding this comment

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

I'm not sure why the if checks were different in the renderTarget.depthBuffer && ! renderTarget.stencilBuffer branch compared to renderTarget.depthBuffer && renderTarget.stencilBuffer. Why did you choose the below pattern?

if ( isUseMultisampledRTT ) {

	multisampledRTTExt.renderbufferStorageMultisampleEXT( _gl.RENDERBUFFER, samples, glInternalFormat, renderTarget.width, renderTarget.height );

} else if ( isMultisample ) {

	_gl.renderbufferStorageMultisample( _gl.RENDERBUFFER, samples, glInternalFormat, renderTarget.width, renderTarget.height );

} else {

	_gl.renderbufferStorage( _gl.RENDERBUFFER, glInternalFormat, renderTarget.width, renderTarget.height );

}

and not this one:

if ( isMultisample || useMultisampledRTT( renderTarget ) ) {

	if ( useMultisampledRTT( renderTarget ) ) {

		multisampledRTTExt.renderbufferStorageMultisampleEXT( _gl.RENDERBUFFER, samples, glInternalFormat, renderTarget.width, renderTarget.height );

	} else {

		_gl.renderbufferStorageMultisample( _gl.RENDERBUFFER, samples, glInternalFormat, renderTarget.width, renderTarget.height );

	}

} else {

	_gl.renderbufferStorage( _gl.RENDERBUFFER, glInternalFormat, renderTarget.width, renderTarget.height );

}

Are they functional equivalent?

Copy link
Collaborator Author

@gkjohnson gkjohnson May 22, 2024

Choose a reason for hiding this comment

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

Yup! They're the same. I just thought it was easier to read. We can check using a table to make sure they get into the same branches.

Condition Layout 1

if ( isUseMultisampledRTT ) {

	// Branch A

} else if ( isMultisample ) {

	// Branch B

}

Table

isUseMultisampledRTT = true isUseMultisampledRTT = false
isMultisample = true A B
isMultisample = false A None

Condition Layout 2

if ( isMultisample || isUseMultisampledRTT ) {

	if ( isUseMultisampledRTT ) {

		// Branch A

	} else {

		// Branch B

	}

}

Table

isUseMultisampledRTT = true isUseMultisampledRTT = false
isMultisample = true A B
isMultisample = false A None

Condition Layout 3

if ( isMultisample && isUseMultisampledRTT === false ) {

	// Branch B

} else if ( isUseMultisampledRTT ) {

	// Branch A

}

Table

isUseMultisampledRTT = true isUseMultisampledRTT = false
isMultisample = true A B
isMultisample = false A None

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good! I definitely prefer the PRs approach.

@gkjohnson gkjohnson merged commit bb47c32 into mrdoob:dev May 22, 2024
12 checks passed
@gkjohnson gkjohnson deleted the textures-update branch May 22, 2024 13:53
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.

2 participants