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

Consider refactoring some of the code. #14031

Closed
wants to merge 9 commits into from

Conversation

pailhead
Copy link
Contributor

@pailhead pailhead commented May 9, 2018

Hi, i propose an api that could help refactor and remove a lot of code from some of the examples and loaders and such. It's similar to onBeforeCompile, completely opt in and doesn't conflict with any existing functionality.

@donmccurdy ?

@pailhead pailhead changed the title Material includes cleanup Proposal to reduce the amount of boilerplate required when extending built-in materials May 9, 2018
@pailhead pailhead changed the title Proposal to reduce the amount of boilerplate required when extending built-in materials Small proposal with refactoring demos (remove 160 lines and onBeforeRender from GLTFLoader) May 9, 2018
@pailhead
Copy link
Contributor Author

pailhead commented May 11, 2018

(Optional read)

Goal

Convenient, concise, declarative and robust interface to tweaking the GLSL code in the built-in materials (MeshPhongMaterial, MeshStandardMaterial etc.). With the broader goal to open doors for developers to write robust 3rd party modules for three.js.

import makeFancyMaterial from 'make-fancy-material'

const myMaterial = makeFancyMaterial( new THREE.MeshStandardMatarial() )

myMaterial should be expected to behave as any other material while having the fancy effect persist. Any conflicting behavior should be documented within a defined framework, eg:

If you intend to use this package (fancy) with the module cool, you need to set the workWithCool flag to true.

Current state of the world.

Monkey patching

This basically comes down to mutating the THREE.ShaderChunk dictionary. It has to be wrapped with #ifdef FOO statements to branch into the desired extended effect. An example is provided here.
#12977 (comment)

Pros
This actually works for the most part, but Material.defines is private (undocumented?) and is nuked with every clone (or construction). It wouldn't actually clone, but having a list of define keys in a dictionary is a decent structure to manage how certain extensions could be combined.

Cons
It may not be the best practice to mutate an object such as THREE.ShaderChunk?

onBeforeCompile callback

This is a simple extension with the power to transform any shader in any way possible by providing the raw shader string before being parsed by three, and before being compiled by WebGL.
An official example exists.

Pros

  • provides access to Material's underlying raw shader string, the string can be manipulated in any way.
  • it's three lines of code

Cons

  • Can affect which Mesh uses which material in the cache, producing behavior that one may perceive as a bug (however it is not, it's by design).
  • The function should not have any branching logic ie. no if, else foo ? bar : baz string templates etc. unless one can work with the feature [BUG]: onBeforeCompile hashing #13192
  • It's very imperative
  • It defers logic that is otherwise synchronous with various Materials including the ShaderMaterial
    this is asnyhconous
    shader.uniforms.time = { value: 0 };

    causing a need for a check (material's interface is just available without the check):
    if ( materialShader ) {
  • It's very verbose, a lot of boilerplate. Complicated to solve because writing a loop would cause the [BUG]: onBeforeCompile hashing #13192 feature so it would randomly work with only one material in the scene.
  • It couples a material's instantiation with three's pre compilation step (parsing) and the first renderer
    under usual circumstances this is all that is needed to create a whole material (it doesn't rely on the next render to lazy declare a property)
const myMaterial = new AnyMaterial({foo:'foo'})
myMaterial.foo //'foo'
myMaterial.color // THREE.Color

const shaderMaterial = new ShaderMaterial(params)
shaderMaterial.uniforms.uFoo.value // THREE.Vector4
  • It is complicated to sub class materials
  • It's complicated to clone materials [Enhancement] onBeforeCompile may break material's .clone() method #14009
  • There is anecdotal evidence that attempting to do either sub classing or cloning may create a rip in the time-space continuum causing a massive black hole to be created that will swallow us all. [Enhancement] onBeforeCompile may break material's .clone() method #14009 (comment)
  • Behavior (like cloninng and [BUG]: onBeforeCompile hashing #13192) is not well documented.
  • It's redundant. It's possible that it's not desirable to have side-effects in the callback and it's working on a relatively simple shader object which has a uniforms dictionary and two strings (possibly some flags as well, and maybe defines). This is the same interface already available in ShaderMaterial synchronously. The WebGLRenderer already knows how to work with this interface.
  • It defers and obscures the THREE.ShaderMaterial interface
  • regular expressions can be hard to work with
  • could be dangerous (exposes a callback that can change the lowest level shader code directly to the user)
  • blackbox, once the instructions are written in the function, there is no way of knowing whats in it
  • it only causes side effects (mutates the shader object, not initially available, created by the Renderer?)

Copying templates

Another common approach present in some examples and possibly on Stack Overflow, is to just copy the underlying shader template of a desired built-in material, and recreate the whole thing as a THREE.ShaderMaterial with appropriate adjustments / extensions.

An example can be seen in the GLTFLoader:
https://github.com/mrdoob/three.js/blob/dev/examples/js/loaders/GLTFLoader.js#L576

Also in the lambert instancing example:
https://github.com/mrdoob/three.js/blob/dev/examples/webgl_buffergeometry_instancing_lambert.html

Pros

  • Academic value of some examples - it demonstrates how templates work with various Materials and the general idea behind extending and tweaking the underlying GLSL code.
  • Robust

Cons

  • verbose, requires referencing or copying the templates
  • THREE.ShaderMaterial has a different interface than built-in materials, certain properties that are primitives or references to classes like THREE.Color or THREE.Texture all have the shape of a Uniform [name]: { value: }, care needs to be taken to rewire these back into a familiar interface
  • One may be tempted to try to couple the extension with the rendering cycle like in the example of the GLTFLoader. But this is redundant because THREE.WebGLRenderer already does this automagically, and it is inconvenient because it may not clone, or just generally takes up the onBeforeRender which could be used for something else, not exclusively for keeping the material in sync.

Proposal

Achieve built-in material shader enhancements extensions by:

  • leveraging code in THREE.WebGLRenderer
  • reducing redundancy
  • concise syntax
  • declarative API
  • exposing a powerful interface to the advanced user, but obscuring it safely from the average user
  • providing helpful documentation
  • preserving the meaning of the Material one wants to extend (if it quacks like a duck its a duck, not only if it has an accompanying onBeforeRender)

Pros

  • declarative
  • concise
  • easy to document
  • reuses code
  • flexible
  • not opinionated (allows you to write a ternary operator or an if else statement)
  • reduces boilerplate
  • easy to use
  • easy to modularize, could allow the community to build shader extensions and publish on npm for example
  • available since early 2017
  • it doesn't conflict with onBeforeCompile (one can opt to use onBeforeCompile if one wants)
  • scales well, we could give more meaningful names to chunks and break them up further
  • treats validated shader code as data ie. a regular user can just rely on "put that thing there" and work with a simple dictionary, a power user can write the shader code

Cons

  • not NodeMaterial
  • more than three lines of code
  • it doesn't conflict with onBeforeCompile (onBeforeCompile doesn't have to be removed)

Ask

Since it doesn't conflict with anything, could it be available for a little while before NodeMaterial comes in so users can choose to opt in? If someone prefers an imperative approach, is good with regex and likes the way materials get hashed, they could use onBeforeCompile. For scalability and a declarative interface one could try to use this proposal. The two wouldn't have to conflict.

Myself, i'd prefer more modularity, maybe in the (near?) future when NodeMaterial replaces all, i could still use the old shader templates, and either this or onBeforeCompile?

Even as a rough example, the net gain is 224 lines removed from just a couple of examples. I believe there could be much more.

@pailhead pailhead changed the title Small proposal with refactoring demos (remove 160 lines and onBeforeRender from GLTFLoader) Consider allowing users to extend GLSL in built-in materials. May 11, 2018
@pailhead pailhead changed the title Consider allowing users to extend GLSL in built-in materials. Consider refactoring some of the code. May 11, 2018
cleanup
add uniform stage destination
@pailhead
Copy link
Contributor Author

pailhead commented May 15, 2018

Here's an example of using a generic onBeforeRender wrapper to do some stencil management. It's a hack, for illustrative purposes only (doesn't work well on these models and overall). Shows a slight tweak of the specular gloss material. It adds a simple halo around the clipping area.

It's just a simple example, but the interesting thing i think is that it can be written with no knowledge of how the GLTFLoader works. The effect (simple halo) is agnostic of how PBR chooses to do it's lighting, and can apply to both. onBeforeCompile could also liberate the onBeforeRender but it would make further extensions complicated. (this effect would work metal/rough not on the other).

With the material includes prosal, both callbacks would be free when used with GLTFLoader.

http://dusanbosnjak.com/test/webGL/gltf-without-onbeforerender/index.html

gltf_onbeforerender

With this one, using this approach can help draw the lines easily.
gltf_clipping

@mrdoob
Copy link
Owner

mrdoob commented May 21, 2018

As discussed last week, this PR has too many changes at once 😕

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.

2 participants