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

Seems like Tween.js won't animate getters/setters. #267

Closed
trusktr opened this issue Jun 12, 2016 · 28 comments
Closed

Seems like Tween.js won't animate getters/setters. #267

trusktr opened this issue Jun 12, 2016 · 28 comments

Comments

@trusktr
Copy link
Member

trusktr commented Jun 12, 2016

I don't know why, but in my case the object who's values I'm trying animate isn't being animated. I have code like this:

let positionTween = new TWEEN.Tween(letterNode.position)
    .to(cellPosition, 1000)
    .onUpdate(function() {
        console.log('   final position', cellPositions)
        console.log('     tween value?', this)
        console.log('  actual position', letterNode.position)
    })
    .onComplete(function() { positionTween.completed = true })

let task = Motor.addRenderTask(time => {
    if (!positionTween.started) {
        positionTween.start(time)
        positionTween.started = true
    }
    else positionTween.update(time)

    if (positionTween.completed) {
        console.log('completed')
        Motor.removeRenderTask(Task)
    }
})

The problem is demonstrated by the console.log output.

screen shot 2016-06-12 at 2 05 23 am

In the screenshot we can see what the final position should be, and what the actual tween values are. For some reason, the actual values aren't changing, and are not approaching the expected end values.

Am I missing something?

@trusktr
Copy link
Member Author

trusktr commented Jun 12, 2016

I added another console.log statement to show that the object I'm trying to animate is the same object as this inside of onUpdate:

let positionTween = new TWEEN.Tween(letterNode.position)
    .to(cellPosition, 1000)
    .onUpdate(function() {
        console.log('   final position', cellPositions)
        console.log('     tween value?', this)
        console.log('  actual position', letterNode.position)
        console.log('     same object?', letterNode.position === this) // true!
    })
    .onComplete(function() { positionTween.completed = true })

let task = Motor.addRenderTask(time => {
    if (!positionTween.started) {
        positionTween.start(time)
        positionTween.started = true
    }
    else positionTween.update(time)

    if (positionTween.completed) {
        console.log('completed')
        Motor.removeRenderTask(Task)
    }
})

and the output shows true. So, that seems to be all good, but I don't understand why the values aren't changing:

screen shot 2016-06-12 at 2 18 39 am

By the way, the x, y, and z properties that I'm trying to animate are getters/setters. For example, if I open one of the XYZValues objects, you can see it has the matching getters:

screen shot 2016-06-12 at 2 19 51 am

The setters/getters store values into the corresponding underscored properties.

@trusktr
Copy link
Member Author

trusktr commented Jun 12, 2016

I was able to work around the issue: instead of providing the actual object to new Tween, I made a made a new object, then update the values in onUpdate, and it works! So I changed the code to this:

let positionTween = new TWEEN.Tween({
    x: letterNode.position.x,
    y: letterNode.position.y,
    z: letterNode.position.z,
})
    .to(cellPosition, 1000)
    .onUpdate(function() {
        console.log('   final position', cellPositions)
        console.log('     tween value?', this)
        console.log('  actual position', letterNode.position)
        console.log('     same object?', letterNode.position === this) // true!

        letterNode.position.x = this.x
        letterNode.position.y = this.y
        letterNode.position.z = this.z
    })
    .onComplete(function() { positionTween.completed = true })

let task = Motor.addRenderTask(time => {
    if (!positionTween.started) {
        positionTween.start(time)
        positionTween.started = true
    }
    else positionTween.update(time)

    if (positionTween.completed) {
        console.log('completed')
        Motor.removeRenderTask(Task)
    }
})

Any idea why it works when I do it the second way, but not the first way?

@trusktr trusktr changed the title Object passed to new Tween is not being modified. Seems like Tween.js won't animate getters/setters. Jun 12, 2016
@trusktr
Copy link
Member Author

trusktr commented Jun 12, 2016

I've been pulling y hair out twiddling with my code for an hour or two, until finally it worked when just using a new object instead of the target object directly. But, I'd like to use the target object directly because it's cleaner.

@mikebolt
Copy link
Contributor

This is probably a bug in Tween.js. If you could make a codepen that reproduces the issue that would be a big help, because then I could just step through the update function.

If those are all the properties and methods of XYZValues, then I might be able to reproduce this on my own.

@mikebolt
Copy link
Contributor

I was unable to reproduce this issue. Animating properties that have both a getter and setter method seems to work for me. Here is my attempt to reproduce the issue:

https://github.com/mikebolt/tween.js/blob/issue267/examples/002_get_and_set_closure.html

I will need the code for the XYZValues constructor to properly investigate this issue.

@sole
Copy link
Member

sole commented Jun 20, 2016

Could it be a combination of trying to guess which properties are in the object itself, in https://github.com/tweenjs/tween.js/blob/master/src/Tween.js#L117-L119 and then values in the object at animation time in https://github.com/tweenjs/tween.js/blob/master/src/Tween.js#L160 ?

@trusktr
Copy link
Member Author

trusktr commented Jun 20, 2016

@sole I ran a small test in Chrome console:

let o = {
  set n(v) {this._n = v},
  get n() {return this._n}
}
o.n = 5
for (var i in o) console.log(i)

The for-in loop does indeed log the n getter/setter, so it does see it.

Also, none of my starting values are undefined. The object that I've passed into TWEEN.Tween has pre-defined numerical values set by default when constructed, so that check for undefined doesn't seem like it will throw anything off.

I'm not sure what's happening. Maybe I can put a bunch of console.logs everywhere and see the difference in output with literal properties vs getters/setters... (gotta find some time for it, I'll be back, noted it in my relevant source code for when I get back to it).

@sole
Copy link
Member

sole commented Jun 21, 2016

Thanks for looking into it.

When the library was first written there was no way of having setters and getters on JS :-)

@brenwell
Copy link

brenwell commented Jun 22, 2016

Hey everyone, I have been using tween.js with pixi.js for a while now and have been having the same issues with the latest version (I believe).

toObj = {
      y: 800,  
      alpha: 0, 
      rotation: Math.random()
}
new TWEEN.Tween(snowflake)
.to(toObj,1400)
.start()  

The Y value is nolonger interpolated via the setter. I have to split it to get it to work. Not sure if its an intentional change.

toObj = {
      alpha: 0, 
      rotation: Math.random()
}
new TWEEN.Tween(snowflake)
.to(toObj,1400)
.start()  

toPosObj = {
      y: 800
}
new TWEEN.Tween(snowflake.position)
.to(toPosObj,1400)
.start() 

Just throwing my 2 cents in here. Thanks

@mikebolt
Copy link
Contributor

Maybe the problem is caused by transpiling the code? To emulate getters and
setters it would have to replace certain assignment expressions with
function calls, but it would not be able to replace e.g. obj[propertyName] = 4 with its appropriate setter unless it knew the value of propertyName.
On Jun 22, 2016 7:11 AM, "Brenwell" notifications@github.com wrote:

Hey everyone, I was have been using tween.js with pixi.js for a while now
and have been having the same issues, I believe, with the latest version.

toObj = {
y: 800,
alpha: 0,
rotation: Math.random()
}new TWEEN.Tween(snowflake).to(toObj,1400).start()

The Y value is nolonger interpolated via the setter. I have to split it to
get it to work. Not sure if its an intentional change.

toObj = {
alpha: 0,
rotation: Math.random()
}new TWEEN.Tween(snowflake).to(toObj,1400).start()
toPosObj = {
y: 800
}new TWEEN.Tween(snowflake.position).to(toPosObj,1400).start()

Just throwing my 2 cents in here. Thanks


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#267 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ACCav7G-a09NRhB9lG5m4XghxWbKnZTeks5qOUKfgaJpZM4Izvnu
.

@mikebolt
Copy link
Contributor

I meant to ask if you were transpiling the code. If so, can you post the
unminified transpiled version?
On Jun 22, 2016 10:21 AM, "Michael Casebolt" mikebolt@gmail.com wrote:

Maybe the problem is caused by transpiling the code? To emulate getters and
setters it would have to replace certain assignment expressions with
function calls, but it would not be able to replace e.g. obj[propertyName] = 4 with its appropriate setter unless it knew the value of propertyName.
On Jun 22, 2016 7:11 AM, "Brenwell" notifications@github.com wrote:

Hey everyone, I was have been using tween.js with pixi.js for a while now
and have been having the same issues, I believe, with the latest version.

toObj = {
y: 800,
alpha: 0,
rotation: Math.random()
}new TWEEN.Tween(snowflake).to(toObj,1400).start()

The Y value is nolonger interpolated via the setter. I have to split it to
get it to work. Not sure if its an intentional change.

toObj = {
alpha: 0,
rotation: Math.random()
}new TWEEN.Tween(snowflake).to(toObj,1400).start()
toPosObj = {
y: 800
}new TWEEN.Tween(snowflake.position).to(toPosObj,1400).start()

Just throwing my 2 cents in here. Thanks


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#267 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ACCav7G-a09NRhB9lG5m4XghxWbKnZTeks5qOUKfgaJpZM4Izvnu
.

@trusktr
Copy link
Member Author

trusktr commented Jun 22, 2016

I'm transpiling with a recent version of Babel. Setters/getters aren't transpiled by default (I'm using the same settings).

It seems to be a runtime issue.

@trusktr
Copy link
Member Author

trusktr commented Jun 22, 2016

@brenwell In your case is a different issue: there's no way the y value in the toObj can map to position.y of the snowflake. Tween.js looks for the matching property names on the object passed to the Tween constructor, and snowflake.y doesn't exist, which is what Tween.js looks for based on the toObj. That's why it works when you pass the snowflake.position object to the Tween constructor.

@martikaljuve
Copy link
Contributor

martikaljuve commented Jul 7, 2016

The problem seems to be that properties are only enumerable when using simple assignment or object initializers, so Tween.js cannot find the starting values in its for...in loop.

An object initializer creates enumerable properties:

var obj = {
  get x() { return this._x; },
  set x(value) { this._x = value; }
};

console.log(obj.propertyIsEnumerable('x')); // true

Using a class or Object.defineProperty creates non-enumerable properties:

class Obj {
  get x() { return this._x; }
  set x(value) { this._x = value; }
}

var obj2 = new Obj();

console.log(obj2.propertyIsEnumerable('x')); // false

One solution is to copy only those starting values from the target object that have destination values in .to(properties, duration), as can be seen in martikaljuve@a50474b. This works because the first argument of .to is usually a simple object with enumerable properties.
Unit tests are passing, but I haven't considered if/how this would break other use cases.

@dalisoft
Copy link
Collaborator

@trukstr Try to enable {writable:1,enumerable:1} to setter to allow change and tween (tween only numbers)

@mikebolt
Copy link
Contributor

@martikaljuve That fix looks good to me. The behavior would change: the tweened object's starting properties would be saved when .to is called instead of when the tween is constructed. We can bump the major version number in npm to indicate that there is a minor backwards compatibility issue. I don't think it's a major break because we never specified in the documentation exactly when the starting properties are saved.

We need some new unit tests for tweening getter/setter properties before I can pull the fix and close this issue.

@trusktr
Copy link
Member Author

trusktr commented Aug 30, 2016

@martikaljuve

Using a class or Object.defineProperty creates non-enumerable properties:

That seems horrible.

@dalisoft

Try to enable {writable:1,enumerable:1} to setter to allow change and tween (tween only numbers)

It works! But the solution is ugly when using classes. For example, this is what one of my classes looks like now:

class XYZValues {
    constructor(x = 0, y = 0, z = 0) {
        this._x = x
        this._y = y
        this._z = z
    }

    onChanged() {}

    set x(value) {
        this._x = value
        this.onChanged()
    }
    get x() { return this._x }

    set y(value) {
        this._y = value
        this.onChanged()
    }
    get y() { return this._y }

    set z(value) {
        this._z = value
        this.onChanged()
    }
    get z() { return this._z }
}

const xDesc = Object.getOwnPropertyDescriptor(XYZValues.prototype, 'x')
xDesc.enumerable = true
Object.defineProperty(XYZValues.prototype, 'x', xDesc)

const yDesc = Object.getOwnPropertyDescriptor(XYZValues.prototype, 'y')
yDesc.enumerable = true
Object.defineProperty(XYZValues.prototype, 'y', yDesc)

const zDesc = Object.getOwnPropertyDescriptor(XYZValues.prototype, 'z')
zDesc.enumerable = true
Object.defineProperty(XYZValues.prototype, 'z', zDesc)

export {XYZValues as default}

@trusktr
Copy link
Member Author

trusktr commented Aug 30, 2016

I made a small helper, which I guess I'll be using all the time with classes that I intend to animate with Tween.js:

// Utility.js
export function makeAccessorsEnumerable(object) {
    const props = Object.getOwnPropertyNames(object)
    for (let prop of props) {
        const descriptor = Object.getOwnPropertyDescriptor(object, prop)
        if (descriptor && (descriptor.set || descriptor.get)) {
            descriptor.enumerable = true
            Object.defineProperty(object, prop, descriptor)
        }
    }
}

then the code is cleaner:

import {makeAccessorsEnumerable} from './Utility'

class XYZValues {
    constructor(x = 0, y = 0, z = 0) {
        this._x = x
        this._y = y
        this._z = z
    }

    onChanged() {}

    set x(value) {
        this._x = value
        this.onChanged()
    }
    get x() { return this._x }

    set y(value) {
        this._y = value
        this.onChanged()
    }
    get y() { return this._y }

    set z(value) {
        this._z = value
        this.onChanged()
    }
    get z() { return this._z }
}

// This is needed in order for Tween.js to detect and use the accessors,
// otherwise Tween.js won't be able to tween them.
makeAccessorsEnumerable(XYZValues.prototype)

export {XYZValues as default}

@dalisoft
Copy link
Collaborator

Whoops, i am not a advanced developer. My mine limited. I am not think even about Classes, because don't know about it. I hope you find way to work it

@martikaljuve
Copy link
Contributor

@trusktr, you could also use ES7 decorators:

// utils/decorators.js
export function enumerable(target, key, descriptor) {
    descriptor.enumerable = true;
    return descriptor;
}
// xyz_values.js
import { enumerable } from './utils/decorators';

export default class XYZValues {
    constructor(x = 0, y = 0, z = 0) {
        this._x = x;
        this._y = y;
        this._z = z;
    }

    onChanged() {}

    @enumerable
    get x() { return this._x; }
    set x(value) { this._x = value; this.onChanged(); }

    @enumerable
    get y() { return this._y; }
    set y(value) { this._y = value; this.onChanged(); }

    @enumerable
    get z() { return this._z; }
    set z(value) { this._z = value; this.onChanged(); }
}

@trusktr
Copy link
Member Author

trusktr commented Aug 31, 2016

@martikaljuve Decorators would be nice! But seems the spec is being changed, and Babel dropped support for now, so will wait on that. I wanted to use decorators for some other things too, but just waiting.

@trusktr
Copy link
Member Author

trusktr commented Aug 31, 2016

@martikaljuve In your example, would

    @enumerable
    get z() { return this._z; }
    set z(value) { this._z = value; this.onChanged(); }

be equivalent to

    get z() { return this._z; }
    @enumerable
    set z(value) { this._z = value; this.onChanged(); }

or

    @enumerable
    get z() { return this._z; }
    @enumerable
    set z(value) { this._z = value; this.onChanged(); }

?

Is there some rule, like if you apply the decorator to one of the get/set pair then it applies to both?

@trusktr
Copy link
Member Author

trusktr commented Aug 31, 2016

@sole For the documentation, maybe you can just make clear that Tween.js works with enumerable properties, and briefly describe that with getter/setters on classes we have to modify the descriptors so they are enumerable. You could show my makeAccessorsEnumerable helper, if that would help.

@martikaljuve
Copy link
Contributor

@trusktr, the decorator most likely works the same whether it's on the getter or the setter. babel-plugin-transform-decorators-legacy transpiles it to Object.getOwnPropertyDescriptor(XYZValues.prototype, 'z') in both cases.

@sole
Copy link
Member

sole commented Oct 3, 2016

Thanks for the investigations!

@trusktr: what do you think about making a PR with these suggestions, as you can express your findings more easily than me? That would be lovely

@simonmurdock
Copy link

simonmurdock commented Oct 12, 2016

Ran into this issue also, Using a transpiled Aurelia application, and when trying to tween properties on a singleton class injected into my class it won't work. Spent an hour wondering WTF is going on.

Worked when I used an intermediate object like @trusktr suggested. Have to have my code like this now: directly tweeting globals.entireOffset did nothing.

let tweenFrom = {
  x: this.globals.entireOffset.x,
  y: this.globals.entireOffset.y
};

let tweenTo = {
  x: this.globals.entireOffset.x += this.dragOffset.x,
  y: this.globals.entireOffset.y += this.dragOffset.y
};

let tween = new TWEEN.Tween(tweenFrom);

tween.to(tweenTo, 500);
let that = this;
tween.onUpdate(function () {
  that.globals.entireOffset.x = this.x;
  that.globals.entireOffset.y = this.y;
});
tween.start();

@mikebolt
Copy link
Contributor

This issue should be fixed now with jonobr's PR #291. @martikaljuve's fix would have worked great too. In retrospect I should have merged it, and neither jonobr nor simonmurdock would have ran into this issue. I hope you don't blame me for being confused :)

As a side note, if you are using the old version of the code and defining properties with Object.defineProperty, you can make the properties enumerable by passing "true" as the second parameter. I think this is the workaround that jonobr used.

@trusktr
Copy link
Member Author

trusktr commented Sep 16, 2017

Cool! I don't need to make my class accessors enumerable now, and @simonmurdock you don't need your workaround anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants