TWEEN animations are not associated with Screens

[pixelzoom]

In Sim.stepSimulation, all model.step is called only for the active screen. But all TWEEN animations are run, and will continue to use resources, regardless of which Screen created them. This seems like a significant defect in the main animation loop. And based on Skype discussion, it would seem that some developers are not aware of this, e.g.:

[2/20/17, 12:02:09 PM] Chris Malley: When the user switches screens in Unit Rates, I'd like to cancel animations and interactions that are in progress. I considered observing Sim.screenIndexProperty, but it's visibility is "@public (joist-internal)". How are other devs handling this? [2/20/17, 12:04:02 PM] Sam Reid: I normally let animations pause [2/20/17, 12:04:15 PM] Sam Reid: And punt on cancelling user interaction [2/20/17, 12:04:19 PM] Chris Malley: TWEEN animations continue to run, there is no association to a screen.

[jonathanolson]

Assigning, since it looks to be blocking https://github.com/phetsims/joist/issues/401 (to which I'm handling).

[jonathanolson]

Tweens' start/stop automatically adds them to global list that gets stepped with window.TWEEN.update().

It looks like we would want to:

1. Not call window.TWEEN.update() ourselves.
2. Add tweens to a Screen's modelTweens or viewTweens (or ignore the distinction). Possibly we could support the models and screenViews having a 'tweens' array?
3. Step these tweens manually with tween.update( dt ). If false is returned, have it removed from the list of tweens to update (this is what TWEEN.update basically does)

The downside is that either the Screen has to be passed all the way to TWEEN usage, or the model / screenView needs to be (with a somewhat more tricky API).

So potentially:

// If we don't support a distinction between model/view tweens
screen.addTween( tween );

// If we do support a distinction, but keep it on Screen
screen.addModelTween( tween );
screen.addViewTween( tween );

// If we do support a distinction, and add them to the model/screenView
// This is tricky, since we have no model supertype. Custom code on every sim that needs it?
model.addTween( tween );
screenView.addTween( tween );

@pixelzoom, @samreid, @jbphet thoughts?

[pixelzoom]

My 2 cents:

401 deals with correcting the ordering of model/view step. Presumably model/view tweens would require similar ordering, and we would therefore need a distinction between model/view tweens. So unless there's a good argument for why the model/view distinction isn't needed for tweens, I think we need to go with this options (proposed above):

model.addTween( tween );
screenView.addTween( tween );

We could add joist.ScreenModel type that implements addTween. Use of this type would preferably be optional, so we don't need to retrofit all sims. And we'd need to add addTween to ScreenView.

If possible, I'd like to see TWEEN fully encapsulated in twixt. I.e., no calls to window.TWEEN or similar in joist or other common-code repos.

[jonathanolson]

We could add joist.ScreenModel type that implements addTween. Use of this type would preferably be optional, so we don't need to retrofit all sims. And we'd need to add addTween to ScreenView.

Sounds good to me.

If possible, I'd like to see TWEEN fully encapsulated in twixt. I.e., no calls to window.TWEEN or similar in joist or other common-code repos.

I'm not familiar with this conversion, so I probably wouldn't handle it. There appear to be about ~46 places tweens are created with new TWEEN.Tween.

[samreid]

But all TWEEN animations are run, and will continue to use resources, regardless of which Screen created them.

Most (all?) tweens are short transient animations that will complete quickly and stop consuming resources. Hence this does not seem like a high priority problem.

We could add joist.ScreenModel type that implements addTween.

Would the screen models be globals, or would we have to pass them through everywhere? If they are globals, how do we name them to proceed safely? Or perhaps have a global that provides access to the current screen, like window.phet.joist.currentScreen. If they are not globals, then this is not much better than passing step(dt) recursively to the point that needs it. Also, I'm wondering if Tween would be the right level of abstraction (say, instead of generalizing to addSteppable or something like that).

[pixelzoom]

Most (all?) tweens are short transient animations that will complete quickly and stop consuming resources. Hence this does not seem like a high priority problem.

I agree, not high priority. But while we're overhauling stepAnimation in #401, it would be a good idea to consider this. That said...

I know of no guideline saying that tween should be used only for "short transient animations", and the same could probably be said of "most" model.step animations.

Use of resources is also not limited to the animation itself. When a tween animation ends, there is sometimes (in my experience) a non-trivial amount of work that is done. That work (and not the tween) may result in a hiccup on the screen that is currently selected.

It's also odd (and potentially confusing?) to treat animations differently. model.step animations pause when a screen is not active, while tween animations continue to run. The user (and the design team!) has no idea whether an animation was implemented with model.step or tween, and it's not unreasonable for them to expect animations to behave similarly when switching screens. This confusion (and hence this Github issue) occurred in Unit Rates -- if both a bag of apples and the marker editor are animating, switching away from the screen will cause the bag to pause, while the marker editor will continue to move.

