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: Conflicting names for varying and temp var. #15666

Closed
donmccurdy opened this issue Jan 30, 2019 · 13 comments
Closed

NodeMaterial: Conflicting names for varying and temp var. #15666

donmccurdy opened this issue Jan 30, 2019 · 13 comments

Comments

@donmccurdy
Copy link
Collaborator

@sunag sorry this is a complex node graph and I haven't been able to come up with a simpler test case, but the "varying" created for my AttributeNode gets a name that conflicts with a temp variable, but has a different type, and causes a compile error:

JSON:
{
  "nodes": {
    "8AC8FB7D-9744-407F-9C1B-CBCC78DC568F": {
      "uuid": "8AC8FB7D-9744-407F-9C1B-CBCC78DC568F",
      "nodeType": "Standard",
      "position": "50A2FC35-6E01-4A05-B479-8FEA2D783BF8",
      "color": "7569E324-63BC-4F78-A01B-71CC8E3F3073",
      "roughness": "503E236F-6E24-4AB6-8A01-F11C8F2BC96D",
      "metalness": "437B0DD3-11F5-4036-9084-66AD0EEAB69C",
      "alpha": "891A11FA-268B-451A-97B1-830BD630307A",
      "ao": "50114A99-15AB-408F-A505-9984E69ED078",
      "emissive": "83D4F7C2-35C1-42E7-984D-2BA940294AEE"
    },
    "50A2FC35-6E01-4A05-B479-8FEA2D783BF8": {
      "uuid": "50A2FC35-6E01-4A05-B479-8FEA2D783BF8",
      "nodeType": "Operator",
      "a": "A9C03A17-283E-4439-A953-FD9A487E4C87",
      "b": "62259BC4-12D0-4B3E-999D-99760C10294E",
      "op": "+"
    },
    "A9C03A17-283E-4439-A953-FD9A487E4C87": {
      "uuid": "A9C03A17-283E-4439-A953-FD9A487E4C87",
      "nodeType": "Position",
      "scope": "local"
    },
    "62259BC4-12D0-4B3E-999D-99760C10294E": {
      "uuid": "62259BC4-12D0-4B3E-999D-99760C10294E",
      "nodeType": "Operator",
      "a": "7263E40B-854C-4547-B006-AF14A3DFC681",
      "b": "384E286D-C16F-4828-8609-9586BA3CC6F3",
      "op": "+"
    },
    "7263E40B-854C-4547-B006-AF14A3DFC681": {
      "uuid": "7263E40B-854C-4547-B006-AF14A3DFC681",
      "nodeType": "Operator",
      "name": "Vector 3D <Vector 3D>",
      "a": "3DD0D645-BD2D-4969-A080-821C84613EB7",
      "b": "BAA064F6-007C-4E41-961E-0DBCE78D680E",
      "op": "+"
    },
    "3DD0D645-BD2D-4969-A080-821C84613EB7": {
      "uuid": "3DD0D645-BD2D-4969-A080-821C84613EB7",
      "nodeType": "Join",
      "inputs": {
        "x": "45E61B50-3FBD-461C-85CE-514ED00B86BA",
        "y": "A4D6E022-3F81-4D99-8336-87B9900731E8",
        "z": "A4D6E022-3F81-4D99-8336-87B9900731E8"
      }
    },
    "45E61B50-3FBD-461C-85CE-514ED00B86BA": {
      "uuid": "45E61B50-3FBD-461C-85CE-514ED00B86BA",
      "nodeType": "Math2",
      "name": "Remap 1 <Remap>",
      "a": "D3BF0F9B-518C-4451-BD7C-DB95A6F4A897",
      "b": "E11A9972-AEBA-46B2-8D7D-CF26D46ED5FE",
      "method": "mod"
    },
    "D3BF0F9B-518C-4451-BD7C-DB95A6F4A897": {
      "uuid": "D3BF0F9B-518C-4451-BD7C-DB95A6F4A897",
      "nodeType": "Attribute",
      "name": "instanceID",
      "type": "float"
    },
    "E11A9972-AEBA-46B2-8D7D-CF26D46ED5FE": {
      "uuid": "E11A9972-AEBA-46B2-8D7D-CF26D46ED5FE",
      "nodeType": "Float",
      "name": "Relay <Relay>",
      "value": 10
    },
    "A4D6E022-3F81-4D99-8336-87B9900731E8": {
      "uuid": "A4D6E022-3F81-4D99-8336-87B9900731E8",
      "nodeType": "Float",
      "name": "ZERO",
      "readonly": true,
      "value": 0
    },
    "BAA064F6-007C-4E41-961E-0DBCE78D680E": {
      "uuid": "BAA064F6-007C-4E41-961E-0DBCE78D680E",
      "nodeType": "Join",
      "inputs": {
        "x": "A4D6E022-3F81-4D99-8336-87B9900731E8",
        "y": "A4D6E022-3F81-4D99-8336-87B9900731E8",
        "z": "DC19F550-F679-4EFA-A9C9-9E7F8556C4C6"
      }
    },
    "DC19F550-F679-4EFA-A9C9-9E7F8556C4C6": {
      "uuid": "DC19F550-F679-4EFA-A9C9-9E7F8556C4C6",
      "nodeType": "Operator",
      "name": "Remap <Remap>",
      "a": "5EF830DB-8352-4F23-9ACF-081534D7EFAC",
      "b": "E11A9972-AEBA-46B2-8D7D-CF26D46ED5FE",
      "op": "/"
    },
    "5EF830DB-8352-4F23-9ACF-081534D7EFAC": {
      "uuid": "5EF830DB-8352-4F23-9ACF-081534D7EFAC",
      "nodeType": "Operator",
      "name": "Subtract <Subtract>",
      "a": "D3BF0F9B-518C-4451-BD7C-DB95A6F4A897",
      "b": "45E61B50-3FBD-461C-85CE-514ED00B86BA",
      "op": "-"
    },
    "384E286D-C16F-4828-8609-9586BA3CC6F3": {
      "uuid": "384E286D-C16F-4828-8609-9586BA3CC6F3",
      "nodeType": "Join",
      "inputs": {
        "x": "A4D6E022-3F81-4D99-8336-87B9900731E8",
        "y": "0F1CA407-A4D0-45AB-B0B0-76D4B53506A5",
        "z": "A4D6E022-3F81-4D99-8336-87B9900731E8"
      }
    },
    "0F1CA407-A4D0-45AB-B0B0-76D4B53506A5": {
      "uuid": "0F1CA407-A4D0-45AB-B0B0-76D4B53506A5",
      "nodeType": "Operator",
      "name": "Noise2D <Noise 2D>",
      "a": "2D5BEA03-C79D-4261-9E4F-43ED4437122E",
      "b": "313ABFDB-D7B5-4E1E-B5DC-D8FE3BC4B1FE",
      "op": "+"
    },
    "2D5BEA03-C79D-4261-9E4F-43ED4437122E": {
      "uuid": "2D5BEA03-C79D-4261-9E4F-43ED4437122E",
      "nodeType": "Switch",
      "name": "Vector 2D <Vector 2D>",
      "node": "7263E40B-854C-4547-B006-AF14A3DFC681",
      "components": "rb"
    },
    "313ABFDB-D7B5-4E1E-B5DC-D8FE3BC4B1FE": {
      "uuid": "313ABFDB-D7B5-4E1E-B5DC-D8FE3BC4B1FE",
      "nodeType": "Join",
      "inputs": {
        "x": "AE983391-A8BD-42E2-A5E9-BFA1201578D3",
        "y": "A4D6E022-3F81-4D99-8336-87B9900731E8"
      }
    },
    "AE983391-A8BD-42E2-A5E9-BFA1201578D3": {
      "uuid": "AE983391-A8BD-42E2-A5E9-BFA1201578D3",
      "nodeType": "Timer",
      "name": "Number 1 <Number>",
      "scope": "global",
      "scale": 1,
      "timeScale": false
    },
    "7569E324-63BC-4F78-A01B-71CC8E3F3073": {
      "uuid": "7569E324-63BC-4F78-A01B-71CC8E3F3073",
      "nodeType": "Vector4",
      "x": 1,
      "y": 0.62894594669342,
      "z": 0,
      "w": 1
    },
    "503E236F-6E24-4AB6-8A01-F11C8F2BC96D": {
      "uuid": "503E236F-6E24-4AB6-8A01-F11C8F2BC96D",
      "nodeType": "Float",
      "value": 0.59210526943207
    },
    "437B0DD3-11F5-4036-9084-66AD0EEAB69C": {
      "uuid": "437B0DD3-11F5-4036-9084-66AD0EEAB69C",
      "nodeType": "Float",
      "value": 0.4243420958519
    },
    "891A11FA-268B-451A-97B1-830BD630307A": {
      "uuid": "891A11FA-268B-451A-97B1-830BD630307A",
      "nodeType": "Float",
      "value": 1
    },
    "50114A99-15AB-408F-A505-9984E69ED078": {
      "uuid": "50114A99-15AB-408F-A505-9984E69ED078",
      "nodeType": "Float",
      "value": 1
    },
    "83D4F7C2-35C1-42E7-984D-2BA940294AEE": {
      "uuid": "83D4F7C2-35C1-42E7-984D-2BA940294AEE",
      "nodeType": "Vector4",
      "x": 0,
      "y": 0,
      "z": 0,
      "w": 1
    }
  },
  "materials": {
    "61BD1149-7B5C-4217-8E94-EF79A898263A": {
      "uuid": "61BD1149-7B5C-4217-8E94-EF79A898263A",
      "type": "StandardNodeMaterial",
      "alphaTest": 0.99,
      "userData": {
        "instanceCount": 100,
        "timerNodes": [
          {
            "uuid": "AE983391-A8BD-42E2-A5E9-BFA1201578D3",
            "nodeType": "Timer",
            "name": "Number 1 <Number>",
            "scope": "global",
            "scale": 1,
            "timeScale": false
          }
        ]
      },
      "fog": false,
      "lights": true,
      "vertex": "8AC8FB7D-9744-407F-9C1B-CBCC78DC568F",
      "fragment": "8AC8FB7D-9744-407F-9C1B-CBCC78DC568F"
    }
  },
  "material": "61BD1149-7B5C-4217-8E94-EF79A898263A"
}
GLSL:
#ifdef TEXTURE_LOD_EXT
  #define texCube(a, b) textureCube(a, b)
  #define texCubeBias(a, b, c) textureCubeLodEXT(a, b, c)
  #define tex2D(a, b) texture2D(a, b)
  #define tex2DBias(a, b, c) texture2DLodEXT(a, b, c)
#else
  #define texCube(a, b) textureCube(a, b)
  #define texCubeBias(a, b, c) textureCube(a, b, c)
  #define tex2D(a, b) texture2D(a, b)
  #define tex2DBias(a, b, c) texture2D(a, b, c)
#endif
#include <packing>
#include <common>
attribute float instanceID;
varying vec3 vViewPosition;
#ifndef FLAT_SHADED
  varying vec3 vNormal;
#endif
#include <fog_pars_vertex>
#include <morphtarget_pars_vertex>
#include <skinning_pars_vertex>
#include <shadowmap_pars_vertex>
#include <logdepthbuf_pars_vertex>
#include <clipping_planes_pars_vertex>
varying vec3 vPosition;

varying float nVv0;

uniform float nVu0;
uniform float nVu1;




void main() {
 vec3 nVv0;
 vec3 nVv1;
 float nVv2;
 vec3 nVv3;
 float nVv4;
 float nVv5;


#include <beginnormal_vertex>
#include <morphnormal_vertex>
#include <skinbase_vertex>
#include <skinnormal_vertex>
#include <defaultnormal_vertex>
#ifndef FLAT_SHADED
  vNormal = normalize( transformedNormal );
#endif
#include <begin_vertex>
nVv2 = mod( instanceID, nVu0 );
nVv1 = vec3( nVv2, 0.0, 0.0 );
nVv5 = ( instanceID - nVv2 );
nVv4 = ( nVv5 / nVu0 );
nVv3 = vec3( 0.0, 0.0, nVv4 );
nVv0 = ( nVv1 + nVv3 );

transformed = ( transformed + ( nVv0 + vec3( 0.0, ( nVv0.xz + vec2( nVu1, 0.0 ) ).x, 0.0 ) ) );
#include <morphtarget_vertex>
#include <skinning_vertex>
#include <project_vertex>
#include <fog_vertex>
#include <logdepthbuf_vertex>
#include <clipping_planes_vertex>
  vViewPosition = - mvPosition.xyz;
#include <worldpos_vertex>
#include <shadowmap_vertex>
nVv0 = instanceID;
vPosition = transformed;

}

