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

add piano mode (F6) #820

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add piano mode (F6) #820

wants to merge 1 commit into from

Conversation

dgruss
Copy link

@dgruss dgruss commented Mar 17, 2024

In the usdx editor you can press F6 to toggle piano mode.

The keyboard keys then work like piano keys on the current note... arrow keys and some other keys are unaffected.

P is moved to the Return key in piano mode as P is used as a piano key in this mode

@barbeque-squared
Copy link
Member

Haven't tried this yet, but looking at the code, a quick note so that I don't forget about this when testing: if I'm reading the code correctly, this probably assumes QWERTZ (with no way of rebinding anything because USDX doesn't currently have support for any kind of key rebinding)?

I do like the reasoning behind the PR though, even with my locally built versions (I can move pitch up/down with just Numpad -/+) pitching is still quite an intensive task.

I'm assuming Qwerty+Qwertz covers 99% of USDX' userbase, so if it doesn't make the code unreadable, this can probably be done with a (not in-game) config.ini setting. Yeah rebinding whatever keys would be even better (it's come up in past issues, but this is the first feature where it would be more than just a nice-to-have) but that's a lot of work.

Some other things to look for in testing:

  • In-game documentation (when Tab is pressed)
  • stray end of line whitespace
  • how does this match up with the keys on a physical piano

@barbeque-squared
Copy link
Member

Sorry this took so extremely long to get around to. The piano emulation is fine (it actually does skip the non-existing black keys) but sadly it's bound to QWERTZ only, with no way for users to change that. I don't know the differences between Qwertz and Qwerty exactly, but Qwertz as a default is already quite specific, the fact that there's no way to change it (even if it's just a hidden ini thing that must be added manually -- provided this doesn't create unreadable code) makes this not really fit for merging sadly.

The other comments I made earlier (whitespace, no help) also still stand. I won't close it because it is quite a nice feature, but I will make it a draft for the time being.

@barbeque-squared barbeque-squared marked this pull request as draft April 14, 2024 11:55
@dgruss
Copy link
Author

dgruss commented Apr 14, 2024

I could imagine having an ini entry like

[Editor]
PianoKeysLow=60,97,121,115,120,100,99,118,103,98,104,110,109,107,44,108,46,246,45
PianoKeysHigh=49,113,50,119,51,101,114,53,116,54,122,117,56,105,57,111,48,112,252,96,43

This would be straight-forward to use instead of the current static mapping.

I just don't have the time right now to figure out how to read from the ini file and get this to the ScreenEditSub object, or to find where the help screen / documentation is...

@barbeque-squared
Copy link
Member

barbeque-squared commented Apr 14, 2024

I like this way of defining the keys a lot! We can provide some copy-pasteable layouts on the wiki.

All the ini reading is done in UIni.pas. I'm not sure if there's any current support for this particular way (it's some kind of array), but if not and that proves difficult just define the layout for Qwertz or Qwerty in UIni.pas somewhere hardcoded and I'll write the part that fills that from the actual ini.
EDIT: if there's any special requirements for what the values can be / array length, please do put those as comments near where you define it. I'd like to have some sanity checks on this when reading them.

Don't worry about the help screen and other documentation, I can always add them as separate commits later on. A lot of PR's don't have them because they're in completely unrelated places, which is really more of an USDX-wide problem, and I can't fault people for not finding it (or forgetting to update it).

@dgruss
Copy link
Author

dgruss commented May 15, 2024

customizable now and also added to the help screen

these config settings should work fine for english keyboards:

[KeyBindings]
PianoKeysLow=92,97,122,115,120,100,99,118,103,98,104,110,109,107,44,108,46,59,47
PianoKeysHigh=49,113,50,119,51,101,114,53,116,54,121,117,56,105,57,111,48,112,91,61,93

I also added an alternative to SDLK_SLASH with SDLK_KP_DIVIDE which was not used so far, allowing for non-english keyboards to split and merge notes / rows

@dgruss dgruss marked this pull request as ready for review May 19, 2024 15:33
@dgruss
Copy link
Author

dgruss commented Jun 9, 2024

anything that should still be fixed in this one?

@dgruss
Copy link
Author

dgruss commented Jul 25, 2024

rebased to new master and squashed the commits

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.

2 participants