-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Knife Tool for Sample Clips (Again) #5524
Conversation
🤖 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
Windows
macOS🤖{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://12522-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.101%2Bg572b6a8-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12522?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://12525-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.101%2Bg572b6a8d5-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12525?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://12523-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.101%2Bg572b6a8d5-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12523?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/l47dfyj97k2o0epx/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/37736706"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/dwxivln8okg8jmnm/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/37736706"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://12521-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.101%2Bg572b6a8d5-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12521?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "4c604d86dc8cb3b49bdb590639b4a1aca413f333"} |
Tested, works great! |
Issues with move and resize should be fixed. While I was at it I ended up making box select behave in an (IMO) better and (IME) more standard way. Rather than toggling over to box select mode when control is held, it just starts a box selection when you control + click an empty space, and doesn't visually affect the selected edit mode. On release the edit mode is restored by re-triggering the selected edit button, which seems to be less buggy than the previous implementation based on my quick tests (no more getting stuck in select mode). |
Is the stuck Ctrl issue #5394? |
Yes. I can still reproduce that issue in 1.3-alpha.1, but not in the current state of this PR. |
On
There are two issues I noticed with this block:
To be honest I'm not sure why |
And as mentioned in Discord, there is a small bug I still didn't manage to track down, could use some help on this one. The steps to reproduce it:
It will fail to paste the TCO, pasting a infinitely small SampleTCO instead. If you move or resize the left-most SampleTCO before copying and pasting however it does work. Which makes me think that it has something to do with some update routine that is not being called and is causing the TCO data to be corrupt or something. I still didn't find out what it is, I only know that copying the right-most SampleTCO works fine. I also unresolved a change request from a one-liner |
Already messaged on Discord, but just for reference, apparently the cause of the bug mentioned above are the following lines: lmms/src/gui/TrackContentObjectView.cpp Lines 606 to 607 in 245160d
Even though the comment says lmms/src/gui/TrackContentObjectView.cpp Lines 966 to 970 in 245160d
Because |
- Fixes a typo where QWidget::mousePressEvent was being called inside mouseReleaseEvent. - Avoids unnecessarily disabling journalling on the Split action, since it doesn't require it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on Discord, I pushed the last review's suggestions to the PR, gave the code another broad look and it all looks fine.
Style guidelines kept aside, the code LGTM
@DomClark Do you still want to review this? |
* Initial PianoRoll razor feature * Restore PianoRoll edit mode after focusOut and in razor mode. * Show changes directly after cut. * Fix hanging note after adjusting vol/pan with razor action. * Extract the split action to a separate method This PR addresses some suggestions from a review, the most important ones being: - Extracting the note split action to a separate method, called Pattern::splitNotes - Removing getMouseTickPos method - Adding a variable that holds the current razor position and a method to update it (quantizing if CTRL is not pressed) - Using [this] to capture "this" on the lambda function instead of [=], since the latter doesn't work as intended from C++20 forward - Fixing some code style and adding comments * Removes an extra call to noteUnderMouse By removing "&& noteUnderMouse()" from the mousePressEvent conditional, we avoid an extra call to noteUnderMouse. The only difference in the behavior of the tool is that now clicking on a place that doesn't have a note will exit Razor mode. * Style change suggested by @russiankumar * Cancel razor action on SHIFT release. * Make razor cut-line (color) themable. * Add razor cut-line color to classic theme style.css * Rename razor to knife. * Change pixmap from razor to knife (from #5524) * Remove SHIFT behavior. * Change knife shortcut to SHIFT+K Co-authored-by: CYBERDEViL <[email protected]> Co-authored-by: Ian Caio <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. include/Track.h
and src/core/Track.cpp
only have formatting changes; can you revert these before merging?
* Revert format changes in Track.h * Revert formatting changes in Track.cpp
Reverted, and merging now! Huge thanks to everyone who reviewed and helped out, including of course BaraMGB for making the PR this is rooted in. |
Causes #5949 . |
* Initial PianoRoll razor feature * Restore PianoRoll edit mode after focusOut and in razor mode. * Show changes directly after cut. * Fix hanging note after adjusting vol/pan with razor action. * Extract the split action to a separate method This PR addresses some suggestions from a review, the most important ones being: - Extracting the note split action to a separate method, called Pattern::splitNotes - Removing getMouseTickPos method - Adding a variable that holds the current razor position and a method to update it (quantizing if CTRL is not pressed) - Using [this] to capture "this" on the lambda function instead of [=], since the latter doesn't work as intended from C++20 forward - Fixing some code style and adding comments * Removes an extra call to noteUnderMouse By removing "&& noteUnderMouse()" from the mousePressEvent conditional, we avoid an extra call to noteUnderMouse. The only difference in the behavior of the tool is that now clicking on a place that doesn't have a note will exit Razor mode. * Style change suggested by @russiankumar * Cancel razor action on SHIFT release. * Make razor cut-line (color) themable. * Add razor cut-line color to classic theme style.css * Rename razor to knife. * Change pixmap from razor to knife (from LMMS#5524) * Remove SHIFT behavior. * Change knife shortcut to SHIFT+K Co-authored-by: CYBERDEViL <[email protected]> Co-authored-by: Ian Caio <[email protected]>
* Rebase BaraMGB's Knife Co-authored-by: Steffen Baranowsky <[email protected]> * Draw marker * Refactoring and shift mode * Allow resizing * Add Icon * Fix stuck marker on RMB, remove unnecessary cast * Remove redundant line, more const * Fix * Review fixes * Only perform split logic for SampleTCO * Add unquantizedModHeld function * missed one * Don't use copy/paste * Don't use copy/paste * More git troubles * Fix undo * git dammit * Cleaner solution? * Set cursor, add copy assignment to SampleBuffer * Add TODO comment * Make it build * Fixes from review * Make splitTCO virtual * Make splitTCO more generic Co-authored-by: IanCaio <[email protected]> * Prevent resizing of MIDI clips in knife mode * Fix move/resize and rework box select via ctrl * Apply suggestions from code review. Co-authored-by: IanCaio <[email protected]> * Don't show inaccurate/useless/empty text float in knife mode * Addresses Github review - Fixes a typo where QWidget::mousePressEvent was being called inside mouseReleaseEvent. - Avoids unnecessarily disabling journalling on the Split action, since it doesn't require it. * Revert format changes in Track * Revert format changes in Track.h * Revert formatting changes in Track.cpp Co-authored-by: Hyunjin Song <[email protected]> Co-authored-by: IanCaio <[email protected]>
* Initial PianoRoll razor feature * Restore PianoRoll edit mode after focusOut and in razor mode. * Show changes directly after cut. * Fix hanging note after adjusting vol/pan with razor action. * Extract the split action to a separate method This PR addresses some suggestions from a review, the most important ones being: - Extracting the note split action to a separate method, called Pattern::splitNotes - Removing getMouseTickPos method - Adding a variable that holds the current razor position and a method to update it (quantizing if CTRL is not pressed) - Using [this] to capture "this" on the lambda function instead of [=], since the latter doesn't work as intended from C++20 forward - Fixing some code style and adding comments * Removes an extra call to noteUnderMouse By removing "&& noteUnderMouse()" from the mousePressEvent conditional, we avoid an extra call to noteUnderMouse. The only difference in the behavior of the tool is that now clicking on a place that doesn't have a note will exit Razor mode. * Style change suggested by @russiankumar * Cancel razor action on SHIFT release. * Make razor cut-line (color) themable. * Add razor cut-line color to classic theme style.css * Rename razor to knife. * Change pixmap from razor to knife (from LMMS#5524) * Remove SHIFT behavior. * Change knife shortcut to SHIFT+K Co-authored-by: CYBERDEViL <[email protected]> Co-authored-by: Ian Caio <[email protected]>
* Rebase BaraMGB's Knife Co-authored-by: Steffen Baranowsky <[email protected]> * Draw marker * Refactoring and shift mode * Allow resizing * Add Icon * Fix stuck marker on RMB, remove unnecessary cast * Remove redundant line, more const * Fix * Review fixes * Only perform split logic for SampleTCO * Add unquantizedModHeld function * missed one * Don't use copy/paste * Don't use copy/paste * More git troubles * Fix undo * git dammit * Cleaner solution? * Set cursor, add copy assignment to SampleBuffer * Add TODO comment * Make it build * Fixes from review * Make splitTCO virtual * Make splitTCO more generic Co-authored-by: IanCaio <[email protected]> * Prevent resizing of MIDI clips in knife mode * Fix move/resize and rework box select via ctrl * Apply suggestions from code review. Co-authored-by: IanCaio <[email protected]> * Don't show inaccurate/useless/empty text float in knife mode * Addresses Github review - Fixes a typo where QWidget::mousePressEvent was being called inside mouseReleaseEvent. - Avoids unnecessarily disabling journalling on the Split action, since it doesn't require it. * Revert format changes in Track * Revert format changes in Track.h * Revert formatting changes in Track.cpp Co-authored-by: Hyunjin Song <[email protected]> Co-authored-by: IanCaio <[email protected]>
* Initial PianoRoll razor feature * Restore PianoRoll edit mode after focusOut and in razor mode. * Show changes directly after cut. * Fix hanging note after adjusting vol/pan with razor action. * Extract the split action to a separate method This PR addresses some suggestions from a review, the most important ones being: - Extracting the note split action to a separate method, called Pattern::splitNotes - Removing getMouseTickPos method - Adding a variable that holds the current razor position and a method to update it (quantizing if CTRL is not pressed) - Using [this] to capture "this" on the lambda function instead of [=], since the latter doesn't work as intended from C++20 forward - Fixing some code style and adding comments * Removes an extra call to noteUnderMouse By removing "&& noteUnderMouse()" from the mousePressEvent conditional, we avoid an extra call to noteUnderMouse. The only difference in the behavior of the tool is that now clicking on a place that doesn't have a note will exit Razor mode. * Style change suggested by @russiankumar * Cancel razor action on SHIFT release. * Make razor cut-line (color) themable. * Add razor cut-line color to classic theme style.css * Rename razor to knife. * Change pixmap from razor to knife (from LMMS#5524) * Remove SHIFT behavior. * Change knife shortcut to SHIFT+K Co-authored-by: CYBERDEViL <[email protected]> Co-authored-by: Ian Caio <[email protected]>
* Rebase BaraMGB's Knife Co-authored-by: Steffen Baranowsky <[email protected]> * Draw marker * Refactoring and shift mode * Allow resizing * Add Icon * Fix stuck marker on RMB, remove unnecessary cast * Remove redundant line, more const * Fix * Review fixes * Only perform split logic for SampleTCO * Add unquantizedModHeld function * missed one * Don't use copy/paste * Don't use copy/paste * More git troubles * Fix undo * git dammit * Cleaner solution? * Set cursor, add copy assignment to SampleBuffer * Add TODO comment * Make it build * Fixes from review * Make splitTCO virtual * Make splitTCO more generic Co-authored-by: IanCaio <[email protected]> * Prevent resizing of MIDI clips in knife mode * Fix move/resize and rework box select via ctrl * Apply suggestions from code review. Co-authored-by: IanCaio <[email protected]> * Don't show inaccurate/useless/empty text float in knife mode * Addresses Github review - Fixes a typo where QWidget::mousePressEvent was being called inside mouseReleaseEvent. - Avoids unnecessarily disabling journalling on the Split action, since it doesn't require it. * Revert format changes in Track * Revert format changes in Track.h * Revert formatting changes in Track.cpp Co-authored-by: Hyunjin Song <[email protected]> Co-authored-by: IanCaio <[email protected]>
New version of #4998, with copy constructors for SampleBuffer and SampleTCO and a scalpel cursor when hovering over sample tracks in Knife/Split mode. Also adds copy assignment operator for SampleBuffer and a comment about this being missing in SampleTCO.h.
It's worth noting that this PR currently creates a new sample buffer with each cut, because apparently sample clips rely on having a unique buffer. This means that memory use will increase by the samplebuffer size with each cut. Cloning clips currently behaves the same way, so this isn't a regression, but I believe it eventually needs to be fixed as this leads to performance issues.
TODO: