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

support updating only dirty CompressedArrayTexture layers #27972

Merged

Conversation

HunterLarco
Copy link
Contributor

@HunterLarco HunterLarco commented Mar 22, 2024

Related issue: n/a

Description

ThreeJS's CompressedArrayTexture is useful to pack multiple compressed textures into a single sampler (making more efficient use of GPU resources). However, it's critical for performance that when making changes to the texture array, that the CPU only update subsets of the bound GPU texture. It turns out that ThreeJS does not currently support modifying individual texture array layers, instead it always flushes the entire texture array to the GPU on every change. This PR changes ThreeJS by adding new functionality to update specific layers individually resulting in much less data transmission during render and therefore better performance.

Case Study

My company SOOT, uses CompressedArrayTexture regularly with large numbers of 256x256 layers. We observed that if we needed to update N layers in a frame this would result in a command buffer issued to the GPU with N * byteSize(textureArray) when we expected N * byteSize(layer). This resulted in 16777216 bytes transmitted from CPU to GPU multiple times per frame instead of 65536. By authoring this change we observed a 99.6% reduction in command buffer size between the CPU and GPU and a 423% increase in framerate.

Implementation Strategy

I designed this change to be minimally invasive and not impact existing ThreeJS projects. The feature is gated by the new field layerUpdates which was not previously used by the codebase. As a result, if ThreeJS users don't call texture.addLayerUpdate(...) then this change has no impact.

This contribution is funded by SOOT

Copy link

github-actions bot commented Mar 22, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
674.7 kB (167.2 kB) 675.7 kB (167.4 kB) +1.07 kB

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
454.1 kB (109.6 kB) 455 kB (109.9 kB) +959 B

@HunterLarco HunterLarco marked this pull request as ready for review March 22, 2024 20:05
@RenaudRohlinger
Copy link
Collaborator

Awesome work! @HunterLarco

This enhancement appears to also be beneficial for non-compressed array textures (DataArrayTexture). Consequently, we should consider updating texSubImage3D to reflect this change.

Also what about using a private prefix and a Set() instead of an array?
texture.__layersNeedsUpdate = new Set() instead of texture. dirtyLayers = []

for (const layerIndex of texture.__layersNeedsUpdate) {

    const layerSize = mipmap.width * mipmap.height;
    state.compressedTexSubImage3D(_gl.TEXTURE_2D_ARRAY, 0, 0, 0, layerIndex, mipmap.width, mipmap.height, 1, glFormat, mipmap.data.slice(layerSize * layerIndex, layerSize * (layerIndex + 1)), 0, 0);

}

texture.__layersNeedsUpdate.clear();

// Apply the same approach for state.texSubImage3D

@RenaudRohlinger
Copy link
Collaborator

By the way threejs somehow already supports modifying individual texture array layers through copyTextureToTexture3D 😁

@HunterLarco
Copy link
Contributor Author

HunterLarco commented Mar 23, 2024

Thanks! I'm more than happy to use a private prefix and use a Set. I wasn't sure if private prefixes was in the ThreeJS style given that they aren't used much for other variables I'd expect to be private such as version. In fact, does it make sense for dirtyLayers to remain public for clients which want to manipulate the set themselves instead of using layerNeedsUpdate?

By the way ThreeJS somehow already supports modifying individual texture array layers through copyTextureToTexture3D 😁

My team was wondering about this option and had a question. If I create (but do not use in a material) a texture, does it get allocated in GPU memory? One of the benefits of the texture array is that we've already allocated the GPU memory and can subsequently write to it. Whereas copying from a different texture requires that we allocate new/additional memory. This may be good in some scenarios to mitigate texture locking, but not all.

@RenaudRohlinger
Copy link
Collaborator

RenaudRohlinger commented Mar 23, 2024

Ah you're right, it's usually using single _ rather than using __ (you can take BatchedMesh for reference). Let's use texture._layersNeedsUpdate = new Set() so it's still exposed to the users but not recommend as we have a new setter texture.layerNeedsUpdate = N for this.

Simply creating a texture shouldn't allocate in GPU memory. It would need to be used in a uniform somewhere in order to call uploadTexture and then allocate memory.

src/renderers/webgl/WebGLTextures.js Fixed Show fixed Hide fixed
src/renderers/webgl/WebGLTextures.js Fixed Show fixed Hide fixed
src/renderers/webgl/WebGLTextures.js Fixed Show fixed Hide fixed
src/renderers/webgl/WebGLTextures.js Fixed Show fixed Hide fixed
src/renderers/webgl/WebGLTextures.js Fixed Show fixed Hide fixed
src/renderers/webgl/WebGLTextures.js Fixed Show fixed Hide fixed
src/renderers/webgl/WebGLTextures.js Fixed Show fixed Hide fixed
@Mugen87 Mugen87 added this to the r164 milestone Mar 28, 2024
@mrdoob
Copy link
Owner

mrdoob commented Mar 29, 2024

There is no other instance of the word "dirty" in the library.
We usually use "needsUpdate" for these use cases.

@mrdoob
Copy link
Owner

mrdoob commented Mar 29, 2024

BufferAttribute has a similar API:
https://github.com/mrdoob/three.js/blob/master/src/core/BufferAttribute.js#L62-L72

@HunterLarco
Copy link
Contributor Author

Updated to reflect the previously established naming convention of "needsUpdate" instead of "dirty".

@HunterLarco
Copy link
Contributor Author

Apologies for the delay, a few things took my time at work but I now have time to land this.

@mrdoob mrdoob modified the milestones: r164, r165 Apr 25, 2024
@HunterLarco
Copy link
Contributor Author

Bump~ is there anything remaining on my end to help land this?

@Mugen87 Mugen87 merged commit b98f98a into mrdoob:dev May 4, 2024
12 checks passed
@Mugen87
Copy link
Collaborator

Mugen87 commented May 4, 2024

Sorry, for the delay! The PR looks good now and can be merged.

@HunterLarco HunterLarco deleted the hunter/update-individual-array-texture-layers branch May 7, 2024 17:15
@mrdoob
Copy link
Owner

mrdoob commented May 27, 2024

Hmm, any chance we could add an example for this? Or implement the feature in any of the existing examples?

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.

4 participants