Skip to content
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 includes sans global hook #13198

Closed

Conversation

pailhead
Copy link
Contributor

@pailhead pailhead commented Jan 29, 2018

Bug with onBeforeCompile:
#13192

Original discussion around onBeforeCompile:
#11475

Original pull request for this feature:
#10791

This is a revamped PR with the minimum set of changes needed to mimic the onBeforeCompile. I've removed a "hook" (empty shader chunk) from all the templates which removed the massive amount of files being touched.

This doesn’t have to eliminate onBeforeCompile, but I looked into the bug and it didn’t seem obvious how to fix it.
It would make it easy to add code to the shader, but for most materials it can be done by using uv_pars...

changes the api from onBeforeCompile

https://codepen.io/anon/pen/KQPBjd

myMaterial.onBeforeCompile = shader =>{
  shader.vertexShader = shader.vertexShader.replace('#include <common>', myCommonSnippet)
}

vs:

https://codepen.io/anon/pen/rJNgYZ

myMaterial.shaderIncludes = { begin_vertex: myBeginVertexGLSLSnippet } 
myMaterial.shaderUniforms = { foobar: { value: myValue } }

The bug can be worked around, but it feels as an additional hack (and more code) to already hacking up the shader string. Literally hacking, hacking code, hacking shaders, hacking them to pieces 🗡️ 😀

Without extra hooks though, it's not easy to prepend a shader for example, but it reduced the size of the PR.

next steps?

Adding a global hook, maybe with different syntax could be the next small diff and a step to adding more. The uniforms and chunks could be grouped in own class that could even make something like auto injection work. For example with this:

class ShaderModifier{
   constructor({ vertexUniforms, fragmentUniforms, chunks }){
     this.vertexUniforms = vertexUniforms
     this.fragmentUniforms = fragmentUniforms
     this.chunks = chunks
   }
}

One doesn't have to worry about writing uniform type FOO; in GLSL at all. Attributes could also be added with {value,type} and again would be injected in the best place (below extension calls, guaranteed above all the functions etc.). It would make this less awkward. Maybe even functions could make sense.

comments

For the sake of brevity, the numbers in the comments are blocks:

  1. (WebGLRenderer) if a user provides a uniform in the shape of {value, type} i was thinking the renderer could inject the GLSL automagically. This entire block could be 2 lines of new code, and two lines of changed code (4 total). Or it could totally be removed because it may not be the best idea. (will include the same uniform in both vertex and fragment). TL:DR; we dont need this

  2. (WebGLRenderer) The same way all the other "known" uniforms are being refreshed, we can refresh the custom ones. AKA if MaterialFoo has a .bar property that is to be uniformed, it can also have a .uniforms property that is to be... uniformed :D

  3. (WebGLProgram) Modifies the parser. Similar to material.defines i think? This is nothing more than saying "this function can look in another place for chunks not just THREE.ShaderChunk.

  4. (WebGLPrograms) Hash with injected chunks?

Without the examples only 3 files are changed. It's basically 5 lines of addition if you don't count the closing bracket, and some aliasing for the sake of brevity. 4 changed lines.

pailhead and others added 24 commits February 10, 2017 16:25
adds optional shader chunk overrides per material instance
a simple vertex deformation injection,
works with Object3D matrix,
doesn’t do normals correctly,

needs uniform hookup for animation
+ example
mrdoob#10750

Allows for adding stuff to commons chunk for example.
Add vertex_global and fragment_global chunks to the material templates
Add custom uniform injection in WebGLRenderer
@pailhead pailhead mentioned this pull request Jan 30, 2018
12 tasks
@mrdoob
Copy link
Owner

mrdoob commented Feb 27, 2018

I'm having a hard time to figure out an approach that is not a hack to implement this over the current material system. The #11475 hack was included instead.

As I proposed in a previous meetup, I think we should study how to implement this on top of MeshNodeMaterial instead.

https://threejs.org/examples/?q=nodes#webgl_materials_nodes

@mrdoob mrdoob closed this Feb 27, 2018
@pailhead
Copy link
Contributor Author

pailhead commented Feb 27, 2018

@mrdoob any particular thought about this approach? The node approach seems really good, but it seems like a big overhaul (making all the default materials out of this?), in the meantime onBeforeCompile could be revisited.

@mrdoob
Copy link
Owner

mrdoob commented Feb 27, 2018

I think the problem of all these approaches is that it's forcing WebGL concepts into the API. I like the NodeMaterial approach because it hides all that while still providing you with the required hooks.

Or at least that's what I want to think it does 😇

@pailhead
Copy link
Contributor Author

pailhead commented Feb 27, 2018

I think the problem of all these approaches is that it's forcing WebGL concepts into the API.

How so? I think the problem with onBeforeCompile is that it's not configuration over convention (?). I have to think about the string, i have to think about the compile moment. Given a string, i have to modify it and return it to the same place.

Existing chunks don't feel that much different than nodes. Why can't there be a state in between onBeforeCompile and node materials until node materials are done? I'm generally counting the time from last february :) even if @sunag's material makes it in tomorrow, we could have had a poor man's version of that much sooner.

Injecting glsl:

SomeNodedMaterial.someFunction = THREE.FunctionNode(rawGLSL)

vs (almost the same?)

SomeCurrentMaterial.someGLSLFunction = rawGLSL

vs

someCurrentMaterial.onBeforeCompile = shader => shader.fragmentShader = rawGLSL + shader.fragmentShader 

@pailhead
Copy link
Contributor Author

pailhead commented Feb 27, 2018

The Node material example is 2000 lines unfortunately. I think this was a deterrent for me to get deeper into it, but i think i have some idea of what's going on.

var mask = new THREE.SwitchNode( posNorm, 'y' );

var colors = new THREE.Math3Node(
	colorB, //i imagine these get automatically injected as uniforms
	colorA, // ^^
	mask,   // this one looks like a result of a function?
	THREE.Math3Node.MIX
);

Could be today (very very similar)

myMat.glslFunctions.push(rawGLSLforSwitchNode)
myMat.uniforms.colorA = new THREE.Vector3() //has name, type
myMat.uniforms.colorB = new THREE.Vector3()

But (unfortunately for me):

myMat.onBeforeCompile = // <- i dont care when it gets compiled, 
   shader => { // <- i don't need to know about the entire shader
      myMat.fragmentShader = 'uniform vec3 colorA;\n' + myMat.fragmentShader // <- have to repeat fragmentshader twice, have to format the string 
      myMat.fragmentShader = 'uniform vec3 colorB;\n' + myMat.fragmentShader //<- out of the box i have to do this for every uniform

       myMat.fragmentShader = rawGLSLforSwitchNode + myMat.fragmentShader 

I really strongly feel that if you do give this a tiny bit of thought, you will realize that it's not much different than the node approach, just simpler and available now :)

@sunag
Copy link
Collaborator

sunag commented Feb 27, 2018

colorB, //i imagine these get automatically injected as uniforms

If it is a ColorNode you are right.

I really strongly feel that if you do give this a tiny bit of thought, you will realize that it's not much different than the node approach, just simpler and available now :)

Your solutions are good at the primary level or as programmer view, already imagined adapting this in graph language like this? Where vars convertions and optimization should be automatically.

https://www.youtube.com/watch?v=SRTI0dCm0Qo

sc_switchnode

Expressions a + b are bad for artists and very bad if having to learn a programming language. The simple fact of declare uniform is bad.

@pailhead
Copy link
Contributor Author

pailhead commented Feb 28, 2018

I’m looking at this from a programmers perspective. The visual editor is impossible to beat from an artists standpoint.

My question was:

As is, does this approach seem more in line with yours compared to onBeforeCompile

All I want is something useful and programmer friendly. I'm not currently thinking of the artist. Nor am i looking for boundless creativity that comes from being able to write shaders visually without code, just less verbosity.

So:

Your solutions are good at the primary level or as programmer view, already imagined adapting this in graph language like this?

Definitely, but for now do you think that this solution could work, at least for non artists?

@sunag
Copy link
Collaborator

sunag commented Feb 28, 2018

Definitely, but for now do you think that this solution could work, at least for non artists?

I think @mrdoob concern is add new features that will soon fall into disuse. Instead because you do work in an isolate class like your ShaderModifier instead of change the core?

@pailhead
Copy link
Contributor Author

pailhead commented Feb 28, 2018

I guess. I give up, waiting on your implementation and hope it comes soon :)

In the meantime, If you have suggestions on ShaderModifier class I’m all ears, I’m not sure how to overcome the hashing issue.

@@ -2063,6 +2077,17 @@ function WebGLRenderer( parameters ) {

}

//2. same pattern as refreshUniformsFoobar
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pailhead
Copy link
Contributor Author

pailhead commented May 7, 2018

Amongst the many things this would solve and make easier, it seems some bugs with the gltf loader are amongst them:

#11573

@mrdoob

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants