Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New event system that has more of an AS3 look-and-feel #185

Closed
wants to merge 2 commits into from

Conversation

charlie-rbchd
Copy link
Contributor

Some quick information about this :

  • A basic Observer Pattern implementation;
  • Support for context-passing and event priority;
  • All the same events that were there before are available through the new event system;
  • Features new events such as : keyboard events, a mousewheel event, added/removed from stage events, enter/exit frame events and a catch-all event;
  • There is no true capture nor bubbling phase (it works the same way it did with the previous event system);
  • Users can make their own custom events, just like in flash.

@Mithrandir0x
Copy link

So, a DisplayObject (or a Bitmap, Stage, etc) click event implementation would be something like:

var entity = new DisplayObject();
entity.addEventListener('click', function(){
    console.log('It tickles!');
});

And a class inheriting DisplayObject would be implemented like:

var DisplayObject = createjs['DisplayObject'];

var MyLittleEntity = function(){
    this.initialize();
};

MyLittleEntity.prototype = new DisplayObject();

MyLittleEntity.prototype.DisplayObject_initialize = MyLittleEntity.prototype.initialize;
MyLittleEntity.prototype.initialize = function(){
    this.DisplayObject_initialize();
    this.addEventListener('click', this.handleClickDefault, this);
};

MyLittleEntity.prototype.handleClick = function(){
    console.log('STOP POKING ME!!!!');
};

Am I wrong?

Also, there's a little thing at line 123 of file EventDispatcher.js. Why sorting the event array when an event is being dispatched, instead of doing it when a new event listener is being created?

@charlie-rbchd
Copy link
Contributor Author

Yes, you got the syntax right.

Also, there's a little thing at line 123 of file EventDispatcher.js. Why sorting the event array when an event is being dispatched, instead of doing it when a new event listener is being created?

Because the list of event handlers that is sorted is a merge between the handlers of the dispatched event type and the handlers of the "all" event type. The combined array is thus different from the array of handlers registered for that type and needs to be sorted every time.

@imarkahann
Copy link

👍 For this request to:

I actually use signalsJS: in the DisplayObject Prototype

createjs.DisplayObject.prototype.click = new signals.Signal();
createjs.DisplayObject.prototype.mouseover = new signals.Signal();
createjs.DisplayObject.prototype.mouseout = new signals.Signal();
createjs.DisplayObject.prototype.doubleclick = new signals.Signal();
createjs.DisplayObject.prototype.press = new signals.Signal();
createjs.DisplayObject.prototype.mousemove = new signals.Signal();

But EventDispatcher system could be very usefull. addEventListener with the possibility to change the call context (scope problem )

addEventListener(Class.EventName, function, context);

@imarkahann
Copy link

Sorry I don't see your Work @joelrobichaud , very good job ! I hope it will and to the master
Old French Flasheur rocks ! :p

@imarkahann
Copy link

A comment about MouseUp :
https://github.com/joelrobichaud/EaselJS/blob/master/src/easeljs/display/Stage.js#L483

In my tests mouseUp event is never dispatch on a Container ( for example ).

if (o.event && o.event.hasEventListener("mouseUp")) { o.event.dispatchEvent(mUpEvent); }
o.event is always undefined on the pointerUpHandler

"click" handler use o.target, to dispatch its Event.

Something I don't understand ? An error in the class ?

@imarkahann
Copy link

Another comment on your classes

https://github.com/joelrobichaud/EaselJS/blob/master/src/easeljs/display/Container.js#L512
throw an error : Uncaught ReferenceError: DOMElement is not defined

I change line 512 like
if (child instanceof createjs["DOMElement"]) {
or
if (child instanceof createjs.DOMElement) {

It is possible that I forgot something before use your classes. An idea ?

Thanks, and good job again for your fork !

@gskinner
Copy link
Member

hey @joelrobichaud - one thing that would be really helpful for me in deciding whether to accept this pull request is a performance comparison between this & the current handlers.

I'm not concerned about click interactions (they don't fire often enough to be a major issue), but ideally we'd move the whole event model over, and that means onTick would be affected.

I'd love to see a perf test with, say 500 particles using onTick or addEventListener("tick") to animate themselves (ie. you'd wind up with 500 different handlers or listeners). It's obviously going to be slower, but the important question is by how much.

Thanks!

@Mithrandir0x
Copy link

Well, I've done a little test using @joelrobichaud code (at his repository's "original_branch"). You can find it here.

