Add support for Saved loops

[Holzhaus]

This is a first implementation of a "Saved Loops" feature similar to the
one found in Serato.

The basic workflow is as follows:

1. Create a loop (e.g. by using the beatloop_activate control while the song is playing)
2. Trigger the savedloop_X_sethotcue_X_setloop CO to save the current loop in slot X
3. Change the current loop
4. Trigger the savedloop_X_apply hotcue_X_activate CO to restore the loop that you previously saved in slot X

EDIT: See here for more info: #2194 (comment)

Related bug report: https://bugs.launchpad.net/mixxx/+bug/692926 https://bugs.launchpad.net/mixxx/+bug/1367159

• Add CO to lock a loop (protect it from being changed/deleted by accident) (separate PR because this PR is already large enough and becomes unreviewable)
• Track loop changes (move, scale) so that changes are saved automatically
• Add proper previewing behaviour
• Add tests
• Set beatloop size when saved loop is activated: Add support for Saved loops #2194 (comment)

Added/Changed COs

See mixxxdj/manual#231.

Proposed Changelog entry

- Add support for saving loops as hotcues #2194

[ferranlala]

@Holzhaus Thank you very much for working on this. Saved loops is a highly requested feature.

There are two approaches to saved loops: Serato style and Traktor style.

In Traktor, a saved loop is just a particular kind of cue point.

In Serato, saved loops and cue points are two different things and they are numbered independently. For example you can have loop_1 and hotcue_1 set at different track positions. This is the approach you chose for this PR.

Each approach has upsides and downsides:

Traktor style Pros

• Users can mix and match loops and cue points, and sort them in a specific order. So I can have a loop at position 1, a cue point at position 2, another loop at 3, etc. There's a total order among saved loops and cue points. Also, I don't need to press a modifier in my controller to trigger a loop, since cue points and loops are in a single page. This is useful for people doing cue-juggling, scratching and other routine based performances.

Traktor style Pros

• It can be challenging to know the type of a cue point since loops and cues are mixed. This is specially true on controllers without RGB buttons.

Serato style Pros

• There can't be no confusion about the type of the "cue point/loop" I'm about to trigger, since I'm either on the cue points page or the loops page.

Serato style Cons

• A cue point juggling dj can't trigger both loops and cues without changing page. [1]

I'm not sure which approach is best. Are there any particular reasons why you chose Serato style?

[1] In this video you can see how loops and cues are in different pages, which forces the dj to toggle a modifier: https://www.youtube.com/watch?v=Qh9qqLhOEsM

[ferranlala]

About controllers: There are controllers more suitable to Traktor style (Behringer CMD studio 4A) and others more suitable to Serato style (like the pioneer range)

[Holzhaus]

@ferranpujolcamins The controller I am using the a Serato-style controller, so that's probably the main reason I chose this approach. Serato controllers are a lot more common (at least in my experience).

Also, I have a hunch that Traktor-style loops would require a lot more code changes to integrate them properly, but take that with a grain of salt.

If the Traktor way of doing things is desired too, I think someone could step up and implement an extra pointer-like layer on top of this implementation. Maybe something like this:

• metacue_X_type (0 == Hotcue, 1 == Saved Loop)
• metacue_X_number
• metacue_X_activate
• ...

Metacue might be stupid name, feel free to suggest something better. When metacue_X_activate is triggered, then metacue_X_type and metacue_X_number determine which Hotcue or Saved Loop will be activated.
That way, both Serato- and Traktor-style controllers could be supported properly.

[Holzhaus]

Looks like there's a bug somewhere in the Looping controls. When a loop is active and I apply a saved loop by setting loop_start_position and loop_end_position, the Waveworm display show the saved loop, but the previous loop is used for the actual looping.

EDIT: Sorry, this was just caused by some signals that didn't fire because I'm stupid.

[daschuer]

Without thinking about the Serato/Traktor models to much, I did expect a saved loop is just a special cue point. So we have two main aspects we have to consider:

• Controller layouts either optimized for Traktor, Serato, or without dedicated controls.
• Importing cues and saved loops from other tools.

How about putting cues and loops into the same list? The user will have the ability to arrange the loops with an offset on a "separate page" so this solution may cover both. This way our current hot-cues are already meta-cues.

But there is an issue how to enable a saved loop. Pressing the CUE button will jump to the loop start cue.
This is probably not desired. In many cases I want just enable the loop without a seek. This can be done with am modifier. A combination of reloop + hotcue would be natural. Unfortunately reloop cannot be used for it because it immediately jumps to the old loop-in, the hotcue buttons are also immediately jumping, Ideas?

[daschuer]

Some more comments.

[ferranlala]

How about putting cues and loops into the same list? The user will have the ability to arrange the loops with an offset on a "separate page" so this solution may cover both.

Agree, I think this is the model we should go with. Also, separating cue points from loops forces us to change all the skins right away. By having everything on the same list. we don't need to change anything.

But there is an issue how to enable a saved loop.

I think we need a dedicated CO to enable saved loops without jumping. In case the hotcue is not a loop hotcue this CO will just do nothing.

[daschuer]

OK, this would work for me. The tricky thing remains though, how to present this on a controller and in the skins. Another set of buttons is probably to cluttering.

[uklotzde]

Please merge master instead of cherry-picking ranges of commits. Otherwise the PR becomes unreviewable as is now the case.

Since this is stil WiP I would propose rebasing on master.

[Holzhaus]

First of all, sorry that this branch became so messy, I'll clean it up and squash some commits later (and move 455340c to a separate branch).

I removed the dedicated saved loop controls and added updated/added the hotcue controls (new stuff is emphasized):

• hotcue_X_set sets normal hotcues (unchanged)
• hotcue_X_setloop sets a saved loop hotcue (new)
• hotcue_X_activate triggers a hotcue (if set) by jumping to it (normal hotcue, unchanged) or by applying and toggling the saved loop (new). If the hotcue is not set, hotcue_X_set is used (unchanged).
• hotcue_X_activateloop behaves like hotcue_X_activate except that hotcue_X_setloop is used instead of hotcue_X_set (new).
• hotcue_X_reloop applies the saved loop and jumps to the start position (if the hotcue is a saved loop, otherwise nothing happens)
• hotcue_X_length is like hotcue_X_position and contains the loop length (new)

Generally speaking, is this what you had in mind?

[ferranlala]

Thank you very much for working on this.

I have a few comments on the CO you added:

• I would remove hotcue_X_activateloop and tweak the behaviour of hotcue_X_activate. I propose:
  • When hotcue_X_activate istriggered and there's no hotcue assigned, If there's an active loop, a loop hotcue is set, otherwise a hotcue is set.
    This matches the Traktor behaviour, which in my experience works pretty well.
• hotcue_X_reloop should only turn the loop on without jumping to its start position isn't it? Otherwise it's just like hotcue_X_activate

[Holzhaus]

@ferranpujolcamins

• I would remove hotcue_X_activateloop and tweak the behaviour of hotcue_X_activate. I propose:

  • When hotcue_X_activate istriggered and there's no hotcue assigned, If there's an active loop, a loop hotcue is set, otherwise a hotcue is set.
    This matches the Traktor behaviour, which in my experience works pretty well.

This sounds reasonable, but for Serato-style controllers with dedicated loop buttons it makes sense to be able to switch between normal hotcues and loops manually.

How about this:

• hotcue_X_activate (determines cue type automatically like you described)
• hotcue_X_activatecue (always tries so set normal hotcues)
• hotcue_X_activateloop (always tries to set saved loops)

This way both Traktor- and Serato-controllers the be used as intended (on my Roland DJ-505, I'd use hotcues 1-8 with hotcue_X_activatecue for the HOT CUE mode and hotcues 9-16 with hotcue_X_activateloop for the LOOP mode).

We could even let the user choose what hot cue style he/she prefers - for Traktor-like hotcues, the LOOP mode on Serato controllers would just provide access to additional hotcues 9-16. (Ideally, controller mappings should be able to publish configuration options that are displayed in the settings window so that users do not need to edit the JS source code, but that is a discussion for another time).

• hotcue_X_reloop should only turn the loop on without jumping to its start position isn't it? Otherwise it's just like hotcue_X_activate

No. If hotcue_X_setloop has already been called, then

• hotcue_X_activate sets the loop and enables it (without jumping to it) if the saved loop is not already set. If the saved loop is already set, it toggles it on off (without any jumps).
• hotcue_X_reloop set the loop, enables it and jumps to the loop in point`

[ferranlala]

@Holzhaus thanks for the explanation.

hotcue_X_activate (determines cue type automatically like you described)
hotcue_X_activatecue (always tries so set normal hotcues)
hotcue_X_activateloop (always tries to set saved loops)

Agree, this should cover all cases 👍

hotcue_X_activate sets the loop and enables it (without jumping to it) if the saved loop is not already set. If the saved loop is already set, it toggles it on off (without any jumps).
hotcue_X_reloop set the loop, enables it and jumps to the loop in point`

I understand. IMHO, the behaviour of this two CO should be the other way around: hotcue_X_activate should enable the loop and jump, and hotcue_X_reloop should enable the loop only. My rationale is that for normal cues, hotcue_X_activate jumps to the cue point. It's kind of inconsistent that for loop cues it does not jump.

[Holzhaus]

@ferranpujolcamins

I understand. IMHO, the behaviour of this two CO should be the other way around: hotcue_X_activate should enable the loop and jump, and hotcue_X_reloop should enable the loop only. My rationale is that for normal cues, hotcue_X_activate jumps to the cue point. It's kind of inconsistent that for loop cues it does not jump.

I agree that this is somewhat inconsistent and unintuitive for developers. From the user's standpoint however, I'd rather keep the current behaviour, since it's a lot more common that a DJ wants to enable a loop without jumping to it (at least in my experience).
For Serato-like controller mappings, it doesn't make much of a difference because I could just swap the COs in LOOP mode, but Traktor-like mappings would require pressing some kind of Modifier/shift key to enable a loop without jumping if I swap the CO behaviours.

In Serato, pressing a pad on my Roland DJ-505 will toggle the loop (current hotcue_X_activate behaviour), pressing the pad while holding SHIFT will reloop (current hotcue_X_reloop behaviour).
Does Traktor behave the other way around?

[Holzhaus]

Ok, I cleaned up the branch a bit. The original commits can be found here.

[daschuer]

A merge conflict has developed.

[Holzhaus]

I'll rebase as soon as #2213 has been merged, since that will likely introduce further conflicts.

[Be-ing]

How about this:
hotcue_X_activate (determines cue type automatically like you described)
hotcue_X_activatecue (always tries so set normal hotcues)
hotcue_X_activateloop (always tries to set saved loops)

Hmm, let's be careful about changing the behavior of the existing hotcue_X_activate Control. Remember we have many existing mappings that already use it. If we change the behavior of old mappings as described, it will give old mappings access to this new feature without any changes, which I think is good. However, it might be a bit odd with Serato style controllers. We'll need to give some guidance on https://mixxx.org/wiki/doku.php/updating_controller_mappings how to modify mappings for Serato style controllers. Should we say that the old hotcue_X_activate mappings should be changed to hotcue_X_activatecue and the loop page should be mapped with hotcue_X_activateloop?

[Holzhaus]

@Be-ing

However, it might be a bit odd with Serato style controllers. We'll need to give some guidance on https://mixxx.org/wiki/doku.php/updating_controller_mappings how to modify mappings for Serato style controllers.

Yup, I'll update the wiki page when this PR has been merged.

Should we say that the old hotcue_X_activate mappings should be changed to hotcue_X_activatecue and the loop page should be mapped with hotcue_X_activateloop?

Exactly. Here's how I integrated saved loops on the Roland DJ-505, which is a Serato-Style controller: 163629b

[Be-ing]

I think we should now introduce the concept of a hotcue type indicated by a new Control, let's say hotcue_X_type. Skins could use this to show a loop icon together with the number of the hotcue. I think we should also introduce an ability to set a default hotcue color per type. The latter part might come in a following PR after we've figured out the details of how loop hotcues should work.

[Be-ing]

When hotcue_X_activate istriggered and there's no hotcue assigned, If there's an active loop, a loop hotcue is set, otherwise a hotcue is set.

Let's be careful to still let the user jump to a hotcue from an active loop. A new loop hotcue should not override jumping to an existing jump cue.

[Be-ing]

Can you split off the commits for the Roland DJ 505 mapping to a separate branch? I suggest using Git worktrees so you can work on both branches at the same time.

[Holzhaus]

When hotcue_X_activate istriggered and there's no hotcue assigned, If there's an active loop, a loop hotcue is set, otherwise a hotcue is set.

Let's be careful to still let the user jump to a hotcue from an active loop. A new loop hotcue should not override jumping to an existing jump cue.

It doesn't override that. The if-condition only comes into play when the cue is not set yet.

[Holzhaus]

I think we should by default trust the DJ. Having many loops with the same color should be a exception. Jumping when hitting a cue button is a very normal pattern and not supprising. Jumping back to a loop is a known and useful behavior from reloop_toggle. So IMHO the extra safety net for the costs of loosing a feature is a bad trade of.

When I'm djing, I vastly prefer to do things explicitly than some magic auto-behaviour. If we enable the jumping, there will be no way to toggle a saved loop without the jumping. If you want the jump, you can just invoke gotoandloop.

I trust that the DJ knows whether he/she wants to to jump to loop in or not beforehand and presses the appropriate key. I can be sure that without pressing shift Mixxx won't jump, and with shift it will jump. If we change this to "without shift, it sometimes jumps", it becomes much harder to grasp IMHO. Mixxx tries to be smarter than the DJ.

And to be honest, I think the benefit of not having to press SHIFT is small compared to the risk of jumping back by accident.

Without a safe way to activate loops, this feature becomes pretty useless for me. I almost never use reloop_toggle to re-enable loops for this exact same reason (and the risk is smaller there because there is only a single loop on the waveform).

[daschuer]

It is all a question of consistency. All your arguments apply to the reloop_toogle knob as well.
For me it does not make sense to to it differently, this is the worst thing.

If you want the jump, you can just invoke gotoandloop. That is simply not there on the GUI.

I understand your concerns, and I must admit they are valid in some situations. In other, the exiting pattern is the right one,
It is "the Mixxx" solution since some years.

However, I don't want to delay this PR any longer. Can we add the reloop_toogle behaviour, here for a consistence and postpone the jumping issue to a new PR?
I have some ideas to overhaul the loop section that the DJ has full control over jumping, or just enabling.
This will give also the chance to get feedback fro a wider audience.

What do you think?

[Holzhaus]

It is all a question of consistency. All your arguments apply to the reloop_toogle knob as well.

I know, that's why I have a patch that makes loop_enabled read/write (it's also part of this PR). My controller has a "Reloop/Exit" button and a "Loop Active" that does the same without the jumping. I didn't use the "Reloop/Exit" in a long time.

I understand your concerns, and I must admit they are valid in some situations. In other, the exiting pattern is the right one,
It is "the Mixxx" solution since some years.

However, I don't want to delay this PR any longer. Can we add the reloop_toogle behaviour, here for a consistence and postpone the jumping issue to a new PR?
I have some ideas to overhaul the loop section that the DJ has full control over jumping, or just enabling.
This will give also the chance to get feedback fro a wider audience.

Hmm, but if I change the underlying CO to jump if we are behind the loop, it will also jump when activating from the controller.
Even if we had a better GUI section, I have no idea how that would fix the CO behaviour on controllers? Or do you propose to revert the change that adds the jumping once the GUI is in place?

[Holzhaus]

@daschuer I added the requested changes in daf442e, even if it pretty much makes the Saved Loops feature unusable for me. Let's make the GUI changes and revert that commit ASAP.

[daschuer]

Or do you propose to revert the change that adds the jumping once the GUI is in place?

Yes exactly. Let's collect opinions and use cases and than decide how we get the best out of it.

[daschuer]

LGTM.

Thank you very much for this great feature that improves usability of looping a lot.
A good reason to use Mixxx 2.4 even if it is beta :-)

[daschuer]

I have started a Zulip thread:
https://mixxx.zulipchat.com/#narrow/stream/109122-general/topic/Reloop.20behaviour

[daschuer]

Unfortunately this suffers a nasty bug:
https://bugs.launchpad.net/mixxx/+bug/1903002

[daschuer]

Sorry, This has been slipped trough:

mixxx/src/engine/controls/cuecontrol.cpp

Line 2402 in ffb541a

m_pHotcueStatus->forceSet(newPosition == Cue::kNoPosition ? 0.0 : 1.0);

please use HotcueControl::Status

[cmorgan]

This is a great feature! Thanks for this.
I think the way that you are reusing the Hotcue buttons for the loops is very neat and intuitive.
Is there a way to activate Hotcues via the keyboard?

[Holzhaus]

Is there a way to activate Hotcues via the keyboard?

https://manual.mixxx.org/2.4/en/chapters/appendix.html#keyboard-mapping-table

[Russe]

* [ ]  ~Add CO to lock a loop (protect it from being changed/deleted by accident)~ (separate PR because this PR is already large enough and becomes unreviewable)

Is there any way to delete loops from a track with this PR?
I'm not able to find this feature in most recent beta 2.3 or alpha 2.4 and remember from Mixxx Forum that this is not possible yet.
Otherwise I would open a feature request at launchpad.

[Holzhaus]

The PR is only in 2.4 alpha, not 2.3 beta.
You can delete any hotcue by right clicking the button or marker and then clicking the trash button. Many controllers have happed this to SHIFT+Pad

[Russe]

Thanks for your feedback.
I do not mean hotcues, I would like to remove loops. With hotcues everything is fine, I can add and delete them like you said.
But I never figured it out to remove loops (the green markers in waveform) so I hoped this would be possible with this PR.

[ronso0]

what's the issue with those loops?
if they are only clutterinh the track overview for you, you may move/re-set them somewhere out of sight, like beyond the track end.
try triggering "reloop" to see what consequences that produces in order to be safe when that happens accidentally.

[Holzhaus]

Saved Loop as just special types of hotcues, you can delete them just the same. The green loop (in 2.3/2.4) can t be deleted, just disabled using the loop controls.

[ronso0]

Yes, since this was first request on the forum I assumed it's about 2.3

[Russe]

Yes, your are right. I don't use loops at all and would like to have as less things and functions on the screen as possible.
So I'm testing with moving them beyond the track's end like you said.
As well I'm trying to remove/disable the loop controls.

[ronso0]

Note that with LateNight in 2.3 you can use compact or even minimal decks to unclutter your view (while having effects and samplers etc.)

[Holzhaus]

Please use the forum at https://mixxx.discourse.group/, this discussion is completely unrelated to this PR.