allow for overlapping beatlooprolls#1812
Conversation
This is necessary if multiple beatlooproll keys are pressed on a controller. Previously, Mixxx would stop the loop roll if any keys were released. Now it tracks all active rolls and will activate the last active one that is still active (button still down) instead of deactivating all rolls. Additionally, if a loop roll is active, activating another beat loop roll will start the loop from the start of the current loop, instead the current play position.
|
Thank you for the Pull request. |
daschuer
left a comment
daschuer
left a comment
There was a problem hiding this comment.
Works good, I have only some minor comments.
Thank you.
| slotBeatLoop(pBeatLoopControl->getSize(), false, true); | ||
| slotBeatLoop(pBeatLoopControl->getSize(), m_bLoopRollActive, true); | ||
| m_bLoopRollActive = true; | ||
| m_activeLoopRolls.push(pBeatLoopControl); |
There was a problem hiding this comment.
taking a plain pointer in slotBeatLoopActivateRoll() promises not to store it. This rule is violated here.
Can we just store the size instead?
There was a problem hiding this comment.
Yes. I'll make that change.
There was a problem hiding this comment.
If we store doubles in here, are we going to have any issues comparing them using ==? I don't understand floats well enough to say. The values should be exact though, so my gut says this is fine.
| Q_UNUSED(pBeatLoopControl); | ||
| setLoopingEnabled(false); | ||
| pBeatLoopControl->deactivate(); | ||
| m_activeLoopRolls.removeAll(pBeatLoopControl); |
There was a problem hiding this comment.
RemoveAll is not available in QT 5.2
Can you use this instead?
int i;
while (0 <= (i = m_activeLoopRolls.indexOf(pBeatLoopControl))) {
m_activeLoopRolls.remove(i);
}
There was a problem hiding this comment.
Will update. In theory there should only be one of each beat loop in here max. Actually thinking about it, if a controller had two ways to activate a loop roll we might end up with duplicates. So looping here is necessary.
There was a problem hiding this comment.
Made the removal loop O(n) (though the stack will always be so small it won't matter). It is slightly more complex now though.
|
Ooouh, sounds interesting!! I got used to the way it works right now, but your idea makes sense. Didn't test this yet. Is this the right place to ask for taking a look at lp:1721881 beatloop shouldn't alter beatloop size in spinbox ? Also, just yesterday I wished I could transfrom a rolling beatlopp into a 'regular' one, to resize it and move it. I got stucked when thinking about the controller implementation... Edit there's also #1428 that stalled somehow |
|
Thanks! To make sure we dont break this in the future could you please add some unit tests? |
|
Not sure if there is a launchpad bug for it, I didn't check. I'll work on some tests. |
Tests validate the following: - beatlooproll activation and deactivation work - overlapping beatlooprolls remain active until the last roll is released - overlapping beatlooprolls are properly activated as they stack and unwind - new beatlooprolls start at the play position - beatlooprolls activated while existing beatloops are in progress preserve the start point
|
@rons0 transforming a loop, in a way that you can release you finger and the loop keeps looping? |
|
I have just filed a bug. |
|
The Travis failure is unrelated. |
4 participants
This is necessary if multiple beatlooproll keys are pressed on a controller. Previously, Mixxx would stop the loop roll if any keys were released. Now it tracks all active rolls and will activate the last active one that is still active (button still down) instead of deactivating all rolls. Additionally, if a loop roll is active, activating another beat loop roll will start the loop from the start of the current loop, instead the current play position.
I believe this loop roll behavior is more intuitive and natural.