mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.06k stars 35.34k forks source link

Vertex to extend Vector3? #1703

Closed mrdoob closed 12 years ago

mrdoob commented 12 years ago

Right now Vertex has just a position property which is a Vector3. Historically Vertex had more properties but we have been moving them out to Geometry and position is the only one left.

I've seen some people "struggling" with this already. Somehow they expect to be able to do:

var vertex = new THREE.Vertex( Math.random() * 100 - 50, Math.random() * 100 - 50, Math.random() * 100 - 50 );

or

var vertex = new THREE.Vertex();
vertex.x = Math.random() * 100 - 50;
vertex.y = Math.random() * 100 - 50;
vertex.z = Math.random() * 100 - 50;

I wonder if it'd be better to just have Vertex extending Vector3?

mrdoob commented 12 years ago

Of course, the mess this change would create is guaranteed ;)

alteredq commented 12 years ago

Or maybe just drop Vertex altogether and just use Vector3?

alteredq commented 12 years ago

Of course, the mess this change would create is guaranteed ;)

Hehe, indeed. Any such change would also need to be throughly tested for performance / memory effects. This is at the core of everything, it could go either way, better / worse / not much changed.

mrdoob commented 12 years ago

Or maybe just drop Vertex altogether and just use Vector3?

Yeah, I thought about that, but It's API-wise nicer to have this:

geometry.vertices.push( new THREE.Vertex( 10, 10 10 ) );

than this:

geometry.vertices.push( new THREE.Vector3( 10, 10 10 ) );

Even if it would virtually be the same.

alteredq commented 12 years ago

Yes, API-wise it's nicer indeed.

Just who knows with modern JS engines which effect it has on performance, I have no idea, usually the simpler is the better, gives more chance to let the engine do the optimizations.

mrdoob commented 12 years ago

Hehe, indeed. Any such change would also need to be throughly tested for performance / memory effects. This is at the core of everything, it could go either way, better / worse / not much changed.

Well, in theory it should be better as it's an object less to create. But yeah, who knows...

I may give it a go during r50dev.

sole commented 12 years ago

As a member of the struggling group I can only "upvote" the change.

Three.js is quite intuitive, except for a couple of things like this :-)

chandlerprall commented 12 years ago

I would appreciate changing it to extend Vector3. I've hit that pitfall many times, thinking the vertex itself has x,y,z properties.

alteredq commented 12 years ago

Well, in theory it should be better as it's an object less to create. But yeah, who knows...

I meant pure Vector3 vs Vertex as subclass of Vector3.

mrdoob commented 12 years ago

Ah, right.

zz85 commented 12 years ago

Flattening Vertex + Vector would be a good thing :) I'm just trying to think of any cases

geometry.vertices.push( new THREE.Vertex( 10, 10 10 ) ); is better than geometry.vertices.push( new THREE.Vector3( 10, 10 10 ) );

The subclassing of javascript objects is a interesting question - with subclassed objects, javascript would supposedly require to lookup different prototypical objects for methods and properties. I remember some advice a while ago is to reduce subclassing if possible - I wonder how this advice fare with optimizations in browsers these days...

abstractalgo commented 12 years ago

I vote for not changing.

mrdoob commented 12 years ago

I vote for not changing.

The reason being... ?

abstractalgo commented 12 years ago

Sorry for that. It's more intuitive to add Vertex to geometry and Vector3 for calculating angles, directions and rays.

It's the same, yeah, but Vector is (0,0,0) -> (x,y,z), while Vertex is just point in space (x,y,z). It would be much more confusing if one is deriving the other. If the complexity of code will be lower, and rendering faster for some reason, than who am I to complain. :)

mrdoob commented 12 years ago

Right now you have to do this:

vertices[0].position.x = 10;

What we're suggesting is this:

vertices[0].x = 10;
chandlerprall commented 12 years ago

I put together a simple test to check the constructor & method call for an inherited object.

Either the way I'm doing it is way off or using inherited objects is a very slow method. I'm seeing ~200ms for the Vector3 and ~650ms for Vertex (all reports are made to console).

alteredq commented 12 years ago

Either the way I'm doing it is way off or using inherited objects is a very slow method. I'm seeing ~200ms for the Vector3 and ~650ms for Vertex (all reports are made to console).

Hmmm, very interesting.

I get ~160 ms for Vector3 and ~520 ms for Vertex in Chrome and ~260 ms for Vector3 and whopping ~1,400 ms for Vertex in Firefox.

zz85 commented 12 years ago

win 7 (now that im without my air)

ff 11, Vector3: 7577ms Vertex: 18553ms

chrome 18 Vector3: 200ms Vertex: 617ms

canary 20 Vector3: 155ms Vertex: 262ms

now i added a 3rd child

  VV = function() { Vertex.apply( this, arguments ) };
    VV.prototype = new Vertex;
    VV.prototype.constructor = VV;

    function testVV() {
        console.time( 'VV' );
        for ( i = 0; i < 10000000; i++ ) {
            j = new VV( 0, 0, 0 );
            j.addScalar( 1 );
        }
        console.timeEnd( 'VV' );
    }
    testVV();

even on canary - another whopping 10617ms. looks like the advice still holds true now.

zz85 commented 12 years ago

hmm... do this

    Vertex = function() { 
// nothing
 };
    Vertex.prototype = new Vector3;
    Vertex.prototype.constructor = Vertex;

and big speed up...

zz85 commented 12 years ago

okay one more comparison to make..

function testVinsideV() {
        console.time( 'VinsideV' );
        for ( i = 0; i < 10000000; i++ ) {
            j = new Object();
            j.p = new Vector3( 0, 0, 0 );
            j.p.addScalar( 1 );
        }
        console.timeEnd( 'VinsideV' );
    }

testVinsideV();

VinsideV: 552ms

This emulates the current way we have vertex.position. Which is slower compare to plain old Vector.

One more thing, I realized the numbers sometimes don't make sense if you click on Vertex first then Vectors. So remember to refresh the browser everytime you run a test...

chandlerprall commented 12 years ago

hmm... do this

Vertex = function() { // nothing }; Vertex.prototype = new Vector3; Vertex.prototype.constructor = Vertex;

and big speed up...

But then the object's x/y/z properties aren't set :)

idflood commented 12 years ago

I would vote for the most simple and fastest solution: keep only the vector3 and mark the vertex as deprecated. Having two identical classes may be a good cause for confusion and unexpected errors.

On a side note it seems that having firebug opened in firefox will make it disable JIT so I created a quick jsperf test case based on the previous comments: http://jsperf.com/three-js-vertex-vector3#nojava

mrdoob commented 12 years ago

This one also has the current style of Vertex: http://jsperf.com/three-js-vertex-vector3/2

alteredq commented 12 years ago

And here is one also with raw JS arrays and raw typed arrays (what @toji suggested in his physics post):

http://jsperf.com/three-js-vertex-vector3/3

Somehow surprisingly typed arrays perform really really bad for me.

chandlerprall commented 12 years ago

Somehow surprisingly typed arrays perform really really bad for me.

See this comment on cannon.js - https://github.com/schteppe/cannon.js/issues/8#issuecomment-5097980

mrdoob commented 12 years ago

Somehow surprisingly typed arrays perform really really bad for me.

Yup, TypedArray seems to be the slowest here too...

Anyway, looking at the results, seems like just using Vector3 is the way to go... VertexC is close but, as far as I understand, is wrong as it's not setting any values when initialising. VertexD is an improvement from OldVertex, but not as nice as Vector3...

Specially like the 2-5x performance boost on mobile browsers.

alteredq commented 12 years ago

Ehm, try this one (I added another option which seems really crazy):

http://jsperf.com/three-js-vertex-vector3/3

mrdoob commented 12 years ago

Ehm, try this one (I added another option which seems really crazy):

http://jsperf.com/three-js-vertex-vector3/3

Crazy. Hard to build an API on top though. It'd require a loooot of changes.

http://jsperf.com/three-js-vector3-static-vector3-object

toji commented 12 years ago

The benchmark results aren't quite as straightforward as one would think. You need to carefully consider the libraries usage of vector classes here.

For one, it should be pointed out that Altered's original test made two minor mistakes that made the TypedArray look worse than it should have otherwise. For one, it wasn't initialized with a size. Given that I'm actually not even sure that it was working at all. Changing it to new Float32Array(3); is more accurate and closer in functionality to the other samples.

Secondly, in all other samples you were adding 1 to 0, which means that the JS engine was likely optimizing those variables into ints. The Float32Array, being explicitly floating point, was the only version not doing integer math which made it appear much slower. That's not a realistic scenario for most 3D apps, so a more accurate test would be to add a floating point value instead of 1 to force the math in all cases to be floating point.

I've made both the above changes here: http://jsperf.com/three-js-vertex-vector3/5

That evens the playing field a bit, but the TypedArray still loses out by a significant margin. This is because TypedArray's are fairly expensive to create. If you redo the test to focus on re-using cached variables a very different picture emerges:

http://jsperf.com/three-js-vertex-vector3/4

On my system the JS Array variant comes out on top with that test, which is somewhat curious but not terribly shocking. TypedArray is very close behind, however, and every object variant is some 80-90% slower.

So this leaves you with a question - What is your library more likely to do? If you are creating new vectors constantly you're probably better off with one of the object variants, since they can be constructed in a more lightweight fashion than the arrays. If you tend to keep vectors around and reuse them for calculations over and over again you'll greatly benefit from using arrays instead.

Of course, what your library does internally and what your users do with it may be very different, but that's what best practices guides are for! :)

One final thing to consider: In many cases WebGL will force you to take that expensive hit of TypedArray creation to pass data into anyway. We've seen here that the cost to initialize a TypedArray is non-trivial, so it may be in your best interest to either use them from the start (my preferred method) or keep a cache of them around explicitly for passing certain data structures (vectors/matrices) into WebGL.

mrdoob commented 12 years ago

Awesome! Thanks Brandon!

As a side note, there is something weird with JS object on Windows, it seems to run way to fast on Windows. I haven't double checked, but my JS object results aren't as impressive.

On Linux TypedArrays seem to be 5x faster than JS objects.

mrdoob commented 12 years ago

So... after all this, I don't know what to do any more :)

I guess deprecating Vertex and using Vector3. And then consider using TypedArrays for Matrix4 as in #1688?

alteredq commented 12 years ago

I guess deprecating Vertex and using Vector3.

Something like that, our current way seemed to be among the worst one, so this change should be for better and still relatively simple (compared to pandemonium that would be changing everything to typed arrays or JS objects).

And then consider using TypedArrays for Matrix4 as in #1688?

This is certainly worth considering. @toji's glMatrix uses typed arrays and it's nowadays the fastest JS math lib out there.

mrdoob commented 12 years ago

I guess deprecating Vertex and using Vector3.

Something like that, our current way seemed to be among the worst one, so this change should be for better and still relatively simple (compared to pandemonium that would be changing everything to typed arrays or JS objects).

I'll get going then... :)

mrdoob commented 12 years ago

I'll do this though:

THREE.Vertex = THREE.Vector3;

If that ends up creating problems removing it would be easy. But this way the performance is the same and the API stays nice.

chandlerprall commented 12 years ago

FWIW - it appears a lot of the extra time taken has to do with scoping & locating the right object. Adding a closure helps to a degree, but not enough for the extended object to have a fighting chance. Interestingly enough, using window.Vector3.apply bumps it from ~64ms to ~185ms

Vector3

    var Vector3 = function( x, y, z ) {
        this.x = x;
        this.y = y;
        this.z = z;
    };

    var i, j;
    console.time('Vector3');
    for ( i = 0; i < 1000000; i++ ) {
        j = new Vector3( 1, 2, 3 );
    }
    console.timeEnd('Vector3');

Average time = 23ms

(function() {
    var Vector3 = function( x, y, z ) {
        this.x = x;
        this.y = y;
        this.z = z;
    };

    var i, j;
    console.time('Vector3');
    for ( i = 0; i < 1000000; i++ ) {
        j = new Vector3( 1, 2, 3 );
    }
    console.timeEnd('Vector3');
})();

Average time = 12ms

Vertex

    var Vector3 = function( x, y, z ) {
        this.x = x;
        this.y = y;
        this.z = z;
    };

    var Vertex = function() { Vector3.apply( this, arguments ) };
    Vertex.prototype = new Vector3;
    Vertex.prototype.constructor = Vertex;

    var i, j;
    console.time('Vertex');
    for ( i = 0; i < 1000000; i++ ) {
        j = new Vertex( 1, 2, 3 );
    }
    console.timeEnd('Vertex');

Average time = 64ms

(function() {
    var Vector3 = function( x, y, z ) {
        this.x = x;
        this.y = y;
        this.z = z;
    };

    var Vertex = function() { Vector3.apply( this, arguments ) };
    Vertex.prototype = new Vector3;
    Vertex.prototype.constructor = Vertex;

    var i, j;
    console.time('Vertex');
    for ( i = 0; i < 1000000; i++ ) {
        j = new Vertex( 1, 2, 3 );
    }
    console.timeEnd('Vertex');
})();

Average time = 55ms

So while closures can be awful when used incorrectly (especially memory wise) they can speed up your variable lookups a fair amount.

Wilt commented 8 years ago

In the end THREE.Vertex disappeared in favor of THREE.Vector3 right?

I liked @MrDoob his THREE.Vertex = THREE.Vector3; though. Were there issues with this?

mrdoob commented 8 years ago

@Wilt Just added a message.