Pitch Bending Directly in the Piano Roll

[regulus79]

This PR modifies how the Detuning tool works in the piano roll to let the user modify the automation curve right on the notes. This is actually fairly simple, as automation clips already contain the functions for dragging and removing points like in the Automation Editor. So essentially all that's being done is calculating the relative pos of the mouse to the nearest note, and setting the drag value in the automation clip to that position and key.

You can still access the old automation editor version by shift-click (should I change it to be double-click?)

pitchbending_demo.mp4

(Apologies for the non-standard theme; I didn't feel like recording another demo)

@messmerd asked if the code could be made general for any future note parameter besides detuning, in preparation for CLAP per-note parameter automation. I have refactored the relevant functions to accept an enum type for the kind of note parameter, although currently only detuning is supported.

Changes

• A new enum type was added, Note::ParameterType, which currently only has Detuning.
• New variables were added to handle the detuning in PianoRoll.h, to store the mouse states, the current notes being operated on, and the last changed tick.
• New functions were added to handle the detuning, for setting up the note vector, updating the drag position, and applying the drag position. Each of these functions take in a Note::ParameterType and use a switch statement to get the right automation clip in the note (currently only detuning is supported). Another function was also added to get the closest note to the mouse taking into account the shape of the automation curve.
• The drawing of the detuning info was changed to have circles around the automation nodes.
• The logic handling the detuning mode in the mouse event functions was updated.

[sakertooth]

@szeli1 I believe you had similar ideas, but it was for manually editing automation clips directly in the Song Editor?

[szeli1]

@szeli1 I believe you had similar ideas, but it was for manually editing automation clips directly in the Song Editor?

#7317
It is stuck because it doesn't improve the speed of automation. Shortcuts would fix this.

[bratpeki]

What if I slide pitch from one note onto a place where another note already exists, and then click on that other note? If you don't get what I mean, I'll record a video demo when I can.

[bratpeki]

Just tried it, it works on selections, so even if you overlap notes, there's no issue! And it supports making selections in the automation mode! Excellent! 😁

[bratpeki]

Mind the quality of the video, I overcompressed it. But I found a bug!

out.mp4

[szeli1]

I didn't test this PR, but I reviewed it.

[szeli1]

It would be better if getSelectedNotes() returned a reference to the selected notes. This way the code is more general and optimized. So essentially I'm asking you to implement something like this:

1. NoteVector m_selectedNotes
2. void setSelectionChanged(Note*) this will add or remove a note from m_selectedNotes, call this when a note is selected or unselected
3. getSelectedNotes() will return m_selectedNotes

I hope this will improve the code quality in the long run

[regulus79]

Are you asking me to change PianoRoll::getSelectedNotes or m_selectedParameterEditNotes? I have not touched PianoRoll::getSelectedNotes, so I'm not sure if that would be in the scope of this PR.

[szeli1]

I'm asking you to rename m_selectedParameterEditNotes to a generic name (I can't decide if it is already), make it always store all selected notes with a method called void setSelectionChanged(Note*), and rewrite getSelectedNotes() so it returns m_selectedParameterEditNotes.

I'm not sure if that would be in the scope of this PR.

I believe it will make future contributions easier, but it might be a bad idea to implement it here. Maybe you or I can refactor it in an other PR. The idea is to make it optimized (so it doesn't have to loop through all notes to return the selected ones) and make it (the selected notes) use a single vector.

[regulus79]

Oh you're right I realized m_selectedParameterEditNotes is always just the currently selected notes lol. So I could technically replace it with calls to getSelectedNotes. But I see why you want to refactor getSelectedNotes, it literally loops through all the notes to find the selected ones, which is probably not ideal. Caching them in a vector would definitely be more efficient (which is sort of what m_selectedParameterEditNotes does, but that's only for this feature).

[szeli1]

If you decide to refactor note selection, I believe you could just select manually that note here, so it is added to a variable called something like m_selectedNotes

[regulus79]

I should add a comment to clarify; the only purpose for selecting the note under the mouse is to keep track of it even if the user's cursor leaves the note while drawing the curve. It's a bit hacky but it works.

[regulus79]

But I found a bug!

Is the bug you are referring to the fact that after duplicating notes, changing one's detuning pattern changes the other? I believe that is also in master #5940 (but should be fixed in #7477)

[bratpeki]

@regulus79, could very well be! My exact steps were Ctrl+A, Shift-Dragging new notes into existence, and then attempting to change the pitch of those new notes, since they were selected afterwards. I'll try that on master and let you know if that's the same behavior.

[regulus79]

I have added a lot of comments and refactored the switch statement out into a separate function. Hopefully the PR is easier to read now.

[Rossmaxx]

