-
-
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
Introduce storageStruct #29908
base: dev
Are you sure you want to change the base?
Introduce storageStruct #29908
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
const bufferName = nodeBuffers[i]; | ||
const struct = structs[i]; | ||
|
||
resultMap.set(bufferName, struct.structName); |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
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.
fixed: .map( part => part.replace( /&/g, '' ) )
instead of .map( part => part.replace( '&', '' ) )
fix lint
fix lint
fix lint
fix lint
fix lint
fix lint
fix lint
I had the review comment in the code but I removed it and am now posting it here because the comment leads to lint issues. I tested this with compute, vertex, fragment shaders. The custom structs are not always at the beginning. This is here now just for easier viewing. Since with TSL the user is not forced to choose a struct name as is the case with the ptr in wgsl, I have to think about how best to do this. When accessing the structs you have to know their names. A name must therefore be specified.
The user could choose exactly the same name as the struct name itself as a parameter name in the struct. This is not normally done, but it is allowed. So that this is not replaced, I would have to adapt the function. Without specifying a name, in the case of TSL the WGSLNodeBuilder would currently use its automatically generated name, e.g. NodeBuffer_554, NodeBuffer_558 and as a user you never see them and you would like to be able to use meaningful names. So I need a TSL specific way to specify names |
more elegant _getWGSLStructBinding
getCustomStructNameFromShader( source ) { | ||
|
||
const functionRegex = /fn\s+\w+\s*\(([\s\S]*?)\)/g; // filter shader header | ||
const parameterRegex = /(\w+)\s*:\s*(ptr<\s*([\w]+),\s*(?:array<([\w<>]+)>|(\w+))[^>]*>|[\w<>,]+)/g; // filter parameters |
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.
It's strange to me to find regular expression here. This shouldn't happen when code is generated from nodes, and not the other way around or reprocessed.
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 originally had the parser in mind that you also use for the pointers because it is part of the WGSLNodeBuilder. However, the parser does not recognize stageData.codes
as WGSL code, so it always thrown an error. At first I thought that this would only be the case with compute shaders because the -> void
expression in stageData.codes
is missing. _getWGSLComputeCode
doesn't have that. But even when trying to use vertex and fragment shaders, the parser slipped into the else expression because it didn't recognize the shaders from stageData.codes
as WGSL shaders.
So far stageData.codes
is not used anywhere. It was only created. In your opinion, does the code from stageData.codes
have to be compatible with the parser?
I then concentrated on reading out only the header, i.e. the part in the ( ), since the rest is not important. That seemed to me to be the safest and most stable way. If you see a better way to extract the datas from the shaders I welcome your idea. I originally wanted to forego the function entirely and do it with the parser.
Is the process clear from the review text?
@sunag what do you think about adjusting the declarationRegexp?
This also affects the vertex and fragment shader. The declarationRegexp is only used by the parser. Therefore it is not critical to change it this way and I also could use it. Even if it were a small change and I could include it directly in this PR, perhaps an extra PR for it would be appropriate for better traceability? |
It is certainly better to create a new PR for |
1e3063b
to
d704d92
Compare
I set this to review. I asked @RenaudRohlinger if he could kindly help me translate the compute shader in my codePen into TSL, but that's probably not such an easy thing, as it's currently only possible with storageObject. There are also special drawIndirect commands for Metal on Macs. The extension to TSL is easier with a functional wgsl version without risking interferences from storageObject. |
Would you have a minimalist example of |
I prepared this as an example: I will now use this to create an example with a struct for the drawBuffer. The advantage of this example is that it shows the use of drawIndirect, storageBuffers, compute shaders, structs and atomic, so pretty much all the basic aspects are combined in a very clear, short example. But I'd like to do an extra PR for this as I've only done one example so far and could use some practice creating examples. I'll do that tonight. With additional buffers such as an instance storage buffer, which entries will also set in the compute shader, you can then control exactly which instances the vertex shader should process. But that would be another example. |
When testing existing examples, I noticed that some of them no longer worked because of my extension. The cause is a very banal mistake that I will correct. During the isArray check at the beginning where the bufferSnippets are created, I did not ensure that the input variables existed. With this all the examples run.
|
@sunag I added the example but I still don't understand the screenshot topic. The E2E test only works if there is hardly anything visible in the screenshot. But the moments where you can see a lot would be better in my opinion. For TSL, a struct name choice wouldn't even be necessary, if I understand correctly, since you don't need a pointer. |
@sunag what do you think of the current status? Can it be merged? For Macs, Atomic apparently requires Metal-specific adjustments in the shader. I can test this with this extension with someone who has a Mac like renauldRolinger. |
Sorry for the delay, in the next release I will dedicate myself to this. I think having |
Introduce storageStruct
This is still a draft as there is still a bit to be done but it is already fully functional in wgslFn and for compute shader.
Of course the whole thing also has to work for Fn and I still have to test it. Just like for vertex and fragment shader. The work for this essentially lies in the name management of the structs, which still needs improvement, and the flow adjustment for the vertex and fragment shader (small but neccessary things)
The struct mechanic itself works exactly as I imagined and this means that complex structs (single and array structs) as well as atomic can be used. I have already tested this extensively.
I'm open to suggestions, but in the meantime I'll move on.
I will also create a small example for this. Basically I have already prepared this on CodePen
https://codepen.io/Spiri0/pen/QWePNeL so that I only need to integrate the struct but here too I would like to have the shaders in Fn.
@sunag I closed the old PR because it went in the wrong direction from the start