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.

Sign up for GitHub

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

Jump to bottom

Add getters and setters for InputAxis and ButtonSettings #3446

Closed
wants to merge 11 commits into from
Closed

Add getters and setters for InputAxis and ButtonSettings #3446

wants to merge 11 commits into from

Conversation

mfdorst
Copy link
Contributor

@mfdorst mfdorst commented Dec 27, 2021

Fixes #3418

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Dec 27, 2021
@alice-i-cecile alice-i-cecile added A-Input Player input via keyboard, mouse, gamepad, and more C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Dec 27, 2021
@alice-i-cecile alice-i-cecile mentioned this pull request Dec 27, 2021
Closed
@alice-i-cecile
Copy link
Member

Thanks for contributing, and the great questions!

I've replied to your questions over in #3446, could you apply them to this PR if you agree with them? :)

@mfdorst
Copy link
Contributor Author

mfdorst commented Dec 28, 2021

@alice-i-cecile I do agree with them, thanks. I'll make sure to run clippy this time too lol. Not sure why CI / build (stable, ubuntu-latest) (pull_request) failed though. From the error it looks like a connectivity problem maybe?

@mfdorst
Make clippy happy
5cb6787
@mfdorst
AxisSettings setters return the new value rather than panicking
34da2f6
@mfdorst
Rename fields in AxisSettings
038a094 
positive_high => livezone_upperbound
positive_low => deadzone_upperbound
negative_low => deadzone_lowerbound
negative_high => livezone_lowerbound
@mfdorst
Copy link
Contributor Author

mfdorst commented Dec 28, 2021

Fixed up AxisSettings. Will get to ButtonSettings soon.

@alice-i-cecile
Copy link
Member

Could you add ButtonSettings to this PR's title too so then it's easier to find when skimming?

alice-i-cecile
alice-i-cecile approved these changes Dec 28, 2021
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Awesome, this looks fantastic. What a nice improvement!

@mfdorst mfdorst changed the title Add getters and setters for InputAxis Add getters and setters for InputAxis and ButtonSettings Dec 28, 2021
@alice-i-cecile alice-i-cecile added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide and removed S-Needs-Migration-Guide labels Jan 20, 2022
alice-i-cecile
alice-i-cecile reviewed Feb 11, 2022
View reviewed changes
crates/bevy_input/src/gamepad.rs
1.0
} else if new_value <= self.negative_high {
-1.0
/// Creates a new AxisSettings instance
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Creates a new AxisSettings instance
/// Creates a new [`AxisSettings`] instance

alice-i-cecile
alice-i-cecile approved these changes Feb 11, 2022
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Awesome. Truly an industrial strength API now 😉 The clarity is very much appreciated here.

IceSentry
IceSentry reviewed Feb 12, 2022
View reviewed changes
crates/bevy_input/src/gamepad.rs
@@ -2,6 +2,18 @@ use crate::{Axis, Input};
use bevy_app::{EventReader, EventWriter};
use bevy_ecs::system::{Res, ResMut};
use bevy_utils::{tracing::info, HashMap, HashSet};
use thiserror::Error;

/// Errors that occur when setting settings for gamepad input
Copy link
Contributor

@IceSentry IceSentry Feb 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

maybe "configuring settings" or "setting up gamepad input". "setting settings" feels weird

Weibye
Weibye reviewed Jun 14, 2022
View reviewed changes
Copy link
Contributor

@Weibye Weibye left a comment

Choose a reason for hiding this comment

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

Some minor nits to doc-comments, otherwise this looks really solid :)

crates/bevy_input/src/gamepad.rs
@@ -2,6 +2,18 @@ use crate::{Axis, Input};
use bevy_app::{EventReader, EventWriter};
use bevy_ecs::system::{Res, ResMut};
use bevy_utils::{tracing::info, HashMap, HashSet};
use thiserror::Error;

/// Errors that occur when setting settings for gamepad input
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Errors that occur when setting settings for gamepad input
/// Errors that occur when attempting to incorrectly configure gamepad input

@alice-i-cecile
Copy link
Member

@mfdorst do you have time to get back to this? This is almost ready to merge :)

@alice-i-cecile alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Sep 21, 2022
@targrub targrub mentioned this pull request Sep 21, 2022
Closed
@alice-i-cecile
Copy link
Member

Closing in favor of the rebased #6050.

@targrub targrub mentioned this pull request Sep 24, 2022
Closed
bors bot pushed a commit that referenced this pull request Oct 17, 2022
@targrub
Add getters and setters for InputAxis and ButtonSettings (#6088)
c11fbfb 
# Objective
Fixes #3418

## Solution

Originally a rebase of #3446.  Work was originally done by mfdorst, who should receive considerable credit.  Then the error types were extensively reworked by targrub.

## Migration Guide

`AxisSettings` now has a `new()`, which may return an `AxisSettingsError`.
`AxisSettings` fields made private; now must be accessed through getters and setters.  There's a dead zone, from `.deadzone_upperbound()` to `.deadzone_lowerbound()`, and a live zone, from `.deadzone_upperbound()` to `.livezone_upperbound()` and from `.deadzone_lowerbound()` to `.livezone_lowerbound()`.
`AxisSettings` setters no longer panic.
`ButtonSettings` fields made private; now must be accessed through getters and setters.
`ButtonSettings` now has a `new()`, which may return a `ButtonSettingsError`.

Co-authored-by: targrub <62773321+targrub@users.noreply.github.com>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 19, 2022
@targrub @james7132
Add getters and setters for InputAxis and ButtonSettings (bevyeng…
c7c9afb 
…ine#6088)

# Objective
Fixes bevyengine#3418

## Solution

Originally a rebase of bevyengine#3446.  Work was originally done by mfdorst, who should receive considerable credit.  Then the error types were extensively reworked by targrub.

## Migration Guide

`AxisSettings` now has a `new()`, which may return an `AxisSettingsError`.
`AxisSettings` fields made private; now must be accessed through getters and setters.  There's a dead zone, from `.deadzone_upperbound()` to `.deadzone_lowerbound()`, and a live zone, from `.deadzone_upperbound()` to `.livezone_upperbound()` and from `.deadzone_lowerbound()` to `.livezone_lowerbound()`.
`AxisSettings` setters no longer panic.
`ButtonSettings` fields made private; now must be accessed through getters and setters.
`ButtonSettings` now has a `new()`, which may return a `ButtonSettingsError`.

Co-authored-by: targrub <62773321+targrub@users.noreply.github.com>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@targrub @james7132
Add getters and setters for InputAxis and ButtonSettings (bevyeng…
76f9dd2 
…ine#6088)

# Objective
Fixes bevyengine#3418

## Solution

Originally a rebase of bevyengine#3446.  Work was originally done by mfdorst, who should receive considerable credit.  Then the error types were extensively reworked by targrub.

## Migration Guide

`AxisSettings` now has a `new()`, which may return an `AxisSettingsError`.
`AxisSettings` fields made private; now must be accessed through getters and setters.  There's a dead zone, from `.deadzone_upperbound()` to `.deadzone_lowerbound()`, and a live zone, from `.deadzone_upperbound()` to `.livezone_upperbound()` and from `.deadzone_lowerbound()` to `.livezone_lowerbound()`.
`AxisSettings` setters no longer panic.
`ButtonSettings` fields made private; now must be accessed through getters and setters.
`ButtonSettings` now has a `new()`, which may return a `ButtonSettingsError`.

Co-authored-by: targrub <62773321+targrub@users.noreply.github.com>
Pietrek14 pushed a commit to Pietrek14/bevy that referenced this pull request Dec 17, 2022
@targrub @Pietrek14
Add getters and setters for InputAxis and ButtonSettings (bevyeng…
1d0ec4d 
…ine#6088)

# Objective
Fixes bevyengine#3418

## Solution

Originally a rebase of bevyengine#3446.  Work was originally done by mfdorst, who should receive considerable credit.  Then the error types were extensively reworked by targrub.

## Migration Guide

`AxisSettings` now has a `new()`, which may return an `AxisSettingsError`.
`AxisSettings` fields made private; now must be accessed through getters and setters.  There's a dead zone, from `.deadzone_upperbound()` to `.deadzone_lowerbound()`, and a live zone, from `.deadzone_upperbound()` to `.livezone_upperbound()` and from `.deadzone_lowerbound()` to `.livezone_lowerbound()`.
`AxisSettings` setters no longer panic.
`ButtonSettings` fields made private; now must be accessed through getters and setters.
`ButtonSettings` now has a `new()`, which may return a `ButtonSettingsError`.

Co-authored-by: targrub <62773321+targrub@users.noreply.github.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@targrub @ItsDoot
Add getters and setters for InputAxis and ButtonSettings (bevyeng…
5696746 
…ine#6088)

# Objective
Fixes bevyengine#3418

## Solution

Originally a rebase of bevyengine#3446.  Work was originally done by mfdorst, who should receive considerable credit.  Then the error types were extensively reworked by targrub.

## Migration Guide

`AxisSettings` now has a `new()`, which may return an `AxisSettingsError`.
`AxisSettings` fields made private; now must be accessed through getters and setters.  There's a dead zone, from `.deadzone_upperbound()` to `.deadzone_lowerbound()`, and a live zone, from `.deadzone_upperbound()` to `.livezone_upperbound()` and from `.deadzone_lowerbound()` to `.livezone_lowerbound()`.
`AxisSettings` setters no longer panic.
`ButtonSettings` fields made private; now must be accessed through getters and setters.
`ButtonSettings` now has a `new()`, which may return a `ButtonSettingsError`.

Co-authored-by: targrub <62773321+targrub@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Weibye Weibye Weibye left review comments

@IceSentry IceSentry IceSentry left review comments

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees
No one assigned
Labels
A-Input Player input via keyboard, mouse, gamepad, and more C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Validate ButtonSettings and AxisSettings using getter / setter pattern
4 participants
@mfdorst @alice-i-cecile @IceSentry @Weibye