Review "inlined Emitter" pattern with the dev team #928
Comments
|
My primary objection to this pattern is that is relies on its listeners being called in the order that they are added. It relies on the first listener (its Another code smell is that, in the use cases where this pattern is proposed, listener is not optional. This suggests that it should be a required parameter of a new class (not Emitter, let's refer to it as DataStreamTask), that leverages the data stream features of Emitter, either through inheritance or composition. Or perhaps Emitter's data stream features should be factored out into a new base class that is shared by Emitter and DataStreamTask. In any case, what's proposed here falls under the category of "clever", but it's using Emitter in an odd way, and adds unnecessary features/code to Emitter. |
Consider the following hypothetical examples: class EmitterA {
constructor( specialListener ) {
this.listeners = [ specialListener ]; // @private
}
emit() {
this.listeners.forEach( listener => listener() );
}
addListener( listener ) {
this.listeners.push( listener );
}
}
class EmitterB {
constructor( specialListener ) {
this.specialListener = specialListener; // @private
this.listeners = []; // @private
}
emit() {
this.specialListener();
this.listeners.forEach( listener => listener() );
}
addListener( listener ) {
this.listeners.push( listener );
}
}Are you saying that EmitterA has a code-smell and anti-pattern and that EmitterB doesn't? If so, can you please explain why? I'm not trying to pick a fight, just trying to understand the root issue here.
I would like clarification on this problem. For instance, let's say that I wanted to add another listener to input.touchStartedEmitter.addListener( ( id, point, event ) => console.log( `touch started at ${point.x},${point.y}!` );Are you saying that most listeners would rely on
Are you picturing something like this? class DataStreamTask extends Emitter {
/**
* @param {function} listener
* @param {Object} [options]
*/
constructor( listener, options ) {
super();
this.listener = listener;
}
emit() {
this.listener();
super.emit();
}
} |
Writing code that relies on
I'm saying that listeners should not depend on the order that listeners are called.
Yes, if there's one task that absolutely needs to be done before listeners are notified. |
That sounds nice in the general case, but I don't think we've held up to that so far. If we randomized the emitter listener call order, I wouldn't be surprised if a number of sims break. If it's important to avoid that assumption, should we ensure sim code works regardless of the order? (Or is this something we could consider relaxing)? |
|
I looked briefly for occurrences where the listener order matters (for cases using this pattern) and couldn't find any. I provided a synthetic example in the middle of #928 (comment) but it wasn't acknowledged. It is not clear that the Emitter(listener) pattern means the given listener must go first. |
|
@jonathanolson asked:
This documentation (which deserves discussion) was recently added to Emitter, similar in Property. Nothing similar in Node. /**
* Adds a listener. When emitting, the listeners are called in the order they were added.
* @param {function} listener
* @public
*/
addListener( listener ) { |
|
@samreid said:
I replied in #928 (comment):
|
|
Can you point out an example using this pattern where the listener order matters? Or is this conversation about hypothetical future usages? |
|
Sim |
|
What other listener is relying on the |
|
??
|
|
Say Profiler is rewritten to use stepSimulationEmitter for profiling, then it doesn't matter whether it is called before or after the other listener. |
|
So you're saying `stepSimulationEmitter notifies listeners when the simulation has been stepped" is a false statement? This is baffling me. |
|
"is being stepped" vs "has been stepped"? |
|
Which brings up another smell.. |
|
I wonder if one reason it made sense for PhET-iO to move the "work" of the Emitters to the first listener, is that the "work" is supposed to be called during playback. |
|
After discussion, we concluded we would like to add |
|
Note we should also revert comments like phetsims/axon@1239bb6 as part of this work. |
|
I am still very much not on board with this entire approach. I still think it's a fundamental mistake to use Emitter and listeners to specify tasks that need to be done before/after notifying other listeners. This is a convenient solution, not a good solution. That said... I still like to hear about #928 (comment). And I'd like to hear why it's not possible/preferable to factor the essential PhET-iO functionality out of Emitter, where it can be more generally used to wrap any task. E.g.: // Responsible for wrapping a task so that it appear in the PhET-iO data stream.
class PhetioTask {...}
// Get PhET-iO data stream features by inheritance or composition.
class Emitter extends PhetioTask {...}
class Emitter {
constructor(...) {
this.task = new PhetioTask(...); // wrap listener notification
}
}
// Do some task, notify listeners that the task was done, do cleanup.
var doTask = new PhetioTask( ... );
var taskCompletedEmitter = new Emitter( ... );
var doCleanup = new PhetioTask(...);
updateTask.perform(...);
updateCompletedEmitter.emit(...)
cleanupTask.perform(...);Compared to what we have now: somethingEmitter = new Emitter( {
first: dt => { ... }, // perform some task in the first listener
last: dt => {...} // so some cleanup in the last listener
} ); |
|
The current implementation also makes the data stream structure look like this, where first/last tasks and notification of listeners are nested together. Are listener0 and listenerN-1 in the above data stream general listeners or these special Imo, there should be a clear separation between tasks and notification, and that separation should be reflected in the data stream. E.g.: |
|
To try to answer #928 (comment): For Perhaps we have settled on the current solution as a shortcut to getting things working with our existing infrastructure. Maybe there is a way it could be designed around startEmitter/listenerEmitter/endEmitter, but it's not obvious how it could be done.
I hear you and am interested in finding a superior solution. Not sure what it will be though given the desire for for record and playback feature.
PhetioObject already contains The points in #928 (comment) are well-taken. Would you like to schedule a working collaboration meeting where we can try out some ideas? That may be more efficient than @zepumph or me to try other ideas and run them past you. |
|
The place where Emitter events are leveraged for playback is handlePlaybackEvent. The pseudocode is: Full details are in https://github.com/phetsims/phet-io-wrappers/blob/efce8697fa12b8a72483245885b84c096a54dd90/common/js/handlePlaybackEvent.js#L37-L72. I can see that our Emitter implementation is focused around facilitating this usage. |
|
Maybe I just don't understand why data stream and playback must be in Emitter, and can't be factored out of Emitter. But here's what I had in mind with the example in #928 (comment). Untested, for illustration only. All of the PhET-iO stuff (data stream, playback) is moved out of Emitter and into // Copyright 2019, University of Colorado Boulder
/**
* Wrapper that handles PhET-iO features for a task.
* A task is some unit of work that needs to be bracketed with start/end in the PhET-iO data stream.
*
* @author Chris Malley (PixelZoom, Inc.)
*/
define( require => {
'use strict';
// modules
const EmitterIO = require( 'AXON/EmitterIO' );
const PhetioObject = require( 'TANDEM/PhetioObject' );
const tandem = require( 'TANDEM/tandem' );
const Tandem = require( 'TANDEM/Tandem' );
const Validator = require( 'AXON/Validator' );
// constants
const TaskIOWithNoArgs = EmitterIO( [] );
const EMPTY_ARRAY = [];
assert && Object.freeze( EMPTY_ARRAY );
class Task extends PhetioObject {
/**
* @param {Object} [options]
*/
constructor( options ) {
options = _.extend( {
task: null, // {function|null}
// {Array.<Object>|null} - array of "Validator Options" Objects that hold options for how to validate each
// argument, see Validator.js for details.
argumentTypes: EMPTY_ARRAY,
tandem: Tandem.optional,
phetioState: false,
phetioType: TaskIOWithNoArgs, // subtypes can override with TaskIO([...]), see TaskIO.js
}, options );
if ( options.phetioPlayback ) {
options.phetioEventMetadata = options.phetioEventMetadata || {};
assert && assert( !options.phetioEventMetadata.hasOwnProperty( 'dataKeys' ),
'dataKeys should be supplied by Emitter, not elsewhere' );
options.phetioEventMetadata.dataKeys = options.phetioType.elements.map( element => element.name );
}
super( options );
// @private
this.task = options.task;
Validator.validate( options.argumentTypes, { valueType: Array } );
if ( assert ) {
// Iterate through all argumentType validator options and make sure that they won't validate options on validating value
options.argumentTypes.forEach( validatorOptions => {
assert && assert(
validatorOptions.validateOptionsOnValidateValue === undefined,
'emitter sets its own validateOptionsOnValidateValue for each argument type'
);
validatorOptions.validateOptionsOnValidateValue = false;
// Changing the validator options after construction indicates a logic error
assert && Object.freeze( validatorOptions );
// validate the options passed in to validate each emitter argument
Validator.validateOptions( validatorOptions );
} );
// Changing after construction indicates a logic error
assert && Object.freeze( options.argumentTypes );
}
// @private {function|false}
this.validate = assert && function() {
assert(
arguments.length === options.argumentTypes.length,
`Emitted unexpected number of args. Expected: ${options.argumentTypes.length} and received ${arguments.length}`
);
for ( let i = 0; i < options.argumentTypes.length; i++ ) {
Validator.validate( arguments[ i ], options.argumentTypes[ i ] );
}
};
}
/**
* Sets the task to be performed.
* @param {function} task - expected parameters are based on options.argumentTypes, see constructor
* @protected
*/
setTask( task ) {
this.task = task;
}
/**
* Does the task.
* @params - expected parameters are based on options.argumentTypes, see constructor
* @public
*/
do() {
assert && assert( this.task, 'do requires task to be set' );
// validate arguments
assert && this.validate && this.validate.apply( null, arguments );
// handle phet-io data stream for the task
this.isPhetioInstrumented() && this.phetioStartEvent( 'do', this.getPhetioData.apply( this, arguments ) );
// Perform the task
this.task.apply( null, arguments );
this.isPhetioInstrumented() && this.phetioEndEvent();
}
/**
* Gets the data that will be emitted to the PhET-iO data stream, for an instrumented simulation.
* @returns {*}
* @private
*/
getPhetioData() {
// null if there are no arguments. dataStream.js omits null values for data
let data = null;
if ( this.phetioType.elements.length > 0 ) {
// Enumerate named argsObject for the data stream.
data = {};
for ( let i = 0; i < this.phetioType.elements.length; i++ ) {
const element = this.phetioType.elements[ i ];
data[ element.name ] = element.type.toStateObject( arguments[ i ] );
}
}
return data;
}
}
return tandem.register( 'Task', Task );
} );Emitter is responsible only for managing and notifying listeners. PhET-iO stuff and notifying listeners is delegated to Task when EDIT: This example uses inheritance; composition might be better? // Copyright 2015-2018, University of Colorado Boulder
/**
* Lightweight event & listener abstraction for a single event type.
*
* @author Sam Reid (PhET Interactive Simulations)
*/
define( require => {
'use strict';
// modules
const axon = require( 'AXON/axon' );
const EmitterIO = require( 'AXON/EmitterIO' );
const Tandem = require( 'TANDEM/Tandem' );
const Task = require( 'TANDEM/Task' );
// constants
const EmitterIOWithNoArgs = EmitterIO( [] );
/**
* @param {Object} [options]
*/
class Emitter extends Task {
constructor( options ) {
options = _.extend( {
phetioType: EmitterIOWithNoArgs // subtypes can override with EmitterIO([...]), see EmitterIO.js
}, options );
super( options );
// @private {function[]} - the listeners that will be called on emit
this.listeners = [];
// @private {function[][]} - during emit() keep track of which listeners should receive events in order to manage
// - removal of listeners during emit()
this.activeListenersStack = [];
// Task to be performed when emit is called.
this.setTask( this.notifyListeners.bind( this ) );
}
/**
* Dispose an Emitter that is no longer used. Like Property.dispose, this method checks that there are no leaked
* listeners.
*/
dispose() {
this.listeners.length = 0; // See https://github.com/phetsims/axon/issues/124
super.dispose( this );
}
/**
* Emits a single event. This method is called many times in a simulation and must be well-optimized. Listeners
* are notified in the order they were added via addListener, though it is poor practice to rely on the order
* of listener notifications.
* @params - expected parameters are based on options.argumentTypes, see constructor
* @public
*/
emit() {
this.do.apply( null, arguments );
}
/**
* Notifies listeners.
* @params - expected parameters are based on options.argumentTypes, see constructor
* @private
*/
notifyListeners() {
// Notify wired-up listeners, if any
if ( this.listeners.length > 0 ) {
this.activeListenersStack.push( this.listeners );
// Notify listeners--note the activeListenersStack could change as listeners are called, so we do this by index
const lastEntry = this.activeListenersStack.length - 1;
for ( let i = 0; i < this.activeListenersStack[ lastEntry ].length; i++ ) {
this.activeListenersStack[ lastEntry ][ i ].apply( null, arguments );
}
this.activeListenersStack.pop();
}
}
// Unchanged:
// addListener, removeListener, removeAllListeners, defendListeners,
// emit1, emit2, emit3, hasListener, hasListeners, getListenerCount
}
return axon.register( 'Emitter', Emitter );
} ); |
|
Considering @pixelzoom objections to this approach, I think this is a decision (if we go down the path) that should have @kathy-phet in the loop to make the call. Feel free to continue discussion in this issue and if a consensus is reached, great...but if not, I think we need to bring this issue up at an iO meeting so implications are fully understood as best as possible. |
|
I'd love to hear @zepumph and @jonathanolson thoughts on the proposal in #928 (comment). |
|
The structuring separation sounds good to me. |
|
I think I understand the separation of the two files. It seems like I don't really see @samreid if we went with the above strategy, do you foresee us using Also note that I think it is confusing for |
pixelzoom
added
meeting:phet-io
dev:phet-io
and removed
meeting:phet-io
project:requires-kathy
labels
|
2/21/19 dev meeting: CM summarized pros/cons. AP asked for everyone to weigh in:
Consensus: Don't want first/last options for Emitter. AP proposal: Make a separate issue to explore factoring out PhET-iO data stream features. CM will create issue and strawman, hash out with MK and SR, bring back to team. CM, SR, MK will set up a time to discuss. |
|
Factoring out PhET-iO data stream responsibilities from Emitter will be investigated in phetsims/axon#222. Closing. |
From #843 (comment)
The simplest and most straightforward way to add an event to the PhET-iO data stream is by instrumenting an Emitter. This is true for both record/playback events as well as purely data-stream events. For instance, scenery/Input.js used to have sections like:
scenery/js/input/Input.js
Lines 465 to 483 in 80af1e4
This has been rewritten into an Emitter so it can be emitted in the data stream and used for recording/playback:
scenery/js/input/Input.js
Lines 366 to 385 in 1021233
A similar pattern is being used in SimpleDragHandler:
scenery/js/input/SimpleDragHandler.js
Lines 97 to 135 in c3c56b7
The general pattern is: when something is happening in a simulation that needs to be emitted to the PhET-iO data stream, this is best accomplished with an instrumented Emitter. Emitter now supports a
listenerconstructor option to facilitate cases like this.The text was updated successfully, but these errors were encountered: