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

WebGLRenderer: Enable subframe upload in copyTextureToTexture, align API to 3d version #28281

Merged
merged 17 commits into from
May 7, 2024

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented May 4, 2024

Related issue: -

Description

Adds a Box2 "bounds" argument to copyTextureToTexture so we can upload just a small portion of a texture.

renderer.copyTextureToTexture( box, position, src, dst, level );

Specifically my use case is to be able copy just a single pixel of an id texture to a render target so it can be read back to the GPU to identify the id at the given pixel (see the EXT_mesh_features Cesium glTF extension). With this change we can copy just a single pixel from a large source texture to a 1x1 render target and then read it back whereas previously we'd have to copy to a render target of equivalent size. As far as I know there's no other way to reliably read just a single pixel from an arbitrary image.

From my testing with reading single pixel from a 2048 x 2048 image this improves the upload and read time from ~10ms to ~2ms. If async read is used presumably the ~2ms would go down quite a bit further, as well.

Benchmark code
const quad = new FullScreenQuad( new THREE.MeshBasicMaterial( { color: 0x0000ff } ) );
const srcTex = new THREE.DataTexture( new Uint8Array( 2048 * 2048 * 4 ).fill( 255 ), 4096, 4096 );
srcTex.needsUpdate = true;

{

	const target = new THREE.WebGLRenderTarget( 1, 1 );

	// initialize target
	renderer.setRenderTarget( target );
	quad.render( renderer );
	renderer.setRenderTarget( null );

	console.time( 'READ' );

	// copy data
	const bounds = new THREE.Box2();
	bounds.min.setScalar( 0 );
	bounds.max.setScalar( 1 );
	renderer.copyTextureToTexture( bounds, bounds.min, srcTex, target.texture );

	// read data
	const result = new Uint8Array( 4 );
	renderer.readRenderTargetPixels( target, 0, 0, 1, 1, result );
	console.log( result );

	console.timeEnd( 'READ' ); // ~2ms

}

{

	const target = new THREE.WebGLRenderTarget( 2048, 2048 );

	// initialize target
	renderer.setRenderTarget( target );
	quad.render( renderer );
	renderer.setRenderTarget( null );

	console.time( 'READ' );

	// copy data
	const bounds = new THREE.Box2();
	bounds.min.setScalar( 0 );
	bounds.max.setScalar( 2048 );
	renderer.copyTextureToTexture( bounds, bounds.min, srcTex, target.texture );

	// read data
	const result = new Uint8Array( 4 );
	renderer.readRenderTargetPixels( target, 0, 0, 1, 1, result );
	console.log( result );

	console.timeEnd( 'READ' ); // ~10ms

}

This contribution is funded by the Cesium GIS Ecosystem Grant

Copy link

github-actions bot commented May 4, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
677.2 kB (167.8 kB) 678.4 kB (168.1 kB) +1.21 kB

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
456.3 kB (110.1 kB) 457.5 kB (110.3 kB) +1.21 kB

src/renderers/WebGLRenderer.js Outdated Show resolved Hide resolved
Comment on lines 2397 to 2427
_gl.compressedTexSubImage2D( _gl.TEXTURE_2D, level, position.x, position.y, srcTexture.mipmaps[ 0 ].width, srcTexture.mipmaps[ 0 ].height, glFormat, srcTexture.mipmaps[ 0 ].data );
_gl.compressedTexSubImage2D( _gl.TEXTURE_2D, level, position.x, position.y, image.width, image.height, glFormat, image.data );
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One other discrepancy I noticed between the copyTextureToTexture and copyTextureToTexture3D implementations is that the 3D variant used "level" to index into the source textures mipmap array whereas it was always 0 in the 2d version. I've aligned to what the 3d function was doing but am happy to change it either way.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 6, 2024

In general, I'm okay with this change. When adding copyTextureToTexture() I've originally wanted the signature as simple as possible. But I agree not having the option to define the source box is unfortunate.

Actually, I would prefer to have sourceBox as an optional parameter at the end of the signature but that would mean changing copyTextureToTexture3D() as well. Not sure this makes sense at this point...

@gkjohnson
Copy link
Collaborator Author

Actually, I would prefer to have sourceBox as an optional parameter at the end of the signature but that would mean changing copyTextureToTexture3D() as well. Not sure this makes sense at this point...

Either way the end result is the same: one function signature incurs a breaking change. If you'd prefer an optional argument now would be the time to do it, though the argument grouping becomes a little strange.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 6, 2024

I vote for:

copyTextureToTexture( position, srcTexture, dstTexture, level, srcBox );
copyTextureToTexture3D( position, srcTexture, dstTexture, level, srcBox );

I agree the grouping is not ideal but always asking the users to define the source box like in the example feels inconvenient. There are for sure use cases where users want to copy the entire source texture into the destination texture and that should work as easy as possible.

@gkjohnson
Copy link
Collaborator Author

Just updated the signatures and update both examples!

@@ -2371,10 +2371,25 @@ class WebGLRenderer {

};

this.copyTextureToTexture = function ( position, srcTexture, dstTexture, level = 0 ) {
this.copyTextureToTexture = function ( position, srcTexture, dstTexture, level = 0, sourceBox = null ) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think srcRegion is more clear and follows the rest of the parameter names.

@mrdoob
Copy link
Owner

mrdoob commented May 6, 2024

Or something like this?

this.copyTextureToTexture = function ( srcCoords: Vector2 | Box2, srcTexture, dstTexture, level = 0 )

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented May 7, 2024

Or something like this?

this.copyTextureToTexture = function ( srcCoords: Vector2 | Box2, srcTexture, dstTexture, level = 0 )

position represents the position to copy the data into in the destination while sourceBox represents the region to copy from the source texture.

We can change the naming to something like the following:

dstPosition, srcTexture, dstTexture, level = 0, srcRegion = null

Though in my opinion something like this might make the most sense. We can change to this with backwards compatibility for 10 releases:

this.copyTextureToTexture = function ( srcTexture, dstTexture, srcRegion = null, dstPosition = null, level = 0 )

@mrdoob
Copy link
Owner

mrdoob commented May 7, 2024

Though in my opinion something like this might make the most sense. We can change to this with backwards compatibility for 10 releases:

this.copyTextureToTexture = function ( srcTexture, dstTexture, srcRegion = null, dstPosition = null, level = 0 )

That looks much nicer indeed 👌

@gkjohnson
Copy link
Collaborator Author

Okay! It should be ready now

@gkjohnson gkjohnson requested review from mrdoob and Mugen87 May 7, 2024 09:21
@gkjohnson gkjohnson merged commit 3bc0cc7 into mrdoob:dev May 7, 2024
9 of 13 checks passed
@gkjohnson gkjohnson deleted the copy-texture-update branch May 7, 2024 10:17
@Mugen87 Mugen87 mentioned this pull request May 7, 2024
this.copyTextureToTexture = function ( position, srcTexture, dstTexture, level = 0 ) {
this.copyTextureToTexture = function ( srcTexture, dstTexture, srcRegion = null, dstPosition = null, level = 0 ) {

// support previous signature with source box first
Copy link
Owner

@mrdoob mrdoob May 7, 2024

Choose a reason for hiding this comment

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

Should be like this?

// support previous signature with dstPosition first

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yeah - copy paste error

Copy link
Collaborator

Choose a reason for hiding this comment

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

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