lmms icon indicating copy to clipboard operation
lmms copied to clipboard

New context menu automation items

Open szeli1 opened this issue 1 year ago • 25 comments

closes #3112

This PR adds new context menu options for automatableModels to improve the automation workflow. This PR aims to make quick edits to automation tracks easier and faster to do. It does not aim to replace the automation editor.

These options are the following:

  • "Add automation node" - this option will search for AutomationTracks that have at least one clip. All clips have to be connected to the model. If a track with these requirements can not be found a new track will be added. After it found a track it will search for a clip before the playback position. If it can not find a clip it will add a new clip. After it found a clip it will add a new AutomationNode to the clip at the playback position. The node's value will be equal to the AutomatableModel's value. The node's placement depends on the quantization setting inside AutomationEditor. If there is a clip before the playback position a new clip won't be created.
  • a new submenu is added labeled "Automations" inside the submenu there are more options:
    • "Add automation node to new clip" - mostly "Add automation node" option, but it adds a new clip at the playback position if possible before adding an automation node.
    • "Update closest automation node" - replaces the value of the closest node to the playback position with the AutomatableModel's current value.
    • "Remove closest automation node" - deletes the closest AutomationNode to the playback position. It will not delete clips and it will leave at least one AutomationNode inside a clip (because of AutomationClip limitations).
    • "Open automation clip" - mostly "Add automation node" option, but instead of adding a new AutomationNode it will open the found / created clip in the AutomationEditor. It will try to open the closest clip, it 2 clips are after each other and the playback position is (1/2 bar) close to the second clip then it will open the second clip.

In my view these options will make the automation system faster to use.

szeli1 avatar Jun 13 '24 15:06 szeli1

There's lots more nesting going on. I am a bit tired to nitpick everything

Rossmaxx avatar Jun 13 '24 17:06 Rossmaxx

There's lots more nesting going on. I am a bit tired to nitpick everything

Feel free to nitpick. I'm open to learn new things.

szeli1 avatar Jun 13 '24 17:06 szeli1

EDIT: Looks like my page didn't refresh with the latest commits for some reason. Not sure why. 🤔

Because I pushed the changes right before when you completed your review.

szeli1 avatar Jun 13 '24 19:06 szeli1

How should the new tracks be named? Currently it uses the default names.

szeli1 avatar Aug 16 '24 10:08 szeli1

Btw forgot to say, this could fall in scope for #7027

How should the new tracks be named? Currently it uses the default names.

I believe it should be named after whatever knob the automation is created from

Rossmaxx avatar Aug 16 '24 10:08 Rossmaxx

I tested this and I loved it, but in my opinion I think it would be better to group these automation options in a submenu; Also that the automation tracks created with these options are created below the track that you are going to automate and that they also inherit the color of said track, this way we will not have to reorder or color them manually.

Gabrielxd195 avatar Aug 20 '24 06:08 Gabrielxd195

I tested this and I loved it, but in my opinion I think it would be better to group these automation options in a submenu; Also that the automation tracks created with these options are created below the track that you are going to automate and that they also inherit the color of said track, this way we will not have to reorder or color them manually.

Thanks for testing! The main focus was to make automation faster. I think navigating a submenu would make the process more complicated. I think I might put the last 3 options behind a submenu. I think your last 2 suggestions are difficult to do. Knobs and buttons can not access the track they are on so they wouldn't know what color to copy. Some Knobs don't even have a track (like the zoom slider).

szeli1 avatar Aug 20 '24 08:08 szeli1

They should also be reworded to something more telling. It seems for example that "add automation node" should rather read "Add new automation track".

I do not agree with this. The main function of "add automation node" is that it adds an automation node / point to a track. If the widget isn't automated yet then it will create an automation track in addition. You might be confused because for some reason Automation Clips will not allow adding 2 points with identical values after each other. This is why it only creates a new node if the widget's value was changed.

With regards to the entry "add automation node to new clip" I do not understand the difference to "add automation note" as seems to attempt to do the same.

It will attempt to make a new clip. If the playback position is over an existing clip then it will do the same as "add automation node".

