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

new loop and beatjump controls #1187

Merged
merged 96 commits into from
May 25, 2017
Merged

new loop and beatjump controls #1187

merged 96 commits into from
May 25, 2017

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Feb 15, 2017

This PR introduces 3 new ControlObjects:

  • beatloop_size: indicates the size of a beatloop to set
  • beatloop_toggle: sets a beatloop of the size indicated by beatloop_size
  • beatloop_enabled: whether a beatloop is enabled

This also introduces a new widget for displaying beatloop_size in skins, although it may be used for any CO that indicates a number of beats. I have added tests for the new COs as well as a test for the old "beatloop" CO that had no tests. The behavior of that old CO remains the same, but now it also updates beatloop_size.

This approach has several advantages to the current approach of displaying a huge grid of buttons for different loop sizes:

  • Deere 2.1 #940 will be able to fit in its minimum width using default settings, allowing it to be used on netbook screens.
  • Less cluttered GUI.
  • Any beatloop size supported by Mixxx (1/32 to 64) can be shown without worrying about taking up way too much screen space with more buttons.
  • A place to show the selected loop size on screen for controllers that have an encoder for selecting loop size but no LED digit readout or screen on the controller. This can also be used for controllers that only have pads for loop controls by mapping buttons to beatloop_toggle, loop_halve, and loop_double. I will write JS Components to make that easy.

I tried hacking something like this together in #940 using a WidgetStack with loop_halve/loop_double as the prev/next controls, but that had several issues:

  • Using beatloop_X_toggle, which many controllers that use pads for setting beatloops do, updates beatloop_size and the display in skin. This was not possible with the WidgetStack hack.
  • "1/64" as the text label for a WPushButton got very wide.
  • loop_halve/loop_double have a lower and upper bound for the size of loops, but with a WidgetStack the displayed number wrapped around and got out of sync with the actual loop size.

fixing https://bugs.launchpad.net/mixxx/+bug/1429331


Known remaining issues, none of which I think should delay merging:

  • WBeatSpinBox does not scale with skin
  • If reloop_toggle is pressed when the loop is disabled, the play position is before the loop, and the track is paused, when the track is played, it will jump to the loop instead of playing normally then catching the loop.
  • If reloop_toggle is pressed while the deck is playing and the play position is before a disabled loop, then the deck seeks to another position outside the loop, the deck will then seek inside the loop.

beatloop_size: indicates the size of a beatloop to set
beatloop_toggle: sets a beatloop of the size indicated by beatloop_size
beatloop_enabled: whether a beatloop is enabled
The first use case for this is the new beatloop_size CO, but it could be
used for displaying other COs that represent a number of beats.
@daschuer
Copy link
Member

Thank you for the pull request. This looks like to be a good step forward.

Playing with the existing controls, I notice some other usability issues. For examples, how many loop enable buttons do we need? How would a reasonable GUI look like that allows to access the hidden advanced features, sleeping inside the engine?

I have not a yet a good plan, but It bugs me for a while that I cannot start beat loops at cue points.
Maybe this new prepare mode can help ...

What are the use-cases?

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 15, 2017

how many loop enable buttons do we need

Two: one for toggling beatloops, one for reloop.

I have not a yet a good plan, but It bugs me for a while that I cannot start beat loops at cue points.

Likewise, but that is beyond the scope of this PR. My intention here is only to clean up the cluttered GUI and have a way for controllers to show the loop size on screen.

@daschuer
Copy link
Member

The code looks good, I need to test it.

Likewise, but that is beyond the scope of this PR. My intention here is only to clean up the cluttered GUI and have a way for controllers to show the loop size on screen.

The only thing is in case we have a hot idea, to check how it fits to this.

Now we have "beatloop_size" and "beatloop" both do not tell you what will happen.
"beatloop" is actually "beatloop_size_and_enable" while "beatloop_size" dos not enable the loop.
Any idea how to fix this?
Call the new control "beatloop_prepare"
.. or alias the old control ... is it actually used or can it be removed.
I think only scripts can make use of it and they should be happy with the new interface.

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 15, 2017

Running git grep showed that the old "beatloop" CO is used by some controller scripts. I agree it is poorly named. I think it should be marked as deprecated and no new alias should be created.

I will implement a "beatlooproll_toggle" CO later today.

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 15, 2017

I will attempt to make loop_in and loop_out update beatloop_size if the new loop is an integer number of beats or a neat fraction of beats (maybe divisions of 4 or 8?). This would allow beatloops for less common time signatures like 6/8 and 7/8.

I will push an update to #940 later today making use of this. In the meantime you can use Developer Mode to test.

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 15, 2017

Actually, I think it would be a better idea to make the skin widget a subclass of QDoubleSpinBox so users could easily type in unusual beat sizes. loop_in and loop_out would not need to determine whether the loop is a beatloop.

@daschuer
Copy link
Member

Can't we always display the loop size in beats? This would gives a hint that manual set in and out markers are out of beat.

@daschuer
Copy link
Member

QDoubleSpinBox is probably not very suitable because in only supports a fixed offset.
It looks like we need a custom Edit box, that allows also to understand math, like 1/2 or [old fractional value] * 2 (just append * 2 or whatever you need.

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 15, 2017

Can't we always display the loop size in beats? This would gives a hint that manual set in and out markers are out of beat.

I suppose so. I'll try that.

QDoubleSpinBox is probably not very suitable because in only supports a fixed offset.
It looks like we need a custom Edit box, that allows also to understand math, like 1/2 or [old fractional value] * 2 (just append * 2 or whatever you need.

QDoubleSpinBox can support this by reimplementing the virtual function QAbstractSpinBox::stepBy.

Activate a rolling loop of beatloop_size beats while this button is
pressed
These will be used for intuitive control of loops with just 2 buttons in
skins and controllers.

loopauto_toggle: If beatloop_size > 1, press to set a beatloop of that
size. If beatloop_size <= 1, set a rolling beatloop while button is
held.

loopmanual_toggle: If loop is disabled, set loop in point when pressed
and loop out point when released. If loop is enabled, exit the loop on
press.
@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 15, 2017

I have implemented a beatlooproll_toggle ControlPushButton. It only keeps the loop active while the button is held down. I have also implemented two new ControlPushButtons which will be used for intuitive control of loops with just 2 buttons in skins and controllers:

loopauto_toggle: If beatloop_size > 1, press to set a beatloop of that size. If beatloop_size <= 1, set a rolling beatloop while button is held.

loopmanual_toggle: If loop is disabled, set loop in point when pressed and loop out point when released. If loop is enabled, exit the loop on press.

I am open to suggestions for different names these 2 new COs.

I will also implement JS Components utilizing these in a way that will provide comprehensive control over loops with just 4 pads and a shift button, without a need to constrain beatloops to a narrow range of sizes or switch between manual and beatloop modes.

Button 1: loopauto_toggle by default, loop_in with shift
Button 2: loopmanual_toggle by default, loop_out with shift
Button 3: loop_halve by default, loop_move_backward with shift
Button 4: loop_double by default, loop_move_forward with shift
Loop moving would move by 1 beat if beatloop_size > 1, otherwise move by beatloop_size

Buttons 1 & 2 with shift would be for modifying the in/out points of an existing loop. Pressing button 2 would be the more convenient way of setting a new manual loop.

For controllers with encoders for controlling loops:
Press: loopauto_toggle by default, loopmanual_toggle with shift
Turn: loop_halve/loop_double by default, loop_move_forward/backward with shift

@radusuciu
Copy link
Contributor

Thanks for this. Makes it a lot simpler to do complex loop operations! Are you planning on also re-working loop move and beatjump in a similar fashion?

Somewhat unrelated, butI sometimes use loops as markers of a section, ie. I find the start of a phrase, set a loop of size 16 or 32 and move it 16/32 back to indicate (I don't actually enable the loop) where I can start mixing in or out. I realize this type of thing may be better suited to a cue-point like thing but it's a lot easier to do on the fly with loops since I can manipulate their size and move them by set amounts.

@Be-ing Be-ing changed the title new beatloop interface new loop controls Feb 15, 2017
Move the loop by 1 beat if its size >= 1; otherwise move by the loop
size.
@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 15, 2017

Are you planning on also re-working loop move and beatjump in a similar fashion?

No, I have realized that would be overcomplicated. I just added new loop_move_forward/backward COs that move by 1 beat if beatloop_size >= 1 or by beatloop_size for small loops. This is how I have mapped shift + turn for the loop encoder on my P32, and I find it to be intuitive and provide all the control for moving loops I need. What separates loop moving and beatjumping from selecting a loop size is that loop moving and beatjumping can be repeatedly executed to move further, but setting a 2 beat loop 4 times does not make an 8 beat loop. Having to set the beatjump or loop move length in advance would be adding another step to the workflow that is not necessary. I did map an encoder to loop move size in my first controller mapping but I never ended up using it because it was easier to just keep moving the loop by 1 beat.

The new skin widget to show and edit a number of beats could be used for a beatjump and loop move size CO, but I think that would clutter the skins. There only needs to be 1 such number showing on screen; if there were multiple, it could get confusing which one does what (this is already the situation in Deere in master and 2.0). I think the widget may be able to be reused for beat size parameters of beat synchronized effects.

What do you think about the way I propose for mapping controllers above?

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 15, 2017

Button 1: loopauto_toggle by default, loop_in with shift
Button 2: loopmanual_toggle by default, loop_out with shift
Button 3: loop_halve by default, loop_move_backward with shift
Button 4: loop_double by default, loop_move_forward with shift

Hmm, there is something important missing from this. I think quick access to reloop on a controller is more important than loop_in/loop_out. If those are all the buttons available on a controller, loop_in/loop_out can still be accessed on the skin. These are less important to have quick access to while mixing. Here's a new proposal:

Button 1: loopauto_toggle by default, seek_loop_in with shift
Button 2: looopmanual_toggle by default, reloop_toggle with shift
Button 3: loop_halve by default, loop_move_backward with shift
Button 4: loop_double by default, loop_move_forward with shift

@radusuciu
Copy link
Contributor

My only argument for adjustable loopmove/beatjump sizes is that a size of 1 is a bit hard to deal with if you're wanting to move 16 or 32 - which I don't think is that uncommon. Moving by 1 beat is nice when you cue something up and it's just a few beats off, but for phrasing it's not so great.

I like the idea of moving the loop by the loop size - even for loops that are larger than one beat - but that doesn't really apply for beat jump.

@JosepMaJAZ
Copy link
Contributor

Ok.. It is obvious that you've made this PR with the Hercules P32 controller in mind, and thinking about translating that hardware interface to a skin. (because it is completely unnecessary for mapping such functionality)

These are the existing controls:

  • beatloop (Setup a loop over the set number of beats.)
  • beatloop_X_activate
  • beatloop_X_toggle
  • beatloop_X_enabled (status)
  • beatlooproll_X_activate
  • loop_double
  • loop_enabled (status)
  • loop_halve
  • loop_in
  • loop_out
  • loop_move (moves by beats)
  • loop_move_X_forward (move by beats)
  • loop_move_X_backward (move by beats)
  • loop_scale ( change loop size by the multiplier)
  • reloop_exit (Toggles the current loop on or off.)

And we have these additional extensions to controls:

  • _up
  • _down
  • _up_small
  • _down_small
  • _set_default
  • _toggle

Loops can be:

  • undefined
  • defined but disabled
  • defined and enabled

We have a loop status and a loop toggle
We have indicators and toggles specific for specific loop sizes.

Now, the controller defines a value that is not a defined loop, but a "going to be a loop when pressing a button". As i said, from a controller mapping perspective, this is just a variable and calling the appropiate control. From a Skin perspective, it needs a control from which to read the value, and a pushable button that tells to do the action with whatever the other control says.

As such, yes. the control about size, maybe beatloopknob_size , readable and writable, would work. This control could also support the extensions _up, _down, _set_default, and maybe even _up_small and _down_small if you want to support special cases (different than doubling/halving).

Then you need a control that defines a loop by using the size defined by the other control.
As said, "beatloop" exists, and is a control that is expected to be used from scripts to set a specific loop size. From a controller script, it still has sense, and can explicitly use the beatloopknob_size.
From a skin perspective, it is not a push button, so it still needs a new control.
Here comes into play the beatloo_toggle that you've implemented. Ok.

I am not so sure about the beatloop_enabled. When we have different buttons to setup loops, it is interesting to have the ability to see which of them is enabled ( that's why it has sense for beatloop_x_enabled ), but when using this... why? you have loop_enabled, and I don't think that you want to have a loop_enabled and beatloop_enabled indicators on the screen if using this beatloopknob.

Then, I got a bit lost on the second part, where you talk about four buttons, and reloop.

In virtualDJ with the Hercules console 4-Mx, it uses just two buttons: Loop in and Loop-out/reloop. I.e. When pressing loop in, it just prepares the loop (Sets the start position). If you press it again, it will move that start position to wherever the current position is. When pressing loop out, it defines the end and starts the loop. This is actually the only way to do it if you want to use it live rather than when preparing the track. Pressing the loop in button can be used to change the start point (probably not the most useful since it is nicer to move the end towards the beginning). Pressing the button out again disables the loop. Pressing it again reenables the loop. (reloop).
The new version of the mapping that i've made recently, implements this too for Mixxx.

So, i don't think "loop_manual" is a good thing. Probably it's better to have the loop in, and loop_out/reloop. (Anyway, a loop_manual button does not have any sense if you can't setup the manual loop).

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 18, 2017

Ok.. It is obvious that you've made this PR with the Hercules P32 controller in mind, and thinking about translating that hardware interface to a skin. (because it is completely unnecessary for mapping such functionality)

Yes, but my main motivation for these new controls is not to match the P32 and controllers like it. The main motivation for this is to clean up the clutter in skins.

This control could also support the extensions _up, _down, _set_default, and maybe even _up_small and _down_small if you want to support special cases (different than doubling/halving).

What are those special cases? Isn't that what loop_in and loop_out are for?

I am not so sure about the beatloop_enabled.

Me neither anymore.

So, i don't think "loop_manual" is a good thing.

Then you don't have to use it. :) My intention is to have a way to use manual loops with fewer buttons. It's useful for controllers like the P32 and Kontrol X1 that only have one push encoder for loops.

Pressing the button out again disables the loop. Pressing it again reenables the loop. (reloop).

How can the loop out point be adjusted for a loop that is enabled?

@JosepMaJAZ
Copy link
Contributor

JosepMaJAZ commented Feb 18, 2017

What are those special cases? Isn't that what loop_in and loop_out are for?

No. I mean that it might be interesting that _up and _down increase decrease by doubling/halving, while up_small/down_small could do it always on one beat (except if less than one beat, where they would be the same than up/down). Also, remember that we're talking about the knob here. loop_double/halve affects the existing loop.

It's useful for controllers like the P32 and Kontrol X1 that only have one push encoder for loops.

Erm... maybe it's my ignorance.. is loop_manual a renaming of "reloop_exit"?

How can the loop out point be adjusted for a loop that is enabled?

First, remember that if the loop is enabled, you can only move the loop start/end into places inside this loop.
I mentioned earlier that on virtualdj, you can reposition the loop start inside this loop while it is active (in my mixxx mapping i ended making it to toggle off the loop to avoid complications).
Moving the end point is not possible with this setup, but then you have loop halve/double, which might work or not depending on the desired effect (I mapped that to the encoder knob if i hold down the loop button).
Anway, if this is not satisfactory, Let's just keep the current three-button setup.

Button 2: looopmanual_toggle by default, reloop_toggle with shift

As for my question above, this is supposedly doing the same thing. with shift it could do the loop_out.

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 18, 2017

No. I mean that it might be interesting that _up and _down increase decrease by doubling/halving, while up_small/down_small could do it always on one beat (except if less than one beat, where they would be the same than up/down). Also, remember that we're talking about the knob here.

What knob? This value is a discrete number of beats.

loop_double/halve affects the existing loop.

I have been thinking about making beatloop_size update to the size of an existing loop on track load if there is one. beatloop_size would also update with loop_in and loop_out. Then changing beatloop_size could change the size of the loop. Thoughts on this?

Erm... maybe it's my ignorance.. is loop_manual a renaming of "reloop_exit"?

No, as I stated above:
loopmanual_toggle: If loop is disabled, set loop in point when pressed and loop out point when released. If loop is enabled, exit the loop on press.

"reloop_toggle" is the new name for "reloop_exit".

@JosepMaJAZ
Copy link
Contributor

What knob? This value is a discrete number of beats.

I am assuming that you want to put that on a control on the skin, and I don't expect that the user writes the value with the keyboard, so I assumed that the natural way to have the control is mimmicking the hardware knob + display.
And yes, it is a discrete number of beats, but having the _up and _up_small controls could mean that it could run like : 8,9,10,11,12,13,14,15,16 or like 4,8,16. (anyway, I realize now that these would only matter with a button, not with a knob.)

No, I would not alter the beatloop_size with track changes. It might be unexpected. Updating with loop_in, loop_out might be logical (At least responds to a direct action made by the user related to loops), but no, you shouldn't directly connect that to changing an existing loop. On my controller, as I said, i alter the loop size with the knob while holding the loop button. At much, let it change the loop size while the loop is on. (you're not supposed to set a new loop while you are playing a loop in the same track, I think....)

loopmanual_toggle

Oh, ok.. So.. the loop in/loop out/reloop in a single button (actually, without reloop).

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 18, 2017

I am assuming that you want to put that on a control on the skin, and I don't expect that the user writes the value with the keyboard, so I assumed that the natural way to have the control is mimmicking the hardware knob + display.
And yes, it is a discrete number of beats, but having the _up and _up_small controls could mean that it could run like : 8,9,10,11,12,13,14,15,16 or like 4,8,16. (anyway, I realize now that these would only matter with a button, not with a knob.)

As mentioned above, I'm going to make the skin widget a subclass of QDoubleSpinBox so users can manually enter any arbitrary beat size with their keyboard. That is needed for loops of set beat numbers to be useful for unusual time signatures. I do not think creating _up/_down(_small) COs like a ControlPotmeter would be necessary when the user can just type in a number.

No, I would not alter the beatloop_size with track changes. It might be unexpected. Updating with loop_in, loop_out might be logical (At least responds to a direct action made by the user related to loops), but no, you shouldn't directly connect that to changing an existing loop. On my controller, as I said, i alter the loop size with the knob while holding the loop button. At much, let it change the loop size while the loop is on. (you're not supposed to set a new loop while you are playing a loop in the same track, I think....)

Currently, loop_halve and loop_double alter beatloop_size and the length of the loop if there is one set. This creates a confusing situation when there is already a loop set with a different size than beatloop_size, because loop_halve/double change the length of the preset loop and beatloop_size together, so they will be out of sync until beatloop_toggle is used. A similar situation occurs with manual loops. There are two general ways to solve this:

1: Halving/doubling beatloop_size could be separated from adjusting the size of the loop.
2: beatloop_size could track the actual size of the loop

I don't think 1 is a good solution. It would require two more buttons on screen and make mapping loop encoders awkward. Would turning the encoder halve/double beatloop_size or the loop?

I think 2 would be a good solution. I think it would be nice to see the length of the loop for loops that are already set and for manual loops.

Oh, ok.. So.. the loop in/loop out/reloop in a single button (actually, without reloop).

Yes. Reloop would be shift + that button.

Be-ing added a commit to Be-ing/mixxx that referenced this pull request May 29, 2017
Be-ing added a commit to Be-ing/mixxx that referenced this pull request May 29, 2017
and remove reloop CO which was removed from PR mixxxdj#1187
radusuciu added a commit to radusuciu/mixxx that referenced this pull request Jun 20, 2017
@esbrandt esbrandt mentioned this pull request Jun 24, 2017
37 tasks
daschuer added a commit that referenced this pull request Jun 26, 2017
Update Mixtrack 3 mapping to support #1187
@Be-ing Be-ing mentioned this pull request Jul 25, 2017
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.

7 participants