playing/recording pianoRoll's chord notes (#4963) #4976
Conversation
NotePlayHandleManager::acquire uses a read lock unless the pool is empty. If two threads try to acquire NotePlayHandle simultaneously when the value of s_availableIndex is 1, one thread will try to read s_available[-1]. If the acquire action and the release action are done at the same time, NotePlayHandleManager::acquire may try to read data before NotePlayHandleManager::release actually writes. This commit prevents them by always using the write lock when acquiring a NotePlayHandle.
|
I'd like to review this. It's ready for review, right? Do you want this to go into 1.2.0 or 1.2.1? |
|
Yes it is ready. It is working for mouse \ keyboard inputs but not for external MIDI controller inputs (for those, the behavior is as it was until now - only base note sounds and recorded). |
|
Unfortunatelly, doesn't always work for me. What I do is:
The notes are often placed at the wrong positions or sometimes missing. How did you test it? |
|
Per the question, 1.2.0 vs 1.2.1, that may also depend on the release date of 1.2.0. I think 1.2.1 is OK, but we'll see. |
|
Weird - I tested it with quite the same flow you described. Clicking "record" is not a mandatory step though, since it should play the chord live as well. Anyway, I'll be able to check this only later. I'll update after I do that. |
|
It works for me. Attaching a screenshot. Do you have any idea on what can I do to find why it doesn't for you? I even tried to change language to see if that's the problem (since the chord is picked by its name) but it also worked. |
|
Heh, I just found out it worked on another computer. The reason was that I had "SDL" selected for audio device in the settings on this computer. With other backends, it works. I'll go on with the review. |
|
I had an issue with both jack and pulse: Open triple oscillator piano roll, select minor, press key "C5" for a short duration (maybe 1/8 note) by simply using the left mouse key. When I did that, only the base key did release. Can you reproduce this? |
Yes. This is a bug (all clicks directly on the pianoKeys graphics will release the base note only). Sorry. I'll fix that in addition to the other comments asap. |
|
Thank you for finding the bug, and once again sorry for that. |
|
Np for the bug. Good you could fix it. Btw, this is unrelated to your PR, but: If you take triple oscillator and slide over the keyboard fast (e.g. from C2 to C4), are some notes displayed as still pressed (while they don't sound anymore)? |
|
Functional and style review passed. Tests passed. You can squash-merge it, or let me know if I should. |
|
Oh, note, I'm not sure about the merge strategy, I'll ask on discord. Sorry, please wait. |
|
Regarding the notes that keep being displayed when you slide really fast over them - I noticed this as well. Planned to open an issue for it later on. |
|
Usually I leave it to the author to merge. However, I can't see whether an author has the rights to merge. If you don't see the merge button, then I can do it. I'll first have to ask if we can merge shortly before the 1.2.0 release. |
|
I don't have the 'merge' button here, so after you'll get a reply I'd appreciate if you can do it |
|
It looks like this PR is a feature (as stated in #4989), and stable-1.2 only allows fixes. If you agree, it should be better re-based and re-targetted on/to master, and this PR should be closed instead. Sorry for the overhead! |
|
Alright. I hope I understood correctly - I'll fetch the upstream master branch, copy the changes I did into this branch and open a new PR. |
|
Yeah, |
This change makes pianoRoll chord settings to generate notes accordingly, and record them (fixes #4963). It can be triggered by mouse and keyboard, however it does not supported with an external MIDI controller (as current behavior). I'll work on another change that will enable the chord playing (and thus recording) of a base note that was generated by an external MIDI device in future (because it needs different approach, maybe MIDI messages manipulating).