Circular dependency in ShaderNode

[patricklx]

Description

ShaderNode.js:3 Uncaught (in promise) ReferenceError: Cannot access 'ShaderNode' before initialization
    at Object.ShaderNode (ShaderNode.js:3:59)
    at eval (SkinningNode.js:8:87)
    at ./node_modules/three/examples/jsm/nodes/accessors/SkinningNode.js (chunk.vendors-node_modules_blueimp-md5_js_md5_js-node_modules_element-resize-detector_src_element-r-f0dec0.bfeca9936d49aeae8903.js:2501:1)
    at __webpack_require__ (chunk.app.0c10a065d695b4093c2e.js:107:42)
    at eval (ShaderNodeElements.js:248:85)
    at ./node_modules/three/examples/jsm/nodes/shadernode/ShaderNodeElements.js (chunk.vendors-node_modules_blueimp-md5_js_md5_js-node_modules_element-resize-detector_src_element-r-f0dec0.bfeca9936d49aeae8903.js:3590:1)
    at __webpack_require__ (chunk.app.0c10a065d695b4093c2e.js:107:42)
    at eval (ShaderNode.js:20:80)
    at ./node_modules/three/examples/jsm/nodes/shadernode/ShaderNode.js (chunk.vendors-node_modules_blueimp-md5_js_md5_js-node_modules_element-resize-detector_src_element-r-f0dec0.bfeca9936d49aeae8903.js:3568:1)
    at __webpack_require__ (chunk.app.0c10a065d695b4093c2e.js:107:42)

Reproduction steps

use NodeMaterial and build with webpack

Code

// code goes here

Live example

n/a

Screenshots

No response

Version

158

Device

Desktop

Browser

Chrome

OS

Windows

[LeviPesin]

It works in browser. So it is rather a Webpack bug...

[marcofugaro]

I think the issue is that ShaderNode is exported two times,

one here

https://github.com/mrdoob/three.js/blob/5f75a1bddf43237e150be0f7195cf5e87a4b0f92/examples/jsm/nodes/shadernode/ShaderNode.js#L203

and one here

https://github.com/mrdoob/three.js/blob/5f75a1bddf43237e150be0f7195cf5e87a4b0f92/examples/jsm/nodes/shadernode/ShaderNodeBaseElements.js#L59

For this it's causing the circular dependency when imported inside SkinningNode.js

https://github.com/mrdoob/three.js/blob/5f75a1bddf43237e150be0f7195cf5e87a4b0f92/examples/jsm/nodes/accessors/SkinningNode.js#L2-L16

[sunag]

Hmm... Better remove it from ShaderNodeBaseElement I think.

[LeviPesin]

I think ShaderNode.js was intended to be rather internal file and ShaderNodeBaseElements.js and ShaderNodeElements.js files to be public?

[sunag]

ShaderNode should be oriented to create functions using ShaderNodeElements, this class must be in ShaderNode.js at public level.

[sunag]

Hmm... Better remove it from ShaderNodeBaseElement I think.

This will certainly have consequences on some files.

[epreston]

More detail for the update:

In examples/jsm/nodes the tools say there are 22 circular dependencies.

Using madge 5.02 (latest has a bug and wont generate graphs) Update: latest is fixed.

From the examples/jsm/ folder run the following command. The warning flag at the end displays files it skipped if any.

npx madge nodes --circular --warning

This gives the following output on the current dev branch:

✖ Found 22 circular dependencies!