[samreid]

I agree, it would be best to pause everything related to a screen when the screen is not selected. What about window.phet.joist.currentScreen.addStepListener as an API?

Note to self if we continue down that path: if we want currentScreen.addStepListener to work during initialization, joist would need to switch the currentScreen variable during initialization.

[jonathanolson]

I can think of a few cases where using currentScreen could trigger adding the tween to the wrong screen, and conceptually it makes sense to be able to step a screen in the background (particularly since model tweens would be affected).

[samreid]

Can you clarify what you mean about "in the background"? I didn't understand this bit:

conceptually it makes sense to be able to step a screen in the background (particularly since model tweens would be affected).

[jonathanolson]

inactiveScreen.model.step( dt ) or other operations on an inactive screen could trigger tweens, which would get attached to a completely different screen.

[samreid]

If joist would be calling inactiveScreen.model.step( dt ) then it should be careful to do it like so:

var tempScreen = currentScreen;
currentScreen = inactiveScreen;
inactiveScreen.model.step( dt )
currentScreen = tempScreen;

But I have a hunch that I don't really understand the problem.

[jonathanolson]

Keeping screens as their own unit that doesn't depend on a ton of extra information makes things like unit tests way easier.

If you rely on a global that gets the current screen, you now can't create screens and test/step them independently, which seems like an anti-pattern.

[samreid]

Agreed, good point! How do you recommend to associate tweens with screens?

[jonathanolson]

The model (newly provided joist.ScreenModel as @pixelzoom noted) and joist.ScreenView would provide functionality for tracking tweens, and the step() function would presumably run those.

[samreid]

So if I have a Model that contains an A, which contains a B, which contains a C, then I would pass the ScreenModel into the Model then into the A then into the B then into the C, then when C wants to start a tween, it would invoke it on its reference to the ScreenModel?

[jonathanolson]

pass the ScreenModel into the Model

The Model would be a subtype of ScreenModel when needed, and yes unfortunately this pattern would include passing it through to where it is needed (or at least an addTween function or similar abstraction, not necessarily the whole model).

[samreid]

Instead of passing through a ScreenModel or TweenContainer to all children, would it more direct/simpler and hence preferable to pass through the step(dt) functions? This may help with modularity because C can be tested without having to pass it a ScreenModel (or addTween function abstraction). Passing through the step functions directly also has the advantage that it is automatically properly scheduled as part of the model or view phase (instead of having to invent something new for that in the containers).

// strategy A
function A(screenModel){
  this.b= new B(screenModel);
}

inherit (Object, A,{
});

function B(screenModel){
  this.c= new C(screenModel);
}

inherit (Object, B,{
});

function C(screenModel){
  // something happened 
  screenModel.addTween(new Tween());
}

inherit (Object, C,{
});

// strategy B
function A(){
  this.b= new B();
}

inherit (Object, A,{
  step:function(dt){
    this.b.step(dt);
  }
});

function B(){
  this.c= new C();
}

inherit (Object, B,{
  step:function(dt){
    this.c.step(dt);
  }
});

function C(){
  // something happened 
  this.tween = new Tween();
}

inherit (Object, C,{
  step:function(dt){
    this.tween && this.tween.step(dt);
    if (this.tween.finished){
      this.tween=null;
    }
  }
});

[jonathanolson]

pass through the step(dt) functions?

Can you describe this with a code segment? Is it like passing through an Emitter that fires with step dts?

That would be more modular, but we'd want to create an abstraction for tweens (through twixt?) that stops calling update( dt ) after the animation is done (and removes the listener).

[samreid]

Can you describe this with a code segment?

I updated my comment in https://github.com/phetsims/joist/issues/404#issuecomment-281758745

[jonathanolson]

To handle multiple (possibly concurrent) tweens in strategy B, which will often have the case, we might have:

function C() {
  this.tweenHandler = new TweenHandler();
  something.link( function() {
    self.tweenHandler.addTween( new Tween() );
  } );
}

inherit( Object, C, {
  step: function( dt ) {
    this.tweenHandler.step( dt );
  }
} );

where the same TweenHandler would be present in strategy A at the model and screenView.

[jonathanolson]

I'll implement the above idea with a type in Twixt.

[jonathanolson]

The above implementation idea fails in a few ways, most notably because we can't be notified of animation start/stops (we'd need to hook into that to tell when to call update()). Without hacking things horribly, this won't be possible.

Will do minor investigation into Twixt as mentioned as a possibility during dev meeting (may be easier to do the longer-term solution up front).

[jonathanolson]

I think getting an animation solution in Twixt is the correct approach for handling the tweens (see https://github.com/phetsims/twixt/issues/3).

This has been more time than I'd like to handle https://github.com/phetsims/joist/issues/401, so I'm going to punt on handling this before finishing https://github.com/phetsims/joist/issues/401 (leaving this open).