Refactor `UserInput` and `InputKind` variants into separate implementations #490

Shute052 · 2024-02-23T18:11:02Z

Objective

Parts of completing Refactoring underlying architecture to support various input types #483
Closes Represent user inputs via traits, rather than a large enum #321
Continues with UserInput as trait #346

The current UserInput and InputKind enums are overly large and challenging to extend with new input types.

Solution

Introduce a new trait called UserInput trait.

Refactor the UserInput and InputKind variants into separate impls of the new UserInput trait, each focusing on a single responsibility.

TODO

Fix broken clashing check.
Documentation and doc examples.

Breacking Changes

renamed Modifier enum to ModifierKey, avoiding potential naming conflicts with other libraries or parts of your codebase.
refactored InputKind variants for representing user inputs, it now represents the kind of data an input can provide (e.g., button-like input, axis-like input).
removed UserInput enum in favor of the new UserInput trait and its impls.
by default, all inputs are unprocessed now, using With*ProcessingPipelineExt methods to define your preferred processing steps.
applied clashing check to continuous inputs, for example:
- MouseScrollAxis::Y will clash with MouseScrollDirection::UP and MouseScrollDirection::DOWN.
- MouseMove will clash with all individual axes and all directional events.

Changelog

added UserInput trait.
added UserInput impls for gamepad input events:
- implemented UserInput for Bevy’s GamepadAxisType-related inputs.
  - GamepadStick: Continuous or discrete movement events of the left or right gamepad stick along both X and Y axes.
  - GamepadControlAxis: Continuous or discrete movement events of a GamepadAxisType.
  - GamepadControlDirection: Discrete movement direction events of a GamepadAxisType, treated as a button press.
- implemented UserInput for Bevy’s GamepadButtonType directly.
- added GamepadVirtualAxis, similar to the old UserInput::VirtualAxis using two GamepadButtonTypes.
- added GamepadVirtualDPad, similar to the old UserInput::VirtualDPad using four GamepadButtonTypes.
added UserInput impls for keyboard inputs:
- implemented UserInput for Bevy’s KeyCode directly.
- implemented UserInput for ModifierKey.
- added KeyboardVirtualAxis, similar to the old UserInput::VirtualAxis using two KeyCodes.
- added KeyboardVirtualDPad, similar to the old UserInput::VirtualDPad using four KeyCodes.
added UserInput impls for mouse inputs:
- implemented UserInput for movement-related inputs.
  - MouseMove: Continuous or discrete movement events of the mouse both X and Y axes.
  - MouseMoveAxis: Continuous or discrete movement events of the mouse on an axis, similar to the old MouseMotionAxisType.
  - MouseMoveDirection: Discrete movement direction events of the mouse on an axis, similar to the old MouseMotionDirection.
- implemented UserInput for wheel-related inputs.
  - MouseScroll: Continuous or discrete movement events of the mouse wheel both X and Y axes.
  - MouseScrollAxis: Continuous or discrete movement events of the mouse wheel on an axis, similar to the old MouseWheelAxisType.
  - MouseScrollDirection: Discrete movement direction events of the mouse wheel on an axis, similar to the old MouseWheelDirection.
added InputChord for combining multiple inputs, similar to the old UserInput::Chord.

Migration Guide

The old SingleAxis is now:
- GamepadControlAxis for gamepad axes.
- MouseMoveAxis::X and MouseMoveAxis::Y for continuous mouse movement.
- MouseScrollAxis::X and MouseScrollAxis::Y for continuous mouse wheel movement.
The old DualAxis is now:
- GamepadStick for gamepad sticks.
- MouseMove::default() for continuous mouse movement.
- MouseScroll::default() for continuous mouse wheel movement.
The old Modifier is now ModifierKey.
The old MouseMotionDirection is now MouseMoveDirection.
The old MouseWheelDirection is now MouseScrollDirection.
The old UserInput::Chord is now InputChord.
The old UserInput::VirtualAxis is now:
- GamepadVirtualAxis for four gamepad buttons.
- KeyboardVirtualAxis for four keys.
- MouseMoveAxis::X.digital() and MouseMoveAxis::Y.digital() for discrete mouse movement.
- MouseScrollAxis::X.digital() and MouseScrollAxis::Y.digital() for discrete mouse wheel movement.
The old UserInput::VirtualDPad is now:
- GamepadVirtualDPad for four gamepad buttons.
- KeyboardVirtualDPad for four keys.
- MouseMove::default().digital() for discrete mouse movement.
- MouseScroll::default().digital() for discrete mouse wheel movement.

Next Steps

Refactor InputStreams into separate implementations.
Find alternative approaches for input mocking that provide more flexibility for future input type extensions, replacing RawInputs

Shute052 · 2024-02-23T18:26:57Z

If there are any parts of the doc comments where examples should be added, please let me know directly.
Because I am the author of the document, I'm not sure where examples should be added to ensure clarity for the readers.
However, adding examples to every part would be too much work, and my holiday is ending soon.
I can only do as much as I can before then

# Conflicts: # src/input_processing/dual_axis/custom.rs # src/input_processing/single_axis/custom.rs

# Conflicts: # examples/action_state_resource.rs # examples/axis_inputs.rs # examples/input_processing.rs # examples/mouse_wheel.rs # examples/virtual_dpad.rs # src/axislike.rs # src/buttonlike.rs # src/input_map.rs # src/input_mocking.rs # src/input_streams.rs # src/user_input.rs # tests/gamepad_axis.rs # tests/mouse_motion.rs # tests/mouse_wheel.rs

# Conflicts: # RELEASES.md # examples/input_processing.rs # src/axislike.rs # tests/gamepad_axis.rs

Shute052 · 2024-05-07T05:21:29Z

This PR is completed and awaits the merging of Add digital conversion processors #521.

# Conflicts: # RELEASES.md # src/input_mocking.rs

Shute052 · 2024-05-07T05:43:34Z

src/user_input/gamepad.rs

+///
+/// [`InputMap::set_gamepad`]: crate::input_map::InputMap::set_gamepad
+///
+/// ```rust,ignore


Doc tests for this module are ignored, since they required to add usable testing controllers for each example.
I feel that they should be focusing on describing input behavior

# Conflicts: # RELEASES.md

# Conflicts: # RELEASES.md # src/plugin.rs

# Conflicts: # RELEASES.md # examples/input_processing.rs # src/axislike.rs # src/clashing_inputs.rs # tests/gamepad_axis.rs

alice-i-cecile · 2024-06-09T22:18:40Z

@Shute052 I'd like to review and merge this this week: please resolve merge conflicts if you get a chance and let me know if there's anything else that remains to be done.

alice-i-cecile · 2024-06-09T22:21:16Z

src/clashing_inputs.rs

+    /// In most cases, the input simply holds itself.
+    Simple(Box<dyn UserInput>),
+
+    /// The input consists of multiple independent [`UserInput`]s.


I don't understand the distinction between Composite and Group.

This is about handling input conflicts. If I understand correctly, when Input A and Input B conflict, one of them is a proper subset of the other.

Group is used for Chords and other similar input combinations (like Sequences in future), and these are made up of one or more inputs.
Composite is for inputs that are composed of at least two inputs, like VirtualAxis and VirtualDPad, which are made up of two and four buttons, respectively.

But maybe it could be changed to return a Vec and just check the lengths and basic inputs of each.

Ah, got it.

alice-i-cecile · 2024-06-09T22:21:47Z

src/clashing_inputs.rs

