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

Input rebinding phase 3 #274

Merged
merged 2 commits into from Aug 11, 2017

Conversation

Projects
None yet
4 participants
@Xaeroxe
Member

Xaeroxe commented Aug 9, 2017

Things done in this PR:

  • Split out implementation into multiple files
  • Add serde remote derive for VirtualKeyCode and MouseButton
  • Add Bindings structure which can be saved and loaded with amethyst_config
  • Restructure button/key/mouse_button API to use ButtonState to query for inputs in a specific state rather than having a function for each state.
  • Upgrade smallvec dependency to permit serialization and deserialization.
  • Fix pong example.

@Xaeroxe Xaeroxe requested a review from amethyst/engine-devs Aug 9, 2017

@Xaeroxe Xaeroxe assigned Xaeroxe and unassigned Xaeroxe Aug 9, 2017

@torkleyy torkleyy self-assigned this Aug 9, 2017

@omni-viral

omni-viral requested changes Aug 9, 2017 edited

Terms down and pressed are little bit confusing.
I see that by down you mean the button was pressed this frame and pressed is state.
Although pressed does look more like antonym for released than down.

src/ecs/resources/input/bindings.rs
+ pub fn axes(&self) -> Vec<String> {
+ self.axes
+ .keys()
+ .map(|k| k.clone())

This comment has been minimized.

@omni-viral

omni-viral Aug 9, 2017

Member

Use Iterator::cloned

@omni-viral

omni-viral Aug 9, 2017

Member

Use Iterator::cloned

+ /// Removes an action binding that was assigned previously.
+ pub fn remove_action_binding<T: AsRef<str>>(&mut self, id: T, binding: Button) {
+ let mut kill_it = false;
+ if let Some(action_bindings) = self.actions.get_mut(id.as_ref()) {

This comment has been minimized.

@omni-viral

omni-viral Aug 9, 2017

Member

Consider using Entry API to do lookup once.

if let Entry::Occupied(action_bindings) = self.actions.entry(id.as_ref()) {
    let index = action_bindings.get().iter().position(|&b| b == binding);
    if let Some(index) = index {
        action_bindings.get_mut().swap_remove(index);
    }
    if action_bindings.get().is_empty() {
        action_bindings.remove();
    }
}
@omni-viral

omni-viral Aug 9, 2017

Member

Consider using Entry API to do lookup once.

if let Entry::Occupied(action_bindings) = self.actions.entry(id.as_ref()) {
    let index = action_bindings.get().iter().position(|&b| b == binding);
    if let Some(index) = index {
        action_bindings.get_mut().swap_remove(index);
    }
    if action_bindings.get().is_empty() {
        action_bindings.remove();
    }
}

This comment has been minimized.

@torkleyy

torkleyy Aug 9, 2017

Member

@omni-viral Guess .entry(id.as_ref()) ain't gonna work out, since it accepts K which is String here. So we have an unnecessary allocation :(

@torkleyy

torkleyy Aug 9, 2017

Member

@omni-viral Guess .entry(id.as_ref()) ain't gonna work out, since it accepts K which is String here. So we have an unnecessary allocation :(

This comment has been minimized.

@Xaeroxe

Xaeroxe Aug 9, 2017

Member

@torkleyy is correct, the entry API requires you to pass ownership of the key since the entry API might have to create the entry itself.

@Xaeroxe

Xaeroxe Aug 9, 2017

Member

@torkleyy is correct, the entry API requires you to pass ownership of the key since the entry API might have to create the entry itself.

src/ecs/resources/input/input_handler.rs
@@ -81,6 +28,8 @@ impl<'a> Iterator for Buttons<'a> {
/// ```
#[derive(Default)]
pub struct InputHandler {
+ /// Maps inputs to actions and axes.
+ pub bindings: Bindings,
pressed_keys: SmallVec<[VirtualKeyCode; 16]>,

This comment has been minimized.

@omni-viral

omni-viral Aug 9, 2017

Member

Can it be stored also withing big array with state (pressed, released, down) for each VirtualKeyCode for lookup purpose?

@omni-viral

omni-viral Aug 9, 2017

Member

Can it be stored also withing big array with state (pressed, released, down) for each VirtualKeyCode for lookup purpose?

This comment has been minimized.

@Xaeroxe

Xaeroxe Aug 9, 2017

Member

There are complications in doing that, it was definitely the first approach I considered though. While it is possible to use enums as indices into an array it raises a few problems for maintenance purposes

  • No built in alert system for when new enums are added that need to be accounted for.
  • No guarantee that enums as usize values will continue to be contiguous in the future.

With this approach we have the downside that code may occasionally need to iterate a few values, which I consider to be preferable.

@Xaeroxe

Xaeroxe Aug 9, 2017

Member

There are complications in doing that, it was definitely the first approach I considered though. While it is possible to use enums as indices into an array it raises a few problems for maintenance purposes

  • No built in alert system for when new enums are added that need to be accounted for.
  • No guarantee that enums as usize values will continue to be contiguous in the future.

With this approach we have the downside that code may occasionally need to iterate a few values, which I consider to be preferable.

+/// Used for saving and loading input settings.
+#[derive(Default, Serialize, Deserialize)]
+pub struct Bindings {
+ pub(super) axes: HashMap<String, Axis>,

This comment has been minimized.

@omni-viral

omni-viral Aug 9, 2017

Member

Strings are good choice but can we have arbitrary T: Eq + Hash here?

@omni-viral

omni-viral Aug 9, 2017

Member

Strings are good choice but can we have arbitrary T: Eq + Hash here?

This comment has been minimized.

@Xaeroxe

Xaeroxe Aug 9, 2017

Member

The type would have to be Eq + Hash + Serialize + Deserialize but sure.

@Xaeroxe

Xaeroxe Aug 9, 2017

Member

The type would have to be Eq + Hash + Serialize + Deserialize but sure.

@torkleyy

Nice PR! I like that modules are getting smaller now and it seems we can have input config files soon ;)

The API could probably be more self-explanatory, but that below is just an idea I don't have an implementation for, so not sure if it makes sense.

src/ecs/resources/input/input_handler.rs
@@ -81,6 +28,8 @@ impl<'a> Iterator for Buttons<'a> {
/// ```
#[derive(Default)]
pub struct InputHandler {

This comment has been minimized.

@torkleyy

torkleyy Aug 9, 2017

Member

Doesn't this struct totally blow up the stack? Although you did this in another PR, I don't think we need SmallVec here since it's not like a collection of collections.

@torkleyy

torkleyy Aug 9, 2017

Member

Doesn't this struct totally blow up the stack? Although you did this in another PR, I don't think we need SmallVec here since it's not like a collection of collections.

This comment has been minimized.

@Xaeroxe

Xaeroxe Aug 9, 2017

Member

@torkleyy ecs resources go on the heap anyways, the SmallVec just keeps us from having to do even more pointer dereferences. If this makes the structure feel too bloated we can change it but I believe this approach would be more optimal.

@Xaeroxe

Xaeroxe Aug 9, 2017

Member

@torkleyy ecs resources go on the heap anyways, the SmallVec just keeps us from having to do even more pointer dereferences. If this makes the structure feel too bloated we can change it but I believe this approach would be more optimal.

This comment has been minimized.

@torkleyy

torkleyy Aug 9, 2017

Member

Ah right, agreed.

@torkleyy

torkleyy Aug 9, 2017

Member

Ah right, agreed.

+#[derive(Eq, PartialEq, Debug, Copy, Clone, Serialize, Deserialize)]
+pub enum Button {
+ /// Keyboard keys
+ Key(#[serde(with = "LocalVirtualKeyCode")]

This comment has been minimized.

@torkleyy

torkleyy Aug 9, 2017

Member

Given that we already got our own input structs, would it make sense to just get rid of glutin types in the public API and use our own types?

@torkleyy

torkleyy Aug 9, 2017

Member

Given that we already got our own input structs, would it make sense to just get rid of glutin types in the public API and use our own types?

This comment has been minimized.

@Xaeroxe

Xaeroxe Aug 9, 2017

Member

@torkleyy I had that thought, since glutin types are also winit types I figured this way might be easier so we don't have to write conversions, but ultimately I don't really have strong feelings one way or another.

@Xaeroxe

Xaeroxe Aug 9, 2017

Member

@torkleyy I had that thought, since glutin types are also winit types I figured this way might be easier so we don't have to write conversions, but ultimately I don't really have strong feelings one way or another.

src/ecs/resources/input/input_handler.rs
@@ -358,9 +309,10 @@ impl InputHandler {
// Pressed actions has been omitted as the function is a little difficult to write and I
// can't think of a use case for it.
- /// Checks if the given button is currently pressed.
+ /// Checks if the given action is currently pressed.
pub fn action_is_pressed<T: AsRef<str>>(&self, action: T) -> Option<bool> {

This comment has been minimized.

@torkleyy

torkleyy Aug 9, 2017

Member

Maybe we could have a more unified API for actions which just returns some ActionState object. It could provide getters like is_active and this_frame. Not sure, but it could make things easier to understand (the differences in naming we currently have aren't too clear about that).

@torkleyy

torkleyy Aug 9, 2017

Member

Maybe we could have a more unified API for actions which just returns some ActionState object. It could provide getters like is_active and this_frame. Not sure, but it could make things easier to understand (the differences in naming we currently have aren't too clear about that).

This comment has been minimized.

@Xaeroxe

Xaeroxe Aug 9, 2017

Member

Not a bad idea. I'll certainly consider it.

@Xaeroxe

Xaeroxe Aug 9, 2017

Member

Not a bad idea. I'll certainly consider it.

This comment has been minimized.

@Xaeroxe

Xaeroxe Aug 9, 2017

Member

@torkleyy Current rough idea is to add this enum:

/// Indicates a button's state.  Inner bool is true if the state changed this frame.
pub enum ButtonState {
    Pressed(bool),
    Released(bool),
}

Then have functions return this enum, or take it as a parameter.

@Xaeroxe

Xaeroxe Aug 9, 2017

Member

@torkleyy Current rough idea is to add this enum:

/// Indicates a button's state.  Inner bool is true if the state changed this frame.
pub enum ButtonState {
    Pressed(bool),
    Released(bool),
}

Then have functions return this enum, or take it as a parameter.

This comment has been minimized.

@Xaeroxe

Xaeroxe Aug 10, 2017

Member

@torkleyy I implemented a modified version of the ButtonState idea, please review.

@Xaeroxe

Xaeroxe Aug 10, 2017

Member

@torkleyy I implemented a modified version of the ButtonState idea, please review.

+/// Two of these could be analogous to a DPAD.
+#[derive(Serialize, Deserialize)]
+pub struct Axis {
+ /// Positive button, when pressed down axis value will return 1 if `neg` is not pressed down.

This comment has been minimized.

@torkleyy

torkleyy Aug 9, 2017

Member

Stupid question: Couldn't an axis also be e.g. Mouse Movement Y?

@torkleyy

torkleyy Aug 9, 2017

Member

Stupid question: Couldn't an axis also be e.g. Mouse Movement Y?

This comment has been minimized.

@OvermindDL1

OvermindDL1 Aug 9, 2017

Traditionally I had 3 types of inputs in my old system.

Boolean: Simple on/off, like a button or a key.

Absolute: Can be like a joystick axis or a button that is pressure sensitive, regardless they have defined min/max values (which can be queried for based on the type).

Relative: Like a mouse axis, it can move an 'arbitrary' amount in a single call with no defined min or max. I also exposed a 'scale' value with the type to allow for scaling based on user settings for per-hardware.

Do note, my system supported and distinguished between multiple keyboards, mice, and gamepads (and a Wiimote since I had one to test with) without issue (even on Windows, I used a low-level hook to get input instead of directinput, which let me distinguish between multiple input devices, on linux this was trivial).

@OvermindDL1

OvermindDL1 Aug 9, 2017

Traditionally I had 3 types of inputs in my old system.

Boolean: Simple on/off, like a button or a key.

Absolute: Can be like a joystick axis or a button that is pressure sensitive, regardless they have defined min/max values (which can be queried for based on the type).

Relative: Like a mouse axis, it can move an 'arbitrary' amount in a single call with no defined min or max. I also exposed a 'scale' value with the type to allow for scaling based on user settings for per-hardware.

Do note, my system supported and distinguished between multiple keyboards, mice, and gamepads (and a Wiimote since I had one to test with) without issue (even on Windows, I used a low-level hook to get input instead of directinput, which let me distinguish between multiple input devices, on linux this was trivial).

This comment has been minimized.

@Xaeroxe

Xaeroxe Aug 9, 2017

Member

@torkleyy This particular Axis struct? Not really. Although I do see the merit in introducing a more abstract Axis API so I'll consider that.

@Xaeroxe

Xaeroxe Aug 9, 2017

Member

@torkleyy This particular Axis struct? Not really. Although I do see the merit in introducing a more abstract Axis API so I'll consider that.

This comment has been minimized.

@OvermindDL1

OvermindDL1 Aug 9, 2017

@Xaeroxe Thoughts about my old design? I was able to emulate every input device I came across with those 3 styles (the value types being bool, float constraint, float unconstrained).

@OvermindDL1

OvermindDL1 Aug 9, 2017

@Xaeroxe Thoughts about my old design? I was able to emulate every input device I came across with those 3 styles (the value types being bool, float constraint, float unconstrained).

This comment has been minimized.

@Xaeroxe

Xaeroxe Aug 9, 2017

Member

I like the idea, we already have pretty good support for bool and float constraint, although the float constraint could be improved upon to be more generic rather than using strictly boolean inputs. float unconstrained seems like a good idea for mouse stuff but I think I already emulate that pretty well with mouse_position and mouse_position_change. What other input devices have you encountered that needed float unconstrained?

@Xaeroxe

Xaeroxe Aug 9, 2017

Member

I like the idea, we already have pretty good support for bool and float constraint, although the float constraint could be improved upon to be more generic rather than using strictly boolean inputs. float unconstrained seems like a good idea for mouse stuff but I think I already emulate that pretty well with mouse_position and mouse_position_change. What other input devices have you encountered that needed float unconstrained?

This comment has been minimized.

@OvermindDL1

OvermindDL1 Aug 9, 2017

@Xaeroxe A Wiimote is a good example. It gets exposed as a device with (as I recall) 7 buttons, 3 unconstrained axis (translation in 3d space), 3 constained axis (rotation in 3d space), force feedback, and a speaker.

In addition, the Steam Controller. It can expose itself to a game as an unbounded number of buttons (well I think 256 max), some rotation axis (constrained), and the 2 touchpads on top can become any number of buttons, axis (both constrained or unconstrained) and more, depending on how it is set up, and it can change on-the-fly.

Among others as well.

For note, the 'hardware' in my system was not even exposed as keyboards/mice/gamepads/whateverCustomThings, but rather just exposed as an Input device with those 3 exposed things, and a few more things, the Output devices were things like monitors and InputOutput devices were things like my keyboard that had both inputs (boolean and a single constrained axis, yes on a keyboard) and outputs of a texture (each button can be colored on my keyboard mapped to I think it was a 28x96 RGB image that could be sent to it). Everything was 'tagged' (with my Atom64 type) so like my keyboard was tagged as both a KEYBOARD and a DISPLAY (of size 28x96) and a couple other things. My mouse is similar but it has 2 unconstrained axis, 17 buttons, and it was also an Output device because it accepted commands to be able to change the DPI and colors on the fly, yes it was also a display type.

Everything that used the inputs (I've talked about my mapper before) had no clue what the device type was, they just took inputs and translated them to game commands, easily remappable. The hardware devices were mapped in my 'virtual' filesystem at /dev/whatever (potentially in multiple places there depending on its tags), and commands 'to' them or callbacks 'from' them could be routed around the virtual filesystem to issue commands to the engine (everything had a mount somewhere in the virtual filesystem and you could tie things together with impunity), even the ECS system had a mount in the filesystem (even the VFS was an ECS, a game level would be another distinct ECS, perhaps even a singular entity in the level could have its own embedded ECS for very complex entities, I routinely hit 4 levels deep of ECS). But I've talked about the VFS in depth elsewhere in some other issues around. ^.^;

There are a lot of weird devices out there, my system ended up being able to handle them all (not the prettiest C++ code, but it worked really well and it worked fast). :-)

@OvermindDL1

OvermindDL1 Aug 9, 2017

@Xaeroxe A Wiimote is a good example. It gets exposed as a device with (as I recall) 7 buttons, 3 unconstrained axis (translation in 3d space), 3 constained axis (rotation in 3d space), force feedback, and a speaker.

In addition, the Steam Controller. It can expose itself to a game as an unbounded number of buttons (well I think 256 max), some rotation axis (constrained), and the 2 touchpads on top can become any number of buttons, axis (both constrained or unconstrained) and more, depending on how it is set up, and it can change on-the-fly.

Among others as well.

For note, the 'hardware' in my system was not even exposed as keyboards/mice/gamepads/whateverCustomThings, but rather just exposed as an Input device with those 3 exposed things, and a few more things, the Output devices were things like monitors and InputOutput devices were things like my keyboard that had both inputs (boolean and a single constrained axis, yes on a keyboard) and outputs of a texture (each button can be colored on my keyboard mapped to I think it was a 28x96 RGB image that could be sent to it). Everything was 'tagged' (with my Atom64 type) so like my keyboard was tagged as both a KEYBOARD and a DISPLAY (of size 28x96) and a couple other things. My mouse is similar but it has 2 unconstrained axis, 17 buttons, and it was also an Output device because it accepted commands to be able to change the DPI and colors on the fly, yes it was also a display type.

Everything that used the inputs (I've talked about my mapper before) had no clue what the device type was, they just took inputs and translated them to game commands, easily remappable. The hardware devices were mapped in my 'virtual' filesystem at /dev/whatever (potentially in multiple places there depending on its tags), and commands 'to' them or callbacks 'from' them could be routed around the virtual filesystem to issue commands to the engine (everything had a mount somewhere in the virtual filesystem and you could tie things together with impunity), even the ECS system had a mount in the filesystem (even the VFS was an ECS, a game level would be another distinct ECS, perhaps even a singular entity in the level could have its own embedded ECS for very complex entities, I routinely hit 4 levels deep of ECS). But I've talked about the VFS in depth elsewhere in some other issues around. ^.^;

There are a lot of weird devices out there, my system ended up being able to handle them all (not the prettiest C++ code, but it worked really well and it worked fast). :-)

+ self.text_this_frame.push(c);
+ }
+ Event::KeyboardInput(ElementState::Pressed, _, Some(key_code)) => {
+ if self.pressed_keys.iter().all(|&k| k != key_code) {

This comment has been minimized.

@omni-viral

omni-viral Aug 10, 2017

Member

I don't think you need to add more logic here. If backend says key was pressed - it was pressed.
Actually I think this piece of code will return true all the time.

@omni-viral

omni-viral Aug 10, 2017

Member

I don't think you need to add more logic here. If backend says key was pressed - it was pressed.
Actually I think this piece of code will return true all the time.

This comment has been minimized.

@Xaeroxe

Xaeroxe Aug 10, 2017

Member

@omni-viral Additional pressed signals are sent by some backends when the key "repeats" due to being held down, it's very important to the remove code that any entry be present once and only once, so this keeps the repeat signals from getting added in.

@Xaeroxe

Xaeroxe Aug 10, 2017

Member

@omni-viral Additional pressed signals are sent by some backends when the key "repeats" due to being held down, it's very important to the remove code that any entry be present once and only once, so this keeps the repeat signals from getting added in.

@torkleyy

A few comments for now ;) Sorry, it's not yet a complete review, I hope I have some more time later.

+
+/// Describes an input state for a button.
+#[derive(Eq, PartialEq, Debug, Copy, Clone)]
+pub enum ButtonState {

This comment has been minimized.

@torkleyy

torkleyy Aug 10, 2017

Member

I like the structure here 👍 We should probably think about at which level we want to abstract over different input methods and we should design a clear and obvious API for that.

@torkleyy

torkleyy Aug 10, 2017

Member

I like the structure here 👍 We should probably think about at which level we want to abstract over different input methods and we should design a clear and obvious API for that.

This comment has been minimized.

@torkleyy

torkleyy Aug 10, 2017

Member

Some methods would be nice for this struct.

@torkleyy

torkleyy Aug 10, 2017

Member

Some methods would be nice for this struct.

This comment has been minimized.

@Xaeroxe

Xaeroxe Aug 10, 2017

Member

@torkleyy Such as? I feel Rust's pattern matching fits most needs for this.

@Xaeroxe

Xaeroxe Aug 10, 2017

Member

@torkleyy Such as? I feel Rust's pattern matching fits most needs for this.

+//! World resource that handles all user input.
+
+use engine::{ElementState, WindowEvent, Event, VirtualKeyCode, MouseButton};
+use smallvec::SmallVec;

This comment has been minimized.

@torkleyy

torkleyy Aug 10, 2017

Member
std imports

dependency imports

local imports

Could we sort inputs like that?

Also see the fmt RFC issue

@torkleyy

torkleyy Aug 10, 2017

Member
std imports

dependency imports

local imports

Could we sort inputs like that?

Also see the fmt RFC issue

This comment has been minimized.

@Xaeroxe

Xaeroxe Aug 10, 2017

Member

Sure, I'll do that.

@Xaeroxe

Xaeroxe Aug 10, 2017

Member

Sure, I'll do that.

+ }
+
+ /// Checks if a key matches the description given by state.
+ pub fn key_is(&self, key: VirtualKeyCode, state: ButtonState) -> bool {

This comment has been minimized.

@torkleyy

torkleyy Aug 10, 2017

Member

Would be nice to have that as method as noted above ^

@torkleyy

torkleyy Aug 10, 2017

Member

Would be nice to have that as method as noted above ^

This comment has been minimized.

@Xaeroxe

Xaeroxe Aug 10, 2017

Member

@torkleyy Sorry, I'm not sure I understand. A method for what? If you're talking about a method for ButtonState then I don't quite see what the point is as it would have to take an InputHandler reference, it'd basically just be a rearrangement of this interface. As I understand it you want

impl ButtonState {
    pub fn key_is(&self, input: &InputHandler, key: VirtualKeyCode) -> bool {
        input.key_is(key, *self);
    }
}
@Xaeroxe

Xaeroxe Aug 10, 2017

Member

@torkleyy Sorry, I'm not sure I understand. A method for what? If you're talking about a method for ButtonState then I don't quite see what the point is as it would have to take an InputHandler reference, it'd basically just be a rearrangement of this interface. As I understand it you want

impl ButtonState {
    pub fn key_is(&self, input: &InputHandler, key: VirtualKeyCode) -> bool {
        input.key_is(key, *self);
    }
}
+ }
+
+ /// Returns an iterator over all keys in the given state, does not support Released(Currently).
+ pub fn keys_that_are(&self, state: ButtonState) -> KeyCodes {

This comment has been minimized.

@torkleyy

torkleyy Aug 10, 2017

Member

Could we have iterators which can easily be filtered? Would probably be easier (although requires Box due to missing impl Trait).

@torkleyy

torkleyy Aug 10, 2017

Member

Could we have iterators which can easily be filtered? Would probably be easier (although requires Box due to missing impl Trait).

This comment has been minimized.

@Xaeroxe

Xaeroxe Aug 10, 2017

Member

@torkleyy This implements Iterator which has the .filter() method, so this can be filtered.

@Xaeroxe

Xaeroxe Aug 10, 2017

Member

@torkleyy This implements Iterator which has the .filter() method, so this can be filtered.

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Aug 10, 2017

Member

I think the ideal Input API would look something like this:

InputManager

  • action(&Self, Action) -> Option<ActionState>
  • axis(&Self, Axis) -> Option<f32>

Not sure about @OvermindDL1's "absolute" type. I think it would be enough to have the above distinction for now. Action and Axis are probably just Strings, configurable via RON config files. Not only actions should allow multiple bindings, but also axes (think about binding stuff to mouse and controller sticks). Next thing is iterating over both types of input:

  • iter_actions(&Self) -> Iter<(&Action, ActionState)>
  • iter_axes(&Self) -> Iter<(&Axis, f32)>

I don't think we should provide many other methods which automatically collect keys or something like that; a simple system like the above makes it very easy to use and understand the code. There are two other things left; mouse movement and action / axis insertion; I think the former is fine as is, for the latter I'd expose something similar to the Entry API, which at the same time allows for very flexible (and fast) access to these actions / axes.

Many things are already implemented like that, but I just wanted to share my idea of what an Input manager should look like. If you prefer button over action, we can do that, but it would be nice to be consistent about that.

Methods on ActionState

That's something I already noted in my review; it would be nice to have simple methods like

  • pressed

so you could just do

if input.action("shoot").pressed() {}

instead of

if input.action_is("shoot", Pressed(Currently)) {}

I would hope that we can remove more special-casing from the input manager that way. Moving more functionality to returned types (like to ActionState and Entry-like actions / axes) makes the structure easier.

Member

torkleyy commented Aug 10, 2017

I think the ideal Input API would look something like this:

InputManager

  • action(&Self, Action) -> Option<ActionState>
  • axis(&Self, Axis) -> Option<f32>

Not sure about @OvermindDL1's "absolute" type. I think it would be enough to have the above distinction for now. Action and Axis are probably just Strings, configurable via RON config files. Not only actions should allow multiple bindings, but also axes (think about binding stuff to mouse and controller sticks). Next thing is iterating over both types of input:

  • iter_actions(&Self) -> Iter<(&Action, ActionState)>
  • iter_axes(&Self) -> Iter<(&Axis, f32)>

I don't think we should provide many other methods which automatically collect keys or something like that; a simple system like the above makes it very easy to use and understand the code. There are two other things left; mouse movement and action / axis insertion; I think the former is fine as is, for the latter I'd expose something similar to the Entry API, which at the same time allows for very flexible (and fast) access to these actions / axes.

Many things are already implemented like that, but I just wanted to share my idea of what an Input manager should look like. If you prefer button over action, we can do that, but it would be nice to be consistent about that.

Methods on ActionState

That's something I already noted in my review; it would be nice to have simple methods like

  • pressed

so you could just do

if input.action("shoot").pressed() {}

instead of

if input.action_is("shoot", Pressed(Currently)) {}

I would hope that we can remove more special-casing from the input manager that way. Moving more functionality to returned types (like to ActionState and Entry-like actions / axes) makes the structure easier.

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Aug 10, 2017

Member

@torkleyy I tried something very similar to this structure originally, however I ended up not liking it as it created inconsistent semantics. For example if the action function returns Pressed(ThisFrame) then that would be != Pressed(Currently) and while you could certainly resolve this by providing the .pressed() function that requires you to know Pressed(ThisFrame) != Pressed(Currently) which for a veteran like us seems obvious but for any beginning game devs this seems unfair. "I asked if it was pressed currently, and it is, so why is this not working?" With this approach if I ask for Pressed(Currently) I will also get true if it was pressed on this frame. I feel this approach is simpler and requires the user of the API to know less about how it works internally.

Member

Xaeroxe commented Aug 10, 2017

@torkleyy I tried something very similar to this structure originally, however I ended up not liking it as it created inconsistent semantics. For example if the action function returns Pressed(ThisFrame) then that would be != Pressed(Currently) and while you could certainly resolve this by providing the .pressed() function that requires you to know Pressed(ThisFrame) != Pressed(Currently) which for a veteran like us seems obvious but for any beginning game devs this seems unfair. "I asked if it was pressed currently, and it is, so why is this not working?" With this approach if I ask for Pressed(Currently) I will also get true if it was pressed on this frame. I feel this approach is simpler and requires the user of the API to know less about how it works internally.

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Aug 10, 2017

Member

With this approach if I ask for Pressed(Currently) I will also get true if it was pressed on this frame.

Huh? I think that behavior can be really confusing. With pattern matching, you can just do

match .. {
    ActionState::Pressed(_) => {}
}

(but that's of course not necessary; examples just show this using the methods, which will then be used most of the time)

Member

torkleyy commented Aug 10, 2017

With this approach if I ask for Pressed(Currently) I will also get true if it was pressed on this frame.

Huh? I think that behavior can be really confusing. With pattern matching, you can just do

match .. {
    ActionState::Pressed(_) => {}
}

(but that's of course not necessary; examples just show this using the methods, which will then be used most of the time)

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Aug 10, 2017

Member

@torkleyy Currently -> Now -> ThisFrame

It makes sense to me, if something is happening this frame then it is also happening currently.

Member

Xaeroxe commented Aug 10, 2017

@torkleyy Currently -> Now -> ThisFrame

It makes sense to me, if something is happening this frame then it is also happening currently.

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Aug 10, 2017

Member

Right. In that case: could we just not make it an enum / wrap the enum as an implementation detail?

Member

torkleyy commented Aug 10, 2017

Right. In that case: could we just not make it an enum / wrap the enum as an implementation detail?

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Aug 10, 2017

Member

well maybe. What do you think of this?

pub struct ButtonState {
    pressed: bool,
    this_frame: bool,
}

impl ButtonState {
    pub fn is_pressed(&self) -> bool {
        self.pressed
    }

    pub fn was_pressed_this_frame(&self) -> bool {
        self.pressed && self.this_frame
    }

    pub fn was_released_this_frame(&self) -> bool {
        !self.pressed && self.this_frame
    }
}

EDIT: Personally I quite like this approach, as it makes populating the input states much easier as well.

Member

Xaeroxe commented Aug 10, 2017

well maybe. What do you think of this?

pub struct ButtonState {
    pressed: bool,
    this_frame: bool,
}

impl ButtonState {
    pub fn is_pressed(&self) -> bool {
        self.pressed
    }

    pub fn was_pressed_this_frame(&self) -> bool {
        self.pressed && self.this_frame
    }

    pub fn was_released_this_frame(&self) -> bool {
        !self.pressed && self.this_frame
    }
}

EDIT: Personally I quite like this approach, as it makes populating the input states much easier as well.

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Aug 10, 2017

Member

Yeah, let's go for that 👍

Member

torkleyy commented Aug 10, 2017

Yeah, let's go for that 👍

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Aug 10, 2017

Member

Well crap I'm actually not so sure I like my suggestion @torkleyy because now it's more difficult to query for buttons in a specific state. That works pretty well for receiving a state but there's points in the API where it's useful to send a state too. We need the ability to send so something like a "key rebinding" menu can be programmed, as you need the ability to generically query for "any input in this state" in order to do that.

Member

Xaeroxe commented Aug 10, 2017

Well crap I'm actually not so sure I like my suggestion @torkleyy because now it's more difficult to query for buttons in a specific state. That works pretty well for receiving a state but there's points in the API where it's useful to send a state too. We need the ability to send so something like a "key rebinding" menu can be programmed, as you need the ability to generically query for "any input in this state" in order to do that.

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Aug 10, 2017

Member

Are you sure? I mean you didn't yet share implementation details with me, but with the above approach you could just have a hash map mapping action -> (Buttons, ActionState). Iterate over it, filter and map it and you should be able to do that.

Member

torkleyy commented Aug 10, 2017

Are you sure? I mean you didn't yet share implementation details with me, but with the above approach you could just have a hash map mapping action -> (Buttons, ActionState). Iterate over it, filter and map it and you should be able to do that.

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Aug 10, 2017

Member

But I'm super tired right now, so maybe that doesn't make sense. And I won't be able to respond for the next hours :(

Member

torkleyy commented Aug 10, 2017

But I'm super tired right now, so maybe that doesn't make sense. And I won't be able to respond for the next hours :(

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Aug 10, 2017

Member

@torkleyy I suppose, it just isn't as well optimized as the current approach. Which isn't necessarily a huge deal but I kind of don't really see why, as the current approach works. Is there anything you could do with the prior suggestion that you couldn't do with the current approach?

Member

Xaeroxe commented Aug 10, 2017

@torkleyy I suppose, it just isn't as well optimized as the current approach. Which isn't necessarily a huge deal but I kind of don't really see why, as the current approach works. Is there anything you could do with the prior suggestion that you couldn't do with the current approach?

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Aug 11, 2017

Member

PR to smallvec merged, now we just need to wait for a cargo release. Next release PR is here: servo/rust-smallvec#58

Member

Xaeroxe commented Aug 11, 2017

PR to smallvec merged, now we just need to wait for a cargo release. Next release PR is here: servo/rust-smallvec#58

@Xaeroxe Xaeroxe changed the title from [WIP] Input rebinding phase 3 to Input rebinding phase 3 Aug 11, 2017

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Aug 11, 2017

Member

smallvec just got a new version released, this PR has been updated to use it. I consider this ready to merge.

Member

Xaeroxe commented Aug 11, 2017

smallvec just got a new version released, this PR has been updated to use it. I consider this ready to merge.

@Xaeroxe Xaeroxe requested a review from amethyst/engine-devs Aug 11, 2017

@torkleyy

I'd still like some more simplicity in this input system. If you don't mind, I'd try to create a PR. Another thing I'd like to have is support for mouse / joystick as axis. Not sure if there is a library out there for that.

- plank.position += plank.velocity * delta_time;
+ // Move plank according to axis input.
+ if let Some(value) = input.axis_value("P1") {
+ plank.position += plank.velocity * delta_time * value;

This comment has been minimized.

@torkleyy

torkleyy Aug 11, 2017

Member

This indention is weird

@torkleyy

torkleyy Aug 11, 2017

Member

This indention is weird

@@ -0,0 +1,14 @@
+(
+ axes: {

This comment has been minimized.

@torkleyy

torkleyy Aug 11, 2017

Member

Niice!

@torkleyy

torkleyy Aug 11, 2017

Member

Niice!

@torkleyy

I think this is fine for now and we should just try it out and see what changes are needed later. So thank you @Xaeroxe! Looking at the examples, I'm excited to try out how the input stuff works in practice. Last thing - please move the input stuff to a crate and squash the commits a bit.

@torkleyy torkleyy merged commit 65b3b8f into amethyst:develop Aug 11, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@Xaeroxe Xaeroxe deleted the Xaeroxe:input-phase3 branch Aug 11, 2017

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