/amethyst

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

Refactor input to use shrev #385

Merged
merged 9 commits into from Sep 30, 2017
+484 −356

Conversation

Reviewers

@omni-viral omni-viral

@Rhuagh Rhuagh

@jojolepro jojolepro

@torkleyy torkleyy

@Aceeri Aceeri

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
5 participants
@Xaeroxe @torkleyy @Rhuagh @jojolepro @omni-viral
@Xaeroxe
Member

Xaeroxe commented Sep 29, 2017

Problem with the current InputHandler: It queries for if a key was pressed or release ThisFrame which isn't very helpful to systems and state functions that don't trigger every frame. Solution: Integrate shrev with the InputHandler so users can acquire and use ReaderIDs with an EventHandler<InputEvent> specs resource. More enhancements to the input system are on the way, but I just wanted to share this much since it is ready.

Notice: This PR also sneaks in a few other small improvements, such as Scancode buttons, and some new new() constructors.

cc @Rhuagh @jojolepro

@Xaeroxe Xaeroxe added project: input status: ready type: improvement labels Sep 29, 2017

@Xaeroxe Xaeroxe requested review from omni-viral and torkleyy Sep 29, 2017

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Sep 29, 2017

Member

I'm in favor of making InputHandler optional, as @Rhuagh suggested (together with allowing custom bindings). Not sure if that's what this PR is doing, I'll have to review this tomorrow.
Member

torkleyy commented Sep 29, 2017

I'm in favor of making InputHandler optional, as @Rhuagh suggested (together with allowing custom bindings). Not sure if that's what this PR is doing, I'll have to review this tomorrow.
@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Sep 29, 2017

Member

I'll have a PR for this PR soon, that does that.
Member

Rhuagh commented Sep 29, 2017

I'll have a PR for this PR soon, that does that.
@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Sep 29, 2017

Member

Thanks for your PR @Rhuagh, I also had a few improvements of my own on top of your ideas. This is ready for review now.
Member

Xaeroxe commented Sep 29, 2017

Thanks for your PR @Rhuagh, I also had a few improvements of my own on top of your ideas. This is ready for review now.
@Rhuagh

Rhuagh approved these changes Sep 29, 2017
View changes

@jojolepro

This comment has been minimized.

Show comment
Hide comment
@jojolepro

jojolepro Sep 29, 2017

Collaborator

Code looks fine to me. I asked questions on gitter, and I'll approve when I have the answers ;)
Collaborator

jojolepro commented Sep 29, 2017

Code looks fine to me. I asked questions on gitter, and I'll approve when I have the answers ;)
amethyst_input/src/bindings.rs
}
/// Add a button to an action.
///
/// This will insert a new binding between this action and the button.
- pub fn insert_action_binding<T: AsRef<str>>(&mut self, id: T, binding: Button) {
+ pub fn insert_action_binding(&mut self, id: AC, binding: Button) {

This comment has been minimized.

Show comment
Hide comment
@omni-viral

omni-viral Sep 29, 2017

Member

I think it can be less restrictive

pub fn insert_action_binding<A>(&mut self, id: AC, binding: Button)
    where A: Hash + Eq + Into<AC>,
          AC: Borrow<A>
{

For AC = String id can type of type &str with allocation only if not present in map yet.
@omni-viral

omni-viral Sep 29, 2017

Member

I think it can be less restrictive

pub fn insert_action_binding<A>(&mut self, id: AC, binding: Button)
    where A: Hash + Eq + Into<AC>,
          AC: Borrow<A>
{

For AC = String id can type of type &str with allocation only if not present in map yet.

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Sep 29, 2017

Member

I considered how that could be done, couldn't figure it out in the time I had, but our almighty omni figured it out :)

I don't foresee this method being called very often, but being less restrictive is always good.
@Rhuagh

Rhuagh Sep 29, 2017

Member

I considered how that could be done, couldn't figure it out in the time I had, but our almighty omni figured it out :)

I don't foresee this method being called very often, but being less restrictive is always good.
@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Sep 29, 2017

Member

r? @omni-viral
Member

Xaeroxe commented Sep 29, 2017

r? @omni-viral
src/ecs/input/system.rs
@@ -41,13 +41,13 @@ where
fn run(&mut self, (input, mut handler, mut output): Self::SystemData) {
match input.read(&mut self.reader) {
- Ok(data) => for d in data {
+ Ok(EventReadData::Data(data)) => for d in data {
if let &Event::WindowEvent { ref event, .. } = d {
handler.send_event(event, &mut *output);
}
},

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Sep 29, 2017

Member

Might want a variant for EventReadData::Overflow that do the same things as above, and log a warning ?
@Rhuagh

Rhuagh Sep 29, 2017

Member

Might want a variant for EventReadData::Overflow that do the same things as above, and log a warning ?
@Rhuagh

Rhuagh approved these changes Sep 29, 2017
View changes

@Xaeroxe Xaeroxe requested a review from Aceeri Sep 29, 2017

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Sep 29, 2017

Member

@Aceeri I'd like you to review the InputEvent enum specifically and tell me if you think that gives you enough input data to make the UI framework.
Member

Xaeroxe commented Sep 29, 2017

@Aceeri I'd like you to review the InputEvent enum specifically and tell me if you think that gives you enough input data to make the UI framework.
@jojolepro

jojolepro approved these changes Sep 30, 2017
View changes

You have my seal of approval! https://media.giphy.com/media/uTv1bQNRPFYTS/giphy.gif

Side note: Supporting controllers + VR headset positional data is a must have for the future.

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Sep 30, 2017

Member

Controllers and vr headsets require support in winit first, but yes.
Member

Rhuagh commented Sep 30, 2017

Controllers and vr headsets require support in winit first, but yes.
amethyst_input/src/bindings.rs
-pub struct Bindings {
- pub(super) axes: HashMap<String, Axis>,
- pub(super) actions: HashMap<String, SmallVec<[Button; 4]>>,
+pub struct Bindings<AX, AC> where AX: Hash + Eq, AC: Hash + Eq {

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Sep 30, 2017

Member

formatting?
@torkleyy

torkleyy Sep 30, 2017

Member

formatting?
@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Sep 30, 2017

Member

@Rhuagh winit has stated that is out of scope: tomaka/winit#119 so we'll need another crate that can gather input for that.
Member

Xaeroxe commented Sep 30, 2017

@Rhuagh winit has stated that is out of scope: tomaka/winit#119 so we'll need another crate that can gather input for that.
@Xaeroxe
Run rustfmt on amethyst_input
Loading status checks…
bda4820
@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Sep 30, 2017

Member

Looks great!

bors r+
Member

torkleyy commented Sep 30, 2017

Looks great!

bors r+

bors bot added a commit that referenced this pull request Sep 30, 2017

@bors[bot]
Merge #385 
385: Refactor input to use shrev r=torkleyy a=Xaeroxe

Problem with the current InputHandler: It queries for if a key was pressed or release `ThisFrame` which isn't very helpful to systems and state functions that don't trigger every frame.  Solution: Integrate shrev with the InputHandler so users can acquire and use `ReaderID`s with an `EventHandler<InputEvent>` specs resource.  More enhancements to the input system are on the way, but I just wanted to share this much since it is ready.

Notice: This PR also sneaks in a few other small improvements, such as Scancode buttons, and some new `new()` constructors.

cc @Rhuagh @jojolepro
Loading status checks…
9a127a2

@Xaeroxe Xaeroxe referenced this pull request Sep 30, 2017

Closed

Finalize input design #275

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors bot Sep 30, 2017

Contributor

Build succeeded
Contributor

bors bot commented Sep 30, 2017

Build succeeded

@bors bors bot merged commit bda4820 into amethyst:develop Sep 30, 2017
3 checks passed

3 checks passed

bors Build succeeded
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Xaeroxe Xaeroxe deleted the Xaeroxe:input-refactor branch Sep 30, 2017

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