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

pointerlock controls example : fix key listeners #21055

Merged
merged 2 commits into from
Jan 12, 2021

Conversation

felixmariotto
Copy link
Contributor

@felixmariotto felixmariotto commented Jan 11, 2021

Issue : the pointerlock example, FirstPersonControls and FlyControls checked the keyCode property of the keydown and keyup events, which means it was bound to characters. Some keyboard layouts, like the French one displayed bellow, don't map the WASD keys at the same place, whereas we actually play with the same keys as everybody, only these keys represent different characters.

Screenshot_1

The present PR solves this by using the event.code propery, which is layout-independant. It will also solve the problem for Russian or Chinese layouts for instance.

Mugen87
Mugen87 previously approved these changes Jan 11, 2021
@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 11, 2021

It's probably best to only change misc_controls_pointerlock in this PR and clean up the code base when it was merged.

Especially after a PR was approved, it's problematic to add more commits (since it invalidates existing reviews).

@Mugen87 Mugen87 dismissed their stale review January 11, 2021 11:46

Code was changed after review.

@felixmariotto
Copy link
Contributor Author

It's probably best to only change misc_controls_pointerlock in this PR and clean up the code base when it was merged.

@Mugen87 Yes sorry, initially I just thought of fixing this example, but I found a lot more with problems... I'm done checking the examples now, I will do as you said 👍

@felixmariotto
Copy link
Contributor Author

@Mugen87 I reverted to the state it was at when you reviewed it, and I will make a new PR for the rest of the changes, including the changes in examples/js.
Shall I squash the commits, or is it better to keep them separate ?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 11, 2021

Shall I squash the commits, or is it better to keep them separate ?

In this PR it seems you have force-pused which is totally fine.

@mrdoob mrdoob added this to the r125 milestone Jan 12, 2021
@mrdoob mrdoob merged commit 5b10aa0 into mrdoob:dev Jan 12, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jan 12, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants