Input rebinding phase 2 #261

Xaeroxe · Jul 21, 2017

Things done in this PR

Use smallvec for InputHandler internals, reducing allocations required.
Add functions
- down_buttons
- released_buttons
- down_keys
- released_keys
- down_mouse_buttons
- released_mouse_buttons

These functions will be helpful with assigning bindings in the first place when we just need to generically query for "whatever the user pressed."

Add functions
- axes
- actions

These functions will be helpful for figuring out what to display on input rebinding screens.

Enhance actions API to support multiple bindings per action
Move private functions into closures
Remove the opaque wrappers over iterators, return slice iterators where possible and in other places return a single ButtonIterator type.
Change actions and axes to use string keys instead of integer keys.

Eventually there is going to be a phase 3 to make this interact with the asset system, which I plan on doing as soon as Ron is ready for use.

torkleyy · Jul 21, 2017

torkleyy reviewed Jul 21, 2017

View changes

torkleyy · Jul 21, 2017

src/ecs/resources/input.rs

-    pub fn pressed_keys(&self) -> PressedKeys {
-        self.current_frame.pressed_keys()
+    /// Returns a vector containing pressed down keys.
+    pub fn pressed_keys(&self) -> &SmallVec<[VirtualKeyCode; 16]> {


Could we just coerce this to a slice?

We can and that idea is like 1000x better. Thank you!

torkleyy · Jul 21, 2017

torkleyy reviewed Jul 21, 2017

View changes

torkleyy · Jul 21, 2017

src/ecs/resources/input.rs

     }
 
     /// Returns a vector containing mouse buttons released on this frame.
-    pub fn released_mouse_buttons(&self) -> &SmallVec<[MouseButton; 8]> {
-        &self.released_mouse_buttons
+    pub fn released_mouse_buttons(&self) -> &[MouseButton] {


I would find it more consistent to return an iterator everywhere or a slice everywhere. Since the latter seems to be impossible, let's go for the former. You can just use slice::Iter here.

torkleyy · Jul 21, 2017

torkleyy reviewed Jul 21, 2017

View changes

torkleyy · Jul 21, 2017

src/ecs/resources/input.rs

-    Button::Mouse(*mb)
-}
+    /// Returns an action's bindings.
+    pub fn get_action_bindings(&self, id: i32) -> Option<SmallVec<[Button; 8]>> {


By convention, getters should not start with get if possible.

torkleyy · Jul 21, 2017

src/ecs/resources/mod.rs

@@ -11,6 +11,6 @@ mod broadcaster;
 
 pub use self::broadcaster::Broadcaster;
 pub use self::camera::{Camera, Projection};
-pub use self::input::{Axis, Button, InputHandler, PressedButtons, PressedKeys, PressedMouseButtons};
+pub use self::input::{Axis, Button, InputHandler, ButtonIterator};


Please don't forget to order these imports.

torkleyy · Jul 21, 2017

src/lib.rs

@@ -71,6 +71,7 @@ pub extern crate amethyst_renderer as renderer;
 pub extern crate amethyst_config as config;
 
 extern crate cgmath;
+extern crate smallvec;


Please move it down so the alphabetical order is maintained.

ebkalderon · Jul 21, 2017

ebkalderon requested changes Jul 21, 2017

View changes

Apart from some minor nitpicks, the meat of this PR looks solid! Thank you, @Xaeroxe.

ebkalderon · Jul 21, 2017

src/ecs/resources/input.rs

@@ -333,9 +333,9 @@ impl InputHandler {
     }
 
     /// Returns the value of an axis by the i32 id, if the id doesn't exist this returns None.
-    pub fn axis_value(&self, id: i32) -> Option<f32> {
+    pub fn axis_value(&self, id: &str) -> Option<f32> {


Since this is a user-facing API, could we use AsRef<str> instead of &str in all the axis_ methods so users can easily pass in different kinds of string types?

I disagree. It's more difficult to read and you'll almost always pass a literal. If we argue with "it can save the user a method call sometimes", then Rust's choice being explicit about conversions doesn't make sense.

The AsRef trait exists precisely for this reason, though. It's meant to eliminate boilerplate in case users decide to pass in a String, CStr(ing), OsStr(ing), or perhaps even a simple array of bytes, and more importantly, it can prevent extra heap allocations on the user's end.

The last part is only valid for Into<String>, not for AsRef.

Good point, I looked into it and I must have indeed been thinking about Into<String>. But my first point still stands. Even the Rust standard library makes frequent use of AsRef in exactly this way to avoid such boilerplate, and I think that users might appreciate an API that assists them similarly with zero runtime cost.

If we use Into<String> we'll likely be introducing an unnecessary allocation, as the hashmap prefers to take keys by reference rather than by value. Most usage of this is going to be string literals.

@Xaeroxe Sorry, perhaps I wasn't clear enough. I am suggesting we use AsRef<str>, not Into<String>. I had mistakenly pointed out that AsRef<str> would reduce unnecessary allocations, confusing it for Into<String>.

However, I still stand by my point that AsRef<str> will behave identically to &str except it will allow users to pass in other string slice types like OsStr and CStr with less boilerplate. I also cite the Rust standard library as a frequent user of this pattern.

ebkalderon · Jul 21, 2017

Cargo.toml

@@ -40,6 +40,7 @@ specs = "0.9.1"
 rayon = "0.7"
 ticketed_lock = "0.1"
 wavefront_obj = "5.0"
+smallvec = "0.4"


Please move this entry upward to retain alphabetical order.

ebkalderon · Jul 21, 2017

src/ecs/resources/input.rs

+                }
+            }
+            None => {
+                make_new = true;


Why have a make_new variable? Can't lines 469-471 be copied into this branch verbatim and be easier to read?

No, because it makes the borrow checker mad. self.actions is already borrowed thanks to the match statement.

Ahh, I see. I forgot about the borrow checker. I wonder if the upcoming non-lexical lifetimes in Rust will help with these kinds of issues.

ebkalderon · Jul 21, 2017

src/ecs/resources/input.rs

+                action_bindings.swap_remove(index);
+            }
+            if action_bindings.len() == 0 {
+                kill_it = true;


Same comment as with the make_new variable in insert_action_binding.

ebkalderon · Jul 21, 2017

src/ecs/resources/input.rs

-    pub fn pressed_keys(&self) -> PressedKeys {
-        self.current_frame.pressed_keys()
+    /// Returns a vector containing pressed down keys.
+    pub fn pressed_keys(&self) -> Iter<VirtualKeyCode> {


Could this be made into a type alias called KeyCodes?

ebkalderon · Jul 21, 2017

src/ecs/resources/input.rs

-    pub fn mouse_button_is_pressed(&self, button: MouseButton) -> bool {
-        self.current_frame.mouse_button_is_pressed(button)
+    /// Returns a vector containing pressed down mouse buttons.
+    pub fn pressed_mouse_buttons(&self) -> Iter<MouseButton> {


Could this be made into a type alias called MouseButtons?

ebkalderon · Jul 21, 2017

src/ecs/resources/input.rs

+    }
+
+    /// Returns a vector containing the buttons that were pressed this frame
+    pub fn down_buttons(&self) -> ButtonIterator {


Could ButtonIterator be renamed to Buttons to remain consistent with other iterator names?

@ebkalderon Changes you requested have been made.

Xaeroxe · Jul 21, 2017

@ebkalderon Changes you requested have been made.

torkleyy · Jul 21, 2017

torkleyy requested changes Jul 21, 2017

View changes

That's great! It still needs some polishing, though.

torkleyy · Jul 21, 2017

src/ecs/resources/input.rs

-    /// Returns an iterator for all the pressed down keys.
-    pub fn pressed_keys(&self) -> PressedKeys {
-        self.current_frame.pressed_keys()
+    /// Returns a vector containing pressed down keys.


Doc comment seems outdated.

torkleyy · Jul 21, 2017

src/ecs/resources/input.rs

-    /// Checks if the given key is being pressed.
-    pub fn key_is_pressed(&self, key: VirtualKeyCode) -> bool {
-        self.current_frame.key_is_pressed(key)
+    /// Returns a vector containing keys pressed on this frame.


torkleyy · Jul 21, 2017

src/ecs/resources/input.rs

-    /// Checks if all the given keys are being pressed.
-    pub fn keys_are_pressed(&self, keys: &[VirtualKeyCode]) -> bool {
-        self.current_frame.keys_are_pressed(keys)
+    /// Returns a vector containing keys released on this frame.


torkleyy · Jul 21, 2017

src/ecs/resources/input.rs

+        self.down_mouse_buttons.iter()
+    }
+
+    /// Returns a vector containing mouse buttons released on this frame.


torkleyy · Jul 21, 2017

src/ecs/resources/input.rs

     }
 
     /// Checks if all the given actions are pressed.
     ///
     /// If any action in this list is invalid this will return the id of it in Err.
-    pub fn actions_are_pressed(&self, actions: &[i32]) -> Result<bool, Vec<i32>> {
+    pub fn actions_are_pressed<T: AsRef<str>>(&self, actions: &[T]) -> Result<bool, Vec<String>> {


Can we remove the verb here like it is the case above with the mouse buttons?

Verb is used to visually distinguish this function name, otherwise the function names get too similar and people get confused.

torkleyy · Jul 21, 2017

src/ecs/resources/input.rs

-    /// If one does exist this will replace the action at that id and return it.
-    pub fn insert_action(&mut self, id: i32, binding: Button) -> Option<Button> {
-        self.actions.insert(id, binding)
+    /// Get's a list of all axes


I don't think we need the '.

torkleyy · Jul 21, 2017

src/ecs/resources/input.rs

-        self.actions.insert(id, binding)
+    /// Get's a list of all axes
+    pub fn axes(&self) -> Vec<String> {
+        self.axes.keys().map(|k| k.clone()).collect::<Vec<String>>()


Why collect it?

Cuz I don't want another opaque Iterator type for this function, and this function is called very infrequently.

torkleyy · Jul 21, 2017

src/ecs/resources/input.rs

+    /// 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) {
+        let mut make_new = false;
+        match self.actions.get_mut(id.as_ref()) {


I think we can use the Entry API here.

We can, I used it, it mandated an extra allocation in situations where it wasn't necessary, so I took it out.

torkleyy · Jul 21, 2017

src/ecs/resources/input.rs

+            if let Some(index) = index {
+                action_bindings.swap_remove(index);
+            }
+            if action_bindings.len() == 0 {


Let's use late binding here: Change the line at the beginning of the method to let kill_it; and change this line to kill_it = action_bindings.is_empty().

Not possible, if the given action isn't valid then we can't run kill_it = action_bindings.is_empty() which leaves kill_it uninitialized.

torkleyy · Jul 21, 2017

src/ecs/resources/mod.rs

@@ -11,6 +11,6 @@ mod broadcaster;
 
 pub use self::broadcaster::Broadcaster;
 pub use self::camera::{Camera, Projection};
-pub use self::input::{Axis, Button, InputHandler, PressedButtons, PressedKeys, PressedMouseButtons};
+pub use self::input::{Axis, Button, Buttons, KeyCodes, MouseButtons, InputHandler};


Sorry, but this is not ordered alphabetically. Hope I don't annoy you with that.

Alphabetical ordering together with the maximum line width is super annoying. Gotta redo quite a few lines just to make a simple change :(

So what do you suggest? I just think that alphabetical order helps when you read them (and it looks really clean and tidy).

ebkalderon · Jul 21, 2017

ebkalderon approved these changes Jul 21, 2017

View changes

@torkleyy Requesting either your approval or additional input

Xaeroxe · Jul 21, 2017

@torkleyy Requesting either your approval or additional input

torkleyy · Jul 21, 2017

torkleyy approved these changes Jul 21, 2017

View changes

It's great, thanks for addressing my comments!

Xaeroxe added some commits Jul 21, 2017

Input rebinding phase 2

b4f10c4

Add extern crate

Loading status checks…

9e1af14

Xaeroxe requested a review from amethyst/engine-devs Jul 21, 2017

Xaeroxe referenced this pull request Jul 21, 2017
Closed
Nitro vs Amethyst comparison #245

Xaeroxe added some commits Jul 21, 2017

Remove unneccesary cast

Loading status checks…

d35185f

Add method to get all axis IDs

Loading status checks…

0405e63

Make comments more horizontally friendly

Loading status checks…

29f2f3d

Use slices and an iterator where possible

Loading status checks…

57ff6c3

Change them all into iterators

Loading status checks…

ad3dcbe

Make requested changes

Loading status checks…

a8b5bf0

Xaeroxe added some commits Jul 21, 2017

Fix doc comments and add generic arguments

Loading status checks…

05babf8

Merge branch 'develop' into input-phase2

Loading status checks…

f34a74e

torkleyy merged commit 87a2846 into amethyst:develop Jul 21, 2017
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Xaeroxe deleted the Xaeroxe:input-phase2 branch Aug 8, 2017

amethyst/amethyst

Join GitHub today

Input rebinding phase 2 #261

Conversation

Xaeroxe commented Jul 21, 2017 • edited Edited 1 time Xaeroxe edited Jul 21, 2017 (most recent)

Xaeroxe added some commits Jul 21, 2017

Xaeroxe requested a review from amethyst/engine-devs Jul 21, 2017

Xaeroxe referenced this pull request Jul 21, 2017

Xaeroxe added some commits Jul 21, 2017

torkleyy reviewed Jul 21, 2017 View changes

This comment has been minimized.

This comment has been minimized.

torkleyy reviewed Jul 21, 2017 View changes

This comment has been minimized.

torkleyy reviewed Jul 21, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Xaeroxe added some commits Jul 21, 2017

ebkalderon requested changes Jul 21, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ebkalderon Jul 21, 2017 • edited Edited 1 time ebkalderon edited Jul 21, 2017 (most recent)

This comment has been minimized.

This comment has been minimized.

ebkalderon Jul 21, 2017 • edited Edited 1 time ebkalderon edited Jul 21, 2017 (most recent)

This comment has been minimized.

This comment has been minimized.

ebkalderon Jul 21, 2017 • edited Edited 1 time ebkalderon edited Jul 21, 2017 (most recent)

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ebkalderon Jul 21, 2017 • edited Edited 1 time ebkalderon edited Jul 21, 2017 (most recent)

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Xaeroxe commented Jul 21, 2017

torkleyy requested changes Jul 21, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Xaeroxe added some commits Jul 21, 2017

ebkalderon approved these changes Jul 21, 2017 View changes

This comment has been minimized.

Xaeroxe commented Jul 21, 2017

torkleyy approved these changes Jul 21, 2017 View changes

Hide details View details torkleyy merged commit 87a2846 into amethyst:develop Jul 21, 2017 2 checks passed

2 checks passed

Xaeroxe deleted the Xaeroxe:input-phase2 branch Aug 8, 2017

Xaeroxe commented Jul 21, 2017 •

edited

Edited 1 time

Xaeroxe edited Jul 21, 2017 (most recent)

torkleyy reviewed Jul 21, 2017

View changes

torkleyy reviewed Jul 21, 2017

View changes

torkleyy reviewed Jul 21, 2017

View changes

ebkalderon requested changes Jul 21, 2017

View changes

ebkalderon Jul 21, 2017 •

edited

Edited 1 time

ebkalderon edited Jul 21, 2017 (most recent)

ebkalderon Jul 21, 2017 •

edited

Edited 1 time

ebkalderon edited Jul 21, 2017 (most recent)

ebkalderon Jul 21, 2017 •

edited

Edited 1 time

ebkalderon edited Jul 21, 2017 (most recent)

ebkalderon Jul 21, 2017 •

edited

Edited 1 time

ebkalderon edited Jul 21, 2017 (most recent)

torkleyy requested changes Jul 21, 2017

View changes

ebkalderon approved these changes Jul 21, 2017

View changes

torkleyy approved these changes Jul 21, 2017

View changes

torkleyy merged commit `87a2846` into amethyst:develop Jul 21, 2017
2 checks passed