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

Release the pressed ActionData when it isn't being updated #473

Merged
merged 3 commits into from
Feb 19, 2024

Conversation

Shute052
Copy link
Collaborator

@Shute052 Shute052 commented Feb 19, 2024

Objective

Fixes #471.

Solution

The actual issue lies in the incorrect updating of ActionState because it is now updated through a HashMap instead of a Vec, but the clashed ActionData has been removed (since #450).

Relevant Code

// Handle clashes before #450
pub fn handle_clashes(
    &self,
    action_data: &mut [ActionData],
    input_streams: &InputStreams,
    clash_strategy: ClashStrategy,
) {
    for clash in self.get_clashes(action_data, input_streams) {
        // Remove the action in the pair that was overruled, if any
        if let Some(culled_action) = resolve_clash(&clash, clash_strategy, input_streams) {
            action_data[culled_action.index()] = ActionData::default();
        }
    }
}
// Update ActionData before #450
pub fn update(&mut self, action_data: Vec<ActionData>) {
    assert_eq!(action_data.len(), A::n_variants());

    for (i, action) in A::variants().enumerate() {
        match action_data[i].state {
            ButtonState::JustPressed => self.press(&action),
            ButtonState::Pressed => self.press(&action),
            ButtonState::JustReleased => self.release(&action),
            ButtonState::Released => self.release(&action),
        }

        self.action_data[i].axis_pair = action_data[i].axis_pair;
        self.action_data[i].value = action_data[i].value;
    }
}

// Handle clashes after #450
pub fn handle_clashes(
    &self,
    action_data: &mut HashMap<A, ActionData>,
    input_streams: &InputStreams,
    clash_strategy: ClashStrategy,
) {
    for clash in self.get_clashes(action_data, input_streams) {
        // Remove the action in the pair that was overruled, if any
        if let Some(culled_action) = resolve_clash(&clash, clash_strategy, input_streams) {
            action_data.remove(&culled_action);
        }
    }
}
// Update ActionData after #450
pub fn update(&mut self, action_data: HashMap<A, ActionData>) {
    for (action, action_datum) in action_data {
        match self.action_data.entry(action) {
            Entry::Occupied(occupied_entry) => {
                let entry = occupied_entry.into_mut();

                match action_datum.state {
                    ButtonState::JustPressed => entry.state.press(),
                    ButtonState::Pressed => entry.state.press(),
                    ButtonState::JustReleased => entry.state.release(),
                    ButtonState::Released => entry.state.release(),
                }

                entry.axis_pair = action_datum.axis_pair;
                entry.value = action_datum.value;
            }
            Entry::Vacant(empty_entry) => {
                empty_entry.insert(action_datum.clone());
            }
        }
    }
}

@Shute052 Shute052 changed the title Reset the shorter clashed ActionData instead of removing it Release the shorter pressed clashed ActionData instead of removing it Feb 19, 2024
@Shute052 Shute052 changed the title Release the shorter pressed clashed ActionData instead of removing it Release the pressed ActionData when it isn't being updated Feb 19, 2024
@alice-i-cecile alice-i-cecile added the bug Something isn't working label Feb 19, 2024
Copy link
Contributor

@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.

Can you please add a regression test? Change itself looks great though.

@Shute052
Copy link
Collaborator Author

Shute052 commented Feb 19, 2024

Can you please add a regression test? Change itself looks great though.你能添加一个回归测试吗?变化本身看起来很棒。

I'm on board, and updated the description of the real issues after investigation.

@porkbrain
Copy link

Thank you for tackling this 🫀

@Shute052
Copy link
Collaborator Author

If I've got it right, this test should work.

@alice-i-cecile alice-i-cecile merged commit 19af9d4 into Leafwing-Studios:main Feb 19, 2024
4 checks passed
@Shute052 Shute052 deleted the fix-handle-clashes branch February 19, 2024 21:29
Shute052 added a commit to Shute052/leafwing-input-manager that referenced this pull request Feb 19, 2024
alice-i-cecile pushed a commit that referenced this pull request Feb 20, 2024
* Update for Bevy 0.13

* RELEASES.md

* Fix new test added by #473

* Update for bevy_egui 0.25

* fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClashStrategy::PrioritizeLongest changed its behavior and no longer clears actions
3 participants