Skip to content
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

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

Closed
wants to merge 6 commits into from
Closed

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

wants to merge 6 commits into from

Conversation

rghvdberg
Copy link
Contributor

When moving (dragging) notes, the notes snap to the grid lines.
Same for changing lenght (without shift or alt), note lenghts snap to the grid.

@Spekular
Copy link
Member

Spekular commented May 14, 2018 via email

@rghvdberg
Copy link
Contributor Author

Any other daw let's you drag notes to the grid lines. Current behaviour is imho very unexpected.

@SecondFlight
Copy link
Member

SecondFlight commented May 15, 2018

Are you sure about that @rghvdberg?

@Umcaruje can confirm this in Bitwig as well.

Here's my thinking behind this:
If you make some riff or grace note that uses faster notes than the normal grid division and you want to move it around, you have to go back to the timescale you were using previously to do that. Now, I haven't compiled your branch yet and you didn't put much detail in the PR, but based on your code it looks like it only snaps the first note in a group of notes. Correct me if I'm wrong. That could frustrate the end user in some cases - for example, what if I want to move this over by a quarter note:

image

Currently, I just move it over four 16ths, or switch the snap to quarter if I'm so inclined. With the proposed changes, the only viable option would be to use alt. I used alt to make the lead-in note, so I can't just switch it to a smaller timebase.

Of course, all of this would be nullified if the PR does something considerably different from what I'm saying here, but I can attest that LMMS currently acts very similarly to other DAWs in this regard.

Edit: I just noticed the issue you posted a few days ago. I should have disputed that instead of waiting until this, sorry about that!

@rghvdberg
Copy link
Contributor Author

rghvdberg commented May 15, 2018 via email

@SecondFlight
Copy link
Member

That's actually FL Studio. As I understand it, that behavior is standard across DAWs.

I'll grab your code later when I get the time and make sure I'm understanding the change.

@rghvdberg
Copy link
Contributor Author

rghvdberg commented May 15, 2018 via email

@Spekular
Copy link
Member

Here's the PR I was thinking of, with discussion on why it's valuable to keep fine adjustments when moving at a higher Q: #4043

Here's an example in the piano roll of some recorded chords.

First, the chords are on the offbeat:
image

But perhaps I realize that I want them on the 1/2/3/4, or the recording was delayed by 1/8th for some reason. With the current behavior, I simply move them over two 16ths (though not visible in the screenshot, my Q is at 1/16th here):
image

The chords keep their inconsistencies and slight delays. If your suggestion were implemented, they would now all hit exactly at the right time, with no delay between notes in a chord:
image

Note that to get the notes as they are in the third screenshot, I simply hit the quantize button. So it's currently easy to achieve what I want (shifting the notes by a certain offset) and what you want (snapping the notes to certain positions). If moving notes also snapped them, it would be far more difficult to do what I want.

I do not think this PR should be merged. I've explained why the current behavior is desired and SecondFlight has shown that it is common in other DAWs. That said, I do want to apologize for not bringing this up earlier, before you opened the PR.

@rghvdberg
Copy link
Contributor Author

rghvdberg commented May 15, 2018

I guess I'm used to this behaviour

Ardour
ardour grid

Qtractor
qtractor grid

Drumroll .... LMMS (my PR)
new grid lmms

forgot this: the distance between the notes stays the same
again new grid

@rghvdberg
Copy link
Contributor Author

improved snap
Improved it a bit.
The snap now depends on which note you started dragging.

@SecondFlight
Copy link
Member

SecondFlight commented May 15, 2018

What if we bind that to a modifier key? Alt is taken, and I think shift is too, but we could put that on ctrl + shift or ctrl + alt or something like that. I would be open to this, and it's something I could even see myself using on occasion.

I'm still very much against this as the default, though I will say that your most recent modification makes it considerably more usable in the general case IMO.

