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

NodeMaterial Revision #1

Merged

Conversation

sunag
Copy link

@sunag sunag commented Sep 1, 2019

In order to simplify I would like to propose some changes.

Root nodes like NormalNode, ReflectNode and CubeTextureNode we can put in your external interface only globals parameters, like in NormalNode: VIEW, LOCAL and GLOBAL and try prevent CLEARCOAT, BENT or others characteristics of PBR to that makes this interface a bit more simple to learn and make this a bit more compatible with others nodes like PhongNode, since that this can be added in context.

Other important thing is the anisotropy.code and your sequence. In this case, It is not a complex shader graph, but in others case variables are declared for optimization or other custom nodes is generate in anisotropy.code. anisotropy.result return only inline(final) results.

The sequence of analyze and flow is very important to generate the correct structure of the code, all analyze should be executed before of first flow, and follow the exact sequence. For example:

this.roughness.analyze( builder );
this.metalness.analyze( builder );

// this should follow the same sequence of the analyze, metalness after roughness for example
// any flow should be after all analyze be executed
var roughness = this.roughness.flow( builder, 'f' );
var metalness = this.metalness.flow( builder, 'f' );

sheen.code and sequence I revised in mrdoob#17265

@DanielSturk
Copy link
Owner

Looks great, but I don't think we should be replacing viewNormal with the bent normal in the environment context. Since the bent normal is used only for IBL, I would just add a second bentNormal alongside viewNormal

@sunag
Copy link
Author

sunag commented Sep 5, 2019

Hmm, what do you think of iblViewNormal or in shader iblNormal? seems more suggestive to me since we own iblIrradiance too.

@sunag
Copy link
Author

sunag commented Sep 5, 2019

I will review this detail. 👍

@DanielSturk
Copy link
Owner

iblViewNormal is fine, I do prefer something with bent and normal in the name though, since bent normal is the universal term

@sunag
Copy link
Author

sunag commented Sep 13, 2019

I add ReflectNode in material.environment input and the result seems the expected

carbonMat.environment = new Nodes.ReflectNode( Nodes.ReflectNode.CUBE );

If I add a context isolate to IBL ReflectNode not will work with anisotropic effect without the texture.uv context.

I think that at the moment we could leave it like this?

image

@DanielSturk DanielSturk merged commit 4b23aef into DanielSturk:mainline-anisotropy Sep 16, 2019
@DanielSturk
Copy link
Owner

@sunag For some reason I thought contextEnvironment.viewNormal would affect the direct light's viewNormal, but obviously it only affects the environment, so replacing viewNormal is fine. Thank you for your contribution. Merged.

@sunag
Copy link
Author

sunag commented Sep 17, 2019

@DanielSturk thank you too for your great work.

@sunag sunag deleted the dev2-mainline-anisotropy branch September 17, 2019 14:24
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