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

What's the purpose of EventId? #1070

Closed
Phlosioneer opened this issue May 26, 2016 · 4 comments
Closed

What's the purpose of EventId? #1070

Phlosioneer opened this issue May 26, 2016 · 4 comments

Comments

@Phlosioneer
Copy link
Contributor

The only time the internals of piston use it, is when writing functions to support applying lambdas if the event is of a particular type. This functionality can be reproduced using only enumerations. EventId is never used by any piston backend, and I can't find a single repo that uses the type, nor can I find any that use the lambda-based functions.

If there's no specific reason for having it, it should be removed, because it makes the event code more verbose and more difficult to understand.

@bvssvni
Copy link
Member

bvssvni commented May 26, 2016

The EventId makes it possible to add custom events on top of GenericEvent and abstract over the concrete type of the event. Since the generic code written for the Piston only uses GenericEvent, you don't have to update it when the underlying representation changes.

When somebody needs a different structure they often represent it differently in memory. The lambda based functions makes it possible to use the event without overhead, for example TextEvent takes a &str in the lambda method but String in the text_args method. See https://github.com/PistonDevelopers/piston/blob/master/src/input/src/text.rs#L12.

Since EventId contains a &'static str, it is possible to compare the pointer addresses of two events as an optimization for later. This does not cost more than an enum, and a &'static str is easier to use for event protocols that goes over the network, because the string always has the same value and it never collides with a different namespace if people use the "myengine/event" format.

In earlier days GenericEvent used TypeId<Box<EventTrait>> which LLVM optimized out when compiled with "-O2". See http://blog.piston.rs/2014/09/25/generic-event/.

We changed away from TypeId because TypeId::hash was removed in favor of the Hash trait in PR rust-lang/rust#21165 under Rust's stabilization process. This removed the optimization LLVM did and became a lot slower.

Later when we redesigned, we replaced it with EventId(pub &'static str), because it is very straight forward how to design your own custom events, by using "myengine/event".

@Phlosioneer
Copy link
Contributor Author

Phlosioneer commented May 26, 2016

Coooool. Thanks for the response!

A follow up question, then: Since the Input event type can be specified by an implementation of Window, why not make the extension point for events there? So instead of using myengine/event with GenericEvent, you'd create an enum that'd look like this:

enum InputSplitter {
  PistonEvent(Input),
  EngineEvent(MyCustomEventEnum)
}

And then your match code would look like this:

match Some(event) {
  // Other event code...
  Input(PistonEvent(event)) => do_stuff(event),
  Input(EngineEvent(event)) => do_stuff(event)
}

Backends would have to implement some extra functions to make InputSplitter compliant with the traits that the Piston API expects, but it should all be small enough to be inlined. And it keeps the strong type-safety of the rust compiler intact. Code that uses piston wouldn't be able to define events, but that's undesirable anyway because this event system is for backend code. Is there anything wrong with this idea?

@bvssvni
Copy link
Member

bvssvni commented May 26, 2016

When you write generic code, you have to use a trait. Let's say you have an event Foo. To call code that requires Foo, all methods in the call stack up to the event loop must pass on this constraint.

fn event<T: Foo>(e: &T) {
    sub_event(e)
}

fn event<T: Foo>(e: &T) {
    ...
}

The problem is when you introduce a new trait Bar. You have to change all the methods:

fn event<T: Foo + Bar>(e: &T) {
    sub_event(e)
}

fn event<T: Foo + Bar>(e: &T) {
    ...
}

This is not really back-end related, but for the whole ecosystem that depends on the library that defines the traits. GenericEvent makes it possible to just pass one trait constraint around and you never have to change the code except for the parts where there is a breaking change in a specific event trait.

All event traits in Piston are implemented for all types that implements GenericEvent. For example here: https://github.com/PistonDevelopers/piston/blob/master/src/input/src/cursor.rs#L18

impl<T: GenericEvent> CursorEvent for T {
    ...
}

So, people write:

use piston::input::CursorEvent;

...
if let Some(args) = e.cursor_args() {

}

It is simple and less to type because the Piston core or the back-end does the matching for you.

If there is a breaking change in how GenericEvent is implemented, then that's a back-end issue. However, downstream libraries doesn't notice this. It is designed to keep stuff up and running.

Let's assume that one engine adds a custom event which is really nice feature to use in generic libraries. The engine can then define an event trait for its own use, but other people who write generic libraries can just copy/paste the event trait to make it work. They don't have to use the same trait, just GenericEvent to make it work, as long as it uses same event id and expected argument type.

GenericEvent is the glue that keeps the ecosystem integrated:

         games/applications/scripting environments
                  /                    \
    (stable)    /                        \    (stable)
              /                            \
generic libraries        GenericEvent      back-ends
               \                           /
      (stable)   \                       /   (breaking changes)
                   \                   /
                         Piston core

@Phlosioneer
Copy link
Contributor Author

Thanks for the detailed explanation! I get it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants