-
-
Notifications
You must be signed in to change notification settings - Fork 36.2k
StorageTextureNode: Add TSL read/write support #32734
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
base: dev
Are you sure you want to change the base?
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
|
Can you revert the indentation changes in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR streamlines storage texture read/write operations in TSL (Three.js Shading Language), enabling ping-pong compute shader patterns without requiring raw WGSL code. It addresses three core issues: clone not preserving access modes, incorrect textureLoad signature for storage textures, and inability to pass storage texture nodes to textureStore.
Changes:
- Fixed
StorageTextureNode.clone()to preserve theaccessproperty, enabling.load()to work correctly on storage textures with read access - Updated
WGSLNodeBuilder.generateTextureLoad()to omit the level parameter for storage textures, matching WGSL specification requirements - Enhanced
textureStore()to accept existing StorageTextureNode instances, allowing pre-configured nodes with specific access modes to be passed directly
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/nodes/accessors/StorageTextureNode.js | Added access preservation in clone method and compatibility path in textureStore for accepting StorageTextureNode instances |
| src/renderers/webgpu/nodes/WGSLNodeBuilder.js | Modified generateTextureLoad to conditionally omit level parameter for storage textures per WGSL spec |
| examples/webgpu_compute_texture_pingpong.html | Converted from raw WGSL strings to pure TSL implementation demonstrating the new storage texture read/write capabilities |
| test/unit/src/nodes/accessors/StorageTextureNode.tests.js | Added tests verifying clone preserves access properties for READ_ONLY and READ_WRITE modes |
| test/unit/src/renderers/webgpu/nodes/WGSLNodeBuilder.tests.js | Added tests verifying generateTextureLoad correctly handles storage vs regular textures with level parameters |
| test/unit/three.source.unit.js | Registered new test files for StorageTextureNode and WGSLNodeBuilder |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import * as THREE from 'three/webgpu'; | ||
| import { storageTexture, wgslFn, code, instanceIndex, uniform, NodeAccess } from 'three/tsl'; | ||
| import * as THREE from 'three/webgpu'; | ||
| import { storageTexture, textureStore, Fn, instanceIndex, uniform, float, vec2, vec4, uvec2, ivec2, int, uint, NodeAccess } from 'three/tsl'; |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import uint is unused in this example. Consider removing it to keep the imports clean.
| import { storageTexture, textureStore, Fn, instanceIndex, uniform, float, vec2, vec4, uvec2, ivec2, int, uint, NodeAccess } from 'three/tsl'; | |
| import { storageTexture, textureStore, Fn, instanceIndex, uniform, float, vec2, vec4, uvec2, ivec2, int, NodeAccess } from 'three/tsl'; |
| * @param {?Node} [storeNode=null] - The value node that should be stored in the texture. | ||
| * @returns {StorageTextureNode} | ||
| */ | ||
| export const textureStore = ( value, uvNode, storeNode ) => { |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function documentation lacks clarity about its dual behavior: it can accept either a StorageTexture or a StorageTextureNode as the first parameter. When a StorageTextureNode is passed, it clones the node and updates the uvNode and storeNode properties, preserving the access mode. This important behavior should be documented to help API users understand when to use this pattern versus storageTexture().
| if ( value.isStorageTextureNode === true ) { | ||
|
|
||
| // Derive new storage texture node from existing one | ||
| node = value.clone(); | ||
| node.uvNode = uvNode; | ||
| node.storeNode = storeNode; |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new functionality in textureStore() that allows passing a StorageTextureNode (lines 261-266) lacks test coverage. Consider adding a test that verifies the behavior when value.isStorageTextureNode === true, ensuring that the access mode is preserved and the uvNode and storeNode are correctly updated.
| QUnit.test( 'generateTextureLoad omits level for storage textures', ( assert ) => { | ||
|
|
||
| const context = { | ||
| renderer: { backend: { compatibilityMode: false } } | ||
| }; | ||
|
|
||
| const storageTexture = { isStorageTexture: true }; | ||
|
|
||
| const snippet = WGSLNodeBuilder.prototype.generateTextureLoad.call( | ||
| context, | ||
| storageTexture, | ||
| 'testTexture', | ||
| 'uvec2(0, 0)', | ||
| null, // levelSnippet | ||
| null, // depthSnippet | ||
| null // offsetSnippet | ||
| ); | ||
|
|
||
| // Storage textures should NOT have level parameter (WGSL spec) | ||
| assert.notOk( snippet.includes( 'u32(' ), 'storage texture load should not include level parameter' ); | ||
| assert.strictEqual( snippet, 'textureLoad( testTexture, uvec2(0, 0) )', 'correct WGSL for storage texture' ); | ||
|
|
||
| } ); | ||
|
|
||
| QUnit.test( 'generateTextureLoad includes level for regular textures', ( assert ) => { | ||
|
|
||
| const context = { | ||
| renderer: { backend: { compatibilityMode: false } } | ||
| }; | ||
|
|
||
| const regularTexture = { isStorageTexture: false }; | ||
|
|
||
| const snippet = WGSLNodeBuilder.prototype.generateTextureLoad.call( | ||
| context, | ||
| regularTexture, | ||
| 'testTexture', | ||
| 'uvec2(0, 0)', | ||
| null, // levelSnippet - should default to '0u' | ||
| null, | ||
| null | ||
| ); | ||
|
|
||
| // Regular textures SHOULD have level parameter | ||
| assert.ok( snippet.includes( 'u32( 0u )' ), 'regular texture load should include default level parameter' ); | ||
| assert.strictEqual( snippet, 'textureLoad( testTexture, uvec2(0, 0), u32( 0u ) )' ); | ||
|
|
||
| } ); |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests for generateTextureLoad cover the basic cases without depthSnippet, but the code also has special handling for storage textures when depthSnippet is provided (lines 562-564 in WGSLNodeBuilder.js). Consider adding a test case that verifies storage textures also omit the level parameter when a depthSnippet is provided.
… pong example to TSL
ea44c29 to
17ae820
Compare
17ae820 to
b596349
Compare
Description
Streamlines storage texture read/write operations in TSL, enabling patterns like ping-pong compute shaders without raw WGSL.
Problems
Initially based on trying to implement a ping-pong type compute shader setup based on
examples/webgpu_compute_texture_pingpong.html, but using "pure" TSL, i.e. port the WGSL compute strings to native TSL methods. But:clone loses access:
.load()path calls.clone, which doesn't preserve access. Defaults to write-only.invalid
textureLoad()signature for storage textures: when passing a storage texture togenerateTextureLoad(), it does:...which is fine for regular textures but the WGSL
textureLoad()doesn't accept a levels parameter for storage textures.textureStore(): for this ping-pong type example it ends up being a lot more ergonomic to build out the read/write nodes first then pass them totextureStore()during compute. But out-of-the-box that doesn't work because end up with a full node for the value.Also, incidentally, I noticed while doing this that the original
webgpu_compute_texture_pingpongexample doesn't work in Firefox because naga complains aboutwriteTexnot being a global binding. More of a naga issue but using TSL sidesteps this entirely.Changes
One line update to storageTextureNode.clone to preserve access
Some checks in
generateTextureLoad()to match the WGSLtextureLoadsignature whenisStorageTextureis trueCompatibility path in
textureStore()to allow an existing storageTextureNode to be passed (without overwriting that node)Ported ping-pong example to use this pattern
Added some basic validation tests
Before
After
Unit tests pass, lints clean, etc. Example verified working in chromium and Firefox.
Can work around some of this
texture(tex, uv)+ NearestFilter + some normalization math but the PR method feels a lot more TSL-semantic.