rust-sokoban icon indicating copy to clipboard operation
rust-sokoban copied to clipboard

InputSystem doesn't use input in correct order

Open denniskaselow opened this issue 5 years ago • 8 comments

InputSystem uses input_queue.keys_pressed.pop() and is commented as // Get the first key pressed but pop() removes the last item.

If you input keys fast enough (very easy when not using release mode and batch rendering) this can be seen because of erratic movement of the player character.

denniskaselow avatar Aug 24 '20 07:08 denniskaselow

good catch, we probably want a queue not a stack!

iolivia avatar Aug 24 '20 07:08 iolivia

Something simple as this could also be sufficient:

impl InputQueue {
    pub fn get_first(&mut self) -> Option<KeyCode> {
        if self.keys_pressed.is_empty() {
            None
        } else {
            Some(self.keys_pressed.remove(0))
        }
    }
}

But I'm new to Rust, so I have no idea if this is the Rust way to do things, but it's what I'd do, considering the key_pressed Vec will never have more than a few keys even in worst case (ignoring automated input).

denniskaselow avatar Aug 24 '20 08:08 denniskaselow

I used VecDaque in my implementation:

#[derive(Default)]
pub struct KeyDownQueue {
    pub keys_pressed: VecDeque<KeyCode>,
}

kamil-duda avatar Dec 30 '20 18:12 kamil-duda

that looks great @fineconstant! feel free to submit a PR, will be very helpful for others using the repo 😄

iolivia avatar Dec 31 '20 16:12 iolivia

I looked into it and I think that Vec<KeyCode> is sufficient, so there is no need to make any changes.

With Vec<KeyCode> calling input_queue.keys_pressed.pop() function removes the last element indeed, but input_queue.keys_pressed.push(keycode); appends an element at the back of the collection - it should not cause any problems.

std::vec::Vec - Rust #method.push std::vec::Vec - Rust #method.pop

I've just recently started learning Rust so I may be wrong, but if you agree then I think we can close this issue 😄

kamil-duda avatar Jan 01 '21 12:01 kamil-duda

Like I said in the initial comment:

If you input keys fast enough (very easy when not using release mode and batch rendering) this can be seen because of erratic movement of the player character.

A push can happen several times in the time it takes one pop to move the character. Thus you can input up, left, down and it'll move the character down, left, up. So your VecDeque would be much better.

denniskaselow avatar Jan 01 '21 13:01 denniskaselow

Oh I see now, you are right. I will prepare a PR with changes 😃

kamil-duda avatar Jan 01 '21 14:01 kamil-duda

@iolivia , @denniskaselow Take a look at #91 - what do you think? :)

kamil-duda avatar Jan 02 '21 11:01 kamil-duda