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

[RFC] Prototyping speed improvements #582

Open
jojolepro opened this Issue Feb 26, 2018 · 26 comments

Comments

@jojolepro
Collaborator

jojolepro commented Feb 26, 2018

Problem

Prototyping in amethyst is slow.

100 lines of code seems to be the minimum to make the smallest of game. While its not a lot, it definitely is more than necessary.

Source

I'm taking https://github.com/amethyst/amethyst/blob/develop/examples/sphere/main.rs as a model
Lack of good defaults for generic types.
Imports
Lack of handle_event utils for simple cases -> Quit on any key, quit on some key, is key pressed.

Propositions

Lack of good defaults for generic types.

Adding sensible defaults as type alias for generics.

  1. Add a default.rs file.
  2. Make a DefaultSomething type alias with a default consistent with the others.
  3. Export the module default.

Example:
amethyst_input/src/default.rs

type DefaultInputHandler = InputHandler<String,String>

Imports

Expand the prelude to include as many common types as possible. I'm not too sure how much this slows down compile time when using the prelude, but I do have performance degradation in one of my project where I was using only preludes.

Lack of handle_event utils for simple cases -> Quit on any key, quit on some key, is key pressed.

I already implemented it inside of a custom project, currently debating if this should be inside of the engine.

@jojolepro

This comment has been minimized.

Show comment
Hide comment
@jojolepro

jojolepro Feb 26, 2018

Collaborator

first!

Collaborator

jojolepro commented Feb 26, 2018

first!

@majstudio

This comment has been minimized.

Show comment
Hide comment

Bump !

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Feb 26, 2018

Member

I'm fine with this, but please don't use external libraries which are unreleased (and not even close to complete) as the main example.

Member

Rhuagh commented Feb 26, 2018

I'm fine with this, but please don't use external libraries which are unreleased (and not even close to complete) as the main example.

@Rhuagh Rhuagh closed this Feb 26, 2018

@Rhuagh Rhuagh reopened this Feb 26, 2018

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Feb 26, 2018

Member

Oops, wrong button.

Member

Rhuagh commented Feb 26, 2018

Oops, wrong button.

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Feb 26, 2018

Member

I mean, when is a normal user going to use something else, except in an extremely low amount of edge cases?

Just a personal opinion on this, but I would never use String.

Member

Rhuagh commented Feb 26, 2018

I mean, when is a normal user going to use something else, except in an extremely low amount of edge cases?

Just a personal opinion on this, but I would never use String.

@jojolepro

This comment has been minimized.

Show comment
Hide comment
@jojolepro

jojolepro Feb 26, 2018

Collaborator

Not even for your prototypes?

Collaborator

jojolepro commented Feb 26, 2018

Not even for your prototypes?

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Feb 26, 2018

Member

Not even for my prototypes.

Member

Rhuagh commented Feb 26, 2018

Not even for my prototypes.

@jojolepro jojolepro changed the title from [RFC] Defaults to [RFC] Prototyping speed improvements May 2, 2018

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 19, 2018

Member

MaterialDefaults is a resource, I think it should be a constant.

No, it should not. It's a fallback for when a texture handle is invalid and the user may provide it. Also, a constant would need late initialization (and yeah involve global state, probably not work with multithreaded tests, ..).

Member

torkleyy commented May 19, 2018

MaterialDefaults is a resource, I think it should be a constant.

No, it should not. It's a fallback for when a texture handle is invalid and the user may provide it. Also, a constant would need late initialization (and yeah involve global state, probably not work with multithreaded tests, ..).

@jojolepro

This comment has been minimized.

Show comment
Hide comment
@jojolepro

jojolepro May 19, 2018

Collaborator

Lack of handle_event utils for simple cases -> Quit on any key, quit on some key, is key pressed.
I already implemented it inside of a custom project, currently debating if this should be inside of the engine.

Thinking of adding it in amethyst_input/src/utils.rs instead of amethyst_utils.
Would that make sense @torkleyy ?

ps: I removed the material defaults part.

Collaborator

jojolepro commented May 19, 2018

Lack of handle_event utils for simple cases -> Quit on any key, quit on some key, is key pressed.
I already implemented it inside of a custom project, currently debating if this should be inside of the engine.

Thinking of adding it in amethyst_input/src/utils.rs instead of amethyst_utils.
Would that make sense @torkleyy ?

ps: I removed the material defaults part.

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh May 19, 2018

Member

Sounds like an excellent location.

Member

Rhuagh commented May 19, 2018

Sounds like an excellent location.

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 19, 2018

Member

Hmm I'm not sure, creating a convenience function for everything might obscure things too much.

Member

torkleyy commented May 19, 2018

Hmm I'm not sure, creating a convenience function for everything might obscure things too much.

@jojolepro

This comment has been minimized.

Show comment
Hide comment
@jojolepro

jojolepro May 19, 2018

Collaborator

So, who wants to have this: https://github.com/jojolepro/amethyst-extra/blob/master/src/lib.rs#L193 (lines 193 to 257 included into the engine, and who doesn't? I don't mind keeping it external, but integrating it would make the examples cleaner.

