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

[Feature] Add a .properties method to restrict which properties are tweened. #287

Closed
mikebolt opened this issue Aug 29, 2016 · 8 comments
Closed

Comments

@mikebolt
Copy link
Contributor

mikebolt commented Aug 29, 2016

My idea is to add a .properties method on the Tween class that can be chained like the other methods.

Example:

var myTween = new TWEEN.Tween(thingA).properties(['x', 'y', 'scale']).to(thingB, 500);

It could be especially useful for something like Three.js's MeshPhongMaterial that has lots of numeric properties. You could tween one material to another material, and specify that only the 'shininess', 'opacity', and 'emissiveIntensity' properties should be tweened.

There could also be a .exclude or .excludeProperties method that does the opposite.

I think this feature can be added without changing the existing behavior.

@dalisoft
Copy link
Collaborator

@mikebolt
When users call 1000 props/keys with value (objects, array not matter) users can get about 15fps on older pc,
But when 10 props/keys with value (objects, array not matter) users can get about 17-18fps on older pc.

Are you sure, add this feature?
Another processing of checking properties.

@mikebolt
Copy link
Contributor Author

Good point. However, it only needs to check the properties once in the .properties or .excludeProperties methods. Also, if you used this method to reduce the number of tweened properties, it could improve performance.

@dalisoft
Copy link
Collaborator

@mikebolt Yes, you right about performance, so we need do it in .start method to reduce repesting calls and GC

@samreid
Copy link

samreid commented Sep 11, 2016

The feature proposed in this issue would be very helpful to my group. We have a type with many properties which are expensive to read (from ES5 getter functions), but this is a type which we also often need to Tween (a small number of its attributes). The workaround we've been applying is to pick the attributes we want to animate like so:

  new TWEEN.Tween( { x: baseNode.x } )
    .to( { x: baseNode.x + proteinSynthesisScreenView.distanceMRNATranslated }, 3800 )
    .easing( TWEEN.Easing.Cubic.InOut )
    .onUpdate( function() {
      baseNode.x = this.x;
    } )
    .start( phet.joist.elapsedTime );

Context is here:
https://github.com/phetsims/protein-synthesis/blob/a912c6c3d88f0aeb18320861f9bbe020c90d64ea/js/protein-synthesis/view/ProteinSynthesisScreenView.js#L290-L298

This avoids the work of recording many many initial values that won't be needed. This workaround is not too terrible (we've employed it in half a dozen spots or so), but it would be nice to have built-in Tween support instead of resorting to a workaround.

EDIT: Removed unnecessary loop in example

@samreid
Copy link

samreid commented Sep 11, 2016

Perhaps one way to address this would be to add a 2nd "options" argument to the Tween constructor, which can contain attributes to be tweened. Then the above example would be written like so:

  new TWEEN.Tween( baseNode, { attributes: [ 'x' ] } )
    .to( { x: baseNode.x + proteinSynthesisScreenView.distanceMRNATranslated }, 3800 )
    .easing( TWEEN.Easing.Cubic.InOut )
    .start( phet.joist.elapsedTime );

EDIT: Removed unnecessary loop in example

@dalisoft
Copy link
Collaborator

This feature already have. Only in .to method properties is tweening. Others not

@trusktr trusktr added this to To do in General Project Board via automation Jun 3, 2020
@trusktr
Copy link
Member

trusktr commented Jun 3, 2020

Sorry for the late reply, but what @dalisoft is saying is that what you want to do @samreid already works, you just write this:

  new TWEEN.Tween( baseNode )
    .to( { x: baseNode.x + proteinSynthesisScreenView.distanceMRNATranslated }, 3800 )
    .easing( TWEEN.Easing.Cubic.InOut )
    .start( phet.joist.elapsedTime );

and this only going to animate the x property because the object that was passed into .to() only has an x property.


This request would be useful in making it easier to re-use existing objects in the to() method (like Mike mentioned, for example, just passing in materials that we might already have in a Three.js application) without having to clone them into subset objects.

Would it be worth it? (pull request PoC welcome!)

So something like following,

const matA = new THREE.MeshPhongMaterial(...)
const matB = new THREE.MeshPhongMaterial(...)

var tween = new TWEEN.Tween(matA)
  .to(matB, 500);

would currently cause all of the numerical properties on matA to be animated because it will read all properties on matB as the source of properties to animate. To avoid that, you would update the example to be

const matA = new THREE.MeshPhongMaterial(...)
const matB = new THREE.MeshPhongMaterial(...)

var tween = new TWEEN.Tween(matA)
  .properties(['opacity'])
  .to(matB, 500);

which would tell to animate only the opacity property of matA using matB.opacity as the destination value.

The current alternative is to do something @samreid already did in his example:

const matA = new THREE.MeshPhongMaterial(...)
const matB = new THREE.MeshPhongMaterial(...)

var tween = new TWEEN.Tween(matA)
  .to({opacity: matB.opacity}, 500);

which isn't slightly more verbose than .properties() would be because of the duplicate opacity and matB words.

Also note, with existing tools like underscore or lodash we could write

var tween = new TWEEN.Tween(matA)
  .to(_.pick(matB, 'opacity', 'foo', 'bar', 'baz', ...), 500);

@trusktr trusktr moved this from To do to Backlog in General Project Board Jun 3, 2020
@trusktr
Copy link
Member

trusktr commented Oct 26, 2020

I'm leaning to close this, as I haven't heard from other maintainers, and we can already do what is requested in different ways (f.e. make a new object (same cost as a new array anyway), or use _.pick for simpler syntax)

@trusktr trusktr closed this as completed Oct 26, 2020
General Project Board automation moved this from Backlog to Done Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

No branches or pull requests

5 participants