Out of scope for this PR, i see a chance we can replace lmms::shared_object to std::shared_ptr. Afaik shared_object is used only for detuning. I was supposed to do it for #7677 but was fearing a change in functionality and a few build fails. I am not in a position to test any changes.

[bratpeki]

@regulus79, the named issue definitely exists on master. Adding it to my notes and opening a corresponding issue if there isn't one.

[bratpeki]

I see the MMB issue has been fixed. Saving and loading works. Please merge ASAP! 😎

[regulus79]

I just realized that currently the detuning is locked to whole semitone steps. I guess that's fine, but if velocity/panning/whatever per-note parameter automation is implemented, that might have to change.

[sakertooth]

Cool feature, thanks. The feature only allows detuning up until the max length of the note, which I think wasn't the same as when it was done through the editor. I also think its a problem because you can have release on a note, as to which the user may want to apply the detuning beyond the length of the note.

[sakertooth]

Also wanted to mention something: if you feel the need to add comments (excluding documentation comments) everywhere, it's always a good idea to see if you can improve your naming and simplify your interfaces. It will help developers understand what's going on better. Try not to use comments as a crutch.

[regulus79]

The feature only allows detuning up until the max length of the note, which I think wasn't the same as when it was done through the editor. I also think its a problem because you can have release on a note, as to which the user may want to apply the detuning beyond the length of the note.

Okay, so...... This is a bit more difficult to implement, since previously I was relying on the vertical distance form the mouse to the detuning curve to determine the closest note, but this would require also calculating the distance to the last node in the curve.

But, after a while, I actually did get it to work fairly well 4dbcf93. So now you can pitchbend past the end of the note!

On a separate note, I also changed the color and size of the automation nodes, and I added a TextFloat which appears for 4 seconds when you enter detuning mode to give instructions, in particular how you can shift-click to view the automation editor version.

[bratpeki]

This may not be the right place to discuss this, but I think the bending automation editor is somewhat bugged, since it places the notes off-center, for some reason.

I also think this is generally the "correct" way to pitch bend.

Is there any reason we're keeping the bend automation editor at all now that this is coming to LMMS?

[bratpeki]

I think the addition of an infobox is great, but I think the current text could cause confusion.

---

"Click on a note [...] to edit its detuning curves [...]" could incentivize that clicking begins the editing process, whereas that's "not true" and it actually places a point immediately.

So say someone reads that, clicks on a note to edit it, and immediately places a point they didn't want, so they have to undo.

---

Of course, the user has to get adjusted to the fact that they actually click-and-drag the point into place, but maybe there's a better way to word this?

[bratpeki]

Yeah, the more I think about it, the more I think we just have to incentivize the "click-and-drag" idea, the rest of the text is good as-is!

[regulus79]

This may not be the right place to discuss this, but I think the bending automation editor is somewhat bugged, since it places the notes off-center, for some reason.

I kind of made a PR to fix that a while ago, but I closed it in favor of this one #7465

[bratpeki]

If we are keeping the bend automation editor, and a fix exist, I'm all for merging it upstream!

[rdrpenguin04]

Keeping the automation editor could help for pitch bends with non-slope shapes, so unless this editor also implements those, I think the automation editor is valuable enough to keep around. At least for now.

[bratpeki]

Actually, great point, @rdrpenguin04! Thank you, that slipped my mind! 🙏

I noticed we don't display those curves, it's probably a performance thing. @regulus79 might have more info.

[bratpeki]

Also, @regulus79, have you given a different text in the infobox any thought? Are you for keeping this one or focusing on the "click-and-drag" idea more?

[regulus79]

Keeping the automation editor could help for pitch bends with non-slope shapes, so unless this editor also implements those, I think the automation editor is valuable enough to keep around. At least for now.

Yes, that was kind of what I was thinking. Since changing between discrete, linear, and cubic curves, and changing tangents, or out/in values are not implemented in this PR, we need to have the automation editor version still accessible for advanced editing.

I noticed we don't display those curves, it's probably a performance thing. regulus79 might have more info.

Discrete and linear curves should display just fine in the piano roll, but as of now no one has yet implemented proper drawing for cubic pitch bending curves. Aside from adding the circles, I did not change the drawing code in this PR.

Also, regulus79, have you given a different text in the infobox any thought? Are you for keeping this one or focusing on the "click-and-drag" idea more?

I could change it to something like "Click and drag on a note or selection to edit their detuning curves", or maybe you have ideas on what it should say?

[regulus79]

I have updated the text in the most recent commit.

[bratpeki]

Nice! I'd just like to notice it's one curve, but multiple points. How about:

"Click and drag on a note/selection/point to edit the detuning curve"

?

[regulus79]

You're right, "curves" should be "curve" since it's just talking about a single note, thanks for catching that.