Rewrite Timer.js to use Emitter

[samreid]

This will support animations as described in https://github.com/phetsims/twixt/issues/19 and will also simplify the Timer implementation.

[samreid]

This change will affect all simulations, it should be reviewed. It should be OK for phet-core to depend on axon, since other libraries that use phet-core also use axon.

phet-core unit tests are passing, lint is passing, I tested several sims with animations, including Hooke's Law, and did not see any obvious problems.

@ariel-phet can you please recommend a reviewer?

[samreid]

After review, we can remove the hold on https://github.com/phetsims/phet-core/issues/39

[samreid]

I notified the slack channel:

Sam Reid [1:35 PM] I changed phet-core/Timer to use Emitter, please see https://github.com/phetsims/phet-core/issues/39 for details, and be on the alert for issues this may cause

[ariel-phet]

@pixelzoom this looks like a pretty small file, please review

[pixelzoom]

Review comments:

• [x] Recommended to rename emitter to stepEmitter, in case Timer requires additional Emitters in the future, and for consistency with method names addStepListener, removeStepListener and hasStepListener.

• [ ] Since we have addStepListener, removeStepListener and hasStepListener, perhaps stepEmitter should be @private instead of @public. This would hide the specifics of how Timer is implemented, which has served us well, and allowed this change to be made easily.

[samreid]

In https://github.com/phetsims/twixt/issues/19 we discussed creating a public Timer Emitter so that we can have a uniform interface for animating things.

Also, I've renamed the emitter to stepEmitter in the preceding commit.

[pixelzoom]

Documentation should indicate:

• [ ] that stepEmitter fires emit1 with dt
• [ ] the signature of step listeners

[pixelzoom]

@samreid said:

In phetsims/twixt#19 we discussed creating a public Timer Emitter so that we can have a uniform interface for animating things.

Why can't animation clients use PhetCore.Timer.addStepListener? That's the same API used by other clients of Timer.

[samreid]

The proposal was

class PulsatingButton extends Node {
  constructor( options ) {
    super();
    options = _.extend( {

      // {Emitter} the emitter that drives the animation, or null if the simulation will manually call step()
      stepEmitter: PhetCore.Timer.STEP_EMITTER
    },options );

    if ( options.stepEmitter ) {
      const listener = dt => this.step( dt );
      options.stepEmitter.addListener( listener );

      // @private {function} called on dispose
      this.disposeStepListener = () => options.stepEmitter.removeListener( listener );
    }

    this.disposePulsatingButton = () => {
      this.disposeStepListener && this.disposeStepListener();
    };
  }

  // @public - animate the button
  step( dt ) {
  }

  dispose() {
    this.disposePulsatingButton();
  }
}

This would support (a) passing in an Emitter (b) using the default Timer Emitter or (c) manually calling step. If we require animation clients to use PhetCore.Timer.addStepListener then we would have to detect if a Timer was passed in or an Emitter was passed in. Or we could change the API for PhetCore.Timer to be addListener/removeListener, then we wouldn't need to make the Emitter public, but then the type in PulsatingButton or UI classes or Animation would be {Timer|Emitter}.

[samreid]

that stepEmitter fires emit1 with dt the signature of step listeners

Pushed a commit above that hopefully addresses this.

[pixelzoom]

The proposal was ... class PulsatingButton... ...

The step emitter should be an option to Animation, not the clients of Animation.

Also consider reimplementing Animation's stepper: 'timer' option to call Timer.addStepListener.

If you feel strongly that Animation requires the ability to pass in a general step emitter... Having 2 different APIs for manipulating Timer's stepEmitter (directly vs via methods) has a code smell.

[samreid]

The step emitter should be an option to Animation, not the clients of Animation.

If Carousel creates/uses Animation, and I need to drive it from the simulation, then how can I pass a stepEmitter to the Animation without first passing it to Carousel?

[samreid]

Having 2 different APIs for manipulating Timer's stepEmitter (directly vs via methods) has a code smell.

We could make Timer extend Emitter? Or go with the proposal at the bottom of https://github.com/phetsims/phet-core/issues/39#issuecomment-415897143 ?

[pixelzoom]

If Carousel creates/uses Animation, and I need to drive it from the simulation, then how can I pass a stepEmitter to the Animation without first passing it to Carousel?

What do you mean by "drive it from the simulation"? If you mean "drive it via the phet-core Timer", then specify stepper: 'timer' and have Animation add a listener via PhetCore.Timer.addStepListener. If you mean "drive it via client step functions", then specify stepper: 'manual'.

[samreid]

I meant drive it from the simulation by passing in a simulation-specific Emitter, which is listed as option (a) in https://github.com/phetsims/phet-core/issues/39#issuecomment-415897143 and option (c) in https://github.com/phetsims/twixt/issues/19#issue-353869087

[samreid]

Would Animation not work with an Emitter arg? I feel like we are miscommunicating about something. I'm available on slack/zoom if you want to chat.

[samreid]

At today's meeting, we discussed that nobody has an imminent deploy coming up, and that leaving this for @pixelzoom to review when he is available still sounds like a good plan.

[pixelzoom]

Yes, Animation could be made to work with a general Emitter options for stepper. But I've heard no request or need for that generality.

I'm objecting to 2 things related to the @public visibility of Timer's stepEmitter:

(1) Making stepEmitter @public means that the specifics of how Timer is implemented are no longer hidden. And hiding the implementation has served use well in the past (e.g., this issue).

(2) Making stepEmitter @public results in 2 public interfaces related to it's listeners. Listeners can now be added to stepEmitter directly, or via Timer's add/remove/has methods. Having 2 interfaces in this case is unnecessary, and makes it possible to write code like:

if ( Timer.stepEmitter.hasListener( listener ) ) {
  Timer.removeStepListener( listener );
}

[samreid]

Yes, Animation could be made to work with a general Emitter options for stepper. But I've heard no request or need for that generality.

The top comment in this issue said:

This will support animations as described in phetsims/twixt#19 and will also simplify the Timer implementation.

In twixt#19 it was proposed that we support

(a) animation runs automatically by registering with a global (b) simulation directly calls step (through a nested chain of calls) (c) simulation passes in a steppedEmitter which the component listens to for its updates (edited)

A pattern was proposed for how to achieve this, and @pixelzoom replied:

Happy to use that pattern if/when stepEmitter exists.

Hence I opened this issue to create stepEmitter.

(1) Making stepEmitter @public means that the specifics of how Timer is implemented are no longer hidden.

That's true! We could hide this by adding methods like Timer.addListener to match the Emitter interface and keeping the Emitter private, but that seems unnecessary.

(2) Making stepEmitter @public results in 2 public interfaces related to it's listeners. Listeners can now be added to stepEmitter directly, or via Timer's add/remove/has methods.

I kept the prior API for backward compatibility. I'm inclined to embrace the Emitter API though, since it seems more general/flexible.

What if Timer extended Emitter?

[pixelzoom]

In twixt#19 it was proposed that we support ...

It was proposed by you. I'm still of the opinion that clients (by default) shouldn't be bothered with controlling the animation of UI components, and that stepper: 'timer' and stepper: 'manual' are sufficient to meet PhET's needs in Animation. I don't see (and haven't heard) the need to provide a general Emitter as one of the stepper values to Animation.

@pixelzoom replied: Happy to use that pattern if/when stepEmitter exists.

Instead of "happy to use that pattern", I should have said "happy to consider that pattern". Which is what I've done here. I've considered both how it's implemented and whether it's needed to solve the problem at hand.

We could hide this by adding methods like Timer.addListener to match the Emitter interface and keeping the Emitter private, but that seems unnecessary.

Those methods don't need to be added, they already exist (see addStepListener, removeStepListener, hasStepListener) and are the 2nd interface that I spoke of ("Timer's add/remove/has methods"). Having those methods doesn't provide the ability to pass Timer's stepEmitter to Animation. They do provide the ability for Animation to add a listener in the stepper: 'timer' case.

So the conclusion of my review is that this not the right path. I'm happy to conceded the review to another developer to reach a different conclusion.

[samreid]

It was proposed by you.

Yes, I remember it clearly!

I'm still of the opinion that clients (by default) shouldn't be bothered with controlling the animation of UI components

The default I proposed was to use Timer.steppedEmitter which means clients by default wouldn't be bothered with controlling the animation of UI components.

I don't see (and haven't heard) the need to provide a general Emitter as one of the stepper values to Animation.

Will we ever want to drive Animations from the model? If not, why not?

I don't see (and haven't heard) the need to provide a general Emitter as one of the stepper values to Animation.

It's the more flexible pattern. We previously debated which strategy to use because we didn't know of this more flexible pattern at the time. Now that we know about the most flexible pattern, why not use it? It has the right default behavior, can be driven by step() or emit() the latter of which can be local or global.

