-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
Feature Request: OnBeforeRender on Material #21305
Comments
I was going to suggest that... This whole |
I haven't because I will most likely just add my entire GLSL in one node 😄 Also Right now I like how my shaders are clearly separated and I just use includes. Maybe I should take a closer look at I wish I could just do it on the meshes but that's not that easy, because I just don't know in advanced where my material will be attached. It's fragile I agree, but right now what's happening is that there is a custom path basically for the default materials. Ideally I think it would be nice to be able to do exactly the same thing with any custom material, including having a way to modify transforms / camera. |
@sunag how do you think this could be done with |
Yes. This already is possible in the new
You can update a three.js/examples/jsm/renderers/nodes/accessors/ModelNode.js Lines 39 to 55 in ea11dda
Is possible
|
I can implement a solution for |
I had a look a bit at the Node system today. I still need to connect myself the mesh to the update though no? I don't see how that would work automatically. Where is the EDIT: Ok my bad, now I understand the code to hook that up is only for WebGPU right now |
I made a example, the color is updated per |
What do you mean with "material instance"? |
A single material for multiples meshes. |
If replace |
Okay, after investigating this issue a refactoring of how bindings are managed is required. Right now, the binding and the actual data are handled in a single object. In the below example, three.js/examples/jsm/renderers/webgpu/WebGPUUniformsGroup.js Lines 22 to 23 in cb99e7e
If a material is shared among multiple 3D objects, there is only one instance of bindings objects and thus only one data storage. This is not a problem for textures (because textures are defined on material level) however model related data like the MVP matrix can't be processed this way. They have to be managed per 3D object and the UBO needs to be updated per object with the correct data. I need some time to think about a possible solution for this... |
@sunag let me know if you need any help for the WebGL version (or even WebGPU). |
@Mugen87 I found the problem, is related with the implementation of |
I'm curious about your PR 🤓. This can also be fixed when multiple I'm waiting now with my PR until I've seen your change. |
I mean in case of just update the uniforms with the new |
Ah okay, I only focus on WebGPU in this topic^^. |
I did not do anything particular... I just made |
If this works, it's a good start! Fell free to file the PR 👍 I'd like to find out if it is possible to further optimize the renderer and material system by only having different bindings if required. However, this will take a while and some experimenting. I guess we need similar logic anyway if objects should be able to share render pipelines. |
Any news on the WebGL side? I am available few hours a week to help on that 😄 |
@DavidPeicho In context of WebGL, we have to solve #21117 first. |
Any news here now that #21117 is solved? |
If the transform is unscaled, this may help you avoid passing the inverse transform to the shader: |
@WestLangley thanks for the link but it was just one example out of several. It can happen that some data comes from the mesh and isn't easily accessible. |
The idea is to apply the pattern from the following WebGPU example to https://threejs.org/examples/webgpu_instance_uniform However, this requires a deeper integration of the node material into three.js/examples/jsm/renderers/webgpu/nodes/WebGPUNodes.js Lines 44 to 61 in 8837fac
|
Right now, it's required that the app includes a certain file (WebGLNodes). However, I'm thinking about a different approach based on dependency injection. Meaning if users want to use the node path of |
Thanks for the info. If I sum up, right now we simply need to inject the same method that would allow to do the update? Because the build looks already injected by WebGLNodes. Otherwise the other type of injection sounds nice. Something like I recently tried to start new shaders using the node system, but I have to admit it's a bit difficult and time consuming to use in WebGL in its current state. |
@DavidPeicho Look like we have a WebGL version for that :) #22504 |
That's amazing news, thanks! I guess now I will have to convert my custom materials to the node system |
|
Unfortunately, you can't use https://jsfiddle.net/03xm49dp/
|
I agree with stopping the support for WebGLRenderer+Nodes (regulated by the |
Closing. With the removal of the node material integration from Please use |
Is your feature request related to a problem? Please describe.
Let's say I have a custom material that is shared by two meshes. This material uses the inverse transform of the mesh. In this case, I have no way to send the inverse transform because it's dependent from the actual bound mesh.
When creating an app with Three.js, it's not really an issue because we control the flow of data. However, when wrapping Three.js in another library, it makes it somewhat difficult to share a ShaderMaterial whose parameters aren't independent from the camera / mesh drawn.
Describe the solution you'd like
On the top of my mind:
onBeforeRender
callback on the material, that would be in charge of updating per-object uniforms. This is similarto the
onBeforeCompile
callback, but called each time something is rendered with this material.Describe alternatives you've considered
onBeforeRender
on each meshes. Annoying because all meshes using this material must be updated and a user would need to attach the callback himselfAdditional context
This is highly related to the issue #16922. I think the problem is similar.
The text was updated successfully, but these errors were encountered: