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

RFC: Automation control point operations #744

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kiilerix
Copy link
Contributor

@kiilerix kiilerix commented Oct 24, 2022

The goal of this PR is to make automation editing more smooth. Nothing big, but some workflow improvements I would enjoy.

See the individual clean changesets.

I think some of the changesets are good, ready to be cherry-picked.

I would like some feedback on whether these changes operate on the right levels, or if I violate some MVC-ish or code structure pattern that I haven't realized? For example, it would be nice to pass model objects to AutomationLine::remove_points instead of Items or ControlPoints ... but that doesn't seem to fit in.

More experimental: Make it possible to enter operands like "+= 3" when editing (multiple) control points to increase them all with 3 dB. This could be an advanced feature, documented in the manual but not cluttering the UI.

@kiilerix kiilerix force-pushed the automation-multi-edit branch 2 times, most recently from 5a2558e to 0e8787d Compare October 26, 2022 19:20
@kiilerix
Copy link
Contributor Author

kiilerix commented Nov 4, 2022

I notice elsewhere "postfix the edit with '+' or '-' to enter delta times" - that scheme should perhaps be used instead of '+=' etc ...

@pauldavisthefirst
Copy link
Contributor

I';m not sure why we let this one slide. Any chance of the conflicts being resolved? Apologies ...

@kiilerix
Copy link
Contributor Author

@pauldavisthefirst Can you help speeding it up by briefly answering some questions that came up while cleaning this up:

From a UI and implementation point of view, what is the big difference between GainLineItem. AutomationLineItem, and ControlPointItem. The code shows many differences in details, but it is hard to spot the big conceptual difference that explains the difference in implementation.

Also, what is the status / intent with Del key support? Conceptually, I assume Del works on the active selection ... with elements that might be of the same kind, or at least have a lot in common. How to hit the code path that works? I assume it would be nice if it worked more consistently so it never was necessary to use popup_control_point_context_menu to delete control points, but also the primary goal is to avoid introducing regressions..

The control points were very small and hard to spot, especially when
painted on top of waveforms.

Also, the snap size for automation lines were bigger than for the
control points at the end, making it hard to click the control points.

Fixed by making the control points twice as big - that seems to match
the size of the automation line snapping.

To avoid control points showing up as too big solid blocks blocking too
much, we stop making them filled. The bigger outline is more visible
than the old solid block.
The most common drag operation on an automation point is with the intent
of either moving it in time or in value. That was very hard to achieve
when both dimensions were free.

Slightly similar to "Ignore Y-axis when adding new automation-points",
we thus "snap" the move to either being horizontal or vertical.

In the rare occation where we want to move in both dimensions at once,
we can still and fully intuitively do that with 2 drag operations.
…reference

Avoid retrieving control_point from Item both in
Editor::remove_control_point and in Editor::can_remove_control_point .

remove_control_point is called from Editor::button_release_handler, and
is very view-ish, so it is fine that it takes an Item.

can_remove_control_point is slightly more low level and model-ish
(almost so much that it could live in AudioRegionGainLine), so it kind
of makes sense that it works on the ControlPoint.

There is no extra code. The retrieval of ControlPoint moves from
can_remove_control_point to Editor::popup_control_point_context_menu .

This refactoring will make it easier to implement deletion of multiple
selected points.
It was surprising that removing on multiple selected control points only
removed one of them.

This allows selecting multiple points by dragging a box or
Ctrl-clicking, then removing all from the context menu of one of the
points.

Some more trivial implementations of this suffered from problems when
deletion of control points in the model also invalidated the iterator
over selected control points.
For example, if editing a control point at 7 dB and entering "+= 3"
in the input field instead of "7", it will set the control pointto 10
dB.

This is especially useful when editing multiple automation points at
once.
@pauldavisthefirst
Copy link
Contributor

  1. Re: GainLineItem. AutomationLineItem, and ControlPointItem : the first two are the line itself drawn for automation display, the former being solely for region gain (within audio regions) and the latter being for all other automation lines; the 3rd one is any canvas item used at the location of a control event
  2. I'm going to defer this PR till post-8.0 ... some of the commits have been implemented in other ways and some of the others need preference items, but we're in a string freeze.

@kiilerix
Copy link
Contributor Author

I agree to land things like this early in the development phase, rather than last minute.

I will revisit after 8.0 .

@luzpaz
Copy link
Contributor

luzpaz commented Sep 23, 2023

Is it possible to label this PR post 8.0 or something easy to see what the agenda is for it ?

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.

3 participants