Another option would be to make a toggle in the settings, something like "quantize dragged notes" (there's probably a better way to word that)

@SecondFlight
Copy link
Member

Also, and I forgot to mention this before, you can quickly get a single note or the first of a group of notes to snap to the grid by dragging it all the way to the left and then back again. This might not be practical for really long patterns, but it's an intuitive way to go about it based on the interface. This doesn't beat a shortcut key or extra setting though IMO, especially with the new change you added.

@rghvdberg
Copy link
Contributor Author

I've been pondering about a modifier key too. And maybe have a settings switch to set the default behaviour.
Better yet imho would be an extra move/nudge tool button.
The default being the new drag method and the old method the move/nudge tool.

@Spekular
Copy link
Member

Spekular commented May 16, 2018 via email

@rghvdberg
Copy link
Contributor Author

I've given it some thought and I think a modifier key and maybe a toggle in settings would be the best option.
I was trying to add another button in the piano roll editor but then you'd have 2 'places' for moving notes and that can only lead to confusion.

@SecondFlight
Copy link
Member

I agree, the UX on that is not ideal.

I don't see any reason to have both a modifier key and a settings toggle. I think that would make it harder to find. Someone could turn on the setting and reasonably be confused as to why nothing has changed, unless the setting is worded right, but even then I don't see a good reason to make the user go in there if it's already behind a modifier key.

@rghvdberg
Copy link
Contributor Author

rghvdberg commented May 22, 2018 via email

@PhysSong
Copy link
Member

@rghvdberg Please use tabs for indentation. It's our coding convention.

@rghvdberg
Copy link
Contributor Author

rghvdberg commented May 22, 2018 via email

@rghvdberg
Copy link
Contributor Author

rghvdberg commented May 28, 2018

I've added a combo box.
defaults to the old behaviour (which I named nudge) ofc

nu_snap

@Spekular
Copy link
Member

Spekular commented May 28, 2018 via email

@SecondFlight
Copy link
Member

At some point in the future it may be useful to consolidate some of the odds and ends sorts of options somehow, but at this point I don't think the UI is super cluttered. This is a fantastic solution as far as I'm concerned!

Does it remember your choice between sessions? If that's not trivial then don't worry about it.

@rghvdberg
Copy link
Contributor Author

Never crossed my mind to keep it between sessions. I'll have a look how to implement that.

@SecondFlight
Copy link
Member

SecondFlight commented May 28, 2018

It would be a nice solution to the question of which one to put as the default. I'm not super optimistic about it since our settings require a restart to take effect. If you can find a nice way to keep a setting between sessions then we may as well be using that for all our other settings.

But I suppose that's for another PR :)

@rghvdberg
Copy link
Contributor Author

Do the other options (quantize, note lenght etc) persist between settings?
If not, it would be odd to have this setting persist and not the others.

@SecondFlight
Copy link
Member

Yeah, that's fair, they don't persist.

Does it default to the old behavior?

@rghvdberg
Copy link
Contributor Author

Yup. Nudge == old behaviour and the default.

@ghost
Copy link

ghost commented May 28, 2018

Are we talking about persisting in global config of lmms or in the individual project file? Or both?

My two cents is that this is definitely a worth a new issue.

@SecondFlight
Copy link
Member

@rghvdberg Alright, this can be merged as far as I'm concerned. Fantastic work! Aside from any potential bugs and cleanup of course.

@justnope I was talking about globally. I think in this case it doesn't make sense as @rghvdberg pointed out, but separate from that it would be great to have settings that don't require you to restart the software for a simple buffer change.

@PhysSong
Copy link
Member

@Spekular As the author of #4973, do you think this PR is still useful?

@Spekular
Copy link
Member

@PhysSong yes, in fact I think it's even more warranted now so we have consistent functionality between the editors.

@PhysSong
Copy link
Member

@rghvdberg Do you want to continue working on this, or let some of us take this over? @Spekular Are you interested in this?

@rghvdberg
Copy link
Contributor Author

Would be fun to work on it again. Gimme a bit of time to set things up again.

@PhysSong
Copy link
Member

Continued in #5848.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants