Eliminate frameStartedEmitter and frameEndedEmitter

[samreid]

From https://github.com/phetsims/phet-io/issues/1383, we would like to eliminate frameStartedEmitter and frameEndedEmitter, and replace usages with stepSimulationEmitter (we would make that public).

There are 4 usages of frameStartedEmitter and 3 usages of frameEndedEmitter which would be good to convert, if possible.

[pixelzoom]

You haven't provided enough info here. Is stepSimulationEmitter functionally equivalent to frameStartedEmitter and frameEndedEmitter? What is the proposed conversion?

[pixelzoom]

More questions... frameStartedEmitter and frameEndedEmitter emitted at the start and end of the frame (step) respectively. How will you replace those two Emitters with one Emitter? Will stepSimulationEmitter fire twice, at the start and end of step? Will it provide a callback argument that identifies whether it's the start or end of frame?

[pixelzoom]

And can you please summarize why this replacement is needed? I don't have time to distill that info out of phetsims/phet-io#1383.

[pixelzoom]

I looked at stepSimulationEmitter in Sim.js. A couple of things:

(1) Emitter takes a listener option, and you're apparently relying on that to be called first. As I mentioned recently, relying on the order of listeners is (in general) a code smell and an anti-pattern. And the number of times that I've seen this order dependency leveraged in the past week is alarming. I think this warrants a developer discussion.

(2) If Emitter's listener is indeed called first, and listener performs the step, then all other listeners are notified after the step is completed. This makes stepSimulationEmitter a replacement for frameEndedEmitter, but not for frameStartedEmitter . How do you intended to notify listeners before the step has been performed?

[samreid]

And can you please summarize why this replacement is needed?

PhET-iO playback and record warranted introduction of stepSimulationEmitter. We hypothesize that we do not need listeners at the beginning of frame and end of frame and that all usages can be converted to use stepSimulationEmitter instead.

Let's discuss Emitter listener ordering in another issue such as phetsims/scenery#928

[pixelzoom]

It sounds like you further hypothesized that no one needs frameStartedEmitter, because stepSimulationEmitter is equivalent to frameEndedEmitter. Is that the case? How am I supposed to accomplish phetsims/equality-explorer#148, which needs to be done before the first step happens?

[samreid]

I don't understand why termCreator.positiveLocation and termCreator.negativeLocation have to be set at the beginning of step. Would the behavior be different if those values were set at the end of step?

[pixelzoom]

It may well be the case that we can get away with performing EqEx's callback at the end of the frame. That's not the point. The point is that the callback is work that should semantically be done before the first step, and doing it before the first step ensures that it's future proof. And the general issue is not whether EqEx can be changed or not. The issue is that there is a legitimate need to have this functionality, and that the proposal here does not meet that need.

[samreid]

The point is that the callback is work that should semantically be done before the first step

The documentation for that listener says

// Things to do after the sim has loaded, when this Node has a valid location.

So perhaps adding a listener to sim.endedSimConstructionEmitter would be more appropriate?

[pixelzoom]

So perhaps adding a listener to sim.endedSimConstructionEmitter would be more appropriate?

I don't know. The doc tells me that endedSimConstructionEmitter "indicates sim construction completed". Recommended to replace that with something that actually tells the reader something useful.

TermCreatorNode's frameStartedCallback needs to be called after it has a valid location and before step.

[pixelzoom]

Continuing discussion of endedSimConstructionEmitter in EqEx in phetsims/equality-explorer#148.

[samreid]

It looks like the frameEndedEmitter.addListener usage by PhET-iO is important--we may need to keep that intact. I wonder if that should be powered by stepSimulationEmitter.after, see phetsims/scenery#928 (comment)

[samreid]

I wonder if that should be powered by stepSimulationEmitter.after

Yes, it should use after because if other parts of the simulation perform work during stepSimulationEmitter listeners, we will want to make sure that work is captured before PhET-iO gets the state.

[samreid]

Since PhET-iO seems to have a legitimate use of frameEndedEmitter. It seems reasonable to keep frameStartedEmitter for symmetry. Therefore I recommend to close this issue.

[zepumph]

From this it may be nice to document a preferred way to add a listener to a frame. Which emitter should be used by default? Is the visibility annotation and documentation clear enough for all of them?

[samreid]

I think most simulations should add a listener by implementing Model.step or ScreenView.step. When that is impossible, the Sim emitters can be used. I committed better docs. Anything else to do here?

[pixelzoom]

@samreid said:

I think most simulations should add a listener by implementing Model.step or ScreenView.step. ...

Is something like this what you had in mind?

class MyScreenView extends ScreenView {

  constructor(...) {
    this.isInitialized = false;  
    ...
  }

  step: dt => {
    if ( !this.isInitialized ) {
      this.isInitialized = true;
      // do some view initialization
    }
   ...
  }
}

CM: I edited the above example.

[samreid]

Yes, but this is broader than just doing something on the first try. Normal models and views implement step like this:

From Balancing Act:

    // Step function, called by the framework, clocks time-dependent behavior.
    step: function( dt ) {
      this.plank.step( dt );
      this.massList.forEach( function( mass ) {
        mass.step( dt );
      } );
    },

From Hooke's Law:

step: function( dt ) {
  this.animator.step( dt );
}

Hooking into Sim emitters gives callbacks for every screen. Implementing step in a model or view is called only for the selected screen.

[pixelzoom]

@samreid said:

I think most simulations should add a listener by implementing Model.step or ScreenView.step. When that is impossible, the Sim emitters can be used.

I agree with the first sentence, but not the second. It would have been "possible" to use step to initialize EqEx's TermCreatorNode instances after view creation. But it would have been relatively awkward, and listening to an Emitter seems like a superior solution.

[samreid]

How about "When that is impossible or relatively awkward..."?

[zepumph]

Looks really good. Thank you both.