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

Inverts SE-2 keyboard device actions (Z, X) for yaw command #1030

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

riccardorancan
Copy link
Contributor

@riccardorancan riccardorancan commented Sep 24, 2024

Description

Swapping the keyboard command (X, Z) for yaw in the Se2Keyboard class to have the following mapping:

  • Z - positive yaw
  • X - negative yaw

Corrected also the docstring of the Se2Keyboard class.
See Issue #1029

Note: I double checked the Se3Keyboard class and the implementation there is correct.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have run all the tests with ./isaaclab.sh --test and they pass
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@riccardorancan riccardorancan changed the title swapping keyboard commands (Z, X) for yaw & correcting docs swapping keyboard commands (Z, X) for yaw Sep 24, 2024
@riccardorancan riccardorancan changed the title swapping keyboard commands (Z, X) for yaw Inverting keyboard commands (Z, X) for yaw Sep 24, 2024
@Mayankm96 Mayankm96 changed the title Inverting keyboard commands (Z, X) for yaw Inverts keyboard device actions (Z, X) for yaw command Sep 25, 2024
@Mayankm96
Copy link
Contributor

Thanks a lot for the fix. The issue seems to arise more from different keyboard layouts (US English vs German English). Not sure if there's a way to check the layout and assign the keys accordingly.

@lorenwel
Copy link
Contributor

Thanks a lot for the fix. The issue seems to arise more from different keyboard layouts (US English vs German English). Not sure if there's a way to check the layout and assign the keys accordingly.

If I may weigh in on this.

I don't think we need to discuss keyboard layouts for the main and most important fix of this PR.
The direction of the yaw command for the Z and X keys, as they are on the US keyboard layout, are simply not "right" and incosistent with all other keybindings.
For the numpad, the left key turns in positive yaw (i.e. left), the right key turns in negative yaw (i.e. right).
The same is true for the SE3Keyboard, where 7/Z yaw positive and 9/X yaw negative.

So that part of this PR simply makes everything consistent.

The second part is the docstring and the terminal output.
There, the underlying cause is clearly a keyboard layout mismatch.
I do strongly believe the way @riccardorancan adjusted the info is correct:
The actual keybindings check for the Z key to be pressed. The documentation and terminal output should reflect that.
My suspicion is that whoever wrote the documentation as it is now, was using a physically German/Swiss keyboard, but enabled the US layout in their operating system.
Consequently, when they physically pressed the Y key, the operating system registered Z, and everything works as they expect.
This is understandable, but a somewhat niche use-case, which I don't think should dictate the docstring we use.

I also don't think it is practical to detect all of this programmatically. We would need to find out the physical layout of the connected keyboard, and they keyboard layout selected in the OS.
Then, we would need to figure out which physical key is to the left of the X key. Once we know that, we would need to map the physical key to the "virtual key" actually registered by the OS, since the OS layout might be different from the physical one.
To me, this seems like an unreasonably large effort for such a small benefit for a fringe set of users.

TL;DR: I strongly believe the PR is good as is.

@Mayankm96
Copy link
Contributor

Mayankm96 commented Sep 25, 2024

Thanks for weighing in @lorenwel . I agree that the changes make a lot of sense.

On closer look, the Z-axis rotation is mapped differently in SE(2) and SE(3) keyboard versions. Should we make the two consistent as well?

We can also just do that in a separate MR. Don't want to block anyone :)

@Mayankm96 Mayankm96 added the enhancement New feature or request label Sep 25, 2024
@Mayankm96 Mayankm96 changed the title Inverts keyboard device actions (Z, X) for yaw command Inverts SE-2 keyboard device actions (Z, X) for yaw command Sep 25, 2024
@Mayankm96 Mayankm96 merged commit e6f4ed1 into isaac-sim:main Sep 25, 2024
1 of 2 checks passed
@Mayankm96 Mayankm96 added bug Something isn't working and removed enhancement New feature or request labels Sep 25, 2024
@lorenwel
Copy link
Contributor

Thanks for merging. Good point on the SE3Keyboard. It probably makes sense to use the same keys for the z axis rotation in both cases.

While we're on that, we could then also consider changing the signs for some of those keybindings.
The z-axis translation, for example, is opposite the direction used in the Isaac Sim viewer, for example. There, E is up, whereas SE3Keyboard as E for the negative axis.
The roll (x-axis rotation) I would also find more intuitive if it was inverted. Rolling "to the right" to me feels like it should be a positive X-axis rotation.

@riccardorancan riccardorancan deleted the fix/keyboard_mapping branch September 25, 2024 11:11
iamdrfly pushed a commit to iamdrfly/IsaacLab that referenced this pull request Nov 21, 2024
…m#1030)

# Description

This MR swaps the keyboard command (X, Z) for yaw in the `Se2Keyboard` class
to have the following mapping:
- Z - positive yaw
- X - negative yaw

It also corrects the docstring of the `Se2Keyboard` class.

Fixes isaac-sim#1029

_Note:_ I double checked the `Se3Keyboard` class and the implementation
there is correct.

## Type of change

- Bug fix (non-breaking change which fixes an issue)

## Checklist

- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./isaaclab.sh --format`
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have run all the tests with `./isaaclab.sh --test` and they pass
- [ ] I have updated the changelog and the corresponding version in the
extension's `config/extension.toml` file
- [ ] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants