phetsims / scenery

Scenery is an HTML5 scene graph.
MIT License
53 stars 12 forks source link

Scenery event improvements #460

Open jonathanolson opened 9 years ago

jonathanolson commented 9 years ago

It would be nice to combine the event types (input or otherwise) into the same pattern for listening.

Right now, input events require the pattern:

node.addInputListener( { up: ..., down: ... } );

and other events use:

node.on( 'bounds', ... );

Why not allow node.on( 'up, ... ), etc.? How do we combine event listening into one single interface?

samreid commented 9 years ago

Related question: how/why did we settle on bounds rather than boundsChanged for that listener?

pixelzoom commented 9 years ago

9/8/15 dev meeting:

• Consensus design is below • Big change, so we'll sleep on this and re-discuss at new dev meeting.

//——————————————————————————————————————————————————
// Rename current on/off API to addListener/removeListener, 
// creates fewer objects, important for scenery internals

var callback =  function(…) { … } ;
node.addListener( ‘bounds’, callback );
node.addStaticListener( ‘bounds’, callback );
node.removeListener( ‘bounds’, callback );
node.removeStaticListener( ‘bounds’, callback );

//——————————————————————————————————————————————————
// Unify Node.addInputListener and Node.addEventListener 
// with overloaded forms of addListener/removeListener.

// object literal, map fields to callbacks
var listener =  {
   bounds: function(...) {...},
   up: function(...) {...},
   down: function(...) {...} 
};

node.addListener(  {
   bounds: ...,
   up: …,
   down: … 
} );
node.addListener( new SimpleDragHandler( {
  …
} );
node.addStaticListener( {
  …
} );

// Remove listener
node.removeListener( listener );
node.removeStaticListener( listener );
samreid commented 9 years ago

Today we discussed one kind of axon events, the ones emitted from trigger functions in Events.js. However, there is a (currently orthogonal) pattern for sending events from axon--events that come from property changes. There is a completely different interface/implementation for listening for Property instance changes.

At today's meeting when @jonathanolson was enumerating the different kind of Node events, he mentioned some like bounds and visible. In a normal simulation application, these would not be trigger('bounds',...) and trigger('visible',...) but rather messages sent from boundsProperty and visibleProperty instances.

Can these be unified? Should some scenery events come from property change notifications? Etc.

pixelzoom commented 9 years ago

9/10/15 dev meeting notes:

• Proposal to change behavior of 'on' and 'onStatic'. Have 'on' operate on actual listener list (not a copy), and 'onVolatile' operate on a copy. Set a flag in during notification that indicates that functions that modify the list (e.g. addListener, removeListener) will become unsafe, and throw and Error if one of those functions is called. Tracking this in https://github.com/phetsims/axon/issues/69.

• To help identify an ideal API... Experiment in scenery using axon.Property to see how it would work, what the performance costs are, what changes to axon would be required, etc. Tracking in https://github.com/phetsims/scenery/issues/463

• This issue started with how to add accessibility, but has expanded. It will require breaking changes to scenery (may be able to maintain backward compatibility). It may speed up parts of axon. It's a good longterm thing that will lower future scenery development costs. What is the priority of this issue? @ariel-phet: Let's reevaluate this issue after we're well into S2015R milestone.

jonathanolson commented 8 years ago

See https://github.com/phetsims/axon/issues/80 for performance testing for emitter.