-
-
Notifications
You must be signed in to change notification settings - Fork 36.1k
WebGPURenderer: ShaderMaterial support prototype #20319
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
Conversation
cec16c2 to
2328247
Compare
2328247 to
26e0683
Compare
Um, I'm not sure about this. I was hoping we could go for a pure node based approach and focus the renderer's architecture on it. I think we should give @sunag a chance to implement this in a very basic manner. Especially since there is no need to rush with the
I have the feeling we can implement the JS logic for stuff like background and post-processing with temporary material definitions like we do now. The problem is that with a new |
What do you think of? |
|
I'm not very experienced with Yes, it is a paradigm shift but I believe it is the more versatile solution than starting over with I'm not sure how @mrdoob and the others feel about this but I guess this is a good opportunity to discuss it 👍 . |
|
BTW: I'm going to study your PR in detail over the weekend^^. |
I am a big fan of the |
Yes, as @Mugen87 already proposed in the first PR, better to focus on |
|
@takahirox In any event, I see some bits of the PR we still might want to adapt to |
|
Thanks for the comments. Good to know that
Thanks. Even with node based material system, it finally needs to be compiled to raw shader code. So partly some codes or ideas in this PR can still be usable for example parsing and analyzing uniforms of shader code. And
this Anyways, I'm willing to explain my code if you couldn't get. Let me know anytime. |
|
|
||
| } | ||
|
|
||
| function parseUniformsInternal( shader, isVertexShader ) { |
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.
Ugh, I need better name.
|
|
||
| } | ||
|
|
||
| return attributes.sort( function ( a, b ) { |
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.
Let's do the review step by step^^.
This is very good suggestion which makes the vertex buffer setup more robust. Code like:
layout(location = 1) in vec3 position;
layout(location = 0) in vec2 uv;
now works too.
I've applied your code in dfe538e. However, it was necessary to use slot instead of shaderLocation (since it is undefined on attribute level).
|
To advance in NodeMaterial I think that we need create a Something similar to that let builder = new ShaderBuilder();
let uv = builder.getInput( 'v2', 'uv' );
let position = builder.getInput( 'v3', 'position' );
console.log( uv ); // layout(location = 0) in vec2 uv;
console.log( position ); // layout(location = 1) in vec3 position;This automatic manager the inputs of the shader, it is part of NodeBuilder. |
|
In this case, |
|
|
||
| } | ||
|
|
||
| updateByType( uniform, offset ) { |
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 was also a good inspiration! 😉
I've refactored the UBO updates based on your code here: d5c3089
The idea is to encapsulate each uniform type into a new class (check out WebGPUUniform.js) which encodes the byte length and item size. Besides, setValue() can be used to set the actual value of the uniform at runtime.
This means it's now nicer to compute the total byte size of the UBO and the offset value required for writing at the correct position in the buffer can be computed automatically.
|
Did you adopt all the code you want? If so I close this PR. |
Yes. I'd like to wait for a basic node material before investing more time in this part of the renderer. In any event, your code was a big help in pushing |
|
I'm inspired by the API in #20390 and I locally made The changes are just +11 LOC. In user code, set the following properties of then you can use your custom shader. See webgpu_shader.html for more detail. The advantage is you can test custom shader code without polluting WebGPU classes. It would be good for testing until node based material system lands. Live demo: https://raw.githack.com/takahirox/three.js/WebGPUCustomShader/examples/#webgpu_shader If you are interested in, please locally take this change. |
You might like to wait for node base material for WebGPU and/or WGSL, but let me share my prototype work because I want to hear your folks opinion and feedback.
This draft PR adds
ShaderMaterialsupport toWebGPURenderer.Background
We would like the usage of node based materials right from the beginning #20254 (comment). I agree with that, and we maybe shouldn't add new material support now.
But some basic features 3D graphics engine should have require some freedom of writing special shaders for them, for example textured background and post-processing.
We can't start to work on them until node based material system for WebGPU lands. But it may be a bit of unclear when it does. If we support
ShaderMaterialwe can move such basic features support forward.And even if node based material lands I think there will still be some demands that user wants to directly write their shader codes.
ShaderMaterialsupport can satisfy it.Plus, currently node based material is compiled and it is handled as
ShaderMaterialin (webgl) renderer. WebGL/WebGPU needs raw shader code so this design may not be so bad. If we do the same approach even in WebGPU renderer, we needShaderMaterialsupport.So I think adding
ShaderMaterialsupport into WebGPU renderer now would be ok to me. What do you think?API
We may need to brush up when node based material system for WebGPU lands.
Update: I noticed that @Mugen87 made UniformBufferObject support PR #15562 before (it hasn't been merged yet). Using it may be more right way?
Sample video
https://twitter.com/superhoge/status/1304299232182980608
Implementation note
I add inline.