arpeggiator sorted mode fixed

[szeli1]

closes #6499
closes #4491

with this change the arpeggiator with sorted mode enabled will cycle through all of the active (currently playing) note's chords in order without skipping notes or randomly playing other ones.

this works by combining all of the chord's keys in an array and then playing it back as one single larger chord (sorted as suggested).

[zonkmachine]

Hi! Testing this now (sorry for the long waiting time). Thank you so much for taking care of this issue! It seem to work really well but the arpeggiator has a lot of functions now so it's starting to get really finicky to test. While testing this PR I discovered a bug that I introduced with the 'Repeats' function. This has been fixed in #7007 together with the original bug which that PR takes care of. I think it's better to merge that PR first and then rebase this PR so we can test this one some more with two less bugs to worry about. Are you OK with this?

[szeli1]

Are you OK with this?

What does this mean for me? If it isn't too difficult, then sure! (I do not have this branch locally currently)

[zonkmachine]

I suspect it's a simple rebase. I can push the changes to your branch if you like.
Can you rebase this PR on latest master?

[szeli1]

Do what you think is the best solution here. I will try to rebase soon.

[zonkmachine]

OK - The plan is:

1. you rebase this PR on master
2. We merge Refactor ArpDirection::Down and ArpDirection::DownAndUp #7007
3. This PR is rebased once more against master

[szeli1]

I think i have rebased. You can merge #7007 if I'm correct.

[zonkmachine]

I think i have rebased. You can merge #7007 if I'm correct.

That looks correct. I'll allow some time for feedback on that PR first. A day or two.

[zonkmachine]

This PR is rebased once more on master

I went with merging just my changes from #7007. This keeps your original commits so we can more easily test for regressions.

[zonkmachine]

It looks like this could be a fix also for issue #2606. At least the demo project in the original post is fixed.

Edit: nope

[szeli1]

I have rebased, and changed your changes. This should be tested because unfortunately I do not remember my changes in great detail. There seems to be a display bug when sorted mode is enabled and multiple notes are pressed. This bug makes the pressed notes light up incorrectly (while not being played).

[zonkmachine]

In this test project from issue #2606 you can see the problems with Sync mode over more than one note. It has two identical arpeggios but one inverted so the sound cancels out unless one of the tracks skips a note. The last 6th of the sequence is the one with Sync mode over two notes. Turn up the tempo and enjoy!
arpnotesilence-01.mmp.zip
Edit: PS. This is not a new issue with this PR but it's a good demo nonetheless...

[szeli1]

What are you trying to say? Would you rather me fix that issue as well? And the display issue I talked about? If not, I think this is ready for merge.

[zonkmachine]

What are you trying to say? Would you rather me fix that issue as well?

No, you're welcome to, but I mentioned above that it looks like at least one of the demo projects was fixed by this commit so there is a connection. I'm a bit tired now so I'm happy to stick with just this issue.

I've tested it with a bunch of arpeggios and they are almost all possible to compute ahead what the result will be. Almost all. This doesn't seem to be true for when you have a note that is looped before it's end so that it will overlap itself. I don't think it's reasonable to cover everything and this is an edge case. Just seeing the same result over and over again is fine.
Example: arpdowncycle-02.mmp.txt

[zonkmachine]

Fixes #4491

[Veratil]

The only other thing is perhaps it would be better to pull out all the range + sortOffset into a new variable so it's not repeated over and over and over and over. :)

[szeli1]

Fixed the sorting algorithm and added more comments.
The arp settings should now behave like with the old sorting order.

[zonkmachine]

Added new check box in the envelope settings that makes the envelope use the parent note instead of the sub note. This will allow the user to make the envelope apply over multiple arpeggio notes instead of each note separately.

Interesting, I see what you're trying to do but I don't think this is ready for merge. Some quick observations.

• It's a bit hard to dial in an envelope that works both for the individual arpeggio notes and for the whole arpeggio, because they use the same envelope (this is my understanding anyway).
• I think I've seen similar implementations in synth plugins but they would let the arpeggio continue over the release.
• Also, in this test project you see that there is a bump in volume on key off where this is not the case with the new option disabled.
  arpenvelope.mmp.txt

I think the envelope part is probably better to bud off to it's own PR.

[szeli1]

envelope part is probably better to bud off to it's own PR.

I reverted the changes. Looking back on this, I completely agree with you.

[zonkmachine]

I've tested it again but it doesn't seem to behave properly. The current implementation seem to be octave-piano-arp, referring to @DomClark from his analysis of the arpeggiator in #6499.

Borrowed from #6499 (comment)

Here are some different ways the notes (C-F-A) could be arpeggiated on.

1. The way LMMS does it today (piano-octave-arp)
2. How it currently works on this PR (octave-piano-arp)
3. The behavior I believe we agreed on (octave-arp-piano)
4. The original implementation in this PR (octave-sort(arp-piano))

Project used in testing: arpsorttest.mmp.txt

[szeli1]

Fixed, I really hope the arp is working as intended now.

[zonkmachine]

Looks great and tests fine. I'm approving this and keep testing it ahead of the final word from @DomClark

[DomClark]

I think this PR can be simplified a bit. Firstly, I would introduce the variable const auto arpMode = static_cast<ArpMode>(m_arpModeModel.value());, to avoid repeating that expression every time you want to check the mode. Then, I would do the following:

• The notMinIndex bits can be replaced with an early return. I would check arpType == ArpMode::Sort && _n != cnphv.first() just before the declaration of chord_table, and return if it's true. I think this is a more obvious (and a bit more efficient) way to ensure only the first note drives the sorted arpeggio processing.
• Rather than introducing offsetRange, and replacing range with it, I would put the appropriate value into range in the first place. I'd rename the original range to singleNoteRange (or something like that), and then define const auto range = arpType == ArpMode::Sort ? singleNoteRange * cnphv.size() : singleNoteRange;. Then total_range can be removed too. This will simplify the diff a bit, and is a bit clearer as a variable name (I'm not sure what the "offset" refers to in "offsetRange").
• You can use a modulo operation to find which step of the chord you're at, in the same way you do to find which octave you're in. Then you don't need to build a list of notes in a separate vector, and can just sort cnphv in-place. The note computation will end up looking something like this:

  const auto octaveDiv = std::div(cur_arp_idx, total_chord_size);
  const auto octave = octaveDiv.quot;
  const auto arpDiv = std::div(octaveDiv.rem, cnphv.size());
  const auto arpIndex = arpDiv.quot;
  const auto pianoRollIndex = arpDiv.rem;
  sub_note_key = cnphv[pianoRollIndex]->key()
  	+ chord_table.chords()[selected_arp][arpIndex]
  	+ octave * KeysPerOctave;

  See how the octave-arp-piano order becomes obvious now. The octave is the first quotient, the arp index the second, and the piano roll index the final remainder.

[szeli1]

• You can use a modulo operation to find which step of the chord you're at

Smart idea, I didn't think about that.
Implemented the changes that you suggested.

[DomClark]

Code looks good to me, although I haven't tested it myself.

[zonkmachine]

On the connection with #2606 (Race condition in arpeggio processing). Initially it looked fixed but this was just with the simple test file provided and I couldn't replicate the fix. That issue may have been more intermittent than I remember. I think this PR should be merged and #2606 be kept open.

[zonkmachine]

@szeli1 Thank you for coding this, and for your patience in the review. Top notch work!
@DomClark Thanks for bringing clarity to the issue.
@Veratil Thank you for reviewing.

This is a great step for the arpeggiator and for lmms-1.3
The arpeggiator now can be played via a keyboard live or played back from the piano
roll and it will work like any other 'arp' out there. With some extra cherries on top and
obviously a bug or two but the biggest glitches, in my opinion, have been taken care of.