-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Add get_unclamped to Axis #8871
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
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 with exposing this, and I like this approach.
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.
This change makes sense to me and it already appears to have a use case with LWIM, which is a good sign.
The behavior is also well-documented. I noticed that the Axis<T>
doc comment is slightly out of date now, but I'm not sure if the difference matters.
Maybe one thing to consider is that the get
function is more costly now due to the additional computation, so if you call this a lot it might have a performance impact. I doubt that it will be significant in any meaningful way though.
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 can't comment on the line directly, but in line 7 the comment is maybe not entirely accurate anymore:
/// The values are stored as `f32`s, which range from [`Axis::MIN`] to [`Axis::MAX`], > inclusive.
Now the range is not enforced on the stored f32
s anymore. Mentioning the range in the doc comment still makes sense though, so I'm not sure if it's worth changing / what to change it to
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.
Good catch! I updated the documentation there to better reflect where the clamping happens. Let me know if you think of any improvements!
Uses MouseWheel events instead of Bevy's Axis type which was clamped, causing the test to return 1.0 instead of 5.0. If bevyengine/bevy#8871 gets merged we might be able to go back to using Axis in the future.
We could store both the clamped and unclamped values. I could potentially see an argument for less calculation in hot loops being worth a bit of extra memory use. Personally though, I agree that it isn't enough of a difference to worry about and there might be cases where only doing the clamp when it's used is an improvement anyway. Happy to do more research/benchmarks etc. to get more information if desired! |
Uses MouseWheel events instead of Bevy's Axis type which was clamped, causing the test to return 1.0 instead of 5.0. If bevyengine/bevy#8871 gets merged we might be able to go back to using Axis in the future.
Objective
Add a get_unclamped method to Axis 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