It should be the same like the Cache example at CreateJS/EaselJS site, but instead of having a single tick event, there are 501 tick events (the stage, and 500 shapes). Also, each of them has a click event (check the console's log for output).

The odd thing is that I had to register the tick event through Ticker, is this intended or (obviously) am I doing something wrong? I am looking at the code, and I don't find anything related to the tick event, apart from deleting onTick callback in DisplayObject.

@gskinner
Copy link
Member

Hi everyone. Thanks for all the discussion & code submitted to this thread.

I took a quick stab at a slightly simpler implementation of an EventDispatcher, and would love to get your feedback on it. I tried to incorporate all the feedback here, and implement something that was fast, flexible, and had a nice balance between JS & AS3 sensibilities.

https://gist.github.com/11bd2e88d903c2126ce7

This is untested code, but shows the general mechanism.

  • it currently does not support event priority (though this would be very easy to add)
  • there is no base Event class, you can use typed objects, or just a generic object
  • it can be extended, or mixed in to either instances or prototypes, for DisplayObject we would likely use a prototype mix-in (with defined & documented members)
  • it has an optional mechanism for also calling the event callback (ex. onClick) during the dispatch
  • it uses a double linked list, which seems to provide a 5-10% speed bonus for traversal (minor)
  • it provides scoped callbacks, with the default scope as the target
  • fairly quick check for listeners on any event

I would love to get feedback on this. I still need to benchmark this to ensure it's viable (especially for tick), but I expect the cost should be reasonably low.

@gskinner
Copy link
Member

Latest:
https://gist.github.com/11bd2e88d903c2126ce7

Benchmarks:
Callbacks with EventDispatcher vs current callbacks (10,000 iterations)
Note that this test also represents the displaylist slowdown from this model in general, regardless of whether a callback is actually assigned.
Chrome: 0.3ms slower
Safari: 8.7ms slower

addEventListener vs current callbacks (10,000 iterations)
Chrome: 4ms slower
Safari: 19ms slower

These numbers look pretty manageable to me. The overhead in a top class JS engine (Chrome) appears to be insignificant. For a slower engine like Safari, pushing a particle system of 2500 items, this would introduce ~2ms per tick, which is dwarfed by the render cost.

The performance cost obviously doesn't matter at all for mouse events.

Thoughts?

Next up, I'll add priority.

@Mithrandir0x
Copy link

When you say 10,000 iterations, you mean 10,000 tick events being processed each Tick? Can you post the benchmark?

Maybe using a binary heap would solve the problem of adding and ordering events at the same time.

@gskinner
Copy link
Member

@Mithrandir0x Right. 10,000 events being dispatched (or called) in a single tick. I'll clean up the code a little bit and share it as a gist. In brief, it's a simple particle system where each particle has a tick listener (or callback), and I disable rendering when running the test (to isolate the code execution costs).

I've added priority, and switched back to using arrays. Generic benchmarks showed a performance boost using linked lists over arrays, but I'm not seeing it with this use case, and the added complexity wasn't worth it. I've also added removeAllEventListeners, which I think is critical to resource management.

https://gist.github.com/11bd2e88d903c2126ce7

@charlie-rbchd
Copy link
Contributor Author

@gskinner This is great! I think my version was a bit too focused on replicating the AS3 event system (althought this was done on purpose; I work with actionscript developers and transitioning to HTML5 is pretty hard for most of them).

I don't think the removal of the linked lists in favor of arrays removed that much complexity, but then again the performance gain is really easy to overlook.

As for the features, I think you pretty much covered it all, although it would be nice to have a way to dispatch a wildcard event that fires all the listeners of an object.

@gskinner
Copy link
Member

@joelrobichaud Thanks for the feedback, and even more so for the original pull request. It gave me something to compare my own approaches to, and I'm still using it as a reference as I continue to tweak events. It's fantastic to have an outside voice on this (fairly extensive) change to the libraries.

Some elements of your submission I think I will leave out of this NEXT version, but may reconsider in future (ex. added to stage events, which are very handy, but also have a fairly high performance cost).

I'll keep this pull request open until I've finalized the event model.

Thanks again.

@imarkahann
Copy link

Thanks Grant and Joël for your works :)

I wait to test the New event model

Good evening/morning/Night !

Le 24 oct. 2012 à 18:59, "Grant Skinner" <notifications@github.commailto:notifications@github.com> a écrit :

@joelrobichaudhttps://github.com/joelrobichaud Thanks for the feedback, and even more so for the original pull request. It gave me something to compare my own approaches to, and I'm still using it as a reference as I continue to tweak events. It's fantastic to have an outside voice on this (fairly extensive) change to the libraries.

Some elements of your submission I think I will leave out of this NEXT version, but may reconsider in future (ex. added to stage events, which are very handy, but also have a fairly high performance cost).

I'll keep this pull request open until I've finalized the event model.

Thanks again.


Reply to this email directly or view it on GitHubhttps://github.com//pull/185#issuecomment-9747270.

@gskinner
Copy link
Member

I have pushed an update to EaselJS that includes the new event model. Please see EventDispatcher documentation for details - these methods are currently mixed in to DisplayObject and MouseEvent.

While I was at it, I also added support for DisplayObject.cursor. Please see the docs for details.

I would REALLY appreciate if people can beat this up a bit and provide feedback.

This implementation is very backwards compatible. In fact, EaselJS will even continue to work with callbacks if you don't include EventDispatcher. You can even use both callbacks & addEventListener for the same events.

@gskinner
Copy link
Member

PS. I also included a DragAndDrop_EventDispatcher.html example to show this all in action.

@gskinner gskinner closed this Feb 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants