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

Initial adoption of closures in the math library. #2941

Merged
merged 3 commits into from Jan 21, 2013

Conversation

bhouston
Copy link
Contributor

I've implemented JavaScript closures across the whole math library to remove the need for declaration of public static variables for GC optimization purposes as proposed in issue #2936.

I did have to switch to extending the prototypes of the math classes rather than just replacing their prototypes. I had to do this because instances of a type created within closures of that type are created before the prototype is set, thus they were incomplete types. For example, if inside THREE.Matrix4.prototype = { ... [in here].... } you had a closure that created a var m1 = THREE.Matrix4() variable, it wouldn't yet have its prototype set because only after it has completed executing all the code in the { ...[in here]... } block does it set it to Matrix4.prototype. By extending the prototype, even variables created inside the {...[in here]...} block eventually get full prototypes before they are used in their methods.

I added THREE.extends to handle extending prototypes, I based it on the code here:

http://stackoverflow.com/a/12317051

I tested about 20 examples at random and they all worked. All unit tests pass as well.

Both webgl_performance.html and webgl_performance_static.html perform at the same speed, no slowdowns on my PC.

…ototypes rather than replacing them. This is to ensure that types created in closures within a type's prototype definition get their prototype updated with the full definition.
@mrdoob
Copy link
Owner

mrdoob commented Jan 17, 2013

I did have to switch to extending the prototypes of the math classes rather than just replacing their prototypes.

Oh... that's a bit of a downside..

@bhouston
Copy link
Contributor Author

@mrdoob, the alternative is to define each function on the prototype directly. Such as:

THREE.Matrix4.prototype.set = function ...

THREE.Matrix4.prototype.getInverse = function ....

This has the same effect of keeping the prototype the same.

The pattern that doesn't work is replacing the prototype, e.g.:

THREE.Matrix4.prototype = {...}

@mrdoob
Copy link
Owner

mrdoob commented Jan 17, 2013

I see. Ok, I'll meditate about this one.

@gero3
Copy link
Contributor

gero3 commented Jan 17, 2013

@bhouston

I would change to defining the function on the prototype directly becuase we use this already when we are defining objects on top of object3D.

@bhouston
Copy link
Contributor Author

@gero3, okay. Yes it is common in Three.js to set functions on the prototype directly when there are object inheritance hierarchies.

Such as in children of Object3D:

https://github.com/mrdoob/three.js/blob/master/src/objects/Mesh.js#L31

And here in children of Camera:

https://github.com/mrdoob/three.js/blob/master/src/cameras/OrthographicCamera.js#L23

And here in children of Loader:

https://github.com/mrdoob/three.js/blob/master/src/loaders/SceneLoader.js#L23

@mrdoob
Copy link
Owner

mrdoob commented Jan 18, 2013

I wonder if this does the trick too (I haven't been able to try it):

Vector3.prototype = Object.create( {

    constructor: THREE.Vector3,

    set: function ( x, y, z ) {

        this.x = x;
        this.y = y;
        this.z = z;

        return this;

    },

    ...

} );

@bhouston
Copy link
Contributor Author

@mrdoob, unfortunate that still replaces the prototype and that only affects new Vector3's created after this. :-/ It doesn't affect Vector3's created while executing the { constructor:... set..: } block such as those created in JavaScript closures. The old method of allocating THREE.Vector3.__v1 at the end of the JavaScript file worked because they were allocated after the prototype of THREE.Vector3 was set with the full definitions.

@mrdoob mrdoob merged commit baa928e into mrdoob:dev Jan 21, 2013
@mrdoob
Copy link
Owner

mrdoob commented Jan 21, 2013

Merged! :)
Even if I still wonder if there isn't a "native" method that does the same thing extend() is doing though.

@mrdoob
Copy link
Owner

mrdoob commented Jan 21, 2013

Probably not. All I see are jQuery/Underscore implementations.

@bhouston
Copy link
Contributor Author

EMCAScript 6 actually supports classes:

http://www.nczonline.net/blog/2012/10/16/does-javascript-need-classes/

In this form:

class Animal {
    constructor(name) {
        this.name = name;
    }

    sayName() {
        console.log(this.name);
    }
}

class Dog extends Animal {
    constructor(name) {
        super(name);
    }

    bark() {
        console.log("Woof!");
    }
}

Thus negating the need for *.extend(), but how quickly EMCAScript 6 will be adopted is anyone's guess.

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.

None yet

3 participants