-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
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
WebGPURenderer: Introducing an IndirectStorageBufferAttribute
#29594
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
fixed lint issue
fixed lint issue
fixed lint issue
fixed lint issues
fixed lint issues
fixed lint issues
fixed lint issue
@@ -0,0 +1,17 @@ | |||
import { BufferAttribute } from '../../core/BufferAttribute.js'; | |||
|
|||
class IndirectStorageBufferAttribute extends BufferAttribute { |
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.
This should not be extended from StorageBufferAttribute
? I think that way we wouldn't need so many checks.
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.
We could write indirect = false as standard in the constructor of StorageBufferAttribute and if we pass true, isIndirectStorageBufferAttribute would be triggered. This would allow us to forgo IndirectStorageBufferAttribute.
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.
I think extending the class is better for now.
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.
I take care of it tomorrow. Now I urgently need to go to bed. Thank you for taking the time to look at this
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.
No rush, take your time, thanks for your work :)
I think should included
|
IndirectStorageBuffer
for drawIndirect
@sunag for the passEncoderGPU we will need drawIndirect and drawIndexedIndirect but I think we can keep it analogous to draw and drawIndexed as I suspect you imagine. |
I think we just need to detect if the buffer is indirect and directional to a new function Would you have a minimal example for us to test? It would be very valuable. |
A solid use case could be object-culling in our Excellent progress has already been made by @aardgoose on this topic in #29372. |
I won't find time again until this evening (European time). But I'm looking forward to being able to devote myself to the topic again after work. |
@@ -2,14 +2,18 @@ import { BufferAttribute } from '../../core/BufferAttribute.js'; | |||
|
|||
class StorageBufferAttribute extends BufferAttribute { | |||
|
|||
constructor( array, itemSize, typeClass = Float32Array ) { | |||
constructor( array, itemSize, indirect = false, typeClass = Float32Array ) { |
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.
Hmm.. I thought extends IndirectStorageBufferAttribute
from StorageBufferAttribute
. #29594 (comment)
Changing the constructor signature can be confusing, and having a class to handle indirect buffering can even help with reading the code among some other minor optimizations.
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.
Agreed. should remove the changes in StorageBufferAttribute and simply create a new file IndirectStorageBufferAttribute
:
import { StorageBufferAttribute } from '../../core/StorageBufferAttribute.js';
class IndirectStorageBufferAttribute extends StorageBufferAttribute {
constructor( array, itemSize, typeClass = Float32Array ) {
super( array, itemSize, typeClass );
this.isIndirectStorageBufferAttribute = true;
}
}
export default IndirectStorageBufferAttribute;
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.
ah ok, on this occasion I recommend as a distinguishing criterion to use
"this.isIndirect = false;" in StorageBufferAttribute.js
"this.isIndirect = true;" in IndirectStorageBufferAttribute.js
The reason I did this is because I didn't like the duplication of StorageBuffer.js and NodeStorageBuffer.js. I first doubled this because StorageBuffer.js had no distinguishing criterion which is important for later queries.
This means that with isStorageBufferAttribute in both classes ( StorageBufferAttribute / IndirectStorageBufferAttribute ) we have the opportunity to query anywhere in the code whether it is a storageBufferAttribute and with isIndirect whether it is an indirect storageBuffer. This makes things a lot easier because unnecessary duplication is not necessary in all places where it is only necessary to check whether there is a storageBuffer and / or indirect.
@sunag I don't have a simple example at the moment because I'm testing in my virtual geometry environment. But I have something in mind. The simplest possible example would be an instancedGeometry in which individual instances are randomly made invisible or visible over time with a sin function and a threshold value of 0.5 in the compute shader.
I looked at your reference to the draw function. I'll have to take a closer look at this because so far I've focused on the part with the visibility test. I think it makes sense if the geometries as renderObjects carry the drawBuffer as an attribute if you want to use drawIndirect. This makes it easy to distinguish between a mixture of many geometries that use draw and others that use drawIndirect.
@RenaudRohlinger Thanks for your link, it helped me understand renderBundles. I think if we get both combined in the end we can pop the corks
Suggestions for improvement are welcome
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.
ah ok, on this occasion I recommend as a distinguishing criterion to use
"this.isIndirect = false;" in StorageBufferAttribute.js
"this.isIndirect = true;" in IndirectStorageBufferAttribute.js
The @RenaudRohlinger example already does this just once it has the isIndirectStorageBufferAttribute=true
variable in IndirectStorageBufferAttribute
, this will always be missing (undefined) in StorageBufferAttribute
.
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.
@Spiri0 I made some revisions and I'm going to merge it up, I'll see if I implement these encoders separately, I want to see if I make a simple example too so we can have them in the official examples.
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.
I like it, it became pretty beauty in the end. As a user, I was in awe of tackle so many critical parts of the node system. The topic helped me to understand it much better. It is a very impressive construction.
I will now take a closer look at the custom structs that I have in mind. The big advantage of this will be instead of e.g. 30 individual bindings only need 4 or 5. This reduces overhead, pools cache and makes the shader more compact.
As for the encoder topic. I had looked at the geometries class in the node system because I had in mind to pass the drawBuffers as an attribute with the geometries as a renderObject, but it seams you have a better solution in mind 👍
IndirectStorageBuffer
for drawIndirect
IndirectStorageBufferAttribute
Related issue: #29568
This PR introduces the indirectStorageBuffer class. This uses the WebGPU buffer flag INDIRECT to create DrawBuffers. These DrawBuffers can then be filled in compute shaders and later read by drawIndirect. In this way, the visibility check is moved from the CPU to the GPU.
currently produces the following wgsl scheme ( Numbers and names are relative, the example here is from my test app)
The aim is this scheme here (names are again relative, the node system does this and connects them with the clear names that you choose as a user, I'm concerned with the struct):
The reason why I didn't implement this right away is because a lot more is necessary for the visibility check in the compute shader like camera and frustum elements: ( cameraProjectionMatrix, inverseCameraProjectionMatrix, cameraViewMatrix, cameraNear, cameraFar, cameraPosition, frustum, screenWidth, screenHeight ) and handing these all over individually to a shader seems not the best way to me. It is more elegant to store the matrix elements, vectors, scalars in an f32 storageBuffer and pass them to the compute shader and then have all these elements together with a struct in the shader:
For this I imagine a custom struct builder. Because in addition to the camera and frustum, the ModelWorldMatrices and some mesh informations are also important. Since this requires some changes in the wgsl node builder, this is step two. With these steps and furthers it will be possible to keep the visibility check completely universal in the GPU side and to use the full potential of drawIndirect and drawIndexedIndirect.