Implement InputHandler for mouse movement #186
Conversation
There was a problem hiding this comment.
See CONTRIBUTING.md for how to avoid Merged remote-tracking branch [...] commits, they provide no real usefulness. However git is hard to use sometimes.
I'd prefer it if you completely squashed all of these commits and added a reference to the bug fix you've provided.
I think these additions also show us that we need to supply time information together with InputHandler, for e.g calculating if a series of clicks was a double-click. This can be done in a later stage though.
src/ecs/resources/input.rs
Outdated
| /// _: &mut Pipeline) | ||
| /// -> Trans { | ||
| /// // ... | ||
| /// input_handler.update(events); |
There was a problem hiding this comment.
This wouldn't work. Show how to get input_handler aswell.
src/ecs/resources/input.rs
Outdated
| pressed_mouse_buttons: HashMap<MouseButton, KeyQueryState>, | ||
| // defined to match glium | ||
| mouse_position: Option<(i32, i32)>, | ||
| previous_mouse_position: Option<(i32, i32)>, |
There was a problem hiding this comment.
Why does InputHandler store the previous mouse position?
There was a problem hiding this comment.
previous_mouse_position is the mouse position the previous time mouse_change was called. It allows us to compare to the current mouse position and get the amount the mouse has moved. Using this information, you can work out for example how much to rotate the camera in the ecs etc.
src/ecs/resources/input.rs
Outdated
| @@ -48,30 +90,46 @@ impl InputHandler { | |||
| // second `Pressed` event. | |||
| } | |||
| Entry::Vacant(entry) => { | |||
| entry.insert(KeyQueryState::Queried); | |||
| entry.insert(KeyQueryState::NotQueried); | |||
There was a problem hiding this comment.
This is wierd, I remember that this was working, but following my logic in the code of key_once it shouldn't work.
Note also that you don't reflect this change in the commit.
|
Also, should #181 be closed inplace of this PR? Seems like they are identical except for |
|
I split the PR up because I thought that the mouse stuff might be more controversial. |
derekdreery
force-pushed
the
inputhandler_mouse_move
branch
from
b54a4dc
to
801cf8b
Compare
derekdreery
force-pushed
the
inputhandler_mouse_move
branch
from
801cf8b
to
bc6c030
Compare
|
I just rebased this on my other PR. If you want to add both at the same time, you could just use this PR. |
|
@derekdreery It's not blocked anymore, please rebase onto the current master. I think we can do #225 after this PR is merged. |
kvark
changed the title
derekdreery
force-pushed
the
inputhandler_mouse_move
branch
from
bc6c030
to
13f54c3
Compare
|
This is rebased on current upstream/develop. |
This is my state implementation for mouse movement. It adds to PR #181 .
It suits my needs nicely as I can respond to any change in mouse position since the last frame, so I want to see if the behaviour I want is universal.
One particular aspect I invite comments on is defaulting to (0, 0) rather than using an Option. In my model, a movement of (0, 0) is basically a no-op, where as a position of (0, 0) is not a no-op (it's top left window). Does this seem reasonable?