Collaborator

jojolepro commented May 19, 2018

So, who wants to have this: https://github.com/jojolepro/amethyst-extra/blob/master/src/lib.rs#L193 (lines 193 to 257 included into the engine, and who doesn't? I don't mind keeping it external, but integrating it would make the examples cleaner.

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe May 19, 2018

Member

I agree there shouldn't be a function for everything, but this qualifies as extremely common. I say we should add it.

Member

Xaeroxe commented May 19, 2018

I agree there shouldn't be a function for everything, but this qualifies as extremely common. I say we should add it.

@MrMinimal

This comment has been minimized.

Show comment
Hide comment
@MrMinimal

MrMinimal May 19, 2018

Contributor

Generating a quad and handling basic events should be part of the engine, I'd agree it's okay to add it. As long as we keep @torkleyy's concerns in mind for other (less integral) features.

Contributor

MrMinimal commented May 19, 2018

Generating a quad and handling basic events should be part of the engine, I'd agree it's okay to add it. As long as we keep @torkleyy's concerns in mind for other (less integral) features.

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 19, 2018

Member

Well, I think it would be nice to have a more concise Event type. It should just be

match event {
    Event::KeyPressed(KeyCode::Escapce) => blah,
}

That would make things a lot nicer.

Member

torkleyy commented May 19, 2018

Well, I think it would be nice to have a more concise Event type. It should just be

match event {
    Event::KeyPressed(KeyCode::Escapce) => blah,
}

That would make things a lot nicer.

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 19, 2018

Member

That's why I'm still in favor of exposing our own event type instead of winit::Event.

Member

torkleyy commented May 19, 2018

That's why I'm still in favor of exposing our own event type instead of winit::Event.

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe May 19, 2018

Member

Why not expose both?

Member

Xaeroxe commented May 19, 2018

Why not expose both?

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 19, 2018

Member

Exposing just our own type allows us to upgrade winit without breakage. Additionally, if we convert winit events into our own type, I don't see any need to interface with the winit version.

Member

torkleyy commented May 19, 2018

Exposing just our own type allows us to upgrade winit without breakage. Additionally, if we convert winit events into our own type, I don't see any need to interface with the winit version.

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 19, 2018

Member

Having two ways of dealing with the problem would be confusing IMO.

Member

torkleyy commented May 19, 2018

Having two ways of dealing with the problem would be confusing IMO.

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe May 19, 2018

Member

That's fair. I was mostly thinking of people who might want to use winit events to interface with other winit compatible crates, but I'm not sure I have any good examples at this time.

Member

Xaeroxe commented May 19, 2018

That's fair. I was mostly thinking of people who might want to use winit events to interface with other winit compatible crates, but I'm not sure I have any good examples at this time.

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh May 19, 2018

Member

I do that, I convert winit events myself into other events.

Member

Rhuagh commented May 19, 2018

I do that, I convert winit events myself into other events.

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh May 19, 2018

Member

I do think we can add a system event for the things that are hard events

Member

Rhuagh commented May 19, 2018

I do think we can add a system event for the things that are hard events

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 19, 2018

Member

for the things that are hard events

What do you understand by that?

Member

torkleyy commented May 19, 2018

for the things that are hard events

What do you understand by that?

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh May 19, 2018

Member

Window close, resize etc, things that you usually don't rebind.

Member

Rhuagh commented May 19, 2018

Window close, resize etc, things that you usually don't rebind.

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 19, 2018

Member

I'm not sure I understand what you're suggesting, sorry.

Member

torkleyy commented May 19, 2018

I'm not sure I understand what you're suggesting, sorry.

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh May 19, 2018

Member

I'm not even sure I do myself :P

My current gripe with the current handle_event function calls are that there's absolutely no control from the users perspective what events get propagated there. For the most part if you want input in a usable manner today you'd request access to an EventChannel manually, and just ignore what comes to handle_event. Somehow I guess to basic use case for it was the ability to get easy access to input events.

The problem however is that input events can roughly be divided into two groups of events:

  • What I call hard events - Close, Resize, Focus etc - Things that are never rebound, just acted upon
  • Rebindable events - Mouse, Keyboard, Touchpad etc, i.e. actual input events, which are most likely passed through some kind of rebinding and transformed into an event structure specific to each game.

I'm not sure I even have a proposal right now, just some gripes.

Member

Rhuagh commented May 19, 2018

I'm not even sure I do myself :P

My current gripe with the current handle_event function calls are that there's absolutely no control from the users perspective what events get propagated there. For the most part if you want input in a usable manner today you'd request access to an EventChannel manually, and just ignore what comes to handle_event. Somehow I guess to basic use case for it was the ability to get easy access to input events.

The problem however is that input events can roughly be divided into two groups of events:

  • What I call hard events - Close, Resize, Focus etc - Things that are never rebound, just acted upon
  • Rebindable events - Mouse, Keyboard, Touchpad etc, i.e. actual input events, which are most likely passed through some kind of rebinding and transformed into an event structure specific to each game.

I'm not sure I even have a proposal right now, just some gripes.

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