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

Auto NodeMaterial build #14214

Merged
merged 18 commits into from
Jun 16, 2018
Merged

Auto NodeMaterial build #14214

merged 18 commits into from
Jun 16, 2018

Conversation

sunag
Copy link
Collaborator

@sunag sunag commented Jun 4, 2018

  • THREE.NodePass.build() is deprecated use needsUpdate = true instead.
  • THREE.NodeMaterial.build() is deprecated to update the shader use needsUpdate = true instead.

One more step towards the core.
https://rawgit.com/sunag/three.js/dev-nodeupdate1/examples/index.html?q=nodes#webgl_loader_nodes

@@ -278,7 +275,7 @@

// unserialize
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"deserialize"

@sunag
Copy link
Collaborator Author

sunag commented Jun 4, 2018

Wait... still missing a little revision in sprites.

@pailhead
Copy link
Contributor

pailhead commented Jun 4, 2018

I think i like this, the reasoning being there is no need to change needsUpdate to build(), and onBeforeCompile serves as a callback for onWillRefreshMaterial.

I advocate for more of these hooks with meaningful names to be in the core, so NodeMaterial won't have to.

@sunag
Copy link
Collaborator Author

sunag commented Jun 4, 2018

The code result programCache.getProgramCode( material, parameters ) should be after onBeforeCompile. I am having some geometryProgram conflits because of this.

var code = programCache.getProgramCode( material, parameters );

/cc @WestLangley @Mugen87 @mrdoob

@pailhead
Copy link
Contributor

pailhead commented Jun 4, 2018

onBeforeCompile should maybe be renamed to onBeforeParse

@pailhead pailhead mentioned this pull request Jun 5, 2018
@sunag
Copy link
Collaborator Author

sunag commented Jun 5, 2018

@WestLangley @Mugen87 @mrdoob

This approach is good for you guys? I can fix WebGLRenderer code issue?
This approach makes NodeMaterial an interface more similar those of the core.

@Usnul
Copy link
Contributor

Usnul commented Jun 5, 2018

@sunag

var code = programCache.getProgramCode( material, parameters );

this part also generates a lot of extra CPU load and garbage, as you basically end up generating 2 strings for the shaders, and for some usecases it happens an awful lot (at least once per frame per material).

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 5, 2018

The code result programCache.getProgramCode( material, parameters ) should be after onBeforeCompile.

So the problem is that code is identical even if you modify the shader program via onBeforeCompile, correct? If so, I suggest you apply a fix to WebGLRenderer. It should be an easy one.

This approach is good for you guys?

I'm not yet familiar with NodeMaterial but the general idea behind the change is good 👍

@Usnul
Copy link
Contributor

Usnul commented Jun 5, 2018

So the problem is that code is identical even if you modify the shader program via onBeforeCompile, correct? If so, I suggest you apply a fix to WebGLRenderer. It should be an easy one.

Not quite. That's part of it, but really, for me, the problem is that shader doesn't change at all, WebGLRenderer assumes it might have, those assumptions are bad, then to check those assumptions it constructs the code, which, wha-do-ya-kno' is exactly the same as before :)

It's not related to the commit here. Just an observation of mentioned code.

see more detail here: #14121

@sunag
Copy link
Collaborator Author

sunag commented Jun 5, 2018

Not quite. That's part of it, but really, for me, the problem is that shader doesn't change at all, WebGLRenderer assumes it might have, those assumptions are bad, then to check those assumptions it constructs the code, which, wha-do-ya-kno' is exactly the same as before :)

But if add a simple color instead map the coded is already changed is need update the program.

The idea used in approach is based on top this: #11475

@sunag
Copy link
Collaborator Author

sunag commented Jun 5, 2018

I still not find a good approach to fix WebGLRenderer without use two time getProgramCode.
I add UUID to fix this issue for now, only problem that will not instantiate programCode
(even being more difficult to happen with nodes).
I will leave this to another PR.

@WestLangley
Copy link
Collaborator

@sunag Sorry, but for now, I need to defer to others regarding Node. Thank you for all your work on this.

@sunag
Copy link
Collaborator Author

sunag commented Jun 6, 2018

@WestLangley No problems! 👍

@sunag
Copy link
Collaborator Author

sunag commented Jun 8, 2018

Good news. WIP MeshStandardNodeMaterial:

meshstandardnodematerial

I create a TextureCubeUVNode to this result and several node improvements. This is a proxy material candidate to replace MeshStandardMaterial:

There is still a slight difference in brightness that I will check.

Use: ( not need material.build() after this PR )

material = new THREE.MeshStandardNodeMaterial();
material.roughness = 1; // attenuates roughnessMap
material.metalness = 1; // attenuates metalnessMap

material.map = loader.load( 'Cerberus_A.jpg' );
// roughness is in G channel, metalness is in B channel
material.metalnessMap = loader.load( 'Cerberus_RM.jpg' );
material.roughnessMap = loader.load( 'Cerberus_RM.jpg' );
material.normalMap = loader.load( 'Cerberus_N.jpg' );

@sunag
Copy link
Collaborator Author

sunag commented Jun 8, 2018

// cc @bhouston

@sunag
Copy link
Collaborator Author

sunag commented Jun 8, 2018

I maintain this sequence or change to uvTransform per texture?

// uv repeat and offset setting priorities
// 1. color map
// 2. specular map
// 3. normal map
// 4. bump map
// 5. alpha map
// 6. emissive map

/cc @WestLangley @mrdoob

@bhouston
Copy link
Contributor

bhouston commented Jun 8, 2018

I maintain this sequence or change to uvTransform per texture?

We need a UVTransform per texture for proper glTF compatibility as well as proper FBX compatibility. /ping @donmccurdy

@donmccurdy
Copy link
Collaborator

There is a gap in the core implementation here, and if possible we should not repeat it in NodeMaterial. See #12788 for details, but the gist is that cloning a texture and assigning a new transform results in a duplicate texture upload. Even if that were fixed, I think it's better to represent the UVTransform elsewhere, at the per-map conceptual level rather than per-texture. There is not exactly a "map" concept in NodeMaterial today, but THREE.TextureNode contains a texture and a UV index so I think that's close enough. Putting the transform onto a THREE.UVNode might also be reasonable.

@bhouston
Copy link
Contributor

bhouston commented Jun 8, 2018

@donmccurdy I did solve that issue with my unmerged PR from 2016 that added multiple UV Transform support via the introduction of a "Map" between Textures and Materials: https://github.com/mrdoob/three.js/pull/8278/files#diff-cea5a42c1c15fd23dde252730d82e223 And then every texture in a material got a Map that specified the UV and Texel tansforms. I thought it was elegant and would encourage a similar structure going forward.

Better link: https://github.com/bhouston/three.js/blob/94290c538caffeac12f8c644d8d867907232f63a/src/materials/Map.js

@donmccurdy
Copy link
Collaborator

Yes, I believe that's the right idea, or whatever form it translates into with nodes. :)

@WestLangley
Copy link
Collaborator

I think it's better to represent the UVTransform elsewhere, at the per-map conceptual level rather than per-texture.

material.map is a Texture. Maybe you mean at the material level?

@donmccurdy
Copy link
Collaborator

I mean to say that a map should be distinct from both texture and material in the NodeMaterial system. Really that is already the case — TextureNode behaves as a map between material and texture by having both a texture reference and a UVNode reference.

@sunag
Copy link
Collaborator Author

sunag commented Jun 9, 2018

I maintain this sequence or change to uvTransform per texture?

Only considering that per texture is a uvTransform per map. For example:

vUvMap = ( uvTransformMap * vec3( uv, 1 ) ).xy;
vUvSpecular = ( uvTransformSpecular * vec3( uv, 1 ) ).xy;
vUvNormal = ( uvTransformNormal * vec3( uv, 1 ) ).xy;

This does not seem very optimized.
I was thinking of keeping the sequence and use nodes or values in same slots. Example:

// MeshStandardNodeMaterial a hybrid material, it works both with backward-compatible as nodes
var material = new THREE.MeshStandardNodeMaterial();
var texture = loader.load( 'Cerberus_RM.jpg' )

// not use uvTransform per texture but backward-compatible, avoiding many uvTransform
material.roughness = 1;
material.roughnessMap = texture;

// or use uvTransform in an specific texture in same material
var uv = new THREE.UVTransformNode( new THREE.UVNode(), new THREE.Matrix3Node( texture.matrix ) );
material.roughness = new THREE.TextureNode( texture, uv );

@donmccurdy
Copy link
Collaborator

I'm fine with whatever syntax you prefer; avoiding unneeded transform uniforms makes sense. A similar node graph in Blender would look like this:

40499183-06340f32-5f36-11e8-9551-dde7289c0a8f

@sunag
Copy link
Collaborator Author

sunag commented Jun 9, 2018

I'm fine with whatever syntax you prefer; avoiding unneeded transform uniforms makes sense. A similar node graph in Blender would look like this:

In fact is very similiar with my approach.

@pailhead
Copy link
Contributor

Hello fellow three.js aficionados!

Over the years, users have been confused about the design of this system.

Would you consider discussing the topic around this:

I maintain this sequence or change to uvTransform per texture?

Outside of the NodeMaterial PRs? These issues have been raised in the past and have a place for discussion:

#12788

Discussing them and making decisions here might obscure them from some people. Thank you! I for example have no stake in the NodeMaterial example, but was really curious about texture transformation and it's ordering.

I would like to discuss what a future mapping/texturing system could look like (or at least follow that discussion) but not participate in the shader graph conversation. I hope this makes sense. Thank you!

@pailhead
Copy link
Contributor

The reasoning behind this is:

When i read "Auto NodeMaterial build", I can't even fathom that #12788 would actually be discussed here. I correctly assumed it would have something to do with refreshing updating NodeMaterials but not the texture system.

There are other authoring tools and engines, i'd like to post these shots somewhere:

(texture properties in 3ds max)
max

(texture properties in Unity)
unity

These screenshots most likely don't belong here since this topic is on "auto building" and not "anything related with textures", but since you're having the discussion here i would like to participate.

Three.js is a complex system, and there are many issues and PRs here, if it's not sorted out and categorized properly it will be really hard for people to participate. If i didn't know any better i'd say you l33t people are kinda hiding here 🤣 ❤️

@donmccurdy
Copy link
Collaborator

I had assumed we were just discussing texture transformations for NodeMaterial, and that @sunag was not planning to change the non-NodeMaterial implementation at all here.

I would love to see the issues in #12788 solved generally, so if that's the intention, great! But I expect that is too complex to tackle here. For example #8278 would solve much of that, but is fairly complex and hasn't been merged. Better not to duplicate that in this PR.

@pailhead
Copy link
Contributor

pailhead commented Jun 11, 2018

I'm suggesting to break diffs into smaller ones so they can be used better as bread crumbs. Eventually once you move this into /src this thread will be the one you have to look up when you want to see what decision was behind this.

As is i can't find why texture transforms and texture images work as is. So if this is for auto building (possibly a different term for needsUpdate) it should only tackle that. Then another PR would tackle the texture transforms in NodeMaterial.

If doing this according to the current state of the world (list of priorities) ends up being an issue, what would be the reason to not untangle/solve/test this with /src at this moment?

@sunag
Copy link
Collaborator Author

sunag commented Jun 11, 2018

@pailhead Many issues are solved with nodes by nature itself. Like uv2 for any texture for example:

// backward-compatible (uv1)
material.map = texture; 

// or using nodes (uv2)
material.map = new THREE.TextureNode( texture, new THREE.UVNode(1) );

Our focus right now preserve backward-compatible only to think later make any modification in core.

@pailhead
Copy link
Contributor

@sunag I’d split this into two diffs, one for this refresh pattern, another for texture transform prioritizations.

@sunag
Copy link
Collaborator Author

sunag commented Jun 11, 2018

I’d split this into two diffs, one for this refresh pattern, another for texture transform prioritizations.

As there is nothing very well defined yet I will try to keep the MeshStandardNodeMaterial #14214 (comment) as close to the core as possible, with regard to texture transform prioritizations and others. It is necessary to obtain the same result in order to compare NodeMaterial with current material system then discuss the results.

@donmccurdy
Copy link
Collaborator

@sunag This PR looks good to me, was there more you wanted to change here, or is it ready for review and merge?

@sunag
Copy link
Collaborator Author

sunag commented Jun 15, 2018

@sunag This PR looks good to me, was there more you wanted to change here, or is it ready for review and merge?

Is ready 👍

@mrdoob mrdoob added this to the r94 milestone Jun 16, 2018
@mrdoob mrdoob merged commit d1decbc into mrdoob:dev Jun 16, 2018
@mrdoob
Copy link
Owner

mrdoob commented Jun 16, 2018

Looking good! Thanks!

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.

8 participants