1) accessors/BitangentNode.js > accessors/NormalNode.js > math/MathNode.js > utils/SplitNode.js > core/NodeBuilder.js > shadernode/ShaderNodeElements.js > accessors/CubeTextureNode.js > accessors/ReflectVectorNode.js > shadernode/ShaderNodeBaseElements.js
2) utils/SplitNode.js > core/NodeBuilder.js > shadernode/ShaderNodeElements.js > accessors/CubeTextureNode.js > accessors/ReflectVectorNode.js > shadernode/ShaderNodeBaseElements.js > accessors/MaterialNode.js
3) math/MathNode.js > utils/SplitNode.js > core/NodeBuilder.js > shadernode/ShaderNodeElements.js > accessors/CubeTextureNode.js > accessors/ReflectVectorNode.js > shadernode/ShaderNodeBaseElements.js > accessors/ModelViewProjectionNode.js > accessors/PositionNode.js
4) accessors/NormalNode.js > math/MathNode.js > utils/SplitNode.js > core/NodeBuilder.js > shadernode/ShaderNodeElements.js > accessors/CubeTextureNode.js > accessors/ReflectVectorNode.js > shadernode/ShaderNodeBaseElements.js
5) math/MathNode.js > utils/SplitNode.js > core/NodeBuilder.js > shadernode/ShaderNodeElements.js > accessors/CubeTextureNode.js > accessors/ReflectVectorNode.js > shadernode/ShaderNodeBaseElements.js > accessors/TangentNode.js
6) utils/SplitNode.js > core/NodeBuilder.js > shadernode/ShaderNodeElements.js > accessors/CubeTextureNode.js > accessors/ReflectVectorNode.js > shadernode/ShaderNodeBaseElements.js > accessors/TangentNode.js
7) math/MathNode.js > utils/SplitNode.js > core/NodeBuilder.js > shadernode/ShaderNodeElements.js > accessors/CubeTextureNode.js > accessors/ReflectVectorNode.js > shadernode/ShaderNodeBaseElements.js
8) shadernode/ShaderNodeElements.js > accessors/CubeTextureNode.js > accessors/ReflectVectorNode.js > shadernode/ShaderNodeBaseElements.js 
> shadernode/ShaderNode.js
9) utils/SplitNode.js > core/NodeBuilder.js > shadernode/ShaderNodeElements.js > accessors/CubeTextureNode.js > accessors/ReflectVectorNode.js > shadernode/ShaderNodeBaseElements.js > shadernode/ShaderNode.js
10) shadernode/ShaderNodeElements.js > accessors/ExtendedMaterialNode.js
11) math/MathNode.js > utils/SplitNode.js > core/NodeBuilder.js > shadernode/ShaderNodeElements.js > fog/FogExp2Node.js > fog/FogNode.js   
12) shadernode/ShaderNodeElements.js > functions/BSDF/DFGApprox.js
13) shadernode/ShaderNodeElements.js > functions/PhongLightingModel.js
14) shadernode/ShaderNodeElements.js > functions/PhysicalLightingModel.js
15) shadernode/ShaderNodeElements.js > lighting/LightsNode.js > lighting/AnalyticLightNode.js
16) shadernode/ShaderNodeElements.js > utils/EquirectUVNode.js
17) math/MathNode.js > utils/SplitNode.js > core/NodeBuilder.js > shadernode/ShaderNodeElements.js > utils/SpriteSheetUVNode.js
18) utils/SplitNode.js > core/NodeBuilder.js > shadernode/ShaderNodeElements.js > utils/SpriteSheetUVNode.js
19) Nodes.js > loaders/NodeLoader.js
20) Nodes.js > materialx/MaterialXNodes.js > materialx/lib/mx_hsv.js
21) Nodes.js > materialx/MaterialXNodes.js > materialx/lib/mx_noise.js
22) Nodes.js > materialx/MaterialXNodes.js > materialx/lib/mx_transform_color.js

Graph of just the circular dependency issues:

The complete graph:

red = circular dep blue = normal, with deps green = leaf node, no deps

[LeviPesin]

I'm not sure how this issue can be possibly fixed and can it be fixed at all... The current node system both exports a bunch of nodes from ShaderNodeElements (and due to circular dependencies that file was split in ShaderNodeElements.js and ShaderNodeBaseElements.js) and imports them from it.

[LeviPesin]

And I'm also not convinced it is a bug... It works in browsers, if it does not work in tools -- it is rather a bug in that tools.

[epreston]

The label doesn't matter so much.

Sure, everything operates just fine when it's smashed into one file. The browser execution context will be that way in the end. If it's not separated out into units in a meaningful file hierarchy, it confuses tools and removes their value. Even with the graphs, people would struggle to understand what a change in one place will impact.

It does point to the class hierarchy needing some planning. It can always be fixed.

[LeviPesin]

Maybe one way to fix this problem would be to move things like export const nodeAsFunction = nodeProxy( NodeAsClass ) from ShaderNode(Base)Elements.js to the corresponding node files, import in "core" node system only from them, and export them from Nodes.js (we wouldn't need then ShaderNodeElements.js, I think). I can do a PR if this idea sounds OK to @mrdoob and @sunag.

[patricklx]

The online examples use modules. But it can also be an issue in browser. It just luck that it does not fail. The browser has a slight difference to the tools on how it resolves named imports or * imports. E.g on line https://github.com/mrdoob/three.js/blob/5f75a1bddf43237e150be0f7195cf5e87a4b0f92/examples/jsm/nodes/shadernode/ShaderNode.js#L10 The tools would already import all names from the module and fail since the names are still undefined(because of circular deps) but when i debug it in an online example i see that NodeElements is an empty object (since it did not finish loading it)

[sunag]

Sorry for the confusion with this, the initial idea was to use ShaderNode externally only, but ShaderNode has grown to become the main way to create algorithms on native Nodes because of its simplicity it has become more productive. I like the idea @LeviPesin to create the ShaderElement in the same Node file, mainly because we let it simplify creating more ShaderNodeElements externally.

@LeviPesin maybe we needed made something like the old version because of method chaining? thinking about the possibility of the user to extend the nodes externally. https://github.com/sunag/three.js/blob/86982e1e9c90abb0a1819288a4bea00a366f0a72/examples/jsm/nodes/inputs/FloatNode.js#L54

[LeviPesin]

@LeviPesin maybe we needed made something like the old version because of method chaining? thinking about the possibility of the user to extend the nodes externally. https://github.com/sunag/three.js/blob/86982e1e9c90abb0a1819288a4bea00a366f0a72/examples/jsm/nodes/inputs/FloatNode.js#L54

I like this idea -- and maybe we can also do something similar in Materials.js?

[sunag]

I like this idea -- and maybe we can also do something similar in Materials.js?

But I don't think we would be able to use method chaining with external Nodes if we included a static lib (like a single file).

[LeviPesin]

I mean that we use the pattern from the old nodes system (like, export const nodeLib = {} in Nodes.js (or another file to prevent circular inclusion) and then nodeLib[ className ] = class in each file) and also apply it to Materials.js.

[sunag]

@LeviPesin Ah yeah! I think so.

[epreston]

That removes any type checking or autocomplete in tooling. Might get into a mess with registrations. Why not just plan the hierarchy and improve the exports ?

When you run into this issue, its an indicator improve the design. Not design ways to avoid the indicators.

[LeviPesin]

Why this would break type checking or autocompletion (not sure how it works right now)?

[epreston]

Client code is calling a function in what the interpreter sees as an indexed element in an array, if I understand it correctly. You're missing an opportunity because circular dependencies usually point to great improvements that can be made.

[LeviPesin]

A possible solution to the circular dependency issue is described in https://github.com/mrdoob/three.js/issues/25240#issuecomment-1417067122 -- the following comments are discussing another architecture change that is not so related to this issue.

Client code is calling a function in an what looks the interpreter sees as an array index, if I understand it correctly. 

No -- the second suggested change (not that one that directly solves circular dependency issue but that one that allows extending the nodes lib) is just addition of something like nodeLib[ 'ArrayElementNode' ] = ArrayElementNode (or nodeLib.ArrayElementNode = ArrayElementNode, which is slightly less readable).

If you are talking about using e.g. element/float/other functions -- it wouldn't break type checking if the types include them and also shouldn't break autocompletion.

It may break if functions are used as a node methods, though -- and also break when using other benefits of proxying -- like accessing custom properties (.x or .y) or array indexing... But this should be discussed in a separate issue, I think -- either https://github.com/mrdoob/three.js/pull/25074 or a new issue.

[epreston]

I see. Why does client code need to extend nodelib directly ?

[LeviPesin]

So that something like node.negate().add( otherNode ).functionThatUserAdds( thirdNode ) works.

[LeviPesin]

Maybe one way to fix this problem would be to move things like export const nodeAsFunction = nodeProxy( NodeAsClass ) from ShaderNode(Base)Elements.js to the corresponding node files, import in "core" node system only from them, and export them from Nodes.js (we wouldn't need then ShaderNodeElements.js, I think).

Started working on it, will file a PR soon.

[LeviPesin]

@LeviPesin maybe we needed made something like the old version because of method chaining? thinking about the possibility of the user to extend the nodes externally. https://github.com/sunag/three.js/blob/86982e1e9c90abb0a1819288a4bea00a366f0a72/examples/jsm/nodes/inputs/FloatNode.js#L54

Actually, there is a problem here that such syntax is import-order-dependent -- so with it, to properly use the chaining syntax in "core" nodes we need to maintain a proper import order in Nodes.js (maybe just math classes first). But it's doable...

[sunag]

Since we're reviewing this, I have some concerns about how the NodeMaterials, LightingNodes and ModelLightingAlgorithms are included in Nodes.js, the compilers can to remove unused Nodes if we include everything in Nodes.js?

[LeviPesin]

I think no compiler can remove a class that is exported "outer" -- to the user. So as long as user uses three/nodes, all nodes imported by them would work (and the nodes that these nodes use).

[sunag]

Hmm... ask this why to minification process like Google Closure or Uglify, they already made this kind of savings by testing the call flow, for example testing the code below here: https://skalman.github.io/UglifyJS-online/

const foo = () => {

    let a = 500;

    class foo2 {

        constructor() {}

        compilers() {

            return 'abc';

        }

    }

    return a + 500;

}

Result is:

const foo=()=>{return 1e3};

Since we're going to review this step, I think we could avoid namespace usage like * as myModule for internal usage because that might make it impossible for the compiler to do certain kind of optimizations if used dynamic string as access. I say this because we have some revisions to do, mainly from my code in this sense.

Maybe something like this can work for us include method chaining externally :

import { add, sub } from '...';

// the lib is a weak reference
Nodes.setMethodChaining( { add, sub } );

//Nodes.setMethodChaining( 'add', add );
//Nodes.setMethodChaining( 'sub', sub );

@LeviPesin Anyway, your proposed importing ShaderNodeElement like { add } from './OperatorNode.js' for example, should resolve this issue.

[LeviPesin]

Mostly finished the PR, will test and file it tomorrow.

[LeviPesin]

Finally finished it: https://github.com/mrdoob/three.js/pull/25498