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

Rescale Axis and Button value to avoid compression #3464

Closed
wants to merge 10 commits into from
Closed

Rescale Axis and Button value to avoid compression #3464

wants to merge 10 commits into from

Conversation

thibaudio
Copy link

Objective

Fixes #3450

Solution

Apply the suggested solution to both AxisSettings positive/negative values and ButtonAxisSettings

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Dec 28, 2021
@alice-i-cecile alice-i-cecile added A-Input Player input via keyboard, mouse, gamepad, and more 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 29, 2021
@thibaudio
Copy link
Author

I'll update the test later today

Copy link
Contributor

@sixfold-origami sixfold-origami left a comment

Choose a reason for hiding this comment

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

Looks pretty nice! I have one nitpick, and a weird edge case to consider

crates/bevy_input/src/gamepad.rs Show resolved Hide resolved
crates/bevy_input/src/gamepad.rs Show resolved Hide resolved
Copy link
Contributor

@sixfold-origami sixfold-origami left a comment

Choose a reason for hiding this comment

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

This looks really good! Just one question for you

crates/bevy_input/src/gamepad.rs Show resolved Hide resolved
crates/bevy_input/src/gamepad.rs Show resolved Hide resolved
@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 C-Bug An unexpected or incorrect behavior and removed M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels May 16, 2022
};

if let Some(old_value) = old_value {
if (new_value - old_value).abs() <= self.threshold {
if (new_value >= 0.0
Copy link
Member

Choose a reason for hiding this comment

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

This needs more comments, or a simpler approach.

Copy link
Author

Choose a reason for hiding this comment

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

Tried to split this into several smaller functions. Is that better?

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.

Two changes:

  1. Add inline annotations to the added methods.
  2. The changes on line 203 are far too complex; this needs more comments or preferably structure.

@thibaudio, you can mark comments as resolved by pressing the "Resolve comment" button once they're addressed, which makes it easier for reviewers.

@thibaudio
Copy link
Author

Two changes:

1. Add `inline` annotations to the added methods.

2. The changes on line 203 are far too complex; this needs more comments or preferably structure.

@thibaudio, you can mark comments as resolved by pressing the "Resolve comment" button once they're addressed, which makes it easier for reviewers.

Thanks for the review @alice-i-cecile
I tried to refactor the changes around threshold testing for better readability, looking forward to your comments!

@alice-i-cecile
Copy link
Member

This refactor is definitely an improvement! Spending some time thinking about this, I think the most natural way to model and communicate this in Rust would actually be with:

enum AxisPosition {
   BelowLowZone,
   LowZone,
   DeadZone,
   HighZone,
   AboveHighZone,
}

I think that if we start there, we can make this code much easier to read and refactor in the future. Does that make sense to you?

@thibaudio
Copy link
Author

Thanks for your feedback @alice-i-cecile!
Before going any further, I just wanted to check if you had something like that in mind:

enum ScaledAxisPosition {
    BelowLowZone,
    LowZone(f32),
    DeadZone,
    HighZone(f32),
    AboveHighZone,
}

#[derive(Debug, Clone)]
struct AxisPosition {
    scaled: Option<ScaledAxisPosition>,
    raw: f32
}

impl AxisSettings {
    fn filter(&self, new_value: f32, old_value: Option<AxisPosition>) -> Option<AxisPosition> {
        let axis_position = AxisPosition {
            scaled: Some(self.get_axis_position_from_raw_value(new_value)),
            raw: new_value
        };
        if let Some(old_value) = old_value {
            if self.delta_bellow_threshold(axis_position, old_value) {
                return None;
            }
        }

        Some(axis_position)
    }

    fn get_axis_position_from_raw_value(&self, value: f32) -> ScaledAxisPosition {
        if value <= self.positive_low && value >= self.negative_low {
            ScaledAxisPosition::DeadZone
        } else if value >= self.positive_high {
            ScaledAxisPosition::AboveHighZone
        } else if value <= self.negative_high {
            ScaledAxisPosition::BelowLowZone
        } else if value > self.positive_low {
            ScaledAxisPosition::HighZone(scale(value, self.positive_low, self.positive_high))
        } else {
            ScaledAxisPosition::LowZone(-1.0 * scale(value, self.negative_low, self.negative_high))
        }
    }

@alice-i-cecile
Copy link
Member

Yes, that's lovely!

@thibaudio
Copy link
Author

After toying with that, changing the return type of the filter method is too big of an impact for me to handle.

@alice-i-cecile alice-i-cecile self-assigned this May 18, 2022
@alice-i-cecile
Copy link
Member

Thanks for letting us know. I'll see about making a PR to your branch with the changes :)

@BenjaminBrienen BenjaminBrienen added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Dec 29, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Input Player input via keyboard, mouse, gamepad, and more C-Bug An unexpected or incorrect behavior C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deadzone clipping in AxisSettings and ButtonSettings results in a compression of values
4 participants