There seems to be a bug though because for me it adds a new empty automation track as well as a new track with an automation clip.

Thanks for noticing this, I will look into it.

With regards to "update closest automation node" I am not really sure if it brings a real benefit, especially since it will always only change the first closest clip. Users can also simply open the clip and edit it accordingly.

I think users will be able to start playback by pressing space, see how the widget changes and respond accordingly (update or delete closest node).

I think it could be easier to use that rather than finding the correct automation track, clicking on it, navigating to the correct location and then modifying the node and navigating back to the widget. The whole point of this PR is to improve workflow and make automation faster. You could argue that it only changes the first node, but I would argue that the automation editor can only change one node at a time too. The editor should have the ability to make complex modifications to automation nodes.

The functionality of "remove closest automation node" also seems rather risky to me as it will potentially remove a node that's far away and that might be offscreen for users.

They might look at a certain clip believing that it's the closest, attempt to remove it and then a clip in between the instrument track and automation track is removed which they intended to keep. This is even more worrisome as undo does not work (see below).

Yes that could happen but if the undo gets fixed it will also get fixed. The undo's purpose is to fix small mistakes such as this. I will fix the undo.

Users might also mistakenly remove a clip which the did not intend to remove.

Would you rather don't delete the clips?

So all in all this PR should not be merged as is.

I agree.

szeli1 avatar Aug 20 '24 18:08 szeli1

I think it was already mentioned by someone else in this thread but it seems that the fix to most of these problems would be to have the automation tracks of an instrument track organized right below it. I am pretty sure this is how most other DAWs implement this. Usually it's also possible to show and hide automation tracks to keep things organized. If implemented like this then all automation tracks would be shown close to the corresponding instrument track and there would be no need to hunt for them or to implement some rather brittle workarounds like "do something with the nearest clip/track".

It should ideally be as simple as possible to edit the automation clips/tracks so that it's the first and preferred way for users to interact with the automation.

michaelgregorius avatar Aug 20 '24 18:08 michaelgregorius

They should also be reworded to something more telling. It seems for example that "add automation node" should rather read "Add new automation track".

I do not agree with this. The main function of "add automation node" is that it adds an automation node / point to a track. If the widget isn't automated yet then it will create an automation track in addition. You might be confused because for some reason Automation Clips will not allow adding 2 points with identical values after each other. This is why it only creates a new node if the widget's value was changed.

Where is the node added in the clip? Will it always be added at the position that the users intend? If they will have to move the new node in most cases then they could as well directly open the clip in the automation editor and click the new node at the correct position.

While writing this I noticed that it might perhaps be a useful feature to have something like "Open nearest clip in automation editor" in the menu. Another improvement for workflow might be to add back and forth buttons to the automation editor that lets users cycle through all clips that are associated to the parameter of the current clip, e.g. cycle through all volume automations of a TripleOscillator. This would also improve somewhat with the organization of automation clips.

Users might also mistakenly remove a clip which the did not intend to remove.

Would you rather don't delete the clips?

The quickest way to delete a clip is to locate it and remove it with a middle click. This is much faster and concise than the functionality in the context menu which needs to "guess" what the users meant.

If users are unsure which automation clips belong to a parameter then it might be helpful to implement a "Highlight all associated automation clips" functionality to the context menu. When clicked all automation clips of a parameter are highlighted in the Song Editor.

However, the underlying problem is that LMMS potentially organizes the clips in a totally chaotic way and users can even make it worse. So all of that would just be band-aids.

So all in all this PR should not be merged as is.

I agree.

:+1:

michaelgregorius avatar Aug 20 '24 18:08 michaelgregorius

I think it was already mentioned by someone else in this thread but it seems that the fix to most of these problems would be to have the automation tracks of an instrument track organized right below it.

I do not know how to make this happen. I don't think it is possible without redesigning AutomatableModel.

I am pretty sure this is how most other DAWs implement this. Usually it's also possible to show and hide automation tracks to keep things organized. If implemented like this then all automation tracks would be shown close to the corresponding instrument track and there would be no need to hunt for them or to implement some rather brittle workarounds like "do something with the nearest clip/track".

If I remember correctly there are plans to do this in the future. I'm not sure which feature request is about this, but it was about adding a new type of automation track that is placed under instruments and it can be hidden. Your suggestion would remove one step: finding the automation track. My PR would remove almost every step. Automation would be possible in the same window where the widget is. This would remove the potential need of hiding windows and reopening them.

so that it's the first and preferred way for users to interact with the automation.

Why? Why limit the user to use the automation editor for simple quick edits? While improving the editor should be a focus in the future, I think the user shouldn't be limited to use only the editor. And those workflow problems (closing windows to find Song editor, then finding Automation editor, then finding the point and then modifying it (could be difficult because of scaling issues inside the window) and then navigating back) mostly can not be fixed by improving the automation editor. This PR offers a different solution that is consistent in placement between every widget everywhere so the user doesn't have to search for anything.

szeli1 avatar Aug 20 '24 18:08 szeli1

Where is the node added in the clip? Will it always be added at the position that the users intend? If they will have to move the new node in most cases then they could as well directly open the clip in the automation editor and click the new node at the correct position.

Why would they need to move the node in most cases? AutomationClip handles the point placement, the user can control where the point is placed with the Quantization setting.

While writing this I noticed that it might perhaps be a useful feature to have something like "Open nearest clip in automation editor" in the menu.

Good suggestion, I will implement it.

Another improvement for workflow might be to add back and forth buttons to the automation editor that lets users cycle through all clips that are associated to the parameter of the current clip, e.g. cycle through all volume automations of a TripleOscillator. This would also improve somewhat with the organization of automation clips.

