tweenjs / tween.js

JavaScript/TypeScript animation engine
https://tweenjs.github.io/tween.js/
Other
9.82k stars 1.41k forks source link

Avoiding prototype values? #59

Closed collinhover closed 11 years ago

collinhover commented 11 years ago

Thanks for creating TWEENjs, it is fantastically useful! I've been using TWEENjs to do some THREE property tweening, and I noticed that while tweening the target object's properties are copied all the way up the prototype chain. Is this intended?

I ask because I was going between three ways of tweening properties, for example scale: a. Write an update function that caters to the specific properties and copies the values passed to "to()"... i.e. onUpdate scale.x = from.x, scale.y = from.y... but this is very clunky and not very dynamic. b. Pass the scale vector as the from object and a generic object as the target object, i.e. { x: 3, y: 3, z: 3 }... but this is also not very dynamic. c. Pass the scale vector as the from object and another scale vector as the target object.

Option c seems ideal to me, but the problem is TWEENjs is trying to tween all the target object's prototype properties. I'm not sure why you'd want to tween prototype properties, as that seems to go against the concept of prototyping, so what about the following:

this.start = function ( time ) {

    ...

    for ( var property in _valuesEnd ) {

        // This prevents the engine from interpolating null values
        if ( _valuesEnd.hasOwnProperty( property ) !== true || _object[ property ] === null ) {

            continue;

        }

        ...

    }

};

Alternatively, has using an object's "Keys" method been considered to speed up the Start iteration? See: https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Object/keys. This would avoid the above issue entirely, but also may not have the level of browser support you all are looking for.

sole commented 11 years ago

Hi!

Generally we prefer to have some sort of patch/pull request with code that implements the sort of changes you're suggesting, so that we can benchmark/evaluate them, etc.

If you have the time and feel like writing some code to test how this could work, feel free to go ahead! :-)

collinhover commented 11 years ago

I'll try to knock out a pull request as soon as I get a chance and update the issue.

sole commented 11 years ago

:-)

Thank you!

mrdoob commented 11 years ago

b. Pass the scale vector as the from object and a generic object as the target object, i.e. { x: 3, y: 3, z: 3 }... but this is also not very dynamic.

What do you mean with not very dynamic? That should be the right approach.

collinhover commented 11 years ago

For example, if I'm using a vector to track a target position/scale/rotation, before I can tween the actual property to the target (in a case where I would not want to lerp) I need to create an intermediate generic object with x/y/z properties and copy the x/y/z from the target vector. Why not just pass the vector instead? Wouldn't this save a very minor amount of memory as well?

collinhover commented 11 years ago

Assuming I did the pull request correctly, note the console in the following fiddles:

http://jsfiddle.net/EX5CX/ (current dev branch of tween.js) http://jsfiddle.net/EX5CX/1/ (avoiding prototype values)

mrdoob commented 11 years ago

Why not just pass the vector instead? Wouldn't this save a very minor amount of memory as well?

It's always a trade-off between computation and memory. Have you done any tests to see how much memory you're trying to save?

Your approach require more computation as now it needs to do _valuesEnd.hasOwnProperty( property ) !== true as well. Have you done any tests to see how this affects performance?

tween.js is more about performance than saving memory.

collinhover commented 11 years ago

Fair enough! See consoles in the following:

  1. Current TWEEN.Tween.start method: http://jsfiddle.net/wGd9c/1/
  2. Proposed TWEEN.Tween.start method: http://jsfiddle.net/zCPJg/1/
  3. Alternative TWEEN.Tween.start method: http://jsfiddle.net/yfujr/1/

Tested on latest release builds of Chrome and Firefox, 3 is consistently faster (on Chrome, much faster), while 1 and 2 perform equally. I did not propose 3 because the "Keys" method is not supported on less than IE9 and Opera12 (see: http://kangax.github.com/es5-compat-table/). I've heard the internal clock on Windows is not very accurate... but I would expect 2 to have much worse performance, not similar (see: http://jsperf.com/hasownproperty-performance-analysis/4). Also note that using methods 2 and 3, the tween update loop should be faster, as we ignore the prototype which accounts for half of the properties. Am I testing these wrong?

mrdoob commented 11 years ago

What I meant is that, the way tween.js is supposed to be used is by passing an object with only the values that need to be tweened. The way you're trying to use it requires more complex internal logic.

collinhover commented 11 years ago

Got it. I'm using Tween with object instances created via "new", and I wasn't aware Tween was intended to be used only with the basic Object. So this change is unnecessary as mentioned.