Enhanced snapping in song editor #4973
Conversation
|
Icon ideas/mockups for the "proportional snap" button. Two far left are the current Quantization and zoom icons.
If someone feels like doing a reaction vote: |
Spekular
changed the title
BaraMGB
added
needs code review
Spekular
changed the title
Spekular
changed the title
|
The mockup voting is for a pixmap at the position where the current "Q" is? What where your reasons to use the "Q" letter? |
|
@JohannesLorenz The Q is there because this dropdown sets the snapping size/quantization used when moving and resizing clips (like in piano roll), so I think it works fine. The icon that needs to be made is for the proportional snap button, which is currently using the "add automation" icon. |
|
Something that could be documented is the reason for the setInitialPos() function. The mouseMoveEvent() function is called continuously(?) on mouse move. The old code worked by getting the difference between the latest update and right now. It's possible to preserve offsets with this method, or snap to grid, but you CAN'T toggle between them. Once the pattern is snapped to grid, the latest position is quantized. This means all following positions are based on a quantized position, and as such the user's offset is permanently lost. setInitialPos() is called on mouse click, and saves:
Since you can't click again during a move/resize event (you're holding the mouse down to drag things around), calling setInitialPos() on a click saves the position before move. (Sidebar: can you middle or right click while moving and destroy this logic?). By saving the initial position, you always have access to the initial offset. This means you can toggle between snapping to grid and snapping in increments without losing information. All you have to do is:
Basically, I've rewritten a lot of the code to be relative to the initial position, rather than the latest position. One thing that could be considered a downside to this is that, currently, move and resize events stick to their initial offset the whole time. If you begin a move event by fine-adjusting the position of a clip and then release ctrl/alt, the clip will go back to snapping to the initial offset. If we consider this to be a problem, I believe there's a solution. When fine moving, we should call setInitialPos(), this saving the offset. This might need some additional changes, since it's possible that updating the initial positions this often could break other move code. We'd have to use "relative to latest position" code on fine adjustments, and "relative to initial position" on snapped. EDIT: I've since updated the PR to fix the final paragraph above. |
|
BaraMGB reported a bug with this on Discord. Current steps to reproduce are:
The clip should jump over towards the left.
|
|
@BaraMGB I believe I've fixed the bug you discovered now. |
|
This seems to work now. But I'm not sure about this button. It is quite misleading and I can not imagine a situation where a user needs this function. The snapping works like a charm, though. 👍 |
|
- Perhaps they want to keep snapping to one bar even when zoomed out.
- Maybe they're sequencing drums or similar and want to make sure they know
exactly what increment they're snapping to. With adaptive snap you have to
calculate the current snap level in your head vased on snap setting and
zoom.
I don't see what's misleading about it. The icon is a placeholder, if
that's what you mean.
…On Sun, Jun 2, 2019, 16:46 Steffen Baranowsky ***@***.***> wrote:
This seems to work now. But I'm not sure about this button. It is quite
misleading and I can not imagine a situation where a user needs this
function. The snapping works like a charm, though. 👍
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#4973?email_source=notifications&email_token=ACEBLGS3732N7NGRWASJG2DPYPMNTA5CNFSM4HNFSHSKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWXXGAQ#issuecomment-498037506>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACEBLGSS7TOWARCP5ZGS72LPYPMNTANCNFSM4HNFSHSA>
.
|
|
The code looks fine except the small issues. About the button: perhaps someone else could test it. I find it misleading, because if I set 1 bar, I expect that the TCO also shifts by 1 bar. |
|
Ah, so you want the adaptive snap to be disabled completely. I thought you wanted it to be permanently on. |
|
Please, can somebody look at the proportional snapping? @zonkmachine @tresf @PhysSong @JohannesLorenz |
PhysSong
added
needs testing
|
I will merge this in a few days if there are no objections or new reviews. |
|
@Spekular Hey, thank you for the PR! I've been wanting this feature for a long time now. If your TCO's edge does not match up with a snap line, you can use Shift+drag to make it match up. But, if you use Shift+drag to do that, and let go of the Shift without letting go of the mouse, it will revert to the normal behavior, reverting the changes made by holding Shift even after those changes are displayed on the screen. So if you want to match something up with a snap line and then resize it, you have to Shift+drag it, let go of your mouse button and Shift key, and then resize it normally. That may or may not be intentional, I'm not sure, but it may be confusing to people. It's a very minor thing though, and people may prefer either behavior. It probably isn't a problem to be honest, since you can achieve the same effect by just holding down Shift the entire time when doing that, I just thought I'd point it out just in case. Otherwise, everything seems to be working 100% perfectly. 👍 |
|
The shift behavior is intentional, as it allows you to alternate between
offset snap and grid snap without losing the offset. Otherwise pressing
shift once would lock the whole move to grid snap, leaving no way to go
back.
Thanks a bunch for the testing :)
…On Sat, Jul 20, 2019, 16:25 Lost Robot ***@***.***> wrote:
@Spekular <https://github.com/Spekular> Hey, thank you for the PR! I've
been wanting this feature for a long time now.
I tested it out and I have not really noticed any problems. I tested it
with messed up time signatures, groups of TCOs, individual TCOs,
everything. It all works perfectly, except for one small thing which might
not actually be a bug.
If your TCO's edge does not match up with a snap line, you can use
Shift+drag to make it match up. But, if you use Shift+drag to do that, and
let go of the Shift without letting go of the mouse, it will revert to the
normal behavior, reverting the changes made by holding Shift even after
those changes are displayed on the screen. So if you want to match
something up with a snap line and then resize it, you have to Shift+drag
it, let go of your mouse button and Shift key, and then resize it normally.
That may or may not be intentional, I'm not sure, but it may be confusing
to people. It's a very minor thing though, and people may prefer either
behavior.
Otherwise, everything seems to be working 100% perfectly. 👍
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4973?email_source=notifications&email_token=ACEBLGRS7OSN2X5OHD7EBBLQAMN6RA5CNFSM4HNFSHSKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2NPLUY#issuecomment-513471955>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACEBLGV55KBF2KUFHWGGUV3QAMN6RANCNFSM4HNFSHSA>
.
|
|
@douglasdgi Are you fine with the new shift behavior? |
|
@PhysSong Yes, it's just fine. I approve. |
@PhysSong now? |
|
Okay. Could you write a suitable commit message for this PR? The default message in |
|
A full length one, or the short title? A long one could probably be copied from the first post in this thread. As for a short one, I have trouble with those but here's some attempts
|
|
Thanks. Do you think the summary in the first post is okay for long one? |
|
Yes, I do. Just change "will close" to "closes" and remove everything after that line and it seems good to me. |
|
🎉 |
|
Will there be binaries for tests? |
|
Master branch should get nightly builds at some point in the future, considering that I haven't planned on building binaries for this. |
* New default behavior: Preserves offsets when moving clips, resizes in fixed increments. * Adds shift + drag: Snaps move start position (like current behavior) or end position (new), based on which is closest to the real position. When moving a selection, the grabbed clip snaps into position and the rest move relative to it. * Adds alt + drag: Allows fine adjustment of a clip's position or size, as an alternative to ctrl + drag. * Adds a Q dropdown in the song editor to allow finer or coarser snapping (8 bars to 1/16th bar) * Adds a proportional snap toggle. When enabled, snapping size/Q adjusts based on zoom, and a label appears showing the current snap size. This is disabled by default.
Will close #874.
Ready to merge.
Video Preview
(This shows a version where snapping modes are switched via toggle button. Since then I've switched to using modifier keys, but the snapping shown still works)