cyclic dependencies

[ghost]

Hey everyone.

@kumavis and I have been hard at work trying to find an efficient way to move THREE.js over to a browserify architecture. We made good progress, even up to the point of having all of the files moved over to a browserify build system and being able to generate a three.min.js with gulp.

Unfortunately, the examples don't work, because unlike commonjs browserify cannot handle cyclic dependencies, of which there are many in THREE.js.

I have made an interactive graph depicting the dependency relationships here.

Unless and until these get untangled, we will not be able to move THREE.js over to a browserify build.

I do not consider this a deficiency of browserify, but rather a problem with THREE.js. Circular dependencies are a bad thing to have in software in general, and lead to all sorts of problems.

[kumavis]

Yeah this falls under the same category as Vector3<->Matrix4. They are linked for convenience. I think its a bad idea but not worth fighting that one. I'll mark it as complete

[kumavis]

Shape -> FontUtils could be removed by using a moving methods like triangulate to more generic Utils. But not a big win to do that. Going to mark it as complete.

[kumavis]

Box3 -> BufferGeometry and Box3 -> Geometry could both be cleaned up.

Its another case of not putting class-dependant behaviour on the class itself.

source:

setFromObject: function () {

  // Computes the world-axis-aligned bounding box of an object (including its children),
  // accounting for both the object's, and childrens', world transforms

  /* ... */

  if ( geometry instanceof THREE.Geometry ) {
    /* ... */
  } else if ( geometry instanceof THREE.BufferGeometry && geometry.attributes[ 'position' ] !== undefined ) {
    /* ... */
  }

  /* ... */

}

In both cases its just trying to iterate through the worldCoordinate vertices/positions of the geometry. I wonder if it would greatly simplify code to create a lazy vertices object on BufferGeometry that looks up the values as they are requested. Not sure about perf impact.

[kumavis]

Alternately, we could use Geometry.computeBoundingBox: Geometry BufferGeometry

[ghost]

Box3 is a problem spot when running the browserify build. See notes in coballast/threejs-browserify-conversion-utility#21.

@kumavis Would you mind outlining your recommended solution for dealing with Box3/Geometry/BufferGeometry? If it is quick, I can implement it.

[kumavis]

I can't look at it right now, but I would start with my suggestion above to use geo.computeBoundingBox as implemented on Geometry and BufferGeometry in place of the if/else here. Instead box3.setFromObj should call geometry.computeBoundingBox and then set params based off of the produced box3.

That should remove the Box3 -> BufferGeometry and Box3 -> Geometry end of the circular deps. Let me know if I'm missing something.

Hmm maybe the resulting code is a bit convoluted, what actually makes sense here? Box3.setFromObject shouldnt exist, but thats not an option. Geo's should be able to produce box3s, i have no problem with that. Yeah I guess Box3.setFromObject should ask the geo's for a bounding box / extents, but maybe they should ask the Object3D/Mesh for the bounding box / extents.

sorry got a bit rambly. lemme know what you think.

[kamicane]

Possibly relevant: #6546

Without something like this, it is impossible to analyze those dynamic dependencies from loader scripts.

[kamicane]

Based on my tests, cyclic dependencies are not an issue with commonJSification. They need to be handled correctly, and as previously stated in this thread, they make the dependency graph quite a mess, but they do not prevent THREE.js to work on commonJS environments (when transformed, ofcourse).

I've just published a full commonjs version on npm as three.cjs using my three commonjs transpiler

Note: for this to work I had to manually cherrypick #6546 on master. While dynamic dependencies work well in node.js, they cannot work in Browserify (or any other cjs to browser tool) as they need to perform static dependency analysis.

Browserify proof :http://requirebin.com/?gist=b7fe528d8059a7403960

[WestLangley]

@kamicane FYI - Here is where THREE was added as an argument of an anonymous function to Raycaster (formerly Ray).

[kamicane]

I understand the need for an anonymous function (to prevent global leaks in browsers), the argument is superfluous however, and makes the whole file fall in the category of computed dependencies. While the argument could be dynamically removed with AST modifications, it can never be a bulletproof solution (depending on what is being written to the argument, read from the argument, etc.). Static analysis becomes almost impossible. Manual intervention is needed in this case.

[mrdoob]

Now that Raycaster is more lightweight we could give it go at making it like the rest of the classes.

[roomle-build]

@mrdoob, @Mugen87 we use Rollup to only use the parts of Three.js we really need. When we run the built we still get the following warning:

(!) Circular dependency: node_modules/three/src/math/Vector3.js -> node_modules/three/src/math/Matrix4.js -> node_modules/three/src/math/Vector3.js
(!) Circular dependency: node_modules/three/src/math/Vector3.js -> node_modules/three/src/math/Quaternion.js -> node_modules/three/src/math/Vector3.js
(!) Circular dependency: node_modules/three/src/math/Sphere.js -> node_modules/three/src/math/Box3.js -> node_modules/three/src/math/Sphere.js
(!) Circular dependency: node_modules/three/src/objects/LineSegments.js -> node_modules/three/src/objects/Line.js -> node_modules/three/src/objects/LineSegments.js

Are there still circular dependencies in Three.js or are we doing something wrong?

[bhouston]

Vector3 and Matrix4 are tied to each other, if you pull in one, you need to pull in the other. Circular dependencies should technically be allowed.

[roomle-build]

@bhouston yeah I see, thanks for the hint. Yes circular dependencies are allowed and rollup makes no troubles but I'm not sure if it's good practice to have circular dependencies. Vector3 only depends on Matrix4 because of multiplyMatrices and getInverse, for more details see (https://github.com/mrdoob/three.js/blob/dev/src/math/Vector3.js#L315)

[makc]

@roomle-build Idk, man, just because it explicitly references the Matrix4 constructor? what about

    applyMatrix4: function ( m ) {

        var x = this.x, y = this.y, z = this.z;
        var e = m.elements;

        var w = 1 / ( e[ 3 ] * x + e[ 7 ] * y + e[ 11 ] * z + e[ 15 ] );

        this.x = ( e[ 0 ] * x + e[ 4 ] * y + e[ 8 ] * z + e[ 12 ] ) * w;
        this.y = ( e[ 1 ] * x + e[ 5 ] * y + e[ 9 ] * z + e[ 13 ] ) * w;
        this.z = ( e[ 2 ] * x + e[ 6 ] * y + e[ 10 ] * z + e[ 14 ] ) * w;

        return this;

},

?

you can say that you could pass { elements: [....] } and it will work, but we all know it expects Matrix4 there

[mrdoob]

Lets start with Vector3.

Vector3 depends on Matrix4 because of project and unproject:

    project: function () {

        var matrix = new Matrix4();

        return function project( camera ) {

            matrix.multiplyMatrices( camera.projectionMatrix, matrix.getInverse( camera.matrixWorld ) );
            return this.applyMatrix4( matrix );

        };

    }(),

    unproject: function () {

        var matrix = new Matrix4();

        return function unproject( camera ) {

            matrix.multiplyMatrices( camera.matrixWorld, matrix.getInverse( camera.projectionMatrix ) );
            return this.applyMatrix4( matrix );

        };

    }(),

Vector3 depends on Quaternion because of applyEuler and applyAxisAngle:

    applyEuler: function () {

        var quaternion = new Quaternion();

        return function applyEuler( euler ) {

            if ( ! ( euler && euler.isEuler ) ) {

                console.error( 'THREE.Vector3: .applyEuler() now expects an Euler rotation rather than a Vector3 and order.' );

            }

            return this.applyQuaternion( quaternion.setFromEuler( euler ) );

        };

    }(),

    applyAxisAngle: function () {

        var quaternion = new Quaternion();

        return function applyAxisAngle( axis, angle ) {

            return this.applyQuaternion( quaternion.setFromAxisAngle( axis, angle ) );

        };

    }(),

Suggestions?

[roomle-build]

I'm not sure if we need to remove circular dependencies by all means. But I could imagine to move multiplyMatrices to a Math module. The signature would then change of course to multiplyMatrices( a: Matrix4, b: Matrix4, result: Matrix4 ): Matrix4. Inside Vector3 you could then import { multiplyMatrices } from './Math'; the same could be done in Matrix4 (to keep the API surface of Matrix4 the same).

I just had a quick look (didn't look at the Quaternian case - only Vec3/Mat4) and I'm also not sure about performance implication and consequences for the rest of the code base. Furthermore I'm also not convinced that it's absolutely necessary to remove these circular dependencies. Just wanted to share my thought because @mrdoob asked for suggestions

[makc]

@roomle-build so basically create more modules for the sake of no circular dependencies, but you're still going to use all those modules? this maybe could make more sense if each and every math method would be its own module, then you're pulling in only those you use, but that will be a lot of modules.

[roomle-build]

@makc not really. It would be one big module with a lot of small "helper" functions. This would also help for tree-shaking etc. A math module could look like:

export const multiplyMatrices( a, b, result ) { // ... DO STUFF ... // }
export const getInverse( /* ... */ ) { // ... DO STUFF ... // }
// ...
// ...

And the consuming module would do something like:

import { Matrix4 } from './Matrix4.js';
import { multiplyMatrices } from './math';
const result = new Matrix4( );
multiplyMatrices( a, b, result );

When bundleing everything together, rollup does it's magic and creates the most efficient bundle.

This is what a lot of popular libraries are doing. Actually RxJS switched their import "logic" also to the pattern I described. There it looks something like:

 import { flatMap, map, tap } from 'rxjs/operators';

myObject.run().pipe(
  tap(result => doSomething()), 
  flatMap(() => doSomethingElse()), 
  map(() => doAnotherThing())
);

You can read about the "why and how" they changed this stuff in RxJS 6 in several blogposts for example: https://auth0.com/blog/whats-new-in-rxjs-6/

But as I said, it's just a thought and I'm not sure about all the implications this would have to the rest of the codebase. Also the current math module is not "prepared" to be used like this. Currently all the methods on the math module are attached "kind of static". This also prevents rollup from detecting what's really needed...

[makc]

@roomle-build hmm so you're saying that rollup can understand if the code in the same scope does not actually need the whole scope, nice.

[bhouston]

You are talking about moving towards a functional approach (functions taking objects) rather than object oriented approach (object having member functions.) This is a real thing but given that Three.JS is fully object-oriented, proposing this type of change is a pretty big one and it would break all existing code.

I am not sure that the arguments in favor of this change are that significant at this point to justify breaking all backwards compatibility.

[bhouston]

@makc not really. It would be one big module with a lot of small "helper" functions. This would also help for tree-shaking etc. A math module could look like:

If this is what is being proposed, it should be described correctly. It is the change of Three.JS from a object-oriented style of design to a functional design.

[roomle-build]

@roomle-build hmm so you're saying that rollup can understand if the code in the same scope does not actually need the whole scope, nice.

yes rollup understands how all the imports relate to each other and does tree-shaking, dead code elimination etc. The new versions of rollup can also do "chunking" and a lot of other nice stuff. But the current project structure does not take full advantage of these features.

You are talking about moving towards a functional approach (functions taking objects) rather than object oriented approach (object having member functions.) This is a real thing but given that Three.JS is fully object-oriented, proposing this type of change is a pretty big one and it would break all existing code.

I don't think these two paradigms are mutually exclusive. I think you can mix and match these two paradigms. I also don't propose to change to functional programming. I just wanted to describe a way to get rid of the cyclic dependency. You could also attach the multiplyMatrices method to the Math object. But if someone rewrites this kind of stuff it would make sense to consider using the features of ES6 modules. But as I said, I'm not an expert of the Three.js codebase and it was just a thought how to eliminate the cyclic dependency. I think Three.js is an awesome project with a great code base and I don't want to nag around. So I hope no one feels offended by my comments 😉

I'm not sure if we should discuss design decissions in an issue. Do you have some place where this kind of stuff fits better?

[bhouston]

BTW gl-matrix is a functional math library: https://github.com/toji/gl-matrix/tree/master/src/gl-matrix

[mrdoob]

@roomle-build

Currently all the methods on the math module are attached "kind of static".

How so?

[bhouston]

@mrdoob I believe that with the functional design of gl-matrix each function of say vec3 (in the vec3 file I linked to in my previous comment) is exported individually. This allows you to pick and choose which functions to import. You do not need to bring along all of vec3.

Where as with Three.JS, because it uses an object-oriented design, all of the math functions for Vector3 are attached to the Vector3 object prototype and you import just the Vector3 class itself.

Thus the imports in Three.JS are of whole classes whereas with a functional approach you import individual functions.

(The other very neat thing about gl-matrix library is that all the individual functions do not use other functions, @toji has basically inlined an optimized version of all of the mathematics into each individual operation. This is likely quite efficient in terms of speed but it leads to a difficult to maintain library.)

[bhouston]

I do not think we need to refactor this part of Three.JS except maybe to get rid of any references in /math to other directories in three.js. The math library is fairly small and it never really shows up in my profiling tests these days. Yes, it isn't maximally efficient, but it is close enough while maintainable readability and ease of use.

[mrdoob]

@bhouston Got it. Many thanks for the explanation! 😊

[roomle-build]

I just wanted to follow up on the topic. But I want to come back from importing function vs importing classes to the topic of resolving cyclic dependencies. (Also I don't see why import { someFunction } from 'SomeModule' is less maintainable than import SomeClass from 'SomeModule', but that's definitely not the topic of this issue/conversation.

To resolve the cyclic dependency it would be possible to put the functionality into a separate Class. You could attach a multiplyMatrices method to the Math-Class or create a Multiplier-Class which has the multiplyMatrices method. But as I said before I'm not sure if we have to remove cyclic dependencies. If the decision is to not remove them, I think this issue could be close 😃

[Mugen87]

After resolving #19137, this can be closed now 🎉.

[kumavis]

@Mugen87 wow, 5 years! congrats on finally hitting it :fire: :clap: I had a lot of fun making those graphs back then :smile_cat: