Use stepEmitter/step for driving events

[samreid]

Chris Malley [10:06 AM] I’m converting 2 common-code components (Carousel, Drawer) from TWEEN to TWIXT/Animation. In general, I plan to provide options for the Animation’s duration and stepper. Using default stepper: 'timer' will result in no changes to sims. Using default stepper: 'manual' will requiring creating/propagating step functions in sims. Opinions on which should be the default? (edited) For general UI components, I’m leaning towards stepper: 'timer'. Having to manaully step general UI components seems burdensome. (edited)

Sam Reid [10:09 AM] I think we are generally better off with the manual stepper strategy. My notes are here: https://github.com/phetsims/twixt/issues/3#issuecomment-362050037

Chris Malley [10:10 AM] So for a Carousel that is nested 5 levels down from the ScreenView, you prefer to have have a step method in every container? My other reason for recommending timer in this case is that there’s no harm in (e.g.) a Carousel animation completing when switching screens. … and is in fact probably preferable to the Carousel pausing.

Sam Reid [10:12 AM] Is there no option to pass in a stepEmitter?

Chris Malley [10:12 AM] No // {string} - One of the following config: // ‘manual’ - step( dt ) should be called manually to advance the animation // ‘timer’ - When this animation is running, it will listen to the global phet-core Timer. // TODO #3: {ScreenView} - animates only when the ScreenView is the active one. // TODO #3: {Node} - animates only when the node’s trail is visible on a Display stepper: ‘manual’ What is the benefit of using stepper: 'manual' for general UI components? For example, if we have a common-code button whose color pulses, why should the client have to control that? We can certainly provide the ability to control it (which I’ve proposed by exposing the stepper option), but that’s not the type of animation that typically concerns us in step.

Sam Reid [10:19 AM] Snippets from my notes linked above:

In my opinion, cascaded calls to step() give the most flexibility and control for running animations, and is most consistent with our existing pattern for evolving models or views over time, and compatible with our needs for disposal (no leaks to a registrar).

Tween used a single global registry which seemed problematic

Let’s say a simulation has 30 pulsating buttons on screen 2 and 0 pulsating buttons on screen 1. When running screen 1, should the screen 2 buttons pulse?

We have an established pattern for updating models and views that works well--I fail to see why we would adopt another convention for animations.

Chris Malley [10:20 AM] akf, back later. interested in feedback from other devs too. As a default for general UI components that include animation would you prefer (a) the animation runs automatically, or (b) I have to control the animation via step.

Sam Reid [10:20 AM] The step() or stepEmitter() pattern is more direct--the simulation is in control of what happens when, instead of relying on the ordering of phet core Timer to happen at the right time. This was one of our complaints about Tween’s global timer. (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)

Sam Reid [10:33 AM] Maybe we can cover all our bases by following this pattern:

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();
  }
}

(edited)

Chris Malley [10:54 AM] Happy to use that pattern if/when stepEmitter exists. In the meantime, I'll default to stepper:'manual', change existing call sites to use stepper:'timer', and let responsible devs decide whether to change. I don't have time to implement and test step in sims.

Jesse Greenberg [11:24 AM]

(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

I would think that "a" is the most convenient if using an animation library. For components like Drawer and Carousel my vote would be for stepper: timer to be the default. I can't think of cases where I would need to control animation in step for those. The steppedEmitter example looks nice though.

[samreid]

I changed Timer.js to use Emitter, and the Emitter is available as Timer.emitter. The change is pending review in https://github.com/phetsims/phet-core/issues/39, after that @pixelzoom can proceed at his convenience with the proposed strategy above.

[pixelzoom]

The ability to use PhetCore.Timer.STEP_EMITTER should be an option in TWIXT/Animation, not something that every UI component needs to deal with (e.g. PulsatingButton in https://github.com/phetsims/twixt/issues/19#issue-353869087).

[samreid]

I changed Timer to extend Emitter. This issue is on hold until https://github.com/phetsims/phet-core/issues/39 review is complete. I'll push my proposed changes to Animation to a branch so it is clear what I am recommending for this part (and so I have a stash of my work). Note that the stepper {string} => steppedEmitter {Emitter} API change will affect other repos, so the branch cannot be merged to master without updating all usage sites of Animation.

[samreid]

Now that timer extends Emitter and that review is complete (see https://github.com/phetsims/phet-core/issues/39#issuecomment-417936587) , I'd like to proceed in changing Animation.js to use steppedEmitter {Emitter} with a default value of PHET_CORE/timer instead of stepper {string} with a default value of 'manual'. This will affect several repos that are creating Animation instances or subtyping Animation which are using the stepper option.

This proposed change is in the branch referenced above: https://github.com/phetsims/twixt/commit/93b148ff5e43965edd0cf9f673682bf03ec50a3d (aside from the casing change on timer).

@pixelzoom does this sound ok?

[pixelzoom]

Recommended to consult with the author of Animation, @jonathanolson, because there are a couple of other values that were proposed for stepper that have not been implemented. From Animation.js:

      // {string} - One of the following config:
      // 'manual' - `step( dt )` should be called manually to advance the animation
      // 'timer' - When this animation is running, it will listen to the global phet-core timer.
      // TODO #3: {ScreenView} - animates only when the ScreenView is the active one.
      // TODO #3: {Node} - animates only when the node's trail is visible on a Display
      stepper: 'manual'

[jonathanolson]

Sounds good to me to use {Emitter|null} (presumably you'd want to be able to still manually control it).

When I investigate the "animate only when the node's trail is visible on a display", I'll be able to evaluate whether that's feasible and can re-evaluate the API.

[jonathanolson]

Are there any other concerns before moving forward with the changes?

[pixelzoom]

No other concerns, other than who will do the work :)

And I'd like to see common-code UI components (Carousel, Drawer) default to stepEmitter: PHET_CORE/timer instead of stepper: 'manual'.

[samreid]

No other concerns, other than who will do the work :)

I'd like to give it a try, but would need support for review and may need support for sims I'm not familiar with.

And I'd like to see common-code UI components (Carousel, Drawer) default to stepEmitter: PHET_CORE/timer instead of stepper: 'manual'.

Sounds great, I'm planning that Animation will default to steppedEmitter: PHET_CORE/timer so UI components won't need to specify this option at all.

[samreid]

I noticed a discrepancy between the proposed option names. Some of the preceding comments call for stepEmitter and some call for steppedEmitter. I'll pick one for now (possibly the former), but we can always change it if necessary.

UPDATE: It looks like the latter is in the branch.

[samreid]

I made the change with a merge and decided to go with a 3rd name to avoid conflicts with existing option names, to make renaming easy once we settle on a final name.

I've tested sun demo carousel, capacitor lab basics, circuit construction kit, equality explorer sum to zero fade-out, equality explorer step it face animation, expression exchange coin carousel, function builder drawers, hooke's law converting between 1 vs 2 springs. No problems noted. I should also clarify that I've changed the defaults for Carousel and Drawer to match the Animation default of animationStepEmitter: timer--they were previously 'manual'. Likewise, local lint is passing so this seems like a reasonable point to commit. I'll check on Bayes CT after it does an iteration. If that checks out, I'll reassign to @pixelzoom for review and to help decide on a name for this option.

[samreid]

I should also point out that this is not the kind of error that Bayes CT fuzz testing would be good at catching, since it would likely result in no animation in a particular spot. I did look through all new Animation and Animation.call sites and made sure they looked reasonable though.

[samreid]

• [ ] Reminder to unblock https://github.com/phetsims/function-builder/issues/120 when this issue is closed.

[samreid]

CT had an iteration or two and I don't see any new problems introduced by this change. @pixelzoom can you please review at your convenience? Also, can you please recommend a name for this option? Please note that animationStepEmitter (the current name) occurs 25 times in our repos, stepEmitter occurs elsewhere 57 times and steppedEmitter appears elsewhere 3 times.

[pixelzoom]

@samreid said:

I made the change with a merge and decided to go with a 3rd name to avoid conflicts with existing option names, to make renaming easy once we settle on a final name.

I don't understand why you did this. The other cases where stepEmitter or steppedEmitter are used have nothing to do with Animation, and they are clearly Emitters. Are you inferring that using the same name across the entire code base is undesirable.

The name of the Animation option should be stepEmitter.

[samreid]

I don't understand why you did this.

I used animationStepEmitter (unique) so that when we decided on the final name, we could query replace all 25 occurrences across 7 files fully automatically. I'll rename to stepEmitter now.

[samreid]

Rename complete. @pixelzoom anything else to do for this review?

[samreid]

@jonathanolson pointed out a missed case for the Animation stepEmitter option in https://github.com/phetsims/function-builder/issues/120#issuecomment-434917534, self-assigning to double check the other occurrences.

[pixelzoom]

Looks good. Feel free to close after addressing https://github.com/phetsims/twixt/issues/19#issuecomment-434919590.

[samreid]

I tested through the new Animation occurrences and all sites seemed correct. I also tested usages of Drawer and Carousel. I tested through usages of TransitionNode and didn't see anything amiss. Hopefully if I've missed something it will be caught by dev or QA. Closing.