Yes or something like ctrl + left or right arrow moving between different clips (I don't know if this is only a MIDI clip feature). I think a new PR should be opened for this.

The quickest way to delete a clip is to locate it and remove it with a middle click. This is much faster and concise than the functionality in the context menu which needs to "guess" what the users meant.

You are right about this, I will remove auto deleting clips and tracks.

szeli1 avatar Aug 20 '24 19:08 szeli1

so that it's the first and preferred way for users to interact with the automation.

Why? Why limit the user to use the automation editor for simple quick edits? While improving the editor should be a focus in the future, I think the user shouldn't be limited to use only the editor.

Please note that nowhere did I mention that I meant the current automation editor to be this first and preferred way. When I wrote this I had Bitwig's automation in mind. In Bitwig you can for example wiggle a control in a plugin and it will show as the current parameter in the automation track of that instrument track. You can then directly click points, move them, add tension to curves, etc. It just works straight forward and so far there never was a need to dive into context menus to accomplish something. Of course you can also manually select parameters or show all automation tracks of parameters that are automated.

And those workflow problems (closing windows to find Song editor, then finding Automation editor, then finding the point and then modifying it (could be difficult because of scaling issues inside the window) and then navigating back) mostly can not be fixed by improving the automation editor. This PR offers a different solution that is consistent in placement between every widget everywhere so the user doesn't have to search for anything.

Yes, I agree that the current workflow and organization of automation is "sub-optimal".

michaelgregorius avatar Aug 20 '24 19:08 michaelgregorius

Please note that nowhere did I mention that I meant the current automation editor to be this first and preferred way. When I wrote this I had Bitwig's automation in mind. In Bitwig you can for example wiggle a control in a plugin and it will show as the current parameter in the automation track of that instrument track. You can then directly click points, move them, add tension to curves, etc. It just works straight forward and so far there never was a need to dive into context menus to accomplish something. Of course you can also manually select parameters or show all automation tracks of parameters that are automated.

Sorry I totally misunderstood you. I thought you said exactly the first sentence, sorry. I'm not familiar with other daws sadly so I can't comment on that. I was thinking about widget highlighting and I think my next PR will be about that. I'm planning to implement a class similar to ModelView that would add widget highlighting and unified keyboard shortcuts.

szeli1 avatar Aug 20 '24 19:08 szeli1

I have fixed undo, added a new context menu option to open the closest clip and removed the remove option removing clips (but now it must leave at least one node in the clip to prevent some issues).

szeli1 avatar Aug 21 '24 10:08 szeli1

Some things I have observed while checking the current version of the branch:

  • If I use "Add automation node" and a new track with a new clip is added then undo only removes the clip and keeps the added track.
  • There is still the remove node option in the menu that if I understood correctly was agreed on to be removed.
  • "Open automation clip" does not open the nearest existing clip but instead adds a new one. If there is already one in that place another one will be added anyway and the clips will overlap, i.e. users might have several hidden clips which overlay each other.
  • "Add automation node" only changes an existing node in a single clip for me so it does not really add new nodes in the automation clip. It's not clear to me what's the "rule" where a new node is added and what's governing this behavior.

To be honest, all these features seem totally confusing to me. I don't understand how it should be feasible and helpful to the users to remotely add nodes to automation clips that they might not even see while they are interacting with context menus in controls. I am not really "dog-fooding" on LMMS and seldom do something with the automations. So it should be a good test to see how easy or hard it is to understand this feature. And for me it is hard to understand.

Therefore I'd like some other @LMMS/developers to chime in here and take the decision with regards to whether this functionality should be added or not.

@szeli1, I propose to wait for the result of the discussion with the other developers before any further reviews are done.

michaelgregorius avatar Aug 21 '24 15:08 michaelgregorius

If I use "Add automation node" and a new track with a new clip is added then undo only removes the clip and keeps the added track.

I will fix this.

There is still the remove node option in the menu that if I understood correctly was agreed on to be removed.

I thought you only wanted the auto clip deletion removed (which was removed, now if all the nodes get deleted the clip will not be removed). I would like to hear what other devs say about this.

"Open automation clip" does not open the nearest existing clip but instead adds a new one. If there is already one in that place another one will be added anyway and the clips will overlap, i.e. users might have several hidden clips which overlay each other.
"Add automation node" only changes an existing node in a single clip for me so it does not really add new nodes in the automation clip. It's not clear to me what's the "rule" where a new node is added and what's governing this behavior.

Could you please describe to me exactly what are you trying to do and what do you expect the result to be? I don't know how could this happen and to me it seems like I did not give a good description of the features or there could be bugs.

Edit: I have rewrote the PR description to make the context menu options easier to understand.

szeli1 avatar Aug 21 '24 15:08 szeli1

"Open automation clip" does not open the nearest existing clip but instead adds a new one. If there is already one in that place another one will be added anyway and the clips will overlap, i.e. users might have several hidden clips which overlay each other. "Add automation node" only changes an existing node in a single clip for me so it does not really add new nodes in the automation clip. It's not clear to me what's the "rule" where a new node is added and what's governing this behavior.

Could you please describe to me exactly what are you trying to do and what do you expect the result to be? I don't know how could this happen and to me it seems like I did not give a good description of the features or there could be bugs.

Reading the updated description I think it's a bit clearer to me what you want to achieve. I was not aware that the playhead plays an important role with regards to that feature. Nevertheless I will describe what I expected without have this description available as this will be the situation most users will be in when they interact with the feature.

Open automation clip

Steps to reproduce:

  1. Right click the volume knob of a 3OSC and select "Add automation node" from the context menu.
  2. Open the clip that was just added in the Automation Editor.
  3. Make some changes, i.e. add some nodes, so that the clip becomes recognizable.
  4. Close the Automation Editor.
  5. Right click the volume knob again and select "Automations > Open automation clip".

Instead of opening the existing clip a new clip is added and opened. I'd expect it to open the existing clip because IMO it's more likely that I want to edit it instead of a completely new one with no edits.

Add automation node

Steps to reproduce:

  1. Right click the volume knob of a 3OSC and select "Add automation node" from the context menu.
  2. Open the clip that was just added in the Automation Editor.
  3. Move the playhead in the Song-Editor so that it's still within the clip, e.g. in the middle of the clip.
  4. Adjust the volume knob.
  5. Select "Add automation node".
  6. The opened clip will change.
  7. Change the volume knob to a completely different value.
  8. Select "Add automation node" again.

Instead of adding a new node like the label says the existing node is changed. I'd expect a node to be added.

michaelgregorius avatar Aug 21 '24 16:08 michaelgregorius

If I use "Add automation node" and a new track with a new clip is added then undo only removes the clip and keeps the added track.

Can not be fixed. You can not undo track creation.

szeli1 avatar Aug 21 '24 17:08 szeli1

Instead of opening the existing clip a new clip is added and opened. I'd expect it to open the existing clip because IMO it's more likely that I want to edit it instead of a completely new one with no edits.

This is a bug which can be fixed. I will try to fix it.

Instead of adding a new node like the label says the existing node is changed. I'd expect a node to be added.

This is an AutomationClip limitation. You can not add multiple Nodes in the same location. I think this is the case inside the AutomationEditor also. A name change could fix this issue. (But what should be the name? I think it is reasonable to expect that the user understands that it is impossible to do what they want (especially if they know how the AutoamtionEditor works))

Thanks for that comment, it is in great detail (as I have requested).

I think documentation would need to be rewritten so the users will be able to understand the options better.

szeli1 avatar Aug 21 '24 17:08 szeli1

I tried this and although I didn't have all the time to read your comments I would like to quickly contribute some ideas like:

  1. Remove all automation clips with the "Remove Closet Automation Node" option: since removing a simple node from a clip, or removing the closest or first clip is useless. If I wanted to remove the value of a clip for that I go directly and do it, but if I need to remove all the clips related to the knob I want to automate, how do I do it? Currently this option is useless when I want to remove all the clips simultaneously.

  2. Replace the current "Update Closets Automation Node" option with "Increase or Decrease the value of the clips": where there is the possibility of adding or subtracting values ​​to all the nodes of all the automation clips simultaneously. Basically it would be like the "Transpose" option of the instrument clips but applied to the automation clips. An automation clip can have many nodes with different values ​​but obviously a knob cannot. Currently updating doesn't work for many nodes but for one and the closest one, but this suggestion fixes that. For example: If I have several "Volume" clips with different individual values ​​and I want to increase or decrease the value of all the clips and all their nodes, I would go to the hypothetical option "Increase or Decrease the value of the clips", and the value of all their nodes would change individually, that is, if I have a clip with 25%, another with 50%, and another with 75%, another with several nodes with different values, but I want to reduce those values, I go to the knob menu, open a widget and add -5%, each clip will be updated and all its nodes will be individually reduced by 5%, resulting in a clip of 20%, another of 45%, another of 70% and the one that had several nodes, they would also be reduced, none would exceed their limits, they would increase or decrease as far as they can, basically it is the "Transpose" of the automation clips, but the automation clips would also have that option by themselves you want to "Increase or Decrease" the value of each one separately.

  3. Option to Link cloned automation clips: If you clone an automation clip from one knob to many and select them all together, you would have the "Link clips" option in the menu, so that when you make a change in one clip, that change would affect all the other clips. This would affect the value of all their nodes.

  4. Open the last selected automation clip with the "Open Automation Clip" option: you can do that with the editor buttons, but don't you think it would be better if you added this function to the automation knobs? That would make more sense to have that option there. Currently the option opens the closest clip, that's useless when there are many clips and you forget the last one you used.

  5. Navigate between all assigned automation clips with Alt + Arrows: Currently in the Piano Roll you can open many clips at the same time, by pressing Alt + Left and Right Arrows, the idea is that this also works for automation clips assigned to the knob you are going to automate with the "Open Automation Clip" option. Right now when you press Alt + Arrows the editor head moves, and that head does not work, but this change would make more sense.

I don't know if I'm wrong or how difficult it would be to apply these changes, but without a doubt these suggestions would work better with all clips and their nodes, because for now those options only affect the closest clips and their first node, and that is useless when a knob has many clips and when they have many nodes, I did the test.

(Google Translate)

Gabrielxd195 avatar Aug 22 '24 01:08 Gabrielxd195

Remove all automation clips with the "Remove Closet Automation Node" option since removing a simple node from a clip, or removing the closest or first clip is useless Replace the current "Update Closets Automation Node" option with "Increase or Decrease the value of the clips":

This PR is built on the playback position. Your suggestion doesn't. This PR doesn't aim to add new features, but to make existing features faster to use. I think the AutomationEditor should have the ability to support complex features such as this. I could add it in the context menu but I would rather not remove the existing option. Adding this in the context menu would be like trying to fix the broken automation system with a small patch (without addressing the actual issues).

Option to Link cloned automation clips

This isn't supported and this PR doesn't touch the related files. You could make a new feature request about this.

Open the last selected automation clip with the "Open Automation Clip" option: you can do that with the editor buttons, but don't you think it would be better if you added this function to the automation knobs? That would make more sense to have that option there. Currently the option opens the closest clip, that's useless when there are many clips and you forget the last one you used.

Currently the option opens the closest clip before. I can see why this would be an issue. Usually users would like to playback the section which they are working on, so the playback position is almost always before where the automation clip would start. I will change the code so It opens the appropriate clip (Edit: Done). I can implement your suggestion instead if more people support it.

Navigate between all assigned automation clips with Alt + Arrows

This is again an automation editor feature suggestion. It is not related to this PR.

I don't know if I'm wrong or how difficult it would be to apply these changes, but without a doubt these suggestions would work better with all clips and their nodes, because for now those options only affect the closest clips and their first node, and that is useless when a knob has many clips and when they have many nodes, I did the test.

Your suggestions are valid. There should be a way to edit multiple nodes at the same time, but I think this PR's goal isn't to mass edit multiple nodes, it is to make simple quick edits faster by using the playback position.

szeli1 avatar Aug 22 '24 08:08 szeli1

I would like to provide my own opinion on the PR.


Firstly, I have tested the options and can confirm everything works as intended. I would still like to provide a list of features and what they do, explained in a way better fit for the end user:

Option Description
Add automation node Add a new node, with the currently set value of the parameter, to the appropriate automation clip at the position of the playhead. If a clip doesn't exist, create one, in a new automation track
Add automation node to new clip Same as the previous option, except that it doesn't append the automation node to an already existing clip, but instead to a new clip, one bar long
Update closest automation node Update the closes note to the playhead with the value of the parameter
Remove closest automation node Delete the note closest to the playhead in the clip of the appropriate parameter
Open automation clip Open the editor and edit the appropriate automation clip

NOTE: If any parameter has more than one clip, the one closest to the top is selected.


Secondly, I believe these would have to be appropriately documented, as they are quite confusing out-of-the-box. A YouTube video might do just as well, if not better, because this feature is best explained in-action.


Thirdly, my honest opinion on the feature. I do not think this makes a big impact on the speed of editing automation. I tried making a slow sweep (0-25-100) using both methods.

https://github.com/user-attachments/assets/d91a06bc-8b91-408b-88c4-ad759d702b3e

https://github.com/user-attachments/assets/cc6046be-04aa-417c-85ba-d663452d70c4

I have not seen a notable difference, time-wise. Granted, this kind of test doesn't do justice to the other features, but I still do not think these make a noticable impact, given how they invite more work relating to documenting the feature.

I would invite some more testers to form their opinions, since the feature is really nicely made, but just seems a bit redundant to me.

bratpeki avatar Aug 24 '24 13:08 bratpeki

I would like to provide my own opinion on the PR.

Thanks for testing!

If any parameter has more than one clip, the one closest to the top is selected.

This is not always true and I don't know what you mean by "closest to the top". Some settings select the closest clip, others select the closest clip before the playback position and "Open automation clip" uses a clip's start and end position to choose what it opens.

Thirdly, my honest opinion on the feature. I do not think this makes a big impact on the speed of editing automation. I tried making a slow sweep (0-25-100) using both methods.

I think it helps window management.

I would still like to provide a list of features and what they do, explained in a way better fit for the end user

Thanks for making this description. I think it is better than my description.

szeli1 avatar Aug 24 '24 19:08 szeli1

I don't know what you mean by "closest to the top".

What I mean to say is "the one that appears first when traversing all automation tracks from top to bottom in the song editor".

Thanks for testing!

My pleasure!

bratpeki avatar Aug 24 '24 22:08 bratpeki