Fix material rendering for CurveModifier

[mjurczyk]

Solves #20687 (related #20688)

Since I was losing my mind trying to figure out the connections of shader chunks for materials anyway, I found a second to fix the issue with curve modifier - it should now work with all materials. Tested on:

• MeshNormalMaterial
• MeshBasicMaterial
• MeshStandardMaterial
• MeshPhysicalMaterial
• MeshDepthMaterial
• MeshDistanceMaterial
• MeshLambertMaterial
• MeshMatcapMaterial
• MeshPhongMaterial
• MeshToonMaterial

1. Adding USE_ENVMAP was technically a solution, @WestLangley - since it unlocks this part of the basic material shader. But isn't necessary with this PR.

2. Typo fix (@AdaRoseCannon): I think this is not legal (docs, if I'm not wrong, it just sets the color to default and passes number to setValues):

three.js/examples/webgl_modifier_curve.html

Line 61 in c9ee476

const boxMaterial = new THREE.MeshBasicMaterial( 0x99ff99 );

3. There's no point 3, I'd just like to dedicate a single point on this list to the sanity of the person who will try to maintain these shader chunks in the future. :')

4. Now the main part:

Removed USE_ENVMAP.

Removed includes that may duplicate declarations from the modified shader.

Moved the modified shader code to the beginning of the shader void main and placed includes in the correct order - this fixes the issue with different materials not rendering (incl. MeshBasicMaterial and MeshNormalMaterial.)

The main body of the modified shader is untouched.

This substitution seemed to be the same as the original shader chunk. When commented out it does seem to change nothing, but I may be missing something - though if we'd put the values in right places new code looks just like the old one. 🤔

[Mugen87]

It seems webgl_modifier_curve_instanced is failing now. Do you mind having a look?

[AdaRoseCannon]

Thanks for making this PR I'll, take a look to see why it's not working for the instanced version.

…

On Tue, 17 Nov 2020, 10:09 Michael Herzog, ***@***.***> wrote: It seems webgl_modifier_curve_instanced is failing now. Do you mind having a look? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20691 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABAHSMSFXDOITENXAOI7PJTSQJDVVANCNFSM4TYHGNQQ> .

[AdaRoseCannon]

I don't think I know enough about instancing to put my finger on what the issue could be. I've dumped the compiled shaders and am looking at the diffs but nothing is jumping out at me yet.

Diffs: https://editor.mergely.com/EtzCNhtw/

[mjurczyk]

No worries, I think I know what the reason is - I'll take care of it in a moment.

[mjurczyk]

As I thought, project vertex wasn't 100% same as the replaced code - if included, that chunk adds an instancing matrix. Since instancing calculations for the modifier are done earlier (L155), this wasn't a good idea.

Also - reverted the example visuals to the original state (material colours.)

[WestLangley]

Adding USE_ENVMAP was technically a solution, @WestLangley

I am not sure what technically a solution means.

The renderer sets USE_ENVMAP when it is appropriate. Hard-coding the DEFINE in #20688 is improper.

[AdaRoseCannon]

Thank you so much! Tested it and it works.

Thanks also for spotting the material mistake on the handle boxes in the examples.

Would you also be able to fix the remaining new THREE.MeshBasicMaterial( 0x99ff99 ); in the instanced demo?

[mjurczyk]

@WestLangley, what I meant is that in this case it did achieve the result - with a small sideeffect of non-envmap materials using the envmap chunks when going through this modifier. But yes, you are right.

@AdaRoseCannon missed that one, done.

[Mugen87]

@mrdoob Independently of this PR, I think we should be more careful with adding example code that overrides/patches existing shader chunk code. In the past, I had a different view on this topic but in the meanwhile I've realized that such examples are very fragile. Especially when shader code/chunks are refactored. webgl_materials_envmaps_parallax is a typical example for such a demo.

Thanks to E2E, it's easier now to detect failures but I'm in general no fan of this kind of code injection anymore. I suggest to implement such demos with node material instead in the future. Or in context of CurveModifier just support node materials.

[mrdoob]

Thanks!

[mrdoob]

@Mugen87 I agree. This is definitely not pretty.

[AdaRoseCannon]

Yeah adapting the original code was a very fragile process. If it was in nodes it seems like it would be more resilient.