Refactoring shader customization

[gjcope]

Based on previous discussion with @sdumetz on making Three.js updates easier/more sustainable I've made my 3rd attempt at refactoring the shader customizations here: https://github.com/Smithsonian/dpo-voyager/tree/dev-simplify-shader

The custom shader code was reduced, mainly by removing the AO customization and switching most of the cut plane features to rely on the native local clip plane functionality. The remaining custom code is now injected into the standard shaders on load.

I think this should be relatively sustainable as most of the code is injected at the beginning/end, but is still open to incompatibilities. It could be further reduced by moving Xray and Normal features to their own materials, but it's unclear if its worth it for the overhead (though small in this case) of shader switching.

Changes I'm aware of:

• AO is (as expected) definitely different. Needs more investigation into why it's so much different than comparable viewers. That being said, still seems better to stick with the stock functionality.
• The default cut plane color renders differently due to being shifted after other functions. I think it's actually better this way as it's not affected by light, tone mapping, etc. but would welcome feedback.

Any feedback/thoughts on the implementation appreciated.

[sdumetz]

Great!

AO does seems "lighter", but not necessarily in a bad way. It's a change but I don't think it will "break" anyone's scene.

The cut plane matches its property color much better for all colors I tested it with. So it should be easier to select a different color if needed.

On the implementation, I think it's the best way to do it. There is relatively few "cut points" and they are easy to understand.

You could make it easier to debug, but more verbose by using a method that would allow for no-match detection, like :

const cut_idx = shader.indexOf('#include <opaque_fragment>');
if(cut_idx == -1){
  console.warn("Couldn't find '#include <opaque_fragment>' in shader")
  return shader;
]
return `${shader.slice(0, cut_idx)}
  //Fragment
${shader.slice(cut_idx)}
`;

Not sure if the additional lines are worth the trouble, this will probably never happen. Most probable failure would be some reordering of includes or operations inside the fragment and we won't be able to catch this automatically.

[gjcope]

Thanks @sdumetz, I appreciate the feedback. I've updated https://github.com/Smithsonian/dpo-voyager/tree/dev-simplify-shader with a more aggressive approach that factors out the custom materials as well.

It may be too much, as it does have some drawbacks, but makes it much easier to support Three.js core material types. We have a push this year to improve PBR support/quality and I think this will streamline things.

The main issue I see is from the development side. Instead of custom material variables and functions being nicely packaged with the custom material, they are now distributed to where they are used. This requires knowing the custom defines and uniforms and how to access them. Perhaps this could be further simplified with some aliases on the material object or maybe grouping these things in a separate file/class. Any thoughts appreciated!

[sdumetz]

I'm wondering if it wouldn't be better to completely separate the different additions (wireframe / clay / Normal / cut) into their own material-extension and swap the material in place (~like the mat override thing). I imagine the performance gain to be marginal as when they are disabled we just evaluate a boolean, which is practically free. But in term of code readability and feature extensibility that might be worth it? Not sure how feasible it is?

For what it's worth, I don't find the last revision to be harder to read than the original code or the previous one.

[gjcope]

Thanks! I don't think it's terrible, but for someone jumping into the code for the first time I think it's harder to follow access pattern for the customization. I.e. "material.userData.shader.uniforms.zoneMap.value" instead of "material.zoneMap". But it's a tradeoff.

I think empirically it's better to not swap, but I also think the difference would be negligible here. I'm more concerned about the readability/extensibility. I think some sort of extension mechanism or at least bundling the customization together is feasible and would improve things but not sure if there is a lot of return on investment at this point.