WebGPURenderer: Read Only Storage Buffer Creation #28400
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cmhhelgeson
changed the title
Mugen87
reviewed
May 17, 2024
| @@ -13,6 +13,8 @@ class StorageBufferNode extends BufferNode { | |||
|
|
|||
| this.isStorageBufferNode = true; | |||
|
|
|||
| this.readOnly = false; | |||
There was a problem hiding this comment.
Please make sure to adhere the project's code style. This was already mentioned in #28379 (comment).
* BatchedMesh: add getColorAt and setColorAt * pr fixes and suggestions * bring back previous condition to recompile shader * add colorSpace to DataTexture * change instance to geometry in docs
mrdoob
reviewed
May 17, 2024
Comment on lines
7
to
13
| constructor( nodeUniform ) { | ||
|
|
||
| super( 'StorageBuffer_' + _id ++, nodeUniform ? nodeUniform.value : null ); | ||
| constructor( nodeUniform, readOnly) { | ||
| readOnly = readOnly ? readOnly : false; | ||
| const namePrefix = readOnly ? 'StorageReadOnlyBuffer_' : 'StorageBuffer_' | ||
| super( namePrefix + _id ++, nodeUniform ? nodeUniform.value : null ); | ||
|
|
||
| this.nodeUniform = nodeUniform; | ||
| this.readOnly = readOnly; |
There was a problem hiding this comment.
Apologies for the delay, hopefully formatting should be fixed now that I've manually checked each file as well as disabled a lot of the automatic spacing and indentation VSCode was applying to the files.
There was a problem hiding this comment.
This should also be true for pull request #28379 as well.
* revert mrdoob#28289 * log patch should only cover webgpu * prettify code
* NormalNode: Improve tree shaking using TSL * fix attribute default value
* Editor: Fix serialization of commands. * Editor: Restore order in `fromJSON()`. * Editor: Fix regression when adding/removing scripts.
…SL (mrdoob#28410) * TangentNode: Improve tree shaking using TSL * BitangentNode: Improve tree shaking using TSL * cleanup
* return z based on the the camera forward z value * Fix sort in example * Remove unused argument
* sync script editor title * no dispatching editScript, setValue is enough
Co-authored-by: aardgoose <angus.sawyer@email.com>
* BatchedMesh: default color to white * update comment
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Facilitates the creation of read-only storage buffers in tslFn shaders using a new storageImmutable() function. Although we can currently create storage, read_write array buffers within TSL functions using StorageBufferNode, we are currently unable to create read-only storage buffers within these shaders, which limits our ability to use these buffers in contexts where only read-only buffers are permitted (such as vertex shaders for instance)
The storageImmutable() function changes specific parameters within a StorageBufferNode object to specify to the WGSLNodeBuilder class and the WebGPUBinding Utils class how the storage buffer should be accessed. Currently, the read-only functionality is grafted onto the existing StorageBufferNode class using the new readOnly parameter, but if necessary, it can trivially be extrapolated out into its own class.
To test the code, I located code within webgpu_compute_geometry that was only being read, then changed the storage function to a storageImmutable function
When applying this change, there was no observable change to the visual output, and the console registered no errors. Additionally, I performed a diff on the WGSL code output by the TSL function, which returned no differences aside from the storage buffer being defined within the shader as a <storage, read> buffer rather than a <storage, read_write> buffer.
Currently, given that I am a new contributor, am unfamiliar with all the architectural preferences of this repository's maintainers, and have yet to test this functionality in vertex shaders and other edge case scenarios, I have elected to put the draft in PR. However, feel free to comment on any changes you would like to see.