+    pub fn len(&self) -> usize {
+        match self {
+            Self::Simple(_) => 1,
+            Self::Composite(_) => 1,


It's quite surprising that this is 1. If it's not a bug, it needs a doc comment.

This is the old UserInput::len

src/input_map.rs

alice-i-cecile · 2024-06-09T22:29:19Z

src/raw_inputs.rs

+/// Typically obtained by calling [`UserInput::raw_inputs`](crate::user_input::UserInput::raw_inputs).
+#[derive(Default, Debug, Clone, PartialEq)]
+#[must_use]
+pub struct RawInputs {


Hmm. I don't love this design. Doesn't this just get us back to the same place, where UserInput can't be reasonably extended?

This struct has just been moved out from the original user_input.rs file.

It should be able to replace with a new trait for implementing input mocking

alice-i-cecile · 2024-06-09T22:32:57Z

src/user_input/mod.rs

+    Send + Sync + Debug + DynClone + DynEq + DynHash + Reflect + erased_serde::Serialize
+{
+    /// Defines the kind of behavior that the input should be.
+    fn kind(&self) -> InputControlKind;


I think that the better design here is to have three separate traits ButtonlikeUserInput etc that all rely on a core UserInput trait that captures shared behavior. I think that's likely to be easier to do in a follow-up though.

I had previously considered splitting it into different input kinds: Button, Axis, DualAxis, etc.; and there are corresponding data types for each. Then, modify the ActionState API, something like this:

if let ButtonState(state) = action_state.get(ButtonAction) { let pressed = state.pressed; let pressure = state.pressure; // Gamepad buttons are ranging from 0 to 1, and others are 0 or 1 } if let AxisValue(value: f32) = action_state.get(AxisAction) { }

alice-i-cecile

Okay so I've finally had a chance to review this in earnest. Overall, I'm very impressed with the quality of both the engineering and documentation: thank you very very much.

The net effect of this refactor is great: much more flexible and principled, with some of the code that I hated most (InputStream) gone.

I have a small concern, which should be addressed now, and then two larger ones, which may make sense to wait on:

I would like to avoid breaking the end user API when possible during this: can we keep UserInput::chord and UserInput::modified?
I don't like the BasicInput struct. I understand why it's needed for clash handling, but I think it's the wrong design because it means that custom implemented user input type won't get clashes resolved properly (and we're back to hardcoding the set).
The UserInput trait is too monolithic, and requires the implementation of methods that don't make sense for all input kinds. I think this should be split into buttonlike/single-axis/dual-axis.

Shute052 · 2024-06-09T22:44:56Z

I would like to avoid breaking the end user API when possible during this: can we keep UserInput::chord and UserInput::modified?

If UserInput is a trait, I'm not sure how to add static methods to it when using its implementations as trait objects. However, I have added some builder methods.

Chord:
- pub fn new<U: UserInput>(inputs: impl IntoIterator<Item = U>) -> Self
- pub fn from_single(input: impl UserInput) -> Self
- pub fn with(mut self, input: impl UserInput) -> Self
- pub fn with_multiple<U: UserInput>(mut self, inputs: impl IntoIterator<Item = U>) -> Self
Modifier:
- pub fn with(&self, other: impl UserInput) -> InputChord

alice-i-cecile · 2024-06-09T23:09:25Z

Alright, I can get behind that. Let's get this merged and then we can start experimenting on further refinement.

alice-i-cecile · 2024-06-10T16:40:33Z

Adopted!

Reimplement keyboard inputs

6d1d202

Shute052 added code-quality Make the code faster or prettier breaking-change A breaking change that requires a new major version controversial Requires a heightened standard of review labels Feb 23, 2024

Shute052 added the enhancement New feature or request label Feb 23, 2024

Shute052 marked this pull request as draft February 23, 2024 18:29

Update document

9c3697a

Shute052 force-pushed the new-architecture branch from 8db90b9 to 9c3697a Compare February 23, 2024 18:37

Shute052 mentioned this pull request Feb 23, 2024

Refactoring underlying architecture to support various input types #483

Closed

6 tasks

alice-i-cecile mentioned this pull request Feb 25, 2024

Input Processors #491

Open

4 tasks

Shute052 mentioned this pull request Feb 29, 2024

Replace axis-like input configuration with new input processors #494

Merged

2 tasks

Shute052 added 5 commits April 28, 2024 00:37

Migrate to new input processors and refactor the trait

94f6f9b

Merge branch 'refs/heads/main' into new-architecture

ad9153a

# Conflicts: # src/input_processing/dual_axis/custom.rs # src/input_processing/single_axis/custom.rs

Implement mouse inputs

29195ea

Prefer FromIter<*AxisProcessor> over From<Vec<*AxisProcessor>>

f18ff69

Implement GamepadButtonType and GamepadAxisType

a1c166c

Shute052 mentioned this pull request Apr 29, 2024

Prefer FromIter<*AxisProcessor> over From<Vec<*AxisProcessor>> #512

Merged

Shute052 added 7 commits May 4, 2024 12:12

Add a pipeline method to create input processing pipelines

1f1a878

Typo

6872e2d

Replace old UserInput and InputKind with the new ones

72adc5b

RELEASES.md

5f0d528

Fix clashing inputs

4110309

Merge branch 'refs/heads/main' into new-architecture

7cc1e2c

# Conflicts: # RELEASES.md # examples/input_processing.rs # src/axislike.rs # tests/gamepad_axis.rs

Shute052 marked this pull request as ready for review May 4, 2024 14:04

Shute052 added 3 commits May 4, 2024 22:07

Typo

3fc96b4

Docs

e51967e

Rename

db95418

Shute052 marked this pull request as draft May 4, 2024 14:43

Shute052 marked this pull request as ready for review May 7, 2024 05:21

Shute052 added 2 commits May 7, 2024 13:23

RELEASES.md

9689734

Merge branch 'refs/heads/main' into new-architecture

c935ce5

# Conflicts: # RELEASES.md # src/input_mocking.rs

Shute052 commented May 7, 2024

View reviewed changes

Shute052 added 6 commits May 9, 2024 05:45

Merge branch 'refs/heads/main' into new-architecture

29adad8

# Conflicts: # RELEASES.md

Fix wrong gamepad button value

49d71ef

Rename InputChord::from_multiple to new

0d5e93e

Merge branch 'refs/heads/main' into new-architecture

ee51aa1

# Conflicts: # RELEASES.md # src/plugin.rs

Fix ci

07dca4e

Merge branch 'refs/heads/main' into new-architecture

23c8a28

# Conflicts: # RELEASES.md # examples/input_processing.rs # src/axislike.rs # src/clashing_inputs.rs # tests/gamepad_axis.rs

alice-i-cecile reviewed Jun 9, 2024

View reviewed changes

src/input_map.rs Show resolved Hide resolved

alice-i-cecile reviewed Jun 9, 2024

View reviewed changes

Shute052 closed this Jun 9, 2024

Shute052 reopened this Jun 9, 2024

alice-i-cecile approved these changes Jun 9, 2024

View reviewed changes

alice-i-cecile mentioned this pull request Jun 10, 2024

Swap to a trait-based architecture for UserInput #534

Merged

alice-i-cecile closed this Jun 10, 2024

This was referenced Jun 10, 2024

Split UserInput into multiple traits #535

Closed

Add a trait for input mocking #536

Closed

Swap to an extensible trait-powered design for resolving clashes #537

Closed

Shute052 deleted the new-architecture branch August 21, 2024 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `UserInput` and `InputKind` variants into separate implementations #490

Refactor `UserInput` and `InputKind` variants into separate implementations #490

Shute052 commented Feb 23, 2024 •

edited

Loading

Shute052 commented Feb 23, 2024 •

edited

Loading

Shute052 commented May 7, 2024 •

edited

Loading

Shute052 May 7, 2024 •

edited

Loading

alice-i-cecile commented Jun 9, 2024

alice-i-cecile Jun 9, 2024

Shute052 Jun 9, 2024 •

edited

Loading

alice-i-cecile Jun 9, 2024

alice-i-cecile Jun 9, 2024

Shute052 Jun 9, 2024

alice-i-cecile Jun 9, 2024

Shute052 Jun 9, 2024

Shute052 Jun 9, 2024

alice-i-cecile Jun 9, 2024

alice-i-cecile Jun 9, 2024

Shute052 Jun 9, 2024

alice-i-cecile left a comment •

edited

Loading

Shute052 commented Jun 9, 2024 •

edited

Loading

alice-i-cecile commented Jun 9, 2024

alice-i-cecile commented Jun 10, 2024

Refactor UserInput and InputKind variants into separate implementations #490

Refactor UserInput and InputKind variants into separate implementations #490

Conversation

Shute052 commented Feb 23, 2024 • edited Loading

Objective

Solution

TODO

Breacking Changes

Changelog

Migration Guide

Next Steps

Shute052 commented Feb 23, 2024 • edited Loading

Shute052 commented May 7, 2024 • edited Loading

Shute052 May 7, 2024 • edited Loading

Choose a reason for hiding this comment

alice-i-cecile commented Jun 9, 2024

Choose a reason for hiding this comment

Shute052 Jun 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile left a comment • edited Loading

Choose a reason for hiding this comment

Shute052 commented Jun 9, 2024 • edited Loading

alice-i-cecile commented Jun 9, 2024

alice-i-cecile commented Jun 10, 2024

Refactor `UserInput` and `InputKind` variants into separate implementations #490

Refactor `UserInput` and `InputKind` variants into separate implementations #490

Shute052 commented Feb 23, 2024 •

edited

Loading

Shute052 commented Feb 23, 2024 •

edited

Loading

Shute052 commented May 7, 2024 •

edited

Loading

Shute052 May 7, 2024 •

edited

Loading

Shute052 Jun 9, 2024 •

edited

Loading

alice-i-cecile left a comment •

edited

Loading

Shute052 commented Jun 9, 2024 •

edited

Loading