Adds feature to merge Instrument Track patterns #5700

IanCaio · 2020-10-05T00:19:29Z

This PR implements a new action on the context menu of TrackContentObjectViews for merging note patterns. It's only applicable for InstrumentTrack's TCOs. In terms of changes on the code those were the main ones:

Create a getPattern method on the PatternView so we can add the notes to a pattern obtained from a pattern view.
Adds a context menu action called Merge and the appropriate context menu entry if the selection can be merged.
Create two methods on TrackContentObjectView: canMergeSelection and mergeTCOs. The first check if a vector of TCOVs can be merged (the conditions: more than one TCO being selected, they all belonging to an Instrument Track and they all belonging to the same track). The second does the actual merging, by creating a new TCO and adding all the notes to it while removing the merged TCOs.
Small note: There's a TODO: comment regarding a statement that should be removed once Fixes createTCO method on some classes #5699 is merged.

There are a few warnings that might be unnecessary (they cover some situations that should not normally happen). A placeholder icon was created for the action, which could be later substituted by a better one.

This branch aims to create a feature for merging Instrument Track's patterns in a single pattern. Current changes: - Adds a getPattern method to PatternView. - Adds a context menu action called "Merge". - Inside TrackContentObjectView::contextMenuEvent, we check if the selected TCOs can be merged and store the answer in a boolean called canMergeTCOs. The conditions to merge TCOs are: Having multiple TCOs selected; All TCOs belonging to a InstrumentTrack; All TCOs belonging to the same track. - If canMergeTCOs is true, add an action on the context menu for merging the selection. - Creates a method called TrackContentObjectView::mergeTCOs( tcovs ). That method first finds the earliest MidiTime from all selected TCOs and creates a pattern on that position. Then, notes are added to that pattern and repositioned depending on the relative position from the current TCOV being merged in relation to the new pattern. Some warnings were added to this method in case dynamic_casts failed, though they shouldn't. After all notes being added and positioned, newPattern->updateLength() and newPattern->dataChanged() are called. Known Bugs: - Some added notes don't play until you make a change (i.e.: move them) in the PianoRoll. To Do: - Remove QDebug include on Track.cpp. - Remove some debugging warnings. - Change the PixMap for the Merge Selection action on the context menu. - Fix bugs.

This commit fixes a previous bug where some notes wouldn't play after a merge operation happened. Apparently the root cause was Pattern->rearrangeAllNotes() not being called. It seems to be crucial that the notes are arranged in the right order for them to play properly. Removes QDebug header and includes Note.h.

This commit reorganizes the code a little bit to make it better suited for review. The code that checked whether a selection can be merged was extracted to a separate method called TrackContentObjectView::canMergeSelection(). Two conditionals inside that code were merged into a single one, since they both resulted in the same action. Also updated a comment next to a line of code that is probably going to be removed after the PR LMMS#5699 is merged (it's currently necessary because of a bug that is fixed on that PR).

LmmsBot · 2020-10-05T00:24:09Z

🤖 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.3.0-alpha.1.65%2Bg65346d7-linux-x86_64.AppImage (build link)

Windows

