Add beatloop anchor to set and adjust loop from either start or end

[acolombier]

Add the ability to define what is the static point of the loop or which point gets inherited from the current position.

Better wording suggestion would be greatly appreciated!

Kooha-2024-02-07-10-30-13.webm

Resolves #12737

[daschuer]

Thank you for strating this topic. Can you take s screenshot of the new button in it skin area?

The icon itself does not work well for. It looks like a beat grid edit icon. Did you consider to reuse the icons of the loop-in/out button?

I can think of a loop-out button with a lock overlay for the reverse case.

Or the full loop icon with a pointer or lock overlay right or left.

[daschuer]

The most puzzling question is how make this accessible from the controllers. Did you consider this?

We are currently facing the issue that the loop out button alone does nothing or surprising things. Did you consider to make use of it?

[daschuer]

I think you can read the value of m_pCOBeatLoopAnchorToggle right away using toBool() without any connection.

[acolombier]

I've used toBool() but not sure how I can make the toggle work without connection. Could yous how me a example?

[daschuer]

You need to make the control a toggle pushbutton like here:

mixxx/src/skin/skinloader.cpp

Line 233 in 7b3e485

m_pSelectBigSpinnyCover->setButtonMode(ControlPushButton::TOGGLE);

[acolombier]

Sorry, I'm not sure how you want me to remove the connection. This should already be a ControlPushButton. In the example you've linked, there also seem to be a connect

m_pSelectBigSpinnyCover = std::make_unique<ControlPushButton>(
            ConfigKey("[Skin]", "select_big_spinny_or_cover"), true);
m_pSelectBigSpinnyCover->setButtonMode(ControlPushButton::TOGGLE);
connect(m_pSelectBigSpinnyCover.get(),
            &ControlObject::valueChanged,
            this,
            &SkinLoader::updateSpinnyCoverControls);

Just to make sure you haven't missed it, but this connect is for the toggle (loop_anchor_toggle) control, which flip the state of the main CO (loop_anchor), later use in the loop logic. Do you think I can still remove the connect for the toggle? If yes, could you make a code suggestion?

[daschuer]

The toggling is handled inside the control, via ControlPushButton::TOGGLE
You need only to connect the signal when you like to react on the event. But this is not the case here.

[acolombier]

Why do I not want to react to the toggle event here? Since the loop anchor state is stored in an other CO, do I not need to replicate it here? I'm confused

[daschuer]

I think you can do it with just with one CO. But maybe I miss something?

[acolombier]

Thank you for strating this topic. Can you take s screenshot of the new button in it skin area?

I have put a little screen recording in the PR description. Here is a focus on the section as well

The icon itself does not work well for. It looks like a beat grid edit icon. Did you consider to reuse the icons of the loop-in/out button?

I can think of a loop-out button with a lock overlay for the reverse case.

Yeah, I've used the loop-in SVG as a starting point, but I must confess I have no clue what I'm doing in Inkscape. Some help on the icons would be greatly appreciated :)

I will update change the start one to be loop-in + the lock and same for end.

The most puzzling question is how make this accessible from the controllers. Did you consider this?

Yep, I have already implemented it on my controller, before doing the UI work in order to test the feature on a real setup.

Currently with my S4 MK3 mapping, if you keep the "Sample" pressed, the loop_anchor is set to End. Using loop_anchor_toggle, one may also use a toggle rather than a hold.

We are currently facing the issue that the loop out button alone does nothing or surprising things. Did you consider to make use of it?

Yes, it does now weird thing consistently in the tho modes. At first glance, there seems to be multiple issues, but I believe this will be out of scope for the current piece of work.

[acolombier]

I have rework the icon to follow your suggestion @daschuer

Kooha-2024-02-07-11-09-43.webm

Kooha-2024-02-07-11-19-15.webm

The classic one seems to loose its background when active but I haven't managed to find the CSS rule causing issue yet

[daschuer]

Thank you for the new Icon. I think this is good enough to see the intention. We can polish it at any time. No need to learn Inkscape ;-)

[ronso0]

Just a side note, didn't test this yet:
the loop section is becoming wider with this new button and I think we need to increase Latenight's minimum width, but that is fine because we also should definitely increase the spinbox buttons / separate them from the spinbox.

[acolombier]

I have removed the redundant CO (loop_anchor_toggle) and fixed the last UI weirdness on the classic color scheme. I have also increased the beatloop size spinbox by couple of pixel to align it with the button

Kooha-2024-02-08-10-22-24.webm

Kooha-2024-02-08-10-21-51.webm

[ronso0]

I'm wondering how this solves the use case mentioned in #12737 where you want to spontaneously create a loop backwards from the playhead. Wouldn't that require to always keep the loop anchor set to End, but how does this work with the 'regular' beatloop usecase where you want a forward loop?

[daschuer]

For my understanding you can switch between these two modes by the new toggle button.

I have not yet played with it, but I can imagine that will become my new default.

You can kind of listen the first path of the planed loop before enable it. Before you need to remove the loop quickly before the loop end if you notice something is messed up.

[ronso0]

I'm wondering how this solves the use case mentioned in #12737 where you want to spontaneously create a loop backwards from the playhead. Wouldn't that require to always keep the loop anchor set to End, but how does this work with the 'regular' beatloop usecase where you want a forward loop?

I think we should extend this by LoopAnchorPoint::Auto so the toggle has three states.
With Auto we can check if the desired loop fits in between playpos and track end, if not use LoopAnchorPoint::End and create the loop backwards.
Admittedly that makes slotBeatLoop() even more complex but it would solve the origianl issue.

What do you think?

[acolombier]

I'm afraid it may lead to unexpected behavior, since it could mean that one could "accidentally" set a loop on the last X beats rather than on the next X ones. Say you want to put a 64 beats loop on the outro that just started, but it turns out that the outro only has 32 beats, you unexpectedly end up setting the loop on the last 64 beats, leading to a jump back in the verse or chorus preceding the outro.

It's worth to point out that, this PR in its current state has already solved the issue #12737, by:

• Change the beatloop selector unconditionally of the play position and whether or not the new size would fit within the track boundary
• Allowing the loop to be set from the end

[daschuer]

I am also not sure that AUTO will work. The issue is that you can not rely on it. You need always check if the picked loop is usable or on any outro garbage. The Ideal outro loop is on some beats without lyrics or lead instruments.
For that purpose the reverse loop is ideal because of the pre-listening for free.

All the long press tricks will not work, because we need immediate responds for the end loop in the emergency use case and it will also immediately mess up the mix when doing wrong.

2. Sounds reasonable. Maybe we can implement that in any case.

Ideas:

3.
We have currently "beatloop_activate" and "beatlooproll_activate". Maybe we can remove "beatlooproll_activate" because you can do it with slip mode + beatloop_activate and have the shift + "beatloop_activate" for reverse loops?
This will not work because of the transition form old to new.

4.
Swap in/out, keep size:
1 beatloop_activate at loop in.
2 Push Loop_out to fold the loop reverse

This works for the emergency case where you realize the forward loop will produce rubbish, but probably not on a regular basis.

5.
A set of "beatloop_r4_activate" controls for putting them on a keypad.

[ronso0]

4. Swap in/out, keep size:
   1 beatloop_activate at loop in.
   2 Push Loop_out to fold the loop reverse

So a bit like my loop_out idea? (hold loop_out, press beatloop for backwards loop)

• press & hold beatloop (loop activated on press
• press loop_out for backwards loop from release position

[acolombier]

I like 3 since that would encourage more granularity on the mapping and describe behaviour more explicitly (e.g "while you press this button, you also enable slip" - as it is the case with beatlooproll_activate) - it will still be possible the set slip and unset on release as part of the script callback. Although I guess it will induce a (neglectable?) performance decrease.

4 will not work in case of emergency since you aren't able to set a lop beyond the end of a track, so 4.1 won't set the loop at all

I like 5 too although I thing it is mutual exclusive with 3 - you can also set loop_anchor=end, beatloop_4_activate and loop_anchor=start on release on a keypad mapping callback

[daschuer]

I like 5 too although I thing it is mutual exclusive with 3 - you can also set loop_anchor=end, beatloop_4_activate and loop_anchor=start on release on a keypad mapping callback

Yes, Drawback: It will mess with the loop_anchor button, and is not mappable via XML (GUI)

[acolombier]

not mappable via XML (GUI)

Yeah, that sounds like a major drawback. Perhaps option 5 is the best then? Just add a r version of each beatloop_X_activate and beatlooproll_X_activate?

[acolombier]

I have added the the r CO now for beatloop and beatlooproll. Let me know if there is something more to, otherwise will prepare the manual PR.

[ronso0]

I'm not happy with the LateNight icons, the lock is too small and has cutouts whic makes it hard to read IMO.
Will check if I can prepare a better version sooish.

[acolombier]

I have updated the LateMoon - hopefully it looks better now, although I'm sure it can always been improved

[daschuer]

@ronso0 Your review is pending. Are all your topics fixed?

[ronso0]

I'm taking a look at the LateNight visuals and will send a commit with some fixes & tweaks soonish.

[acolombier]

Are you happy to do it as a subsequent PR or do you think it should come as part of that one?

[ronso0]

Tbh I'd like to finish it now, just to get it off the table ; )

Please check these commits:
81b339c
91899b2

I desaturated the lock icons, because IMO they stood out too much in the loop section.

Looks like this now

[ronso0]

@ronso0 Your review is pending. Are all your topics fixed?

Yes, thanks.

FWIW I'd prefer if we stick to the rule that the reviewer closes his review convos if the issues/request have been addressed. That allows to quickly check for open issues.
If reviews are clsoed by other, and then references later on (like here) it's tedious to revisit convos closed by others just get back on track and check etc.

.,.

[daschuer]

Thank you, I am curious if this will change my habit with beatliops.