Use Emitter (or a variant) instead of Events

[jonathanolson]

At least for Instance, but also consider Node changes.

Expecting something like

instance.events.selfVisibility.addListener( ... );

instead of

instance.on( 'selfVisibility', ... );

If converting Node, may need to keep in place some compatibility with the old Events API, and find places which were relying on Node "extending" Events.

[jonathanolson]

Adding developer-meeting tag to get approval for changes. It will be a beneficial refactoring for Scenery itself, and will increase performance.

[samreid]

This sounds good to me.

[jbphet]

Consensus at 11/12/2015 design meeting - Yes, we should move forward with this. @jonathanolson will do it, and will update existing simulations to use this style once it's ready. This will also require the ability in some cases to observe when listeners are added or removed from a given emitter so that it can efficiently validate bounds.

[jonathanolson]

It looks like Emitter now gets a mandatory Tandem copy and everything from PhetioObject, so it weighs in at ~200 bytes. This probably means it is too heavyweight for event notification (since we'd want to create a whole bunch of them for every Node).

Also I'll want to check how emit() re-entrance works (not just changing listeners during an emit()).

[jonathanolson]

Using a lightweight version of Emitter without all of the tandem/phet-io functionality would be great for this purpose.

[samreid]

Maybe AXON/Emitter would leverage the lightweight emitter (by composition) so we don't end up duplicating code or ending up with different behavior.

[samreid]

Would the new LightweightEmitter have type and predicate checking? If so, would we want this stripped out on builds (even storing those predicates), to go easy on the heap? If we do that, it could lead to a large divergence between heap size in requirejs vs built. I'd like to pursue this, but I think it would be best to synchronize efforts with @jonathanolson either through collaboration or discussion and planning. I'll put on hold for now until we get a few more sims published.

[jonathanolson]

Would the new LightweightEmitter have type and predicate checking? If so, would we want this stripped out on builds (even storing those predicates), to go easy on the heap?

If it would have checking, it would need to be stripped. The require.js heap usage doesn't really bug me. I haven't ever had a particular need for the type/predicate checking, particularly since in Scenery most emitters will have exactly 1 place they are emitted.

[jonathanolson]

Maybe AXON/Emitter would leverage the lightweight emitter (by composition) so we don't end up duplicating code or ending up with different behavior.

That sounds great.

[zepumph]

Today as part of phetsims/axon#222 @samreid and @zepumph added TinyEmitter. @jonathanolson please feel free to take it for a test drive in scenery. As of right now it doesn't use validation, but that has the (slight) possibility of changing.

[jonathanolson]

TinyEmitter looks nice!

For memory, is it possible to only provide this.isDisposed while assertions are enabled? (We only check it while assertions are enabled)

I'm also interested in what this.last refers to in the code. Seems addListener could just push.

Are there objections for moving event handling over to this?

[jonathanolson]

renderer specific event names were converted into instanceRefreshEmitter, but in Instance.js a the listener is still called markRenderStateDirtyListener (renderer specific). This has broken the naming similarity between the two variables. Does that bother you? Is InstanceRefreshEmitter too vague, seeing as it only handles marking the renderer dirty?

This does not bother me, markRenderStateDirtyListener is used for other purposes too, instance refresh is only one of the possible ways it gets called so it's not specific to that usage.

[jonathanolson]

Can you explain why so many Bounds.isValid() assertions were swapped out for Number.isFinite() conditionals?

I believe that is the commit e187f51 for phetsims/joist#608, unrelated to this. This allows attempting e.g. a node.left = with invalid bounds (no-op).

[jonathanolson]

Did you feel like preventSettersOnProperty was helpful during this work, but wasn't needed in perpetuity?

Yes, it was helpful during the development.

[jonathanolson]

RE: Node.rendererSummaryRefreshEmitter, I don't think I like this single emitter as much as specific events called at specific times (hint/accessibleContent/etc). I don't recommend any changes, but I feel like this change has made things a bit less flexible. For example, seeing e93c44d#diff-a15bb7b479c14f8fd92583857f79818f. Thoughts?

This is specifically because if we had e.g. 7 more Emitters per Node, this would add a large chunk of memory and take up a lot of processing time. Fields on these common types are at such a premium (especially internal fields) that the lack of flexibility (in this case for Scenery internally) is WELL worth the memory/performance effects.

[jonathanolson]

This says "general" and "default", does this mean there is a way to bypass this refreshing of everything?

No, I'm updating the docs to "When ANY hint changes, we refresh everything currently (for safety, this may be possible to make more specific in the future, but hint changes are not particularly common performance bottleneck)."

[jonathanolson]

In Node.js we define this.phetioVisibleProperty. We weren't sure whether to null it out in the constructor (adding more keys to every single Node) or to lazily add it to only the Nodes that are PhET-iO instrumented. Keep in mind this will be done for opacity and pickability as well. We weren't sure whether to minimize the number of Node keys or hidden classes. It seems clearer to put them in the constructor, but we weren't sure how essential it is to minimize the number of Node attributes.

That is a really good question. Because we are inheriting from Node all the time, it complicates the hidden class question (so intuitively I think the minimization of Node keys might be better for performance/memory, but I haven't investigated that). Sounds like something good to test, to see what effect it would have.

UPDATE from MK: see #1055

[jonathanolson]

Does axon have a main file like scenery that can be imported to get access to all axon files? I couldn't find one. If it does, we should make sure that Tiny*.js are in there too.

Looks like it does, but it was emptied in phetsims/axon@ac530d0.

I'll be adding that back in.

[jonathanolson]

Please explain, phetsims/area-model-common@a41615d. Does this have something to do with the Events refactor? (promoted to phetsims/beers-law-lab#254)

Constructor super was being called twice, noticed somehow in the conversion.

[jonathanolson]

For phetsims/beers-law-lab@a50707b, doesn't on map to link, and not lazyLink. Please explain your choice for lazyLink, and if this is a pattern in general, should we document it somewhere?

Because .on() does not call the listener immediately (and instead only adds a listener), it maps correctly to lazyLink. In the conversion (which won't be needed in the future) if something was node.on( '...', listener ); listener() then I changed it to a link. Don't see any need to document.

[jonathanolson]

I believe I've handled or responded to everything. @zepumph can you verify my changes?

[zepumph]

#490 (comment)

I also went back to phetsims/chipper#875 and saw that axon was a fluke, and other main.js files seemed to not be altered.

[zepumph]

@jonathanolson, I have gone through everything, it all looks good. Feel free to close.