Error:

THREE.WebGLProgram: shader error:  0 35715 false gl.getProgramInfoLog invalid shaders� ERROR: 0:322: '=' : dimension mismatch
ERROR: 0:322: 'assign' : cannot convert from 'attribute highp float' to 'highp 3-component vector of float'

My temporary workaround is this hack in NodeBuilder:

getTempVar: function ( uuid, type, ns ) {

	var data = this.getVar( uuid, type, ns, this.shader );
	data.name = 'tmp_' + data.name;
	return data;

},

The shader doesn't do anything particularly cool, so as long as it compiles I think the error is fixed.

@donmccurdy
Copy link
Collaborator Author

The final goal is below, but it relies on a couple custom nodes I wrote (RemapNode, Noise2DNode). Those nodes are removed from the example above:

d45fe5d6-fbdd-47b0-86d9-3d1f61b4e4b8-37715-0002410b0caad0cf

@sunag
Copy link
Collaborator

sunag commented Jan 30, 2019

This is easy to fix, I just want to see if I think any better name during this week.
Suggestions?

I wrote (RemapNode, Noise2DNode).

Pretty cool, and this nodes is to your private use or will you make it public?

@donmccurdy
Copy link
Collaborator Author

Pretty cool, and this nodes is to your private use or will you make it public?

