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

[porkbrain]

Version

0.12.0

What you did

I query the ActionState resource with: controls: Res<ActionState<GlobalAction>>.

Simplified, my GlobalAction looks like this:

#[derive(
    Actionlike, PartialEq, Eq, Hash, Clone, Copy, Debug, Reflect, EnumIter,
)]
pub enum GlobalAction {
    /// Going only up.
    MoveUp,
    /// Going only left.
    MoveLeft,
    /// Going both up and left.
    /// Overwrites `MoveUp` and `MoveLeft`.
    MoveUpLeft,
}

In prior version, controls.get_pressed().last() would return MoveUpLeft if I help both Up and Left. After upgrading, the controls.get_pressed() vector contains two actions: the MoveUpLeft and either MoveLeft or MoveUp. Sometimes MoveUp/MoveLeft is on index 0, sometimes on index 1.

I'd expect that when I press the combination of MoveUpLeft, MoveUp nor MoveLeft would be emitted.

Some additional info about how I map the inputs:

impl GlobalAction {
    fn input_map() -> InputMap<Self> {
        let mut input_map = InputMap::default();

        for action in GlobalAction::iter() {
            for input in GlobalAction::default_keyboard_input(action) {
                input_map.insert(action, input);
            }
        }

        input_map
    }

    fn default_keyboard_input(action: GlobalAction) -> Vec<UserInput> {
        use InputKind::Keyboard as Kbd;
        use KeyCode as Key;
        use UserInput::{Chord, Single};

        match action {
            Self::MoveLeft => {
                vec![Single(Kbd(Key::A)), Single(Kbd(Key::Left))]
            }
            Self::MoveUp => {
                vec![Single(Kbd(Key::W)), Single(Kbd(Key::Up))]
            }
            Self::MoveUpLeft => vec![
                Chord(vec![Kbd(Key::W), Kbd(Key::A)]),
                Chord(vec![Kbd(Key::Up), Kbd(Key::Left)]),
            ],
        }
    }
}

[alice-i-cecile]

Thanks for the report! There was a number of complex refactors and our test coverage must have been lacking.

[Shute052]

#466 should not changing those behaviors, could this issue was originated from older commits?

[Shute052]

In prior version, controls.get_pressed().last() would return MoveUpLeft if I help both Up and Left. After upgrading, the controls.get_pressed() vector contains two actions: the MoveUpLeft and either MoveLeft or MoveUp. Sometimes MoveUp/MoveLeft is on index 0, sometimes on index 1.

This issue seems to have been caused by #450, but it was actually committed by #452, which replaced the use of Vec with HashMap to store ActionData. However, HashMap does not guarantee the order when collecting its values.

Relevant Code

// Before
pub fn get_pressed(&self) -> Vec<A> {
    A::variants().filter(|a| self.pressed(a)).collect()
}

// After
pub fn get_pressed(&self) -> Vec<A> {
    self.action_data
        .iter()
        .filter(|(_action, data)| data.state.pressed())
        .map(|(action, _data)| action.clone())
        .collect()
}

[Shute052]

I've identified the actual problem; it appears that some clashes from get_clashes() were missed.

Should be an easy fix.

[Shute052]

This seems like a detour; the issue doesn't appear to be in the handle_clashes function.

[Shute052]

I've pinpointed the actual issue: it seems that the ActionState isn't being updated because the shorter clashed ActionData has been removed, but it should instead be replaced with ActionData::default() (since #450).

Relevant Code

// Before
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();
        }
    }
}

// After
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);
        }
    }
}