Windows 64-bit: lmms-1.3.0-alpha.1.65%2Bg65346d700-mingw-win64.exe (build link)
Windows 32-bit: lmms-1.3.0-alpha.1.65%2Bg65346d700-mingw-win32.exe (build link)
Windows 32-bit: lmms-1.3.0-alpha-msvc2017-win32.exe (build link)
Windows 64-bit: lmms-1.3.0-alpha-msvc2017-win64.exe (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://11485-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.65%2Bg65346d7-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11485?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://11488-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.65%2Bg65346d700-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11488?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://11486-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.65%2Bg65346d700-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11486?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/2y8s81wi95vhlwdn/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36757377"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/lg711cavwv87ef5m/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36757377"}]}, "commit_sha": "1c612163ae20f728635f0b22839f7c214c136f66"}

superpaik · 2020-10-09T16:05:50Z

[Note: this is my first review, so let me know if you need more detail]

The averall functionality works fine with me. Some issues, though:

when you merge two patterns and one is empty, if it's the first the empty one, it merge the two of them, but if the empty is the second, it doesn't merge. I don't know if this behaviour is wanted or not, but seems odd to me.
the functionality of undo (ctrl-z) is weird. If you merge two patterns, you need two undos to undo the merge. If you merge the patterns again (after the undo), then you need three undos. If you do it again, then four, and so on. If you merge more than two, say three, and the pattern to be merge are created in order (e.g. first on beat 5, then on beat 9, another on beat 13) and merge them, it works well (aside from the double undo), but if you create them and then move the pattern from 13 to 1 and merge and undo, the behaviour is weird (first the notes on the pattern on beat1 desappear, and then the pattern) ending with only patterns on beats 5 and 9.

IanCaio · 2020-10-10T02:45:39Z

Thanks for testing and giving feedback!

when you merge two patterns and one is empty, if it's the first the empty one, it merge the two of them, but if the empty is the second, it doesn't merge. I don't know if this behaviour is wanted or not, but seems odd to me.

That is expected because the clip's size is recalculated so it is the shorted possible on the song editor (that also happens on the piano roll, if you delete notes from the last bar the clip will be shortened to have the fewest bars it can). So when you merge an empty clip with a non-empty one on that order it doesn't get shortened, because there are notes at the end of the clip. But if you merge a non-empty clip with an empty one the empty bars at the end are ignored and the clip is shortened to be the smallest size possible. They are still being merged though.

the functionality of undo (ctrl-z) is weird. If you merge two patterns, you need two undos to undo the merge. If you merge the patterns again (after the undo), then you need three undos. If you do it again, then four, and so on. If you merge more than two, say three, and the pattern to be merge are created in order (e.g. first on beat 5, then on beat 9, another on beat 13) and merge them, it works well (aside from the double undo), but if you create them and then move the pattern from 13 to 1 and merge and undo, the behaviour is weird (first the notes on the pattern on beat1 desappear, and then the pattern) ending with only patterns on beats 5 and 9.

Oh yeah, I actually didn't write a history system for merging, so the undo behavior you're seeing is being caused by some calls that are made inside the merge operation (createTCO(), tcov->remove() and I guess addNotes()). Merging works this way:

A new empty clip is created
There's a loop going through each merged clip, writing their notes on the new clip and deleting them

When you merge 2 clips and hit CTRL+Z, it first undo the clips being removed and then the new empty clip being created. You can confirm it by hitting CTRL+Z only once and then moving the clip: you'll see the two clips you had before below it.

I'm not sure it's worth it trying to fix it, specially because the history system is currently in a very unstable state already (I'd advise against using it at all and instead constantly saving the project as a new version) and is among the plans of things to be changed in the future. But I'll take a better look at it!

Spekular · 2020-10-10T09:02:48Z

You should get most of the way to a correct undo by calling addCheckpoint and saveJournallingState(false) at the start of the merge function, then restoreJournallingState at the end. I think it'll be a while until the undo/redo system is overhauled, and until it is we can't just let undo support devolve into a useless state.

IanCaio · 2020-10-10T12:34:08Z

You're right, I'll try to make it work here!

Fixes the history system for the merge operation by adding a journal checkpoint on the track where the merge is happening and disabling the journalling state of everything being handled during the operation, restoring the journalling state at the end.

IanCaio · 2020-10-10T19:34:59Z

The history system was fixed for the merge operation, undo and redo should work fine now. I've a small doubt whether it's a good idea to restore the journalling state of a TCO after removing it, when I disabled it before removing. For now, I'm restoring the state but I left a TODO comment to remind me of removing the method call if it's unnecessary.

superpaik · 2020-10-11T10:38:02Z

It works fine now, both undo as redo. I've tried with like 100 different patterns and it worked!

IanCaio · 2020-10-11T12:04:28Z

Awesome, glad it's working now! Thanks for testing man!

Fixes merge conflict on include/Track.h

After the bug fix from LMMS#5699, there's no need to call movePosition on a TCO that was created with a position on its constructor. The line that called movePosition was removed. Code style was also fixed.

IanCaio · 2020-11-27T04:10:52Z

Merged master and removed line that is now unnecessary since the bug fix from #5699 .
Also fixed the code style.

After recently merge that renames MidiTime to TimePos, the code was updated to use the new class name.

Fixes conflict caused by LMMS#5806, which splitted Track.cpp and Track.h

After removing a TCO I was calling restoreJournallingState on it, thinking it could be necessary for the journalling state be restored when the history Undo brings the TCO back. It doesn't make much sense that this would be necessary, so I'm removing it. After testing, it seems like restoring the TCO through Undo brings it back with the journalling state restored.

IanCaio · 2020-12-08T23:13:36Z

Fixed another conflict introduced by #5806 , changed the pointer declarations so they adhere to Type* var; code style and removed a line that I believe was unnecessary.

src/gui/TrackContentObjectView.cpp

Spekular

LGTM. Suggested a few potential changes to make things shorter, but I haven't tested them to ensure they're totally correct so there might be syntax issues and such.

src/gui/TrackContentObjectView.cpp

Change canMergeTCOs to use logical operators instead of the ternary operator. Avoids repeating the dynamic_cast unnecessarily on the canMergeSelection method.

Saves lots of lines on canMergeSelection by using a data set to store all the TCO owner tracks, so we can later check how many tracks we have. Uses std::min_element to find the earliest TCOV and get it's starting position.

The logic that returned we couldn't merge a single TCO was inside contextMenuEvent, when it suited more the canMergeSelection method. That was moved there and now even if canMergeSelection receives a list with just a single TCO it will return that it can't be merged.

src/gui/TrackContentObjectView.cpp

Calls restoreJournallingState respecting the push/pop order in relation to the saveJournallingState calls. Fix a comment.

Spekular · 2020-12-12T13:18:14Z

Merge^2?

IanCaio · 2020-12-12T13:47:53Z

Sure, I think it's good to go! And it has 2 approvals now 😄

IanCaio added 5 commits October 4, 2020 11:07

Merge branch 'master' into feature/mergeTCOs

d6cd457

Updates the icon for the merge action

97905b2

Merge branch 'master' into feature/mergeTCOs

0782b84

IanCaio added 3 commits October 28, 2020 10:18

Merge branch 'master' into feature/mergeTCOs

df3cfab

Fixes merge conflict on include/Track.h

Merge branch 'master' into feature/mergeTCOs

bd31b11

Removes method call not needed anymore

5312c45

After the bug fix from LMMS#5699, there's no need to call movePosition on a TCO that was created with a position on its constructor. The line that called movePosition was removed. Code style was also fixed.

JohannesLorenz added the needs code review A functional code review is currently required for this PR label Nov 29, 2020

IanCaio added 5 commits November 30, 2020 19:14

Merge branch 'master' into feature/mergeTCOs

192e522

Updates PR to use TimePos

6114b1d

After recently merge that renames MidiTime to TimePos, the code was updated to use the new class name.

Merge branch 'master' into feature/mergeTCOs

a3aecea

Fixes conflict caused by LMMS#5806, which splitted Track.cpp and Track.h

Fixes pointer declaration code style

2a368c0

PhysSong reviewed Dec 9, 2020

View reviewed changes

src/gui/TrackContentObjectView.cpp Outdated Show resolved Hide resolved

src/gui/TrackContentObjectView.cpp Outdated Show resolved Hide resolved

Spekular reviewed Dec 9, 2020

View reviewed changes

src/gui/TrackContentObjectView.cpp Outdated Show resolved Hide resolved

src/gui/TrackContentObjectView.cpp Outdated Show resolved Hide resolved

IanCaio added 2 commits December 9, 2020 13:31

Apply PhysSong suggestions

7eb4b28

Change canMergeTCOs to use logical operators instead of the ternary operator. Avoids repeating the dynamic_cast unnecessarily on the canMergeSelection method.

Applies Spekular's suggestions

0d3ea7d

Saves lots of lines on canMergeSelection by using a data set to store all the TCO owner tracks, so we can later check how many tracks we have. Uses std::min_element to find the earliest TCOV and get it's starting position.

PhysSong reviewed Dec 10, 2020

View reviewed changes

src/gui/TrackContentObjectView.cpp Outdated Show resolved Hide resolved

src/gui/TrackContentObjectView.cpp Outdated Show resolved Hide resolved

Changes order of restoreJournallingState calls

1c61216

Calls restoreJournallingState respecting the push/pop order in relation to the saveJournallingState calls. Fix a comment.

PhysSong approved these changes Dec 11, 2020

View reviewed changes

Spekular approved these changes Dec 12, 2020

View reviewed changes

IanCaio merged commit 3d7d005 into LMMS:master Dec 12, 2020

IanCaio added a commit to IanCaio/lmms that referenced this pull request Mar 28, 2021

Adds feature to merge Instrument Track patterns (LMMS#5700)

df92cff

devnexen pushed a commit to devnexen/lmms that referenced this pull request Apr 10, 2021

Adds feature to merge Instrument Track patterns (LMMS#5700)

ac21715

sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022

Adds feature to merge Instrument Track patterns (LMMS#5700)

848538f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds feature to merge Instrument Track patterns #5700

Adds feature to merge Instrument Track patterns #5700

IanCaio commented Oct 5, 2020

LmmsBot commented Oct 5, 2020 •

edited

Loading

Linux

Windows

superpaik commented Oct 9, 2020

IanCaio commented Oct 10, 2020

Spekular commented Oct 10, 2020 •

edited

Loading

IanCaio commented Oct 10, 2020

IanCaio commented Oct 10, 2020

superpaik commented Oct 11, 2020

IanCaio commented Oct 11, 2020

IanCaio commented Nov 27, 2020

IanCaio commented Dec 8, 2020

Spekular left a comment

Spekular commented Dec 12, 2020

IanCaio commented Dec 12, 2020

Adds feature to merge Instrument Track patterns #5700

Adds feature to merge Instrument Track patterns #5700

Conversation

IanCaio commented Oct 5, 2020

LmmsBot commented Oct 5, 2020 • edited Loading

Linux

Windows

superpaik commented Oct 9, 2020

IanCaio commented Oct 10, 2020

Spekular commented Oct 10, 2020 • edited Loading

IanCaio commented Oct 10, 2020

IanCaio commented Oct 10, 2020

superpaik commented Oct 11, 2020

IanCaio commented Oct 11, 2020

IanCaio commented Nov 27, 2020

IanCaio commented Dec 8, 2020

Spekular left a comment

Choose a reason for hiding this comment

Spekular commented Dec 12, 2020

IanCaio commented Dec 12, 2020

LmmsBot commented Oct 5, 2020 •

edited

Loading

Spekular commented Oct 10, 2020 •

edited

Loading