Review Transition and TransitionNode

[samreid]

From https://github.com/phetsims/area-model-common/issues/99, I need to review the new common code Transition and TransitionNode

[samreid]

I've made a few REVIEW comment commits in TransitionNode and Transition. I still have further to go to understand and review these files.

[samreid]

I tried changing createSomething to:

    function createSomething() {
      return new HSlider(new Property(0),{min:-10,max:10},{
        center: bounds.center
      });
    }

And saw unexpected behavior for all the transitions except dissolve. Am I supposed to embed the content in something that has exactly the same bounds as given to the TransitionNode?

[samreid]

I should also note that Animation.js and AnimationTarget.js still has not been reviewed: https://github.com/phetsims/twixt/issues/3

[samreid]

Here are the files that need to be reviewed, and their number of lines of code per file:

     311 ./Transition.js
     359 ./AnimationTarget.js
     166 ./demo/TransitionDemoView.js
     162 ./demo/AnimationView.js
     256 ./TransitionNode.js
     365 ./Animation.js

I skimmed through everything and made initial top-level review comments, and asked some questions. Now that I better understand the scope, I thought it would be good to bring it up to @ariel-phet for discussion and planning.

The reason this is coming up now is that @jonathanolson (wisely) created Transition/TransitionNode to support the slide and dissolve animations for area-model, and I was reviewing that simulation. TransitionNode uses Transition, which extends Animation, which uses AnimationTarget. I do not know if these latter two have been used in a simulation before. They are the initial draft for our own animation library.

As part of moving toward TransitionNode, we would like to deprecate SlidingScreen #10, and replace usages with TransitionNode. That will be an important part of the review process for TransitionNode. Please review my notes in #10 about how I recommend to proceed for that part.

If the decision is for me to take another pass at reviewing this code, I'd recommend to have @jonathanolson address existing // REVIEW questions and comments first (and triage/complete TODOs that remain).

@ariel-phet let me know where you want to go with this.

[jonathanolson]

And saw unexpected behavior for all the transitions except dissolve. Am I supposed to embed the content in something that has exactly the same bounds as given to the TransitionNode?

It would be good to discuss this situation. The current behavior assumes that it can completely change the transform of what is passed in, so it immediately wipes away the centering that was done. This could be solvable by always wrapping every content node (or having an option to do so).

Thoughts?

[jonathanolson]

// REVIEW: Technically these do not need to be nodes, right?

I was thinking that since all of the common implementations would be Node-specific (that I imagined), that I would type it this way. The code Transition type itself could technically use other types. Is that something we want to use someday (so it would be typed as such)?

[jonathanolson]

start( dt ) - param {number} [dt] - If provided, step this far into the animation initially. REVIEW: Why would someone want to do this? It sounds unnecessary.

It is used in Animation itself for chaining animations together. It avoids having to call step( 0 ) in start() and then step( dt ) immediately after, which would be wasted effort.

[jonathanolson]

// REVIEW: Can we mention property, setValue and object as keys with null values here, and change the corresponding // REVIEW: assertion statement? It seems it would be a clearer API.

There are 13 different AnimationTarget "options" (will be a config). It seems unclean to mention all of them in Animation's options also?

[jonathanolson]

// REVIEW: Shape.bounds purportedly returns a Bounds2, and so does Bounds2.blend. So can we remove the Shape.bounds() part?

The Shape.bounds is the reason why we can't just use a "default" blend, since we take two Bounds2 objects and return a Shape. What are you suggesting here?

[jonathanolson]

Responded to all REVIEW notes for twixt, let me know if it would be easier to have a call to discuss.

[ariel-phet]

@samreid looks like it is back to you since all REVIEW comments were addressed/responded.

[samreid]

I'm more familiar with Animation/AnimationTarget/Transition/TransitionNode. Generally, it seems like it is in good shape, well documented and general and I think it will be a good foundation for us to build on. My main concern is about how to discover which options are applicable.

For instance, slideDownTo options are described like this:

    /**
     * Start a transition to replace our content with the new content, using Transition.slideDown.
     * @public
     *
     * @param {Node|null} content - If null, the current content will still animate out (with nothing replacing it).
     * @param {Object} options - Passed as options to the Animation. Usually a duration should be included.
     */
    slideDownTo: function( content, options ) {
      this.startTransition( content, Transition.slideDown( this.boundsProperty.value, this.fromContent, content, options ) );
    },

Then slideDown says:

    /**
     * Creates an animation that slides the `fromNode` out to the bottom (and the `toNode` in from the top).
     * @public
     *
     * @param {Bounds2} bounds
     * @param {Node|null} fromNode
     * @param {Node|null} toNode
     * @param {Object} [options] - Usually specify duration, easing, or other animation attributes.
     * @returns {Transition}
     */
    slideDown: function( bounds, fromNode, toNode, options ) {
      return Transition.createSlide( fromNode, toNode, 'y', bounds.height, false, options );
    },

It's difficult to find out what would happen if you did not supply a duration attribute, or to know that you need to look in Animation.js options to know what options are supported (which includes AnimationTarget.js options). I think updating the documentation around the Transition and TransitionNode methods will be sufficient to solve the problem.

[samreid]

I just made a minor change to indicate Transition is referring to the Animation type and its options. I think it may be sufficient.

[samreid]

Most of the // REVIEW questions were cleared up by @jonathanolson's remarks or by my further exploration of the code. As far as I know the last part is the discussion in https://github.com/phetsims/twixt/issues/15 which may amount to my misunderstanding of something. @jonathanolson OK to close this issue?

[pixelzoom]

Since Equality Explorer uses TransitionNode, I'd like to see this issue (and the 3 issues created as part of the code review) closed before RC testing begins on 7/11/18.

[jonathanolson]

Most of the // REVIEW questions were cleared up by @jonathanolson's remarks or by my further exploration of the code. As far as I know the last part is the discussion in #15 which may amount to my misunderstanding of something. @jonathanolson OK to close this issue?

It looks like #15 has been cleared up also, so I believe it is OK to close.