Instead of "happy to use that pattern", I should have said "happy to consider that pattern".

Sorry for the miscommunication!

After this discussion, I'm perceiving the Timer as an Emitter that emits at specific times. Do you feel differently?

I've had a good experience in several sims by passing a stepEmitter for model-based animation. Have you seen this pattern? Do you think it should not be used? Timer predates Emitter, so this is our first time considering that possibility.

[samreid]

The default I proposed was to use Timer.steppedEmitter which means clients by default wouldn't be bothered with controlling the animation of UI components.

If we change Timer to extend Emitter then the default would be Timer instead of Timer.steppedEmitter.

[pixelzoom]

I'm OK with Timer extends Emitter, but think we should then remove addStepListener, removeStepListener, hasStepListener.

[samreid]

I'm OK with Timer extends Emitter, but think we should then remove addStepListener, removeStepListener, hasStepListener.

I've rewritten Timer to extend Emitter, and removed the addStepListener, removeStepListener and hasStepListener methods.

Notes:

1. I considered making this change in a branch for review, but there are 6 repos affected by this change, and the overhead of 6 branches did not seem warranted. So I'll push these changes directly to master for review.
2. Timer previously did not explicitly extend anything, and I decided it was preferable to use extends rather than inherit, so I decided to convert Timer to ES6. Timer is a singleton and hence cannot be extended elsewhere, so the caveat described in https://github.com/phetsims/phet-info/issues/82#issuecomment-415522825 does not apply.
3. I ran a local aqua suite (fuzz test, fast build) and every simulation is passing.
4. lint-everything is passing
5. I tested the Carousel component in the sun demo, which uses Animation & Timer, and it behaved as expected.
6. Once review of this part is complete, I'd like to proceed in changing Animation.js to use steppedEmitter {Emitter} instead of stepper {string} with a default value of Timer instead of 'manual'. This will affect several repos that are creating Animation instances or subtyping Animation which are using the stepper option. We will likely make this change in https://github.com/phetsims/twixt/issues/19 but I wanted to identify it here as my goal.

[samreid]

Pushes complete, ready for review.

[pixelzoom]

The use of TimerType and Timer names feels wrong. Relevant lines from Timer.js:

17 class TimerType extends Emitter
72 const Timer = new TimerType();
74 return phetCore.register( 'Timer', Timer );

UpdateCheck and phetioCommandProcessor are two examples of singleton pattern, and they're slightly different. And the above is yet-another variation, which feels like a step in the wrong direction (particularly giving the class a different name).

Perhaps we should review what the PhET singleton pattern is supposed to be, and how it should be applied in the context of ES6 classes.

[pixelzoom]

My vote would be to follow the singleton pattern used in phetioCommandProcessor.js. That is, use a lowercase filename to indicate that it's a singleton instance, and return the singleton.

// timer.js

class Timer extends Emitter { .... }

return phetCore.register( 'timer', new Timer() );

[samreid]

Thanks, that pattern seems great. Changed in the preceding commit, ready for review.

[pixelzoom]

I said:

... That is, use a lowercase filename to indicate that it's a singleton instance, and return the singleton.

// timer.js ...

The filename is still Timer.js.

[samreid]

Thanks, I'll correct that.

[samreid]

I messaged the developer slack channel:

Brace yourself, commits to 30 files across 18 repos incoming for https://github.com/phetsims/phet-core/issues/39#issuecomment-418431333 Also, one of the commits is a casing-only change for a filename, so windows users be alert, and if there are hiccups you may need to reclone phet-core.

After making this change, I tested the first dozen sims in aqua and didn't see any systemic problems. I also tested sun/demo carousel and it animated properly.

[samreid]

I've renamed PHET_CORE/Timer to PHET_CORE/timer. I messaged on slack:

Changes pushed, please pull and report problems.

@pixelzoom back to you for review.

[pixelzoom]

In timer.js:

JSdoc for methods does not meet PhET standards. There are no @param or @returns annotations.

Two comments indicated "a listener (which can take a {number} dt argument)", but I see the listener called without a dt argument, i.e. listener() on lines 28 and 54.

[samreid]

I've clarified the documentation in the preceding commits, please review.

[samreid]

Another commit clarifies the return types.

[pixelzoom]

In the above commits, I fixed a copy-paste typo and added a comment about where dt comes from (timer.emit1 in Sim.stepSimulation).

Everything else looks good.

Closing.