Add 2 new BBTrack features #5951
Conversation
Allows the user to assign all tracks that are contained in a BBTrack to a FX channel.
Changes the menu title for something shorter and uses tr so it can be translated.
🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩
Windows
Linux
macOS🤖{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://13563-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.107%2Bg1bc96b2ff-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13563?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://13562-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.107%2Bg1bc96b2ff-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13562?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/jv6qqinkjlgh5u0l/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38754132"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/x5jgejuk72bteerc/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38754132"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://13565-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.107%2Bg1bc96b2-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13565?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://13561-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.107%2Bg1bc96b2ff-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13561?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "8aa153725c7cfcccc2f1c4611d604007b301dae7"} |
Adds a feature that converts BBTracks to regular tracks on the song editor. This can be accessed by selecting the menu arrow from the "Add BBTrack" button in the song editor.
IanCaio
changed the title
|
Updated PR with 1 new feature. I figured that they are somewhat related (both affect BBTracks) and that they will be easier to rebase as a single PR once the refactoring is finished. |
|
Just a reminder: I found an issue, where I think automatable model IDs are being reused. I think that's due to the fact I'm using I'm going to fix it later, could either use the |
|
What if you only wanted to convert one BB track to song editor? I know this would create lots of duplicate instrument tracks if this action is repeated... But still, the current implementation creates multiple TCOs on top of each other in the same track if an instrument is used in multiple BB tracks in parallell. I don't know what's worse, but at least having duplicate instruments are more obvious than duplicate TCOs (oh if only we had grouping/layering of tracks!) Maybe we could have both features? Convert this and convert all. And put them in the gear menu of the track? |
That's possible and probably simple to add: The gear menu could just call I initially thought about making it per BBTrack, but I changed to affect all BBTracks exactly because of the track duplication issue. I even thought of identifying the tracks that were originated from a BB but that adds extra complexity that I think is unworthy for a feature that would be there just to make do while we don't have linked patterns and the note types (when we wouldn't need BB anymore). Adding both alternatives of converting all and converting a single BBTrack sounds like a good middle ground though, I'll just wait until I fix this TCO copying issue and I can include it. |
Now the user can either convert all BBTracks or convert a single one to the song editor. - Adds non-const method for returning BBTrack pointer from the BBTrackView (necessary for getting the index of the BBTrack on the TrackOperationsWidget method). - Adds parameters to the Song::convertBBtoSE method, so it can either convert all BBTracks or a single one. - Changes the logic to convert a single BBTrack when the method is called with the arguments.
|
@allejok96 I just implemented the single BBTrack conversion on the Gear menu. Now user can both convert all or convert a single one. As for the bug I found earlier, apparently this is not related to this PR (or possibly not a bug at all): When we clone a track that has effects whose parameters have assigned IDs LMMS triggers some messages like Since converting BBTracks to Song editor involves cloning the tracks contained in the BBTrack, this kind of situation can happen. |
|
Good job. Think it would be better if it skipped adding empty TCOs |
|
If/when this becomes the basis for an upgrade routine that removes BB tracks, I think we should remember that even empty tracks can contain user data. If there are tracks in the shared BB trackset that aren't actually used, it'd be tempting to remove them in the conversion. However, that would risk deleting an instrument that the user has configured but not yet used (sample with effects on it, synth with a specific sound designed, etc.). |
Thanks! That's true, I think the way the BB editor works, even tracks with no step notes have an empty TCO so they occupy the right position, haven't thought of skipping them. Should be as easy as adding
For sure, tracks should be kept even if not used. I'll just check for the empty TCOs, as those don't hold any information afaik. |
- Fixes the doxygen comment. - Makes it so the pattern of the tracks inside the BBTrack are only copied if they are not empty.
|
Done! Empty patterns are now skipped during conversion. |
Fixes conflicts from file splitting.
|
Conflicts from file splitting also fixed on this one! |
PhysSong
force-pushed
the
feature/bbAssignAllToFX
branch
from
fb89bc7 to
7891774
Compare
|
The 1st feature - assigning all tracks in a BB Track to a FX channel - seems to work fine. I didn't see any issues while testing it. But for the 2nd feature - converting BB Tracks to regular tracks - it looks like it doesn't convert automation tracks. And unfortunately, the zero length notes from the BB tracks aren't displayed in the Song Editor clips, so all the clips are grayed out. Fixing that is probably out of the scope of this PR though. |
I was going to upload the requested changes tonight, but I noticed the feature of converting BBTracks to the song editor is broken (it copies the tracks, but fails to copy the clips). That seems to be the case even before I made your changes, so I need to figure out why. Could it be you tested a build from an earlier commit? |
- Also fixes the way enums were accessed in a method.
|
After all there was no bug, it was just me being dumb and not adding BB clips on the song editor before converting, so the converted tracks would have no pattern clips. My bad 😅 Only thing I found was that on this line the Track type enum was being accessed without specifying the enum name (seemed to be working, but I changed it to use the full enum name if ((*it)->type() == Track::InstrumentTrack || (*it)->type() == Track::SampleTrack)Also applied @messmerd review suggestions! |
- Had forgotten one small change from the review request on the last commit
|
@IanCaio Maybe the Pattern Editior should be muted after the patterns have been converted to tracks in the Song Editor? I see no use case for me where I would not mute the Pattern Editor manually after a conversion. |
|
Automation tracks don't seem to convert from Pattern Track at all currently. |
That can be done, I'm just trying to figure out the details of how, forgot lots of methods so I'm having to hunt the code base to find what I need. I have this so far: void Song::muteAllPatternTracks()
{
// Get a list of all tracks
const TrackList& tracks = this->tracks();
// Go through them and mute the pattern tracks only
for (Track* t : tracks)
{
if (t->type() == Track::TrackTypes::PatternTrack)
{
t->getMutedModel()->setValue(false);
t->dataChanged();
}
}
}But it seems to be cycling through more tracks than there is in the song editor (maybe it's going through the global automation track and the tracks inside the pattern editor as well?) and it's also not muting the pattern tracks. I'm not sure the right way to mute it is to set the model manually like
Yeah, for now it only converts sample tracks and midi tracks. I never used automation clips inside the BBEditor so I forgot about it. I'll try to change it so it copies them too. |
OK. I think it's better to get it merged and then we (and with 'we' I mean you) can have a look at it at a later point. ;) |
That one might be as simple as changing this line: if (track->type() == Track::TrackTypes::InstrumentTrack || track->type() == Track::TrackTypes::SampleTrack)To this: if (track->type() == Track::TrackTypes::InstrumentTrack || track->type() == Track::TrackTypes::SampleTrack || track->type() == Track::TrackTypes::AutomationTrack)I'd just have to test it to make sure. I was just trying to figure out that mute method, it doesn't work and I'm not sure why. If anyone has a clue, should be something simple, I just been a bit away from coding for a while so I forgot a lot about the codebase. |
- After converting pattern tracks, they are automatically muted - TODO: Maybe it's a good idea to add a mute method to the Track class, so we don't have to set the model manually everytime we want to mute throughout the codebase
Are you sure it isn't
if (track->type() == Track::TrackTypes::InstrumentTrack || track->type() == Track::TrackTypes::SampleTrack || track->type() == Track::TrackTypes::AutomationTrack)
Tested. Works like a charm with automation hooked up correctly to the converted tracks. |
Thanks! That was exactly the issue, I had changed it and made a commit yesterday but I ran into trouble when I merged master. After merging master, when I try to run error: submodule git dir '/home/lmms/.git/modules/plugins/Sid/resid/resid' is inside git dir '/home/lmms/.git/modules/plugins/Sid/resid'
fatal: refusing to create/use '/home/lmms/.git/modules/plugins/Sid/resid/resid' in another submodule's git dir
Failed to clone 'plugins/Sid/resid/resid'. Retry scheduled
error: submodule git dir '/home/lmms/.git/modules/plugins/Sid/resid/resid' is inside git dir '/home/lmms/.git/modules/plugins/Sid/resid'
fatal: refusing to create/use '/home/lmms/.git/modules/plugins/Sid/resid/resid' in another submodule's git dir
Failed to clone 'plugins/Sid/resid/resid' a second time, abortingSo now I'm trying to figure that out 😅
Awesome! Then I'll change it on the next commit as well 😄 |
|
To update past the Sid module changes you do the following. Remove the directories: Then you run the update. |
- TrackTypes was updated to Type, so it had to be updated on the code after merging master
- Automation tracks added on the pattern editor are now converted as well
That worked, thanks a lot! I fixed a small mistake with the types name that I noticed after merging master and changed it so it'll convert automation tracks as well. Seems to be working fine, one behavior that people should be aware of is that if they have a BB clip that is smaller than the BB track length, it will ignore that clip when converting. I.e.: if inside the BB editor they have a instrument patterns being 1 beat long and a automation track that is 4 beats long, the BB track length is 4 beats. If you then add a BB clip on the song editor that is 1 or 2 beats long, it will be smaller than the BB track length and will be ignored during conversion. When you click on the song editor to add BB clips it seems to automatically add clips with the minimum length so that will only happen if someone manually cuts the size of the BB clip down and the behavior is kind of logical (only converting clips with the minimum size of a BB clip), just thought I should mention it. I believe all the changes are done! |
OK with this behavior. Is implementing exporting these shorter tracks depending on instrument tracks not being resizable? |
I've to think, right now this is how the number of clips copied is calculated: auto pos = patternclip->startPosition();
auto patternTcoLen = patternclip->length().getTicks();
auto patternTrackLen = patternStore->lengthOfPattern(i) * TimePos::ticksPerBar();
auto numOfCopies = patternTcoLen / patternTrackLen;Meaning it simply get the length of the BB TCO, divides by the length of the BB pattern, and that's how many times it will copy the clips from each instrument inside the pattern. The problem is that if the user decides to use an automation that is longer than the beat length of the instruments on the pattern (say they are 1 beat long and the user adds an automation 4 beats long, who the hell knows why someone would do that but..) then the pattern length is the highest length, hence the automation length. Then it will only copy the clips on the smallest round number of that division. This behaves a bit differently than the song editor because it will still play a BB TCO that is smaller than that pattern length, so I might actually need to change it :/ Maybe if instead of dividing the auto pos = patternclip->startPosition();
auto clipLen = src->length().getTicks();
auto patternClipLen = patternclip->length().getTicks();
auto numOfCopies = clipLen / patternClipLen;Still have to test that though EDIT: |
- When the length of BBTCOs isn't a perfect multiple of the BB pattern size, now we paste clips if they fit the remainder of the division to better match the behavior of the Song Editor.
|
I think I fixed the pasting behavior for when the pattern clip isn't a perfect multiple of the pattern length, by checking if the current copied clip would fit the remainder: auto pos = patternclip->startPosition();
auto clipLen = src->length().getTicks();
auto patternClipLen = patternclip->length().getTicks();
auto patternTrackLen = patternStore->lengthOfPattern(i) * TimePos::ticksPerBar();
auto numOfCopies = patternClipLen / patternTrackLen;
// If we have an pattern clip that isn't a perfect multiple of the pattern size
// we check if the clip that will be copied would fit the remainder of the pattern clip
if (patternClipLen % patternTrackLen >= clipLen)
{
++numOfCopies;
}If another pair of eyes can check that logic and test it further (seemed to work fine here) it would be appreciated! I did run into another issue though (down the rabbit hole..): When using automation tracks to automate the tracks inside the pattern editor, when they are converted to the song editor they will still be pointing to the parameters of the tracks inside the pattern editor and not the recently converted ones. I'm not sure how to fix that one though, cause afair automation clips are linked to parameters through an ID, and when the tracks are cloned to the song editor, their parameters IDs will be changed cause they'd conflict with the ones from the pattern editor. Right now I can't think of a solution for that (or at least a simple one). |
|
Well, I didn't come up with a solution for the automation track connection issue. I thought about deleting the tracks from the BBEditor before cloning them to the Song Editor, so the cloned track have the connection IDs available, but so far I didn't manage to make it work, plus it sounds a bit hacky. The issue itself is not a bug, simply a behavior that could be improved (the automation tracks are cloned perfectly, but if they are connected to tracks on the BBEditor, they'll still be connected to them, so the user will have to manually connect them to the parameters of the cloned track), so personally I think this can still be reviewed and merged. The behavior can be improved in the future if we can figure out a way to do so. If you all agree, then this is ready for a final review. |
Feature 1:
Allows assigning all tracks in a BBTrack to a FX channel (or to a new one). It works the same way as the regular "Assign to FX channel" but affects all tracks inside the BBContainer instead.
Feature 2:
Allows the user to convert the BBTracks to regular tracks in the Song Editor. That means that every track in the BBContainer will be cloned to the Song Editor and appropriate TCOs will be created in the positions where we had BBTCOs for each BBTrack. The existing BBTracks are not deleted.
The CSS style for QToolButton with menus was copied from #5946 to avoid conflicts and keep consistency.