Refactor instancing lambert example using onBeforeCompile

[pailhead]

@mrdoob

Per our conversation i submit this PR. It shows how to use material glsl injection to reduce some of the boilerplate with alternate approaches.

Still i'd suggest using a dictionary to store the stuff in a known format (uniforms, attributes, functions, chunks).

This will be the first usage of onBeforeCompile outside the example. I'm not super comfortable about that because i think there should be an alternative way to do, so i left a comment pointing to another PR.

ping @WestLangley

Btw how can i squash all these commits that i dont even understand where are they coming from. Why is stuff before 6b34d6f showing up?

[WestLangley]

Actually, I am not in favor of the approach used in this PR (using injections and string substitutions) because the shader code is obfuscated; the modified shader is not readable. In the current listing it is.

Users learn from these examples.

[pailhead]

I stand somewhere in the middle on this. I referred to what you're describing as "academic value" - very basic series of atomic steps needed to create an effect or illustrate an algorithm. But per a water-cooler conversation i had with @mrdoob it sounded like he might be in favor of showing how to "properly" use the api.

My understanding was that, if there already exists an example of how to use templates, then why not give visibility to onBeforeCompile with a well suited example? (I don't know if another one exists though). The current webgl materials modified example is more of an artistic expression than it helps illustrate what kind of problems onBeforeCompile or shader injection in general could solve. This seems like a real world example, and a good usage of this particular API.

The extra 100 lines may make the example daunting, and harder to follow. With comments and proper documentation, a user would know where to look for templates and possible have a better understanding of how they work.

As is, i think the example could use more comments, perhaps explaining the following:

• why is a depth shader needed
• why is the instancing code needed
• why it needs to happen where its happening
• what is the preprocessor syntax, aka why #ifdefs

[mrdoob]

Maybe this is not the best example to add onBeforeCompile to actually. I'm hoping that the instancing examples will no longer be relevant as soon as we manage to introduce instancing in core.

[mrdoob]

Sorry about that!

[pailhead]

Could you elaborate more? The shader code here is not even glsl, it’s just an ordered list of these include statements.

[mrdoob]

Ideally, this example should just look something like this:

var geometry =  new THREE.TorusBufferGeometry( 2, 0.5, 8, 128 );
var material = new THREE.MeshLamberMaterial();
var meshes = new THREE.InstancedMesh( geometry, material );
var positions = new THREE.Curves.TorusKnot( 10 ).getSpacedPoints( 256 );
meshes.setPositions( positions );
scene.add( meshes );

[pailhead]

@mrdoob there are two different things outlined in the example IMHO:

1. how to use a webgl/opengl feature called "instancing" in real life.
2. how to modify existing shader templates to do more stuff.

onBeforeCompile illustrates how to do both better (with less code, closer to real life).

If you remove this example altogether in favor of the suggested API, then this example was a waste of resources. InstancedMesh has been proposed a year ago, and is used a lot as an npm module, a better use of resources would be merging InstancedMesh.

#10750

I would still use this for an example for onBeforeCompile even if InstancedMesh existed, it doesn't matter that it's specifically illustrating instancing in this case. This feature should be so powerful that theoretically there should be hundreds of different examples by now, but there are none.

tldr;

Ideally, this example should just look something like this:

You're talking about webgl-mesh-instanced not webgl-materials-instanced-lambert.

One is an example of how to use InstancedMesh the other is an example of how to write various different materials (instanced + lambert).

tldr2;

We probably don't want:
webgl-materials-instanced-lambert
webgl-materials-instanced-phong
webgl-materials-instanced-standard
webgl-materials-instanced-basic
...

We want:
webgl-materials-modified-but-not-with-that-twist-instead-something-useful-like-instancing-or-normal-maps-etc
instanced-mesh-with-any-material

@WestLangley

Users learn from these examples.

I'm confused, i'm not sure what they're supposed to learn. If this example should look like @mrdoob described, then it seems like users would learn something completely different.

[mrdoob]

I think this example was just to show how to use built-in materials with instancing with the current code.

If you remove this example altogether in favor of the suggested API, then this example was a waste of resources. InstancedMesh has been proposed a year ago, and is used a lot as an npm module, a better use of resources would be merging InstancedMesh.

Please, be mindful with your wording. People don't like to be told that they're wasting their time.

As per the InstancedMesh PRs... As far as I remember I think they still had unresolved issues?

[pailhead]

Sorry i didn't mean it like that but i feel that i am making a valid point. This is an example of how to do something that involves GLSL, what you are suggesting is an example of how to use a javascript class thats part of three.js.

What i meant by wasted time:

With InstancedMesh is there a reason for this example to stay?

yes - not a waste of time
no - ? (for me, if it were my example, it would feel like i wasted time)

With your suggestion, i see absolutely no reason for this to stay. Why would anyone do instancing like this, if it's 3 lines of code with what you presented.

If however this is re-purposed to show how to extend materials, with something useful, then this example could stay despite the availability of InstancedMesh.

---

As far as I remember I think they still had unresolved issues?

No, it never had any issues and worked fine since day one. It had some open questions as to how far the automagic should go (should you be able to change roughness across instances for example). It's downloaded roughly a 100 times a month off of npm.

People seem to use it fine, and surprisingly, are not asking for any features. I just like to think of the next steps when i code or design something, (i'm a chess player).

The issue may be with three's lifecycle though. The code submitted is not the cleanest if it has to work around the limitations of THREE.WebGLRenderer, with a line or two of modifications, the code becomes much cleaner, but also much harder to submit/review.

The npm module for example, has some weird design decisions, stemming from the lack of access to three's core, but it still works just fine. My whole goal with this proposal is to reduce these dependencies and the coupling.

If you have an environment set, you can get this up and running in 1 minute:
https://www.npmjs.com/package/three-instanced-mesh

fun fact:

today marks the anniversary of this npm module going live and just... working :)

[mrdoob]

With InstancedMesh is there a reason for this example to stay?

No. But until then, it's useful.

[pailhead]

Well, I suggest reconsidering this. For learning purposes it would make sense to keep an example like this, and introduce others. I see little reason to have an example for lambert alone, especially if InstancedMesh works with all the materials. With onBeforeCompile this example could (for now) illustrate this concept on all the materials as well. Peace.

[pailhead]

#14069

It's really hard to pinpoint what you guys want :)

@WestLangley @mrdoob

Consequently, rather than adding Lambert shader chunks into a custom ShaderMaterial (as in #14012), it is preferable to add custom instancing code into the built-in MeshLambertMaterial, instead.

It feels odd to modify a global object such as THREE.ShaderLib, but if that's the preferred way to achieve this from the quote, there's very little that can be argued there. Not sure what onBeforeCompile should be used for if it doesn't qualify for add custom instancing code into the built-in (material)

[WestLangley]

@pailhead We are all trying to work through this. We are considering a variety of techniques to see what the pitfalls are. Your contributions and experiments have been very illuminating. It is not always clear what approach is 'best'. Often that is a subjective decision.

All you and I can do is voice an opinion. I have learned to simply state my opinion and avoid protracted debates. @mrdoob will make a decision and we will move on. I don't take it personally if the decision is not my preference. I think that is good advice for anyone to follow.

Be patient, we will get there! :)

[pailhead]

But how do we figure out what's best without debates :D

[WestLangley]

We have some debates, but in general, you and I don't decide what's 'best'. @mrdoob makes that judgment call.

[pailhead]

@WestLangley

But @mrdoob doesn't have the time to even scratch the surface on these debates. When i got some of his precious times, it sounded like he was really in favor of this PR, i think your dismissal influenced his decision, a lot if not completely :)

I would really appreciate a brief discussion here with you, you seemed to have liked this way back in the day:
#10791

Yes, this PR allows for the replacement of shader chunks. It would be convenient to be able to do that on a per-material-instance basis.
It could also be useful when using InstancedBufferGeometry with built-in materials. In such a case, one may want to replace begin_vertex or default_normal, for example, with custom shader code.

Seems like you were in favor of this PR in two other places, but dismissed it?

Would an alternative example of instancing (not just lambert) with onBeforeCompile suffice? I think ill muster this up in the meantime.

Also what mrdoob mentions above is basically the api from https://www.npmjs.com/package/three-instanced-mesh and #10750. Not to offend anyone but i think apples and oranges are somewhat mixed here. I made the npm module to R&D and see what's needed to not have this coupled with the core. I suggested materialIncludes, but onBeforeCompile may work too. InstancedMesh was abandoned because of no further input. There were lots and lots of findings in this process.