pianoroll: nudge/snap while dragging notes #5848

rghvdberg · 2020-12-13T20:02:13Z

Adds combobox for switching behavior of the snapping to grid when dragging.
Old behavior (nudge) is default.
Notice that snapping is relative to the dragged note and not the 1st note in the selected range.

It's essentially the same as #4357 but I lost that fork and just started over.

combobox for switching behavior old behavior (nudge) is default

LmmsBot · 2020-12-13T20:08:49Z

🤖 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.78%2Bgecea6b8-linux-x86_64.AppImage (build link)

Windows

Windows 32-bit: lmms-1.3.0-alpha.1.78%2Bgecea6b8cb-mingw-win32.exe (build link)
Windows 64-bit: lmms-1.3.0-alpha.1.78%2Bgecea6b8cb-mingw-win64.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://12492-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.78%2Bgecea6b8-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12492?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://12491-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.78%2Bgecea6b8cb-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12491?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://12494-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.78%2Bgecea6b8cb-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12494?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/eiybafr1o429xb5u/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/37638110"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/2bt2xo723lomgwqv/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/37638110"}]}, "commit_sha": "8c3c5fc8943286d649fb415ebff6f1e352b5a784"}

Spekular · 2020-12-14T10:25:35Z

Would you consider adding snapping to the end of the note as well? (Choosing whichever is closest to the unquantized position, as with shift drag in the song editor). It would be convenient for reversed samples and such, and shouldn't interfere much with the existing functionality in most situations.

I think this is worth adding regardless, but it's a nice-to-have if you're not opposed.

rghvdberg · 2020-12-14T11:35:21Z

Didn't think of that. Will do. [Email stuff trimmed - Spekular]

include/PianoRoll.h

src/gui/editors/PianoRoll.cpp

superpaik · 2021-01-15T11:08:54Z

Suggestion: to me it's a bit odd that when snap activated notes are snapping both from the start and the end of the note, and make it seem that the movement it's kind of random (though I see it is as intended). Maybe it would be better if snap has in mind the direction of the movement. If going left, snap with the start of the note. If going right, snap with the end of the note.
I don't know it is correct or not, but when in pianoroll inside beat+bass editor, "no duration" notes do not seem to follow the same behavior as "normal" notes.

rghvdberg · 2021-01-15T11:48:21Z

Snapping that depends on the direction of drag seems like a bad idea. What if I just want to move 1/4 note to the right? I'd first have to drag beyond that note and then drag left to finally land there.
We could use a modifier key to enable snap to end and the default being snap to start.
I don't know if there are any modifier keys left to implement in pianoroll though.

rghvdberg · 2021-01-15T11:49:49Z

I only tested (very shortly) in pianoroll in the song editor. I'll look at the bb editor case.

rghvdberg · 2021-01-17T11:05:47Z

