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

animation repository #360

Closed
pixelzoom opened this issue Sep 14, 2015 · 18 comments
Closed

animation repository #360

pixelzoom opened this issue Sep 14, 2015 · 18 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

I've used Tween in 2 sims - Hooke's Law, and now Function Builder. It's used in 13 other sims, where there is duplicate code and varying patterns to accomplish similar animation tasks. Proposal: Rather than embedding Tween this heavily in sims, how about writing wrappers for common animation tasks? This would reduce duplicated/different code, promote reuse, and make it easier to switch to some other animation library in the future. Wrapper examples in function-builder, FadeIn and FadeOut. Thoughts?

@pixelzoom
Copy link
Contributor Author

Skype discussion:

[9/14/15, 10:53:56 AM] Sam Reid: BRAINSTORM:

// Integrated support for animation in scenery. Specify target values and durati
var animationHandle = node.animateTo({x:100,y:100,opacity:0.5},{animationTime:100,type=‘quadratic’});

// Use the handle to communicate with the animation.
animationHandle.stop();


[9/14/15, 10:54:03 AM] Sam Reid: ALTERNATE BRAINSTORM:
[9/14/15, 10:55:22 AM] Sam Reid: // If we are too squeamish (negative connotation not intended) about integrating this into scenery.
var animationHandle = Animate.animate(node,{x:100,y:100,opacity:0.5},{animationTime:100,type=‘quadratic’});
[9/14/15, 10:56:21 AM] Chris Malley: So is that a "yes, a wrapper would be preferrable" ? :)
[9/14/15, 10:56:42 AM] Sam Reid: no, I think we should discuss the pros and cons of making this a scenery API. Scenery API may be best.
[9/14/15, 10:57:01 AM] Sam Reid: Unless we want to animate non-scenery nodes a lot (such as model objects).
[9/14/15, 10:57:09 AM] Chris Malley: Why couldn't scenery use tween?
[9/14/15, 10:57:27 AM] Sam Reid: It could.
[9/14/15, 10:57:34 AM] Chris Malley: Isn't that a wrapper?
[9/14/15, 10:57:51 AM] Sam Reid: > how about writing wrappers for common animation tasks?

The wrapper could be in the form of scenery node call.
[9/14/15, 10:58:00 AM] Chris Malley: My point is that using Tween directly in our code, and duplicating common animations, is a bad direction.
[9/14/15, 10:58:11 AM] Sam Reid: agreed
[9/14/15, 10:58:21 AM] Jonathan Olson: (on call, but using something tween-like in Scenery sounds fine with me. I've noted concerns about making TWEEN a dependency of Scenery before)
[9/14/15, 10:58:37 AM] Sam Reid: scenery API is only one way to get rid of using Tween directly in our code and duplicating common animations. We could use another approach as well.
[9/14/15, 10:59:02 AM] Sam Reid: In the end, we don’t have to use the Tween.js implementation.
[9/14/15, 10:59:09 AM] Chris Malley: How about a new 'animation' repo that depends on Scenery and (for now) Tween?
[9/14/15, 11:00:22 AM] Chris Malley: I would love to use the abstraction that I wrote for function-build (FadeIn, FadeOut) in hookes-law. Suggestions on how I might do that immediately without copying code?
[9/14/15, 11:01:34 AM] Sam Reid: Where do I look in hooke’s law?
[9/14/15, 11:03:46 AM] Chris Malley: IntroView. lines 85-155
[9/14/15, 11:04:34 AM] Chris Malley: compare with function-builder.PatternsView, lines 108-126
[9/14/15, 11:07:31 AM] Sam Reid: You are wondering about moving FadeIn/Fade out to a common repo, then reusing them in HL?
[9/14/15, 11:07:42 AM] Chris Malley: yes
[9/14/15, 11:08:00 AM] Chris Malley: seems like a new repo would be in order, with dependency on Tween.
[9/14/15, 11:08:33 AM] Sam Reid: I’d prefer to discuss as a team and come up with a comprehensive long term strategy. But for a short term, “in the meantime” solution, that would be OK with me.
[9/14/15, 11:09:12 AM] Chris Malley: Thinking about it more... I'd prefer an animation lib that is not integrated into Scenery. There are other things that can be animated. Eg, a Property value in the model.

@pixelzoom
Copy link
Contributor Author

Input from @samreid @jonathanolson @jbphet @aaronsamuel137 @jessegreenberg requested.

@pixelzoom
Copy link
Contributor Author

Related to #358 (managing Tween animations). If we had a common library for animations, this would be easier to assess and fix.

@samreid
Copy link
Member

samreid commented Sep 14, 2015

One issue for using Tween.js is that when an animation starts, it copies all properties of the animated object. This means calling getters for things like get x() get y() get matrix(), etc. This is why we have settled on the patten of passing an object literal instead of a scenery node to be tweened, then passing the values through to the scenery node.

One step we could in making a superior API would be to automatically create this proxy--that would help us get rid of a great deal of duplicated code. Or we could fork or rewrite the important tween.js behavior as desired. At one point I forked tween.js to use the keys in the target instance to determine which original values to copy from the animated object.

@pixelzoom
Copy link
Contributor Author

@samreid wrote:

This is why we have settled on the patten of passing an object literal instead of a scenery node to be tweened, then passing the values through to the scenery node.

You're referring to what is typically called the 'patterns' literal, correct? Eg:

var someNode = ...
var parameters = { opacity: 0 }; 
var tween = new TWEEN.Tween( parameters )
      .to( { opacity: 1 }, 500 )
      .onUpdate( function() {
        someNode.opacity = parameters.opacity;
      } )
...

@samreid
Copy link
Member

samreid commented Sep 14, 2015

I presume you meant the parameters literal. If so, yes.

@pixelzoom
Copy link
Contributor Author

Yep, parameters.

@samreid
Copy link
Member

samreid commented Sep 14, 2015

So it appears there are actually 2 issues at play here:

  1. Factoring out duplicated values/details
  2. Changing the public API

We should discuss both independently--TWEEN.js uses a different convention for its API than we use throughout our libraries.

@samreid
Copy link
Member

samreid commented Sep 14, 2015

My recommendations:

  • Create our own API that makes things easy for us and very consistent with the rest of our development process
  • After we have developed our desired API, then build adapters that use Tween.js or write our own tweening support.
  • After things are working with our new API and implementation, we will be able to judge and decide how to factor out duplicated variable/content/logic, if any.

@samreid
Copy link
Member

samreid commented Sep 24, 2015

From Sept 24 meeting: we would like the API to automatically handles cancelling previous animations (if any)

@pixelzoom
Copy link
Contributor Author

9/24/14 dev meeting:
@ariel-phet says that animation will be used more in design, so this is worth the longterm investment.

@pixelzoom pixelzoom changed the title animation repository? animation repository Sep 29, 2015
@pixelzoom
Copy link
Contributor Author

Unassigning for now. I'll revisit when I start adding animation to function-builder.

@pixelzoom pixelzoom removed their assignment Oct 9, 2015
@ariel-phet
Copy link

@pixelzoom just a ping since you are moving nicely on function builder

@pixelzoom
Copy link
Contributor Author

No plans to work on this in the immediate future. The animation functions in function-builder currently consist of only FadeIn and FadeOut. There will undoubtedly be others, but I'm not there yet. And designing a general library is definitely not on my radar at this point - and might not be until function-builder is well along.

@pixelzoom
Copy link
Contributor Author

3/10/16 dev meeting: It would be nice to be able to wrap Tween (versus using it directly in sim code) so that we can test the animation library when we update to a new version of Tween.

@samreid
Copy link
Member

samreid commented Mar 10, 2016

It looks like the new Tween.js has Universal Module Definition: https://github.com/tweenjs/tween.js/blob/master/src/Tween.js

// UMD (Universal Module Definition)
(function (root) {

    if (typeof define === 'function' && define.amd) {

        // AMD
        define([], function () {
            return TWEEN;
        });

    } else if (typeof module !== 'undefined' && typeof exports === 'object') {

        // Node.js
        module.exports = TWEEN;

    } else if (root !== undefined) {

        // Global variable
        root.TWEEN = TWEEN;

    }

})(this);

So perhaps it does not need to be a global.

@ariel-phet
Copy link

@pixelzoom can this issue be closed now that we have twixt?

@pixelzoom
Copy link
Contributor Author

Yes.

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

3 participants