Probably public, I'll see how it goes. I'm trying to write a loader that can parse a shader graph created in Shade and construct an equivalent NodeMaterial graph. Remap, in particular, is pretty common in their examples... it can be done with a bunch of OperatorNodes, but I found debugging much easier with a single node.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Jan 30, 2019

... I just want to see if I think any better name during this week.
Suggestions?

Does nVv have a meaning? 😅 I guess "local" might be a good prefix for variables scoped to the main function.

Some way of using node.name in the variable name would also make debugging easier, but that's a different and lower priority suggestion.

@sunag
Copy link
Collaborator

sunag commented Jan 30, 2019

Probably public, I'll see how it goes. I'm trying to write a loader that can parse a shader graph created in Shade and construct an equivalent NodeMaterial graph...

It will be amazing.

Does nVv have a meaning? sweat_smile I guess "local" might be a good prefix for variables scoped to the main function.

nVu0 = node variable uniform id
nVv0 = node variable varying id

// I think it should be this!?
nVt0 = node variable temp id

It was an exotic and short name to prevent conflict with external names, but the idea today is to have control of all names, for example custom functions and be able to rename it if necessary in parse time.

For example:

// it is purely demonstrative
var normalize1 = new FunctionNode("vec3 custom_normalize( vec3 v ) { return normalize( v ); }");
var normalize2 = new FunctionNode("vec3 custom_normalize( vec3 v ) { return v / length(v); }");

material.color = normalize1;

// here "custom_normalize" must be renamed in parse to prevent name conflict.
material.emissive = normalize2;

@donmccurdy
Copy link
Collaborator Author

nVt* seems good to me then. :)

I think we could append user-provided names to that (nVt0_windSpeed) without introducing conflicts?

@sunag
Copy link
Collaborator

sunag commented Jan 30, 2019

It was including something close to that in namespace ns:

name = ns ? ns : 'nVv' + index;

But still I think this process of rename would be better being automatic.

The idea will be that the programmer and artist pick up different sources of nodes and never have conflicts with each other the same thing that happens in variables conversions.

@donmccurdy
Copy link
Collaborator Author

My concern is that it's hard to debug code like this, if the programmer isn't sure why their shader isn't working as expected:

nVv2 = mod( instanceID, nVu0 );
nVv1 = vec3( nVv2, 0.0, 0.0 );
nVv5 = ( instanceID - nVv2 );
nVv4 = ( nVv5 / nVu0 );
nVv3 = vec3( 0.0, 0.0, nVv4 );
nVv0 = ( nVv1 + nVv3 );

But there are other ways to solve that too, like a visual editor or creating some NodeUtils.prettyPrint( material ) helper. Your proposed fix of renaming to nVt sounds good to me.

@sunag
Copy link
Collaborator

sunag commented Jan 30, 2019

I understand, really will be useful a label in name for debugging...

About .needsUpdate does anyone know why it stopped working?
//cc @WestLangley

For example, condition in conditional example use needsUpdate to recompile the shader
https://threejs.org/examples/webgl_materials_nodes.html?e=conditional

@sunag
Copy link
Collaborator

sunag commented Jan 30, 2019

I already figured out, it's the program.code and programCache.
I will post an update later.

@sunag
Copy link
Collaborator

sunag commented Jan 30, 2019

I could put material.defines['MATERIAL_VERSION']++ whenever needsUpdate is seted to true but I think this would lose the programCache? Now I remember that this was an issue that I had some time ago.

@sunag
Copy link
Collaborator

sunag commented Jan 31, 2019

@donmccurdy By me you can close the Issue. These doubts about needspdate can be answered in the PR #15674

@donmccurdy
Copy link
Collaborator Author

The example is working for me yes, 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

No branches or pull requests

2 participants