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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Added keyboard scan input event #5495

Closed
wants to merge 12 commits into from
Closed

[Merged by Bors] - Added keyboard scan input event #5495

wants to merge 12 commits into from

Conversation

Bleb1k
Copy link
Contributor

@Bleb1k Bleb1k commented Jul 30, 2022 •
edited

Objective

  • I wanted to have controls independent from keyboard layout and found that bevy doesn't have a proper implementation for that

Solution

@Bleb1k
Copy link
Contributor Author

Bleb1k commented Jul 30, 2022 •
edited

also i added num_enum dependency for this to work

@MrGVSV
Copy link
Member

MrGVSV commented Jul 30, 2022 •
edited

Is there a need for the enum? Couldn't we just pass around the raw u32 wrapped in a newtype?

@Bleb1k
Copy link
Contributor Author

Bleb1k commented Jul 30, 2022

Is there a need for the enum?

I don't know, this is the most obvious approach for me

Couldn't we just pass around the raw u32 wrapped in a newtype?

Maybe, but i'm very new to rust, like, i'm two weeks into it for now

@MrGVSV
Copy link
Member

MrGVSV commented Jul 30, 2022

Maybe, but i'm very new to rust, like, i'm two weeks into it for now

No worries, glad you're contributing already. It's a great way to learn! 😄

I’d say we could replace the enum with struct ScanCode(u32) or something like that. It's easier for others to use and reason with imo

@Bleb1k
Copy link
Contributor Author

Bleb1k commented Jul 30, 2022 •
edited

Ok, thanks!
This way there's no need in adding new dependencies, and code looks so much cleaner!

@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-Input Player input via keyboard, mouse, gamepad, and more labels Jul 30, 2022
alice-i-cecile
alice-i-cecile reviewed Jul 30, 2022
View reviewed changes
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.

I'm on board with this direction. While it's not a full fix, it's the obviously correct next step (and a change I was considering making already).

I would say this probably closes both #2052 and #862 (please mention this in the PR description), but we'll want to make more targeted follow-up issues for things like text input and keyboard button visualization (for input binding menus and prompts).

alice-i-cecile
alice-i-cecile reviewed Jul 31, 2022
View reviewed changes
crates/bevy_input/src/keyboard.rs Outdated
///
/// It is used as the generic `T` value of an [`Input`](crate::Input) to create a `Res<Input<ScanCode>>`.
/// The resource stores the numeration of the buttons of a keyboard and can be accessed inside of a system.
/// Use [`KeyCode`](KeyCode) for more meaningful/readable code
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this captures the distinction quite correctly. ScanCodes are used for semantic values, KeyCodes correspond to the position on a keyboard. We should also update KeyCode's docs to link back to here.

Copy link
Member

Choose a reason for hiding this comment

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

See for example https://wiki.libsdl.org/CategoryKeyboard

Copy link

@vacuus vacuus Aug 1, 2022 •
edited

Choose a reason for hiding this comment

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

I think you mixed up the two

SDL_Scancode values are used to represent the physical location of a keyboard key on the keyboard.
SDL_Keycode values are mapped to the current layout of the keyboard

(from the website you linked)

scancode: ScanCode
Identifies the physical key pressed
virtual_keycode: Option<VirtualKeyCode>
Identifies the semantic meaning of the key

https://docs.rs/winit/latest/winit/event/struct.KeyboardInput.html (winit docs, since Bevy gets KeyboardInput from it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, thank you.

@alice-i-cecile alice-i-cecile mentioned this pull request Aug 1, 2022
Closed
cart
cart approved these changes Aug 5, 2022
View reviewed changes
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I think this is the right call for now. ScanCode might have some issues (as outlined in #2052), but this is the best interim solution we have / is a pretty standard way of doing things.

@cart
Copy link
Member

cart commented Aug 5, 2022

bors r+

bors bot pushed a commit that referenced this pull request Aug 5, 2022
@Bleb1k
Added keyboard scan input event (#5495)
1152111 
# Objective

- I wanted to have controls independent from keyboard layout and found that bevy doesn't have a proper implementation for that

## Solution

- I created a `ScanCode` enum with two hundreds scan codes and updated `keyboard_input_system` to include and update `ResMut<Input<ScanCode>>`
- closes both #2052 and #862

Co-authored-by: Bleb1k <91003089+Bleb1k@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Aug 5, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Added keyboard scan input event [Merged by Bors] - Added keyboard scan input event Aug 5, 2022
@bors bors bot closed this Aug 5, 2022
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
@Bleb1k @inodentry
Added keyboard scan input event (bevyengine#5495)
54acaf7 
# Objective

- I wanted to have controls independent from keyboard layout and found that bevy doesn't have a proper implementation for that

## Solution

- I created a `ScanCode` enum with two hundreds scan codes and updated `keyboard_input_system` to include and update `ResMut<Input<ScanCode>>`
- closes both bevyengine#2052 and bevyengine#862

Co-authored-by: Bleb1k <91003089+Bleb1k@users.noreply.github.com>
maccesch pushed a commit to Synphonyte/bevy that referenced this pull request Sep 28, 2022
@Bleb1k @maccesch
Added keyboard scan input event (bevyengine#5495)
5f026a6 
# Objective

- I wanted to have controls independent from keyboard layout and found that bevy doesn't have a proper implementation for that

## Solution

- I created a `ScanCode` enum with two hundreds scan codes and updated `keyboard_input_system` to include and update `ResMut<Input<ScanCode>>`
- closes both bevyengine#2052 and bevyengine#862

Co-authored-by: Bleb1k <91003089+Bleb1k@users.noreply.github.com>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@Bleb1k @james7132
Added keyboard scan input event (bevyengine#5495)
a4779b2 
# Objective

- I wanted to have controls independent from keyboard layout and found that bevy doesn't have a proper implementation for that

## Solution

- I created a `ScanCode` enum with two hundreds scan codes and updated `keyboard_input_system` to include and update `ResMut<Input<ScanCode>>`
- closes both bevyengine#2052 and bevyengine#862

Co-authored-by: Bleb1k <91003089+Bleb1k@users.noreply.github.com>
This was referenced Dec 4, 2022
Open
Closed
@janhohenheim janhohenheim mentioned this pull request Jan 4, 2023
Closed
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@Bleb1k @ItsDoot
Added keyboard scan input event (bevyengine#5495)
d567d40 
# Objective

- I wanted to have controls independent from keyboard layout and found that bevy doesn't have a proper implementation for that

## Solution

- I created a `ScanCode` enum with two hundreds scan codes and updated `keyboard_input_system` to include and update `ResMut<Input<ScanCode>>`
- closes both bevyengine#2052 and bevyengine#862

Co-authored-by: Bleb1k <91003089+Bleb1k@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile left review comments

@harudagondi harudagondi harudagondi left review comments

@vacuus vacuus vacuus left review comments

@cart cart cart approved these changes

Assignees
No one assigned
Labels
A-Input Player input via keyboard, mouse, gamepad, and more C-Enhancement A new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Bleb1k @MrGVSV @cart @alice-i-cecile @harudagondi @vacuus