-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Scale input to account for deadzones #17015
base: main
Are you sure you want to change the base?
Conversation
I can't work on fixing the unit tests until this is fixed:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this is the right default behavior. Otherwise setting a deadzone makes it very hard / impossible to get small directional inputs.
This PR needs a bit of cleanup still. It would be particularly helpful to cleanup the tests: right now, the values just look like magic numbers. Maybe we should add some algebra + consts inside of the Some arms?
For the alloc error, I think you need to fix the feature flags in a new PR. @bushrat011899, FYI. |
He's already got that fixed in the no_std bevy_input PR. |
Can confirm I have this issue resolved in the |
3eac744
to
91be8f2
Compare
It looks like your PR is a breaking change, but you didn't provide a migration guide. Could you add some context on what users should update when this change get released in a new version of Bevy? |
Let's do a quick migration guide noting this change (since it's otherwise surprising), but this LGTM now. Good work on the tests; I like the balance you struck. |
2a41eee
to
207b9ae
Compare
Sorry for the noise. I realized that I could make the implementation and test data cleaner by storing the raw values in the gamepad, which means the old values will be raw values. Everything is very optimal now, imo. |
…to scaled-deadzones
Migration guide made me realize the doc comment should be improved. Both are done now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the latest changes, and particularly appreciate the comments on the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Left a few small suggestions.
Personally, I think deadzone is a more high-level functionality and should be handled by input managers🤔 But since we already have this feature, it's definitely an improvement!
I handle deadzones in the same way in the bevy_enhanced_input
library.
fn should_register_change(&self, new_value: f32, old_value: Option<f32>) -> bool { | ||
if old_value.is_none() { | ||
return true; | ||
#[inline(always)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never inline private methods. Same for other places. Here is a good article about it.
#[inline(always)] |
} | ||
} | ||
|
||
/// A linear remapping of `value` from `old` to `new` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use .
in the end for all doc comments in Rust:
/// A linear remapping of `value` from `old` to `new` | |
/// A linear remapping of `value` from `old` to `new`. |
(-0.43, Some(-0.84), Some(-0.43)), | ||
(-0.05, Some(-0.055), Some(0.0)), | ||
(-0.95, Some(-0.945), Some(-1.0)), | ||
// enough increase to change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to split this single test into multiple small tests named like enough_increase_to_change
instead of comments. But it's up to you since it was like this before :)
Agreed. In the medium term, I think this functionality should be removed from bevy_input and moved up a level. But until we have a first party input manager / action crate we're blocked there. |
I think deadzones and scaling are good here because they address hardware limitations. Before then, the events are "raw". The part I would move into another crate is actually the clamping/rounding. Those are more about what "feels right". But then again, the same applies for the press/release/change thresholds, and I don't think those can be moved out. |
I would consider We on the same page with @alice-i-cecile, this PR is good. |
Objective
Fixes #3450
Solution
Scale the input to account for the range
Testing
Updated unit tests
Migration Guide
GamepadButtonChangedEvent.value
is now linearly rescaled to be from0.0..=1.0
(instead oflow..=high
) andGamepadAxisChangedEvent.value
is now linearly rescaled to be from-1.0..=0.0
/0.0..=1.0
(accounting for the deadzone).