Feature: Glue notes in Piano-Roll #5482

zonkmachine · 2020-05-02T21:20:36Z

#5216 by CYBERDEViLNL rebased on c755b56 and merge conflict solved.

* Added sorting on note key which is very important with the new loop.

LmmsBot · 2020-05-02T21:34:37Z

🤖 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.697-linux-x86_64.AppImage (build link)

Windows

Windows 32-bit: lmms-1.2.2.697-mingw-win32.exe (build link)
Windows 64-bit: lmms-1.2.2.697-mingw-win64.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)
Windows 32-bit: lmms-1.2.2-msvc2017-win32.exe (build link)
Windows 64-bit: lmms-1.2.2-msvc2017-win64.exe (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://8583-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.697-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8583?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://8586-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.697-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8586?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://8584-15778896-gh.circle-artifacts.com/0/lmms-1.2.2.697-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8584?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/rvl9pyn7l7oph3ef/artifacts/build/lmms-1.2.2-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35046280"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/nlrihueeaua22yif/artifacts/build/lmms-1.2.2-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35046280"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/oah37bsb653sg31o/artifacts/build/lmms-1.2.2-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35046281"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/skuktovhgbfajglk/artifacts/build/lmms-1.2.2-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35046281"}]}, "commit_sha": "d86ba8c17d461922e082b5a93c894b6c63506627"}

zonkmachine · 2020-05-26T17:33:23Z

I have tested the glue feature and it works well. I think it's good to merge. The original developer has left GitHub and wont be able to respond so If someone wants to fix anything, just push to this branch.

zonkmachine · 2020-06-12T22:17:55Z

Merge?

src/gui/editors/PianoRoll.cpp

Spekular · 2020-06-13T15:27:05Z

Does this work for the following cases? I fail to see how this logic could merge more than two consecutive notes with the loop it currently uses:

(Merging three notes on left should result in the note on the right. Ascending sequence above to disambiguate how the notes are stacked)

Something about the buildbot build makes it crash when I open LMMS or I'd test myself.

(EDIT: I misread the loop, I see now that note is only incremented when there are no more overlapping notes, I assumed both note and nextNote were incremented each time)

zonkmachine · 2020-06-13T17:20:18Z

Does this work for the following cases?

Yes.

Something about the buildbot build makes it crash when I open LMMS or I'd test myself.

I have seen crashes and I've had to launch lmms with a file known to work as an argument to make it launch. Not with this build though.

zonkmachine · 2020-06-13T17:23:48Z

Oh. It doesn't seem to work well with beat notes though. The result is really inconsistent there.

zonkmachine · 2020-06-13T17:32:00Z

Yeah, look at the length I managed to create on the first beat note when trying to glue it to a normal one.

<pattern steps="16" type="0" name="Kicker" pos="0" muted="0">
  <note len="-168" key="57" vol="100" pos="12" pan="0"/>
  <note len="-192" key="57" vol="100" pos="108" pan="0"/>
</pattern>

Spekular · 2020-06-13T17:47:24Z

Did you merge with an eight note? Beat notes have a length of -192, IIRC, so the math checks out if so (not that it makes this a good result, haha).

I wonder how much effort it would take to make beat notes a subclass of Note so length and endPos could be overriden. Or even simpler, add a boolean (isBeatNote, for example) so they can have positive lengths. Might be out of scope for this PR but they really cause a lot of trouble with the current hacky implementation.

zonkmachine · 2020-06-13T18:01:35Z

Did you merge with an eight note? Beat notes have a length of -192, IIRC, so the math checks out if so (not that it makes this a good result, haha).

No, more like a whole note.

src/gui/editors/PianoRoll.cpp

zonkmachine · 2020-06-13T20:16:23Z

Oh, dear!

This project
gluetest-02.mmp.zip

<Ctrl> + A
<Shift> + G

Edit: fixed in 587c559

PhysSong · 2020-06-25T05:23:08Z

I changed the if statement a little bit and was able to get the behavior identical to FL.

diff --git a/src/gui/editors/PianoRoll.cpp b/src/gui/editors/PianoRoll.cpp
index 30da834df..86cf08c28 100644
--- a/src/gui/editors/PianoRoll.cpp
+++ b/src/gui/editors/PianoRoll.cpp
@@ -703,11 +703,10 @@ void PianoRoll::glueNotes()
 			if ((*note)->key() == (*nextNote)->key()
 				&& (*nextNote)->pos() >= (*note)->pos()
 				&& (*nextNote)->pos() <= (*note)->pos()
-				+ (*note)->length())
+				+ qMax(MidiTime(0), (*note)->length()))
 			{
-				(*note)->setLength((*note)->length() +
-						((*nextNote)->pos() + (*nextNote)->length()) -
-						((*note)->pos() + (*note)->length()));
+				(*note)->setLength(qMax((*note)->length(),
+					MidiTime((*nextNote)->endPos() - (*note)->pos())));
 				noteToRemove.push_back(*nextNote);
 				++nextNote;
 			}

