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

What are the plans regarding NodeMaterial? #14245

Closed
pailhead opened this issue Jun 8, 2018 · 11 comments
Closed

What are the plans regarding NodeMaterial? #14245

pailhead opened this issue Jun 8, 2018 · 11 comments
Labels

Comments

@pailhead
Copy link
Contributor

pailhead commented Jun 8, 2018

Over the years a common request from users has been the ability to modify built-in materials.

Built-in materials do a lot of stuff, but sometimes you only want to tweak a couple of lines of a giant shader, or inject some GLSL at some stage.

There has been a plethora of PR's and issues posted about this. Some are open, some have been closed:

#14232
#14231
#14206
#14166
#14099
#14098
#14031
#13198
#10791
#13446
#14009
#14011
#13192
#7581
#13364
#12977
#11562

At some point between late 2015 and today, this PR #7522 has been quoted as a blocker for some of these, if not all of these.

As is, #7522 is just another example from /example, until it's not moved to /src it shouldn't be treated as a first-class citizen. Why is an example (a very large one at that) blocking small PRs?

I see this as a problem, and I wonder if others do too.

@donmccurdy
Copy link
Collaborator

I don't think it's possible to answer this issue in the general case. Some of the PRs above are simply not maintainable solutions to the problem they try to solve, and while we point to #7522 trying to suggest paths forward, if #7522 did not exist those PRs would still not be the right approach. Others (e.g. #14099) are quite valid on their own, or as intermediate steps, and probably just haven't been merged because maintainers have limited time to review these changes. Or some combination of the two.

As is, #7522 is just another example from /example, until it's not moved to /src it shouldn't be treated as a first-class citizen. Why is an example (a very large one at that) blocking small PRs?

If you are looking for a strict rule that we should never suggest use of code in /examples instead of changes to /src, that is not realistically going to happen sorry.

@pailhead
Copy link
Contributor Author

pailhead commented Jun 8, 2018

If you are looking for a strict rule that we should never suggest use of code in /examples instead of changes to /src, that is not realistically going to happen sorry.

No, but i feel that this is currently very biased.

if #7522 did not exist those PRs would still not be the right approach.

I think the problem is there is a lack of discussion on this ^, ie. why is something the right approach and why not. If there is no explanation, pointing to #7522 and closing others is a really bad solution.

This for example is very non-informative.

This is subjective:

if #7522 did not exist those PRs would still not be the right approach.

Because, in another place, another person:

I think your approach is valid.

or #10791 (comment)

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.

Ie. i've not heard a single argument on why SOME approach isn't right. On the contrary, as you can see, people seem to have felt the opposite way.

Even you seem to be somewhere in the middle on this:

Decorators (e.g. #14206) certainly improve this from a user's perspective — but if future feature development of materials will be based on increasingly complicated decorators atop the existing material shaders, that's a maintainability hazard in my opinion.

It's your opinion as you have stated. I'm willing to deal with the maintainability hazard, in my own private code. I'd prefer that over something that i will never, ever use like #14239

@pailhead
Copy link
Contributor Author

pailhead commented Jun 8, 2018

If you are looking for a strict rule

I am looking for SOME rule :)

@Usnul
Copy link
Contributor

Usnul commented Jun 10, 2018

I feel there are a few themes here:

what is the material/shader model of THREE.js?

What is a material?
What are the basic parts of a material?
How are these basic parts combined?

what is the interface (API) for extension of default shaders?

Let's say I want to extend a standard shader to work with instanced geometry, what is the "right" way this should be done?

what are the the requirements for meterials/shaders in THREE.js.

This one is a bit of a cheeky addition from my side. It's not clear what are the things we want to achieve, and what are non-goals so to speak. Essentially: boundaries, rules, limitations etc. are not clear.

Not directly related:

One extra point which I feel is largely neglected that I would include is consideration of deferred shading model. Current material implementation does little to support deferred shading model which is by now industry standard and is enabled by WebGL2.

Yet another tangent, performance doesn't seem to be considered well. I have tried running three.js without modifications with a moderate number of materials being used on the scene (15 or so) where each is a PBR material with 4 textures of size 1024x1024 - and it's slow. I have considered undertaking a rewrite, but my estimation is it would take more time than I currently have, so I built a bunch of workarounds instead.

@pailhead
Copy link
Contributor Author

@Usnul i didn't mean to introduce material's to this, i just used it as an example :(

The point i was trying to make is more generic. Replacing materials/shaders with foo might do the trick :)

We can try to consider the Material system as an example though.

Let's say I want to extend a standard shader to work with instanced geometry, what is the "right" way this should be done?

The right way should be the way that is the most convenient for you, your app, your team etc. The emphasis being on something that belongs to you.

All the known, explored ways could be documented. If this wouldn't scale (this kind of documentation would belong on 3rd party blogs, and such) then the underlying mechanism for allowing some ways should be documented. In the case of materials, onBeforeCompile could be documented with everything that has been learned about it - i.e. how it effects cloning, hashing etc.

This one is a bit of a cheeky addition from my side. It's not clear what are the things we want to achieve, and what are non-goals so to speak. Essentially: boundaries, rules, limitations etc. are not clear.

A format needs to be figured out for this. Where is this information written to.

This comment applies to #14231, onBeforeCompile and preferential treatment of loaders:

...but if future feature development of materials will be based on increasingly complicated decorators atop the existing material shaders, that's a maintainability hazard in my opinion.

I think it's wrong to assume that anything would depend on onBeforeCompile, it's just a hook for you, the user, for your app that depends on three.js, Without something documented though, one can make this assumption. It would be good to be on the same page here, having everyone know what the expectations are.

Generally i'm looking for a pattern on how to better discuss these things, and how to better document them. It could still be issues, maybe with a different tag or something, but it's convoluted with a lot of other issues, like doc typos and such.

@Usnul
Copy link
Contributor

Usnul commented Jun 11, 2018

@pailhead

I have a bias when it comes to APIs and design in general. Especially for things that have a substantial audience. There are two ends of the spectrum:

  1. provide strict rules that rely on a well defined assumptions
  2. no rules, no assumptions

Strict rules might be so strict as to preclude certain usecases from being implemented. Assumptions might turn out to be invalid for some usecases. On the other hand - no rules is a breeding ground for chaos, quickly you end up maintaining 2-3 separate ways of achieving the same exact thing. No assumptions make it very hard to reason about your application, since everything is possible - it's hard to say what is going on without inspecting every little detail. Assumptions allow you to make your designs more simple and to optimize performance. The tricks, of course, is to make the "right" assumptions, and figuring that part out is not so easy.

My bias is towards the former. It makes your job a lot easier when going forward, and it makes your design a lot easier to understand for your audience.

@mrdoob
Copy link
Owner

mrdoob commented Jun 14, 2018

@pailhead do you mind renaming the title of this issue to something more descriptive?

@pailhead
Copy link
Contributor Author

@mrdoob

Open to suggestions. Ill try to think of something else in the meantime.

@Usnul

I think we agree on this too, i'd prefer the former.

@donmccurdy
Copy link
Collaborator

This issue is not so broad as something in examples/js blocking changes to src/. I and others have repeatedly given specific concerns about shader patches to the builtin materials as a means of, or replacement for, adding features to the library, and those concerns are independent of the possibility that NodeMaterial provides an alternative.

Unless there's another issue we'd prefer to track it in, I think this is solely about the preferred methods for extending and adding features to materials. A reasonable title could be "builtin material extensions" similar to some of the open PRs.

@pailhead
Copy link
Contributor Author

pailhead commented Jun 15, 2018

@donmccurdy

Can you please elaborate on where this assumption is coming from:

I and others have repeatedly given specific concerns about shader patches to the builtin materials as a means of, or replacement for, adding features to the library,

The PR that was blocked proposes adding feature shader patches to the library. It does in not, even a slightest way, propose that any results made by using this feature should in any way be related, merged, patched or added to the library.

No feature ever produced with a hypothetical shader patching feature of threejs should ever be used in three.js core. If users could contractually be obligated to use this feature only in their apps, i would force them to sign such a contract.

Any usage of any such feature (like onBeforeCompile) in those examples has no relation whatsoever to the core now, and should not have in the future.

If any example, using any feature of the library inspires a core modification to three.js no one should be held accountable for that. It would be seen as "at will" use or inspiration by an arbitrary example, but would receive no preferential treatment, nor block other PRs.

I apologize if i hadn't made myself clear in the past, reading your assumption with clarity, helped me better explain what has been the actual intent all along.

and those concerns are independent of the possibility that NodeMaterial provides an alternative.

You and others have voiced concerns from the context of GLTFLoader and NodeMaterial which are both /examples and in no way related to the core. Others, have voiced concerns that you're thus giving preferential treatment to GLTFLoader by making an /example into a first class citizen:
#14256 (comment)

To reiterate, a hypothetical shader patching feature, that has been proposed many times over the years, makes no assumptions that any core modifications other than the ones needed for the feature itself are to be made to three.js, now, nor ever. If the material system is to ever be overhauled, this feature can just silently be removed.

NodeMaterial on the other hand, makes much more far fetched assumptions, where it aspires to replace the entire material system of three.js, while being in /examples. The momentum for this overhaul seems to be generated by the needs of yet another example - GLTFLoader which is just one of many loaders that three.js has in it's examples.

@mrdoob mrdoob changed the title Non related PR blockers What are the plans regarding NodeMaterial? Jun 18, 2018
@mrdoob
Copy link
Owner

mrdoob commented Jun 18, 2018

Sorry for the confusion...

The fact that NodeMaterial is "blocking" all these PRs is because NodeMaterial feels like a good design (modular, tree-shackeable, extensible, serialisable, ...) design for materials.

The design of the current material system is hacky and hard to maintain. I would rather not add things that make it even harder to maintain on top. I would, instead, focus on exploring NodeMaterial.

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

No branches or pull requests

4 participants