found a bug :-(
when you drag a note over a second note which the same pitch, the second note is moved a bar ...

LMMS.Snap.bug.mp4

IanCaio

Just an initial review. As said in Discord, I think the bug that you found is being caused by calling noteUnderMouse repeatedly to get the note being dragged on the mouseMoveEvent method. When two notes are in the same position you might end up with the wrong note on the dragNotes call. m_draggedNote seems redundant when you have m_currentNote and I think you could get rid of it and use the latter instead, fixing the bug and also removing an unnecessary variable.

I wrote a few other comments, but before going further with the review I was thinking about suggesting a different approach on the logic that snaps the note to the grid. Why not using the mouse position, quantizing it and then just calculating the offset necessary to get the note from where it is to that quantized mouse position? It sounds to me like it would make the logic simpler (wouldn't require copying the note, quantizing it and moving it around) and it seems like it would work as intended. The only difference in behavior that I can think of is that the position of the mouse in relation to the grid would matter more than the position of the mouse against the initial position of the note. Would have to check in terms of UX if that behavior is annoying or not, but if it is it shouldn't be too complicated to add the offset against the initial position of the note to the math.

I can help you out writing this alternative approach if you like!

src/gui/editors/PianoRoll.cpp

include/PianoRoll.h

src/gui/editors/PianoRoll.cpp

include/PianoRoll.h

src/gui/editors/PianoRoll.cpp

superpaik · 2021-02-01T11:17:25Z

It woks good to me! Tested on regular and "no-duration" (beats) notes.
I think that using the mouse position instead the first note was a great idea!
Thanks!

To avoid problems with translations breaking the conditional, PianoRoll::changeSnapMode now uses the value of the ComboBox model, casted to the GridMode enum instead of the text.

A few code style issues were left, those are fixed in this commit.

Since the last enum value was commented out, there's an unnecessary comma left that was removed.

Spekular

The snapping code looks unnecessarily long to me, and reasonably it seems that the logic for snap vs nudge should be on the same level, rather than snap being calculated "inside" the nudge logic and overriding it.

src/gui/editors/PianoRoll.cpp

IanCaio · 2021-02-05T16:01:28Z

The snapping code looks unnecessarily long to me, and reasonably it seems that the logic for snap vs nudge should be on the same level, rather than snap being calculated "inside" the nudge logic and overriding it.

Ideally the whole dragNotes method could use some refactoring that would make it easier to have nudge and snap more explicitly separated. I'm not sure how I would do it without rewriting a lot of the method. Since it doesn't make it worse, I'd go with that approach for now and leave that method overhaul for when we are out of that tight refactoring schedule.

Separates Nudge logic and Snap logic more explicitly. Avoids recalculation of mouse conversion to time line ticks. Disables shift resizing for Snap mode. Removes unnecessary range checks. Fixes bug with note key boundary checks (which is present in master).

Veratil · 2021-02-06T02:45:17Z

src/gui/editors/PianoRoll.cpp

+							ke->modifiers() & Qt::AltModifier,
+							ke->modifiers() & Qt::ShiftModifier,
+							ke->modifiers() & Qt::ControlModifier


Since we repeat this function call a few times, can we at least fix this? Just pass the event or modifiers in and test in the function for what we need.

I'm not strongly for or against this change, but wouldn't that couple dragNotes more closely to (1) the Qt UI and (2) these specific modifiers? If for example the argument "alt" were renamed to "unquantized" (or inverted and renamed to "quantized") then the dragNotes function is independent of which shortcut is used to perform unquantized actions.

Dragging notes is inherently a UI thing, not a core thing.

I do like the idea of changing the names from ctrl alt and shift, and I would rather just pass in a modifiers value to the function.

Dragging notes is inherently a UI thing, not a core thing.

Sure, but at the moment this function contains a lot of logic that doesn't rely on the UI. The same quantization strategies are relevant regardless of how the move is input.

I think the renaming of the arguments and possibly compressing them in a single one seems to be the middle ground: We keep the method decoupled from the Qt event and make the modifiers more generic (allowing us to change them later).

Can I suggest that we do it separately though? As I mentioned on a previous comment, this method needs a bit of an overhaul. Just renaming the arguments would also require some thought because some modifiers are used for multiple actions depending on context. I think both the method refactoring and changing of arguments could be done in a single separate PR, they would both benefit on each other.

Can I suggest that we do it separately though?

Sure. 👍

Spekular

LGTM after Ian's latest changes.

IanCaio · 2021-03-04T19:22:39Z

Merging tomorrow if there are no objections. There's one conflict but it can be quickly fixed in the web editor before merging.

m_gridMode was not being initialized on the PianoRoll constructor. For that reason the first note drag without changing the GridMode would result in m_gridMode being used unintialized and neither the Snap nor Nudge mode being used. Here, the result of this was a note being moved unquantized. This fixes it by calling changeSnapMode() after setting up the snapModel on the constructor.

* pianoroll: nudge/snap while dragging notes combobox for switching behavior old behavior (nudge) is default * snap note start or note end to nearest grid line * fix member variable name * change default width of pianoroll * remove trailing white space * use mouse position instead of m_draggednote * Uses enum instead of text for GridMode selection To avoid problems with translations breaking the conditional, PianoRoll::changeSnapMode now uses the value of the ComboBox model, casted to the GridMode enum instead of the text. * Fixes last code style issues A few code style issues were left, those are fixed in this commit. * Removes forgotten comma on enum Since the last enum value was commented out, there's an unnecessary comma left that was removed. * Rewrites some of the grid logic Separates Nudge logic and Snap logic more explicitly. Avoids recalculation of mouse conversion to time line ticks. Disables shift resizing for Snap mode. Removes unnecessary range checks. Fixes bug with note key boundary checks (which is present in master). * Make noteOffset an int instead of TimePos Co-authored-by: Ian Caio <iancaio_dev@hotmail.com>

m_gridMode was not being initialized on the PianoRoll constructor. For that reason the first note drag without changing the GridMode would result in m_gridMode being used unintialized and neither the Snap nor Nudge mode being used. Here, the result of this was a note being moved unquantized. This fixes it by calling changeSnapMode() after setting up the snapModel on the constructor.

* pianoroll: nudge/snap while dragging notes combobox for switching behavior old behavior (nudge) is default * snap note start or note end to nearest grid line * fix member variable name * change default width of pianoroll * remove trailing white space * use mouse position instead of m_draggednote * Uses enum instead of text for GridMode selection To avoid problems with translations breaking the conditional, PianoRoll::changeSnapMode now uses the value of the ComboBox model, casted to the GridMode enum instead of the text. * Fixes last code style issues A few code style issues were left, those are fixed in this commit. * Removes forgotten comma on enum Since the last enum value was commented out, there's an unnecessary comma left that was removed. * Rewrites some of the grid logic Separates Nudge logic and Snap logic more explicitly. Avoids recalculation of mouse conversion to time line ticks. Disables shift resizing for Snap mode. Removes unnecessary range checks. Fixes bug with note key boundary checks (which is present in master). * Make noteOffset an int instead of TimePos Co-authored-by: Ian Caio <iancaio_dev@hotmail.com>

m_gridMode was not being initialized on the PianoRoll constructor. For that reason the first note drag without changing the GridMode would result in m_gridMode being used unintialized and neither the Snap nor Nudge mode being used. Here, the result of this was a note being moved unquantized. This fixes it by calling changeSnapMode() after setting up the snapModel on the constructor.

* pianoroll: nudge/snap while dragging notes combobox for switching behavior old behavior (nudge) is default * snap note start or note end to nearest grid line * fix member variable name * change default width of pianoroll * remove trailing white space * use mouse position instead of m_draggednote * Uses enum instead of text for GridMode selection To avoid problems with translations breaking the conditional, PianoRoll::changeSnapMode now uses the value of the ComboBox model, casted to the GridMode enum instead of the text. * Fixes last code style issues A few code style issues were left, those are fixed in this commit. * Removes forgotten comma on enum Since the last enum value was commented out, there's an unnecessary comma left that was removed. * Rewrites some of the grid logic Separates Nudge logic and Snap logic more explicitly. Avoids recalculation of mouse conversion to time line ticks. Disables shift resizing for Snap mode. Removes unnecessary range checks. Fixes bug with note key boundary checks (which is present in master). * Make noteOffset an int instead of TimePos Co-authored-by: Ian Caio <iancaio_dev@hotmail.com>

m_gridMode was not being initialized on the PianoRoll constructor. For that reason the first note drag without changing the GridMode would result in m_gridMode being used unintialized and neither the Snap nor Nudge mode being used. Here, the result of this was a note being moved unquantized. This fixes it by calling changeSnapMode() after setting up the snapModel on the constructor.

pianoroll: nudge/snap while dragging notes

0a782b7

combobox for switching behavior old behavior (nudge) is default

PhysSong mentioned this pull request Dec 13, 2020

piano roll editor : better snap to grid #4350 #4357

Closed

snap note start or note end to nearest grid line

740197f

PhysSong reviewed Dec 19, 2020

View reviewed changes

include/PianoRoll.h Outdated Show resolved Hide resolved

fix member variable name

627add5

PhysSong reviewed Jan 9, 2021

View reviewed changes

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

rghvdberg added 2 commits January 10, 2021 16:53

change default width of pianoroll

f194f19

remove trailing white space

e826493

PhysSong reviewed Jan 15, 2021

View reviewed changes

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

IanCaio requested changes Jan 26, 2021

View reviewed changes

use mouse position instead of m_draggednote

d946222

IanCaio added 3 commits February 4, 2021 19:21

Uses enum instead of text for GridMode selection

d27d047

To avoid problems with translations breaking the conditional, PianoRoll::changeSnapMode now uses the value of the ComboBox model, casted to the GridMode enum instead of the text.

Fixes last code style issues

b09da77

A few code style issues were left, those are fixed in this commit.

Removes forgotten comma on enum

e17cce8

Since the last enum value was commented out, there's an unnecessary comma left that was removed.

IanCaio approved these changes Feb 4, 2021

View reviewed changes

Spekular reviewed Feb 5, 2021

View reviewed changes

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

IanCaio added 2 commits February 5, 2021 17:21

Make noteOffset an int instead of TimePos

8c3c5fc

Veratil reviewed Feb 6, 2021

View reviewed changes

Spekular approved these changes Feb 14, 2021

View reviewed changes

IanCaio mentioned this pull request Mar 4, 2021

Implement Note Types #5902

Merged

Merge branch 'master' into master

9cc8eb5

IanCaio merged commit ea295c4 into LMMS:master Mar 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pianoroll: nudge/snap while dragging notes #5848

pianoroll: nudge/snap while dragging notes #5848

rghvdberg commented Dec 13, 2020

LmmsBot commented Dec 13, 2020 •

edited

Loading

Linux

Windows

Spekular commented Dec 14, 2020

rghvdberg commented Dec 14, 2020 via email •

edited by Spekular

Loading

superpaik commented Jan 15, 2021

rghvdberg commented Jan 15, 2021

rghvdberg commented Jan 15, 2021

rghvdberg commented Jan 17, 2021

IanCaio left a comment

superpaik commented Feb 1, 2021

Spekular left a comment

IanCaio commented Feb 5, 2021

Veratil Feb 6, 2021

Spekular Feb 14, 2021

Veratil Feb 14, 2021

Spekular Feb 15, 2021

IanCaio Feb 15, 2021

Veratil Feb 18, 2021

Spekular left a comment

IanCaio commented Mar 4, 2021

pianoroll: nudge/snap while dragging notes #5848

pianoroll: nudge/snap while dragging notes #5848

Conversation

rghvdberg commented Dec 13, 2020

LmmsBot commented Dec 13, 2020 • edited Loading

Linux

Windows

Spekular commented Dec 14, 2020

rghvdberg commented Dec 14, 2020 via email • edited by Spekular Loading

superpaik commented Jan 15, 2021

rghvdberg commented Jan 15, 2021

rghvdberg commented Jan 15, 2021

rghvdberg commented Jan 17, 2021

IanCaio left a comment

Choose a reason for hiding this comment

superpaik commented Feb 1, 2021

Spekular left a comment

Choose a reason for hiding this comment

IanCaio commented Feb 5, 2021

Veratil Feb 6, 2021

Choose a reason for hiding this comment

Spekular Feb 14, 2021

Choose a reason for hiding this comment

Veratil Feb 14, 2021

Choose a reason for hiding this comment

Spekular Feb 15, 2021

Choose a reason for hiding this comment

IanCaio Feb 15, 2021

Choose a reason for hiding this comment

Veratil Feb 18, 2021

Choose a reason for hiding this comment

Spekular left a comment

Choose a reason for hiding this comment

IanCaio commented Mar 4, 2021

LmmsBot commented Dec 13, 2020 •

edited

Loading

rghvdberg commented Dec 14, 2020 via email •

edited by Spekular

Loading