-
-
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
Material: Added onBeforeCompile() #11475
Comments
It does seem a bit hacky to require custom string manipulation when WebGLProgram.js already does a lot of processing. There is a related use case, which is to add new #includes rather than overriding existing ones. You can do this today by modifying THREE.ShaderLib at runtime, but it feels equally dirty. Perhaps a nicer solution for both is to allow you to provide a custom ShaderLib for a material, that would override/augment the built-in chunks without manual string manipulation and without permanently clobbering them. That doesn't preclude an onBeforeCompile hook for other uses, but it seems more in line with what your example is trying to do. That said, shader composition based on raw includes is always going to be very fragile from one version to the next. If you don't mind including all the skeleton code (like export class MyPhongMaterial extends ShaderMaterial {
constructor({ color, radius, map, normalMap, emissiveMap }) {
super();
this.vertexShader = "...."
this.fragmentShader = "....";
this.uniforms = UniformsUtils.clone(ShaderLib.phong.uniforms);
this.isMeshPhongMaterial = true;
this.lights = true;
this.uniforms.time = { value: 1 };
//...
}
} In all cases, you have to rely on the internal organization of built-in shaders not to change too much from one release to the next. Each include is already a function in disguise, with an ill-specified signature. I suspect it is going to be much cleaner and maintainable on both sides to provide function-level rather than include-level overrides. |
Nice! I thought so too on february 10th when i made this pull request: It seems the only difference is that you're doing it through a callback? I tapped into the unused argument in the WebGLRenderer. I like using this for example to get threejs to work with normal maps, but I've linked some issues that seem like they would be trivial to solve with this. You can look at my branch and take this for a spin or check out this super simple example (just tests a custom deformation with lighting/shadows etc, and adds a few uniforms).
Exactly sums up what happens in: #10791 I wish i could write better descriptions :) I must admit that i don't know wth is going on here. I think i know someone who gave up contributing to three, didn't understand it then, but now i find it kinda frustrating. Frustrating mostly because i'm having a deja vu when i read the first paragraph:
This exact same thing happened with the Jeez, twice, with the same dev? I'm obviously doing something wrong here, but what? The first time around was the build files being checked in, i didn't know what to do, and it just got forgotten. But this time around i was on top of this pull request, there's a conflict right now but it's easily solved, were no conflicts for months. Don't get me wrong, i don't think it's vanity in the works here, i think i just want to run ideas by people and get feedback. @unconed it's obvious that you're familiar with the problem and you've probably tried different things. What may have caused an user like you to miss the other PR? I felt pretty good about the per material shaderlib, but i did find one function in the renderer that looked/compared entire shaders as strings to figure out caching, wasn't feeling all that great about that. Wish there was some feedback given... Was the example flawed? The head may make it much more obvious what is going on than my abstract spike ball, but the whole example works with shadows which covers more ground. It's heavy for some reason, but i think it's because sin/cos on many verts. |
I guess if you're starting over... Even though scene kit is absolutely horrid, this api is really nice: https://developer.apple.com/documentation/scenekit/scnshadable Albeit horribly documented, i can't find the part that's of more interest, but basically they have abstracted hooks at many different stages of the shader. |
Sorry for not being able to take care of that PR yet @pailhead. I guess the main advantage of my approach is that it only required 3 new lines. Having said that, I'll now spend some time studying yours and @unconed suggestions. |
This one definitely feels more elegant, only three lines. I was following a different pattern in the other one. Was going to say that it may be a bit more verbose, but not so sure. I guess the only advantage of the other one is that it was good to go a few months ago :) |
Sorry about that. The only thing I can think of is that probably I got overwhelmed. Maybe a long discussion or a complicated PR that would consume too much of my energies, so I decide to leave it for later and move to simpler PRs. It's not only you @pailhead. @bhouston has had a lot of PRs like this, even @WestLangley and @Mugen87. Again, is not that I don't like the PR, is that the PR requires some attention I can't offer at the time. A good example is the Instancing PR. I managed to find time to read through it, did my own experimentation and suggested simplifications. Hopefully I'll be able to revisit it soon. It's difficult to manage all this and give every PR the attention they deserve.
Now you mention it... Yeah, runs at 10fps on a new MacBook Pro when zooming in 😮 |
I think it's the shadows and the rand, sin cos and stuff, but yeah it looks like it takes too much of a hit. |
Either way, i'm surprised you pulled this off in three lines, i was focused too much on how material params get turned into uniforms and followed that pattern. The difference is that I provide an alternative It would be really nice to have this, i haven't worked with that many 3d engines, but unity and scene kit seem to have something like this. |
I guess one benefit from the export class MyMeshPhongMaterial extends MeshPhongMaterial {
constructor( parameters ) {
super( parameters );
this.onBeforeCompile = function ( shader ) {
shader.vertexShader = shader.vertexShader.replace(
'#include <begin_vertex>',
'vec3 transformed = vec3( position.x + sin( position.y ) / 2.0, position.y, position.z );'
);
};
}
}
var material = new MyMeshPhongMaterial();
material.color.setRGB( 1, 0, 0 ); // this still works Messing with #include <begin_vertex>
% vertex %
#include <morphtarget_vertex>
#include <skinning_vertex>
% transformed_vertex %
#include <project_vertex>
% projected_vertex % The replace code will become this: this.onBeforeCompile = function ( shader ) {
shader.vertexShader = shader.vertexShader.replace(
'% vertex %',
'transformed.x += sin( position.y ) / 2.0;'
);
); We'll then remove any hook that hasn't been used before compiling, of course. |
here's what it looks like compared to the per instance ShaderChunks //given some material
var material = new THREE.MeshNormalMaterial();
//and some shader snippet
var myShader = [
'float theta = sin( time + position.y ) / 2.0;',
'float c = cos( theta );',
'float s = sin( theta );',
'mat3 m = mat3( c, 0, s, 0, 1, 0, -s, 0, c );',
'vec3 transformed = vec3( position ) * m;', //and making assumptions about THREE's shader framework
'vNormal = vNormal * m;'
].join( '\n' ); #10791 same as material.shaderIncludes = {
begin_vertex: myShader,
//uv_pars_vertex: [
// THREE.ShaderChunk['uv_pars_vertex'], //this doesn't have to be
// "uniform float time;",
//].join('\n')
};
material.shaderUniforms = { time: { value: 0, type: 'f' || 'float' } }; //because this could just inject it in the right place (but needs type) It's only a dictionary of chunk names, and the uniforms object. The string manipulation here is more along the lines of "i want to reuse this chunk, and do something on top of it" this PR: material.onBeforeCompile = function ( shader ) {
shader.uniforms.time = { value: 0 };
shader.vertexShader = 'uniform float time;\n' + shader.vertexShader; //this feels hacky
shader.vertexShader = shader.vertexShader.replace( //this is more verbose
'#include <begin_vertex>',
myShader
);
};
It works the same in the other PR, so I think it doesn't matter how it's done.
If the
Then this one might make sense #11050 Another argument could be - assuming you know GLSL, assuming you know THREE's shader framework, you still have to be creative and think about where you inject what. I had to think a bit and look through most of the shaders to figure out that It would be better to go from something like this towards an abstraction, have a This PR, as is, seems like it's going in the opposite direction. On top of knowing where you need to tap in, you also need to be explicit and manipulate strings, repeating some of the work that the renderer already does for you. Also, if you ask another question in addition to the first one
you might find yourself doing more string manipulation than just prepending the uniforms to the entire shader. |
I don't think there is a way around that unless we over-engineer. The idea with We're opening a door for allowing built-in materials hacking, but I don't think we can provide proper "support". The user should be aware that chances are things may break. |
I tried to tackle it in the other PR. I added a dummy hook at least for the functions, varyings, attributes and uniforms. Lots of the GLSL logic already happens on structs, scenekit documented each variable (cant find the same documentation anymore though). It would probably need to be refactored as we go along but something along these lines: #ifdef PHASE_FOO
#include <shader_foo>
//someGlobalStruct.mvPosition = modelViewMatrix * myLogic( transformed );
//if there is glsl provided for phase "foo" document that it should operate on "transformed"
//and that it should return "mvPosition"
#end including <shader_foo>
#else
//default
#ifdef USE_SKINNING
vec4 mvPosition = modelViewMatrix * skinned;
#else
vec4 mvPosition = modelViewMatrix * vec4( transformed, 1.0 );
#endif
#ifdef PHASE_BAR
#include <something_like_this>
//mvPosition = myPostDefaultTransformationLogic( mvPosition );
#endif
gl_Position = projectionMatrix * mvPosition;
#endif Of course ifdefs and includes would proably not work like this. But i can see it being relatively straight forward with the other approach. We could add |
We already have the defines stuff available on every material. #10764 suggested to modify But the way this currently works, and is inherited by every material, it is supported - if you put stuff in it, it will affect the shader for that material. Furthermore, if you happen to define something that is already part of the shader library, it may break things. Not trying to be a smart-ass but trying to come up with an example. I mostly used defines with
I'm not entirely sure if it can be broken though, i'd have to try. With that being said, i'd like the option to break things, if it provides a lot of gain. With a pretty encapsulated and tiny GLSL snippet, i could easily change my version of three to render normal maps differently. Taking a normal, and transforming it to get another normal is super straight forward, unless we start doing computer graphics in a completely different way, i can't see this ever breaking :) Instancing was another example:
|
I'm happy to see that this features is finally coming to three.js :). Yes I also created a I like the simplicity of this PR but if we're going for a more structured/clean way to do things I'd prefer to have already a dictionary of hooks to replace with your code and a simpler way to add uniforms or custom code than just replacing strings. |
@fernandojsg any chance you could check out #10791, give feedback, it seems super similar to what you did except i've put the additional hook just outside of main (something like your "pre_vertex") and there's slightly more management. |
@fernandojsg I had forgotten about your PR 😚. I think your PR looks quite a lot of how I was starting to see this working. Subconscious? |
I'm glad the library is hackeable in that way, but we shouldn't use abuse features inernally for things that were not intended for. It's easy to end up with fragile code that way. |
Reading #7581 again... On top of e55898c we can add the THREE.ExtendedMaterial = function ( material, hooks ) {
material.onBeforeCompile = function ( shader ) {
var vertexShader = shader.vertexShader;
var fragmentShader = parameters.fragmentShader;
for ( var name in hooks ) {
vertexShader = vertexShader.replace( '%' + name + '%', hooks[ name ] );
fragmentShader = fragmentShader.replace( '%' + name + '%', hooks[ name ] );
}
shader.vertexShader = vertexShader;
shader.fragmentShader = fragmentShader;
};
return material;
}; Then we can do this: var material = new THREE.ExtendedMaterial(
new THREE.MeshBasicMaterial(),
{ vertex: 'transformed.x += sin( position.y ) / 2.0;' }
); |
So the question is, what hooks should we have?
|
And here for the rest of the materials :) |
|
Hmm, opinionated? How so? |
either not plural, or not uniforms at all |
What's the use case for "injecting" attributes? |
I didn't know you don't have to declare it. Still leaves varyings and functions? |
Good point. |
I think I wrote poorly. What I wanted to mention is
My question is just for the policy of |
If i forget about three.js and the whole rendering system, i think i see the point. As a generic object, if it has I still think that my proposal from above is a valid way to do this, as i've encountered the same problem in other areas (like sharing depth buffers between targets can also be solved with Instead of caching the function, which doesn't work, you would basically cache It also solves the problem of:
It has been a long time, people have used It's essentially a How you do it, i.e whatever the arbitrary code is, doesn't matter. It does not return anything, but it mutates Approaches that i suggest:
Given input like this that originates from inside the
nuke this:
Use this:
Wherever you would have
Have
|
The tl:dr of the stuff above is user does not care about the particular point in time when "compile" (parse) occurs. I think they care more about what happens, and what happens can be isolated to a handful if not a single use case |
I didn't expect (considered) the the use of |
I just got bitten hard by using onBeforeCompile. I have a shader helper class that allows me to make modifications to built-in shaders in a defined way. I use the onBeforeCompile method to do this. Apparently, like stated in the first comment, WebGLProgram uses onBeforeCompile.toString() as a hash. But since my function is generic (it substitutes parts of the vertex and fragment shaders with variables) onBeforeCompile.toString() doesn't look different for different shaders. This meant that all my different shaders got cached as the same program. An eval call and a uuid and now my functions all look different. Took forever to figure this out. |
That sounds very frustrating, sorry. 😞 If the modifications you needed to make seem like they'd make sense in the library itself please feel welcome to open issues, too. |
Would you consider re-opening existing ones? #13192 seems to be the same but it was closed 😿 It would be really nice if a work around was posted in there if it was deemed a feature. |
Hmmm...
^Would this work? I never used |
The introduction of Material.customProgramCacheKey() via #17567 ensures that developers now have the possibility that shader programs are not shared for materials modified via Please discuss other existing issues or enhancements in context of |
analyze rain particle shader for color effects applied via material.onBeforeCompile(); see reference here: mrdoob/three.js#11475
Hey everyone 👋 Read everything and I'm clearly not competent enough to be relevant in the technical implementation details. However, what I can do is give feedback as a (noob) Three.js user (coming from threejs-journey.com). String manipulation felt hacky to me when I discovered the technique. Reading this page, I see that it was a shared feeling from the start (cf two first posts here). The
mrdoob implementation example, for reference: var material = new THREE.ExtendedMaterial(
new THREE.MeshBasicMaterial(),
{ vertex: 'transformed.x += sin( position.y ) / 2.0;' }
); This along with some JSDoc to explicitly state which variable has to be modified ( In the meantime, another thing I can do is updating the doc so that people understand |
I believe that TSL should replace this. This may also be interesting, for the approach in general. https://news.ycombinator.com/item?id=40510468 if you really want to compose materials as before I experimented with this many many years ago: https://github.com/pailhead/three-refactor-chunk-material You don’t have to manipulate strings in onBeforeCompile, you don’t even need to use it. You just… well, compile, and you’re done with it, at the end of the day it’s a compiled ShaderMaterial. |
Oh yeah I've seen that as well: https://github.com/mrdoob/three.js/wiki/Three.js-Shading-Language#new Should I add it to the doc? |
Hm not sure about that, I took a glance and I couldn’t figure out what is added. You made a whole example? This was, for a while at least, the only documentation available: |
Not sure I'm following you here. Maybe I could reach out to you elsewhere so we don't clutter up this thread? |
In both of these cases you can just save a string to a file and load it like myshader.shader. You can thus do what three does, offline. You don’t need do upload one string, then use three to combine it, you can do this any way you want. For example with nodejs. I’m just trying to explain the drawbacks and gotchas of onBeforeCompile. If you look at the source of how the old Webgl renderer “compiles” shaders (it’s not really compiling) you can just copy that method and run it yourself. With node, with python, or just before you even load threejs in the browser. |
Sorry, I realized it still may be confusing. Three assembles “built-in” material when it first encounters them, when rendering. So at best, three will take a bit longer during the first rendering to “compile” all the materials. This can be done before you even try to render anything, before you even create a renderer. |
I might not have been clear enough in my first message, but I was not worried about performance at all.
I'm perfectly happy with using TSL though! In fact I love that it exists! |
Ah, I don’t think you’re the first to propose this lol |
I'm not! Never thought I was. But @mrdoob that I referenced and quoted in my first message probably was. I posted the message because I thought it was a really good idea which might have gone lost along the way. |
Does your thumbs up mean that it is settled and that it won't be implemented, then? |
We are trying to move away from the hacky string replacement approach. |
Got it! Added link to the new TSL approach in the doc with the PR #30118. |
Over the years, a common feature request has been to be able to modify the built-in materials. Today I realised that it could be implemented in a similar way to
Object3D
'sonBeforeRender()
.e55898c
WebGLPrograms
addsonBeforeCompile.toString()
to the program hash so this shouldn't affect other built-in materials used in the scene.Here's an example of the feature in action:
http://rawgit.com/mrdoob/three.js/dev/examples/webgl_materials_modified.html
Not only we can mess with the shader code, but we can add custom uniforms.
Is it too hacky?
/cc @WestLangley @bhouston @tschw
The text was updated successfully, but these errors were encountered: