#5339, #5340: Fix pasting notes from bb editor

[thmueller64]

Addresses #5339 and #5340: When pasting notes from the bb editor to the piano roll, the length of the notes copied is now set to the envelope length of the underlying instrument, dependent on the BPM and envelope settings at the moment of pasting. Previously, copying the notes led to unexpected behavior including them not being registered in the song editor or being ignored by the loop. Thanks @Veratil for the discussion on discord and the change requests.

[LmmsBot]

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

• Linux (AppImage): lmms-1.2.2.684-linux-x86_64.AppImage (build link)

Windows

• Windows 64-bit: lmms-1.2.2.684-mingw-win64.exe (build link)
• Windows 32-bit: lmms-1.2.2.684-mingw-win32.exe (build link)
• Windows 32-bit: lmms-1.2.2-msvc2017-win32.exe (build link)
• Windows 64-bit: lmms-1.2.2-msvc2017-win64.exe (build link)

macOS

• macOS : lmms-1.2.2.684-mac10.13.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://7993-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.684-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/7993?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://7992-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.684-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/7992?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://7994-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.684-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/7994?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/34wupqknvva3mx87/artifacts/build/lmms-1.2.2-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/34458344"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/3tc628c2hrh5yuh3/artifacts/build/lmms-1.2.2-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/34458344"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://7991-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.684-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/7991?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "cef300d946dc077d3ee262b4934815bfb481bcdb"}

[Spekular]

Instead of defining a new constant, why don't you just check if the pasted note has a length of -DefaultTicksPerBar?

[thmueller64]

Because Pattern::addNote is also called by Pattern::addStepNote with a note length of -DefaultTicksPerBar. You would not be able to add any steps, because it would create a note equal to the envelope length of the underlying instrument. The instrument track would immediately get replaced by the piano roll when clicking on a step. Thus, I think there's no obvious way around indicating where the notes to be added come from.

What i noticed while writing the above: This PR fixes the problem where notes are copied from the BB Editors Piano Roll and pasted to an Instruments Piano Roll as described in the issues. I have not thought of the implications of the PR when copying notes from a BB Editor to itself or to another BB Editor. @Veratil, please remove the approval for now.

Per request

[Spekular]

But, instead of setting the note length to a magic number and then resizing it when it's added, couldn't you just set the note to the correct length before adding it?

[thmueller64]

You're totally right, thanks! I missed that the pattern is accessible from the PianoRoll class through the m_pattern member. Update is coming.

[PhysSong]

Could you explain why you update m_steps only in this case, not whenever the length changes?

[thmueller64]

When a user clicks on "Add steps" and creates a pattern on the added steps, if he then deselects all of the steps in the extended range for a moment, the range is shrunk immediately. I thought this would be a bit annoying, though it looks nicer when steps are moved inside the piano roll or removed and the beat pattern adapts. This would be a one line change essentialy.

[IanCaio]

I could be wrong, that's my first glance on the PR. But from what I read, on master m_steps is only changed by methods related to the BBTrack. When you have an MelodyPattern (adding notes through the piano roll), it seems that the length of the pattern changes but m_steps remains the same value. So when you delete the notes and it goes back to being a BeatPattern, the number of steps is the one you set earlier on the BBEditor. I didn't test yet but I think with this line you'd end up updating the number of steps as you add notes and once you go back to a BeatPattern you'd have the last value you had, not the one you set on the BBEditor. Changing that behavior is not really necessary and IMHO it makes sense to keep BeatPattern number of steps separated from the MelodyPattern length.

[IanCaio]

At first sight, the code looks good, though I'd personally remove adjustSteps as I believe it's unnecessary and changes the current behavior of the BBEditor handling of number of steps. I've one concern though: This makes it impossible to paste "step notes" on the Piano Roll. I understand this fixes the mentioned issues, because they are related to the rendering of TCOs and the PianoRoll loop not handling negative length notes, but that also removes the possibility of adding a "step note" on the Song Editor to play a sample until its end, instead of having to adjust the length manually. That's probably rarely used at the moment, but I looked forward for us to handle this kind of notes differently so they could be used in the future outside the BBEditor. Maybe adding a m_type variable to the Note class which would define whether the note is played until the end of the sample or until the end of the note. That would probably require much more changes to the code and we also are running towards the refactor so it's probably something that will have to wait, thus if everyone agrees that this is a suitable solution for now I'm good with it. I'd like that we add a TODO comment though, so we know that if we change the way we handle this kind of note we will have to remove that block of code from PianoRoll::pasteNotes.

[thmueller64]

Close in favor of #5902 .