Skip to content
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

UserInput as trait #346

Closed

Conversation

paul-hansen
Copy link
Collaborator

@paul-hansen paul-hansen commented May 16, 2023

Description

Change our UserInput enum to be a trait. Adds the ability for other libraries to add new inputs that can be mapped to our actions. Also removes all UserInput instances being the size of the largest variant (which was inflated due to Chords using PetiteSets stored on the stack)

Closes: #321

Current Status

Good progress has been made but still some major things to finish implementing such as deserialization.

TODO

  • Implement the new InputLike and relevant ButtonLike/AxisLike traits for the inputs we previously supported
    For reference: old InputStreams & old InputKind
    • Gamepads
    • KeyCode
    • ScanCode
    • MouseButton
    • MouseWheel
    • MouseMotion
    • VirtualDPad & VirtualAxis
    • Chords
  • Remove Input Mocking (see comment)
  • Update branch to Bevy 0.11
  • Figure out how to handle deadzones (e.g. is it handled by the implementor of inputlike, purely in our lib, or some mix of the two?)
  • Reimplement egui and ui features ignored input
  • Address all added TODO comments
  • Fix tests
  • Review project/folder structure (e.g. I currently have a different module for each InputLike implementation, maybe we don't want that)
  • Documentation and re-enable #![forbid(missing_docs)]
  • Performance testing / Comparison / Benchmarks
  • Test all examples
  • Migration Guide

@alice-i-cecile alice-i-cecile added code-quality Make the code faster or prettier performance Code go brrr controversial Requires a heightened standard of review labels May 16, 2023
@@ -78,7 +78,7 @@ use std::marker::PhantomData;
pub struct InputMap<A: Actionlike> {
/// The raw vector of [PetitSet]s used to store the input mapping,
/// indexed by the `Actionlike::id` of `A`
map: Vec<PetitSet<UserInput, 16>>,
map: Vec<PetitSet<Box<dyn InputLikeObject>, 16>>,
Copy link
Contributor

@alice-i-cecile alice-i-cecile May 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just swap to a HashMap here now that we're no longer trying to stack-allocate. Will simplify a lot of code.

I'm pretty sure petitset can be killed as a dependency completely.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we still want "slots" so the inputs are ordered and we can have a "secondary" input in index 1 without the "primary" index 0 being populated. Probably accomplish this with a HashMap<usize, Box<dyn InputLikeObject>?

Copy link
Collaborator Author

@paul-hansen paul-hansen May 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you think of having a type that wraps the HashMap so we can make it more clear what the purpose of the usize is? Named something like Bindings? That way when we call things like InputMap::get we return our Bindings struct instead of a hashmap with a usize as key that is somewhat unclear what the purpose is.

@paul-hansen
Copy link
Collaborator Author

Rebased on main to fix conflict.

The minimal and physical_key_bindings examples are working again!

I think something that needs some thought is ButtonLike and AxisLike, currently they are just marker traits that could easily be replaced with a bool. I feel like they could be doing more. Maybe split up the InputStreams Trait into those traits or something, not sure if that's possible yet. I'll probably implement Gamepad next since that has axes, and might give some insight.

@alice-i-cecile
Copy link
Contributor

I would expect we want a method to create a Buttonlike input from an Axislike input :) We may also want to split it into Buttonlike / SingleAxislike / DualAxislike instead: I think that the methods I really want from something that behaves like an axis tend to depend on the number of dimensions.

github-merge-queue bot pushed a commit to bevyengine/bevy that referenced this pull request Jun 19, 2023
# Objective

Add a get_unclamped method to
[Axis](https://docs.rs/bevy/0.10.1/bevy/input/struct.Axis.html) to allow
it to be used in cases where being able to get a precise relative
movement is important. For example, camera zoom with the mouse wheel.

This would make it possible for libraries like leafwing input manager to
leverage `Axis` for mouse motion and mouse wheel axis mapping. I tried
to use it my PR here
Leafwing-Studios/leafwing-input-manager#346 but
will likely have to revert that and read the mouse wheel events for now
which is what prompted this PR.

## Solution

Instead of clamping the axis value when it is set, it now stores the raw
value and clamps it in the `get` method. This allows a simple
get_unclamped method that just returns the raw value.


## Changelog

- Added a get_unclamped method to Axis that can return values outside of
-1.0 to 1.0
github-merge-queue bot pushed a commit to bevyengine/bevy that referenced this pull request Jun 19, 2023
# Objective

Add a get_unclamped method to
[Axis](https://docs.rs/bevy/0.10.1/bevy/input/struct.Axis.html) to allow
it to be used in cases where being able to get a precise relative
movement is important. For example, camera zoom with the mouse wheel.

This would make it possible for libraries like leafwing input manager to
leverage `Axis` for mouse motion and mouse wheel axis mapping. I tried
to use it my PR here
Leafwing-Studios/leafwing-input-manager#346 but
will likely have to revert that and read the mouse wheel events for now
which is what prompted this PR.

## Solution

Instead of clamping the axis value when it is set, it now stores the raw
value and clamps it in the `get` method. This allows a simple
get_unclamped method that just returns the raw value.


## Changelog

- Added a get_unclamped method to Axis that can return values outside of
-1.0 to 1.0
@paul-hansen
Copy link
Collaborator Author

paul-hansen commented Jun 24, 2023

@alice-i-cecile could use some input on the direction you want to go with deadzones. It used to be that SingleAxis would store the positive_low and negative_low values for deadzone. Now that SingleAxisLike is a trait, we can implement it for GamepadAxis and directly insert that into the mapping instead of converting it into a SingleAxis first which seems like a win. But now it's unclear where to handle the deadzone.

Here's the solutions I've thought of so far:

  1. Let GamepadAxis be inserted directly and don't do anything with deadzones in that case, but keep the SingleAxis around as a wrapper struct that also implements SingleAxisLike can optionally be used to apply deadzones.
  2. Try to prevent GamepadAxis being inserted directly into the input map and force the user to wrap it in SingleAxis first so they have to set the deadzones, e.g. making SingleAxisLike not implement InputLikeObject
  3. Add methods to the SingleAxisLike trait for setting the deadzone values and let the implementation handle what it does with those values. For example, for GamepadAxis it would update Bevy's AxisSettings

Feel free to take some time to respond, I have other things I can keep working on with this.

Currently non-functional and very messy/incomplete.
Looking for input on how to solve the need to be able to Serialize
InputLike but unable to do so while using dynamic dispatch.
Might be ways to make this cleaner, for now just trying to work towards
compiling to make sure the InputLike trait idea works out.
I don't think this makes sense with the new InputLike trait as we don't
know all the possible input types. InputLike::raw_inputs instead returns
the individual inputs for InputLike collections.
It got re-added again as an empty file during a rebase on main.
Previously had implemented InputLikeObject for Box<InputLikeObject>
to allow accepting boxed or unboxed InputLikeObjects, but it also
created the potential to accidentally double box.
By implementing From<InputLikeObject> for Box<InputLikeObject>
and having methods take impl Into<Box<InputLikeObject> we can accept
both without double boxing potential.
@alice-i-cecile
Copy link
Contributor

Preference is for 3 if we can swing it: it definitely feels like managing deadzones is within the domain of the trait.

src/clashing_inputs.rs Outdated Show resolved Hide resolved
# Conflicts:
#	src/action_state.rs
#	src/systems.rs
# Conflicts:
#	benches/input_map.rs
#	examples/multiplayer.rs
#	src/action_state.rs
#	src/axislike.rs
#	src/clashing_inputs.rs
#	src/input_map.rs
#	src/input_mocking.rs
#	src/input_streams.rs
#	src/user_input.rs
#	tests/clashes.rs
#	tests/gamepad_axis.rs
#	tests/mouse_motion.rs
#	tests/mouse_wheel.rs
@paul-hansen
Copy link
Collaborator Author

paul-hansen commented Aug 4, 2023

I might not get back to this seriously before mid September, working more hours to get a demo done for our game for a convention. If anyone wants to pick it up before then I'd be happy to answer any questions.

Current status is that it passes all the tests except for the deadzone ones because that isn't re-implemented yet. Checklist on original post has been updated with some other things that need finished like docs, migration guide etc.
Deadzones might require some bigger changes, especially if we want to keep the functionality from #370 since I think we would have to store something alongside inputs for the deadzone settings instead of only storing the trait object we get from integrations.

@paul-hansen
Copy link
Collaborator Author

I think I'll have to bow out of heading this up. I've been trying to merge or rebase from main but there are enough conflicting changes it might be cleaner to start over and just look at the InputLike and InputLikeObject traits from this PR as a starting point. As it stands, I don't think I can spare the time to commit to that sadly. In hindsight, it might be worth considering a feature freeze or forking the project as a new crate or something while taking on this much of a refactor to avoid having to re-implement changes that gets added while working on it.

Hopefully this was still valuable as an experiment to show a way it could be done using traits.

Closing this PR for now, but if anyone wants to try and resurrect the branch as their own or anything you're more than welcome to use it. I'm also open to doing work related to it if there are ideas on how to make it simpler or break it into smaller changes that could be merged incrementally.

@alice-i-cecile
Copy link
Contributor

Sounds good <3 thanks for doing this investigation; I'll take a crack at this next time I'm in the mood for something crunchy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-quality Make the code faster or prettier controversial Requires a heightened standard of review performance Code go brrr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Represent user inputs via traits, rather than a large enum
2 participants