I also suggest gluing all notes if no notes are selected.

PhysSong · 2020-07-18T08:49:06Z

I've updated this PR with the changes I suggested.

zonkmachine · 2020-08-13T14:09:17Z

Fixed a merge conflict.

zonkmachine · 2020-08-13T14:28:10Z

I've tested this PR again and the only problem I saw earlier was fixed by @PhysSong in 587c559

zonkmachine · 2020-08-16T16:25:36Z

Merge?

Spekular · 2020-08-16T20:25:01Z

Sounds good to me!

IanCaio

There's a particular conditional that I believe always return true on the method. I'd understand if it was kept to make the logic more explicit for future readers, but I think a comment would be better suited for that. Besides that, the rest of the code LGTM 👍

IanCaio · 2020-08-17T03:00:00Z

src/gui/editors/PianoRoll.cpp

+		{
+			// key and position match for glue
+			if ((*note)->key() == (*nextNote)->key()
+				&& (*nextNote)->pos() >= (*note)->pos()


Will that conditional ever be false? Since the notes were previously ordered by key and by pos, if two notes have the same key isn't it always true that the next note will have a position equal or greater than the previous note?

The comment highlighted all 4 lines, so to make it clearer the conditional I meant is (*nextNote)->pos() >= (*note)->pos() which will be evaluated if the keys are the same on the two notes.

It is redundant, but I think there should be a comment which explains what you said.

So, what needs to be done here? Remove && (*nextNote)->pos() >= (*note)->pos() and improve the comment?

Yeah, I think just removing that part of the conditional and having the comment explaining that you don't need to check if the next note is after the first because they are ordered already. Removes an unnecessary comparison but leaves an explanation why.

OK. Hopefully fixed in d86ba8c

PhysSong · 2020-08-17T05:22:17Z

Any ideas how we should deal with per-note pitch automation? Should we ignore it for now?

IanCaio · 2020-08-17T12:51:04Z

Any ideas how we should deal with per-note pitch automation? Should we ignore it for now?

IMHO it could be just removed from any glued note. It's to be expected that pitch automations would be lost in a note glue operation, and removing it we avoid the inconsistency of it being kept if it's in the first note or being removed if it's in the second one. So just removing it altogether sounds like a good idea to me.

Quickly looking at the code I think it has to be done like this?

// While glueing the note
sharedObject::unref( (*note)->detuning() );

Personally, I even think that this sharedObject call should be added to Note.h as a helper method, something like removeDetune(), to make the code less obscure to readers.

zonkmachine · 2020-08-17T16:05:27Z

Any ideas how we should deal with per-note pitch automation? Should we ignore it for now?

I think we can ignore it for now. The effect of notes with pitch automation in a glue operation isn't hard to predict.

@IanCaio

Implement proposed changes by @IanCaio

zonkmachine · 2020-09-06T16:45:01Z

Now merge? :)

Spekular

LGTM.

IanCaio

LGTM too!

zonkmachine · 2020-10-18T17:57:55Z

Rebased in a separate PR #5721. Closing this one.

CYBERDEViLNL added 6 commits May 2, 2020 22:55

Feature: Glue notes

7ddb1db

calling getSelectedNotes() once is enough.

84bd3d4

Better loop approach thanks to @Veratil

1a752bd

* Proper naming of variable names.

6b12014

* Added sorting on note key which is very important with the new loop.

Reverse adding compareKey to Note.h and use lambda function instead.

562705f

Fix note sorting.

bd72adc

zonkmachine mentioned this pull request May 2, 2020

Feature: Glue notes in Piano-Roll #5216

Closed

zonkmachine mentioned this pull request May 24, 2020

Piano Roll Razor/Glue Tool #746

Closed

Spekular reviewed Jun 13, 2020

View reviewed changes

src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved

zonkmachine commented Jun 13, 2020

View reviewed changes

src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved

Fix gluing BB notes and completely overlapping notes

587c559

Merge branch 'master' into glue

29cbb2a

ryuukumar mentioned this pull request Jul 21, 2020

Reorganising LMMS, renaming editors and classes and everything in between (cont. #120) #5592

Closed

56 tasks

Merge branch 'master' into glue

d8278be

IanCaio requested changes Aug 17, 2020

View reviewed changes

Remove redundant code and fix comment

d86ba8c

Implement proposed changes by @IanCaio

Spekular approved these changes Sep 7, 2020

View reviewed changes

IanCaio approved these changes Sep 7, 2020

View reviewed changes

zonkmachine mentioned this pull request Oct 18, 2020

Glue rebase 2 #5721

Merged

zonkmachine closed this Oct 18, 2020

PhysSong deleted the glue branch December 19, 2020 03:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Glue notes in Piano-Roll #5482

Feature: Glue notes in Piano-Roll #5482

zonkmachine commented May 2, 2020 •

edited

Loading

LmmsBot commented May 2, 2020 •

edited

Loading

Linux

Windows

zonkmachine commented May 26, 2020 •

edited

Loading

zonkmachine commented Jun 12, 2020

Spekular commented Jun 13, 2020 •

edited

Loading

zonkmachine commented Jun 13, 2020

zonkmachine commented Jun 13, 2020

zonkmachine commented Jun 13, 2020

Spekular commented Jun 13, 2020

zonkmachine commented Jun 13, 2020

zonkmachine commented Jun 13, 2020 •

edited

Loading

PhysSong commented Jun 25, 2020 •

edited

Loading

PhysSong commented Jul 18, 2020

zonkmachine commented Aug 13, 2020

zonkmachine commented Aug 13, 2020

zonkmachine commented Aug 16, 2020

Spekular commented Aug 16, 2020

IanCaio left a comment

IanCaio Aug 17, 2020

IanCaio Aug 17, 2020

PhysSong Aug 17, 2020

zonkmachine Sep 4, 2020

IanCaio Sep 4, 2020

zonkmachine Sep 5, 2020

PhysSong commented Aug 17, 2020

IanCaio commented Aug 17, 2020

zonkmachine commented Aug 17, 2020

zonkmachine commented Sep 6, 2020

Spekular left a comment

IanCaio left a comment

zonkmachine commented Oct 18, 2020

Feature: Glue notes in Piano-Roll #5482

Feature: Glue notes in Piano-Roll #5482

Conversation

zonkmachine commented May 2, 2020 • edited Loading

LmmsBot commented May 2, 2020 • edited Loading

Linux

Windows

zonkmachine commented May 26, 2020 • edited Loading

zonkmachine commented Jun 12, 2020

Spekular commented Jun 13, 2020 • edited Loading

zonkmachine commented Jun 13, 2020

zonkmachine commented Jun 13, 2020

zonkmachine commented Jun 13, 2020

Spekular commented Jun 13, 2020

zonkmachine commented Jun 13, 2020

zonkmachine commented Jun 13, 2020 • edited Loading

PhysSong commented Jun 25, 2020 • edited Loading

PhysSong commented Jul 18, 2020

zonkmachine commented Aug 13, 2020

zonkmachine commented Aug 13, 2020

zonkmachine commented Aug 16, 2020

Spekular commented Aug 16, 2020

IanCaio left a comment

Choose a reason for hiding this comment

IanCaio Aug 17, 2020

Choose a reason for hiding this comment

IanCaio Aug 17, 2020

Choose a reason for hiding this comment

PhysSong Aug 17, 2020

Choose a reason for hiding this comment

zonkmachine Sep 4, 2020

Choose a reason for hiding this comment

IanCaio Sep 4, 2020

Choose a reason for hiding this comment

zonkmachine Sep 5, 2020

Choose a reason for hiding this comment

PhysSong commented Aug 17, 2020

IanCaio commented Aug 17, 2020

zonkmachine commented Aug 17, 2020

zonkmachine commented Sep 6, 2020

Spekular left a comment

Choose a reason for hiding this comment

IanCaio left a comment

Choose a reason for hiding this comment

zonkmachine commented Oct 18, 2020

zonkmachine commented May 2, 2020 •

edited

Loading

LmmsBot commented May 2, 2020 •

edited

Loading

zonkmachine commented May 26, 2020 •

edited

Loading

Spekular commented Jun 13, 2020 •

edited

Loading

zonkmachine commented Jun 13, 2020 •

edited

Loading

PhysSong commented Jun 25, 2020 •

edited

Loading