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

Safe EventDispatcher #6064

Closed
wants to merge 1 commit into from
Closed

Safe EventDispatcher #6064

wants to merge 1 commit into from

Conversation

greggman
Copy link
Contributor

This is just a suggestion.

The current implementation of EventDispatcher is intrusive. It sticks _listeners on
object it's being added to. If that object already has a _listeners your out of luck.

Above is a non intrusive implementation for your consideration. The listeners
collection is not public whatsoever.

I tested them on jsperf and it's the same speed on chrome and significantly faster
on Firefox

I didn't expose dispatchEvent to the public API of TypedGeometry because it
seemed to me users of TypedGeometry shouldn't be able to dispatch events but
of course that could be exposed too.

Anyway, just passing it on as a suggestion. I'm learning JS as I go and finding that what I learned
at Google was wrong :P They use their Closure Compiler and they basically
treat JavaScript like Java and expect the compiler to make things semi safe but
JavaScript can be safe all by itself so I'm slowly learing more JavaScripty
ways.

@gero3
Copy link
Contributor

gero3 commented Feb 13, 2015

The problem with this solution is that every method is recreated for every object it uses. It isn't just the listeners object. This makes the objects heavier that they should be.

@greggman
Copy link
Contributor Author

The WebGLRenderer uses this style. Heavy is subjective. It's objectively safer, it's objectively faster. Heavy is arguably irrelevant in this case. A heavy Vector3 object would be bad because you've got millions of them. A heavy EventDispatcher is not bad because you've got 10s maybe 100s at most.

This is just a suggestion.

The current implementation of EventDispatcher is intrusive. It sticks `_listeners` on
object it's being added to. If that object already has a `_listeners` your out of luck.

Above is a non intrusive implementation for your consideration. The `listeners`
collection is not public whatsoever.

I [tested them on jsperf and it's the same speed on chrome and significantly faster
on Firefox](http://jsperf.com/event-dispatching)

I didn't expose `dispatchEvent` to the public API of `TypedGeometry` because it
seemed to me users of `TypedGeometry` shouldn't be able to dispatch events but
of course that could be exposed too.

Anyway, just passing it on as a suggestion. I'm learning JS as I go and finding that what I learned
at Google was wrong :P  They use their Closure Compiler and they basically
treat JavaScript like Java and expect the compiler to make things semi safe but
JavaScript can be safe all by itself so I'm slowly learing more JavaScripty
ways.
@gero3
Copy link
Contributor

gero3 commented Feb 13, 2015

not really becuase nearly every object has them (All object3ds, material, geometry)

@greggman
Copy link
Contributor Author

That's still thousands of objects. Not even the memory of 1 texture.

@threejsworker
Copy link

The examples of this pullrequest are now built and visible in threejsworker. To view them, go to the following link:

http://threejsworker.com/viewpullrequest.html#6064

3 similar comments
@threejsworker
Copy link

The examples of this pullrequest are now built and visible in threejsworker. To view them, go to the following link:

http://threejsworker.com/viewpullrequest.html#6064

@threejsworker
Copy link

The examples of this pullrequest are now built and visible in threejsworker. To view them, go to the following link:

http://threejsworker.com/viewpullrequest.html#6064

@threejsworker
Copy link

The examples of this pullrequest are now built and visible in threejsworker. To view them, go to the following link:

http://threejsworker.com/viewpullrequest.html#6064

@mrdoob mrdoob closed this Oct 15, 2015
@greggman greggman deleted the safe-event-dispatcher branch February 10, 2017 18:50
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