Looping/Beatjump: use seconds if track has no beats #12961

ronso0 · 2024-03-13T13:41:56Z

Finally allows looping and beatjumping for tracks with no beats.

If the track has no beats, simply load fake beats: 60 BPM, first beat at track start.
1 'beat' = 1 second
Looping and beatjumping works as desired 🎉

edit: Also sets a m_trueTrackBeats flag so loops don't unexpectedly snap to the (invisible) fake beats.

For completeness we'd need to add one string to adjust all related tooltips, and one for all items in controlpickermenu.cpp.
If this string freeze viloation is considered not worth for having this nice fix in 2.4.1, or if you think this is definitely a new feature, I'll rebase onto main.

TODO

update tooltips
update controlpickermenu
update manual Looping: mention seconds fallback in case track has no beats manual#630

Fixes #11124

ronso0 · 2024-03-13T14:23:33Z

let's see if the tests pass.

Ah, of course not, they assume no-ops for no beats.
I'll take a look.

ronso0 · 2024-03-13T23:32:55Z

I expected that Looping tests would fail now, but why on earth do they pass on macOS arm64??

…eats

ronso0 · 2024-03-14T13:25:12Z

All tests pass 🎉

So it was just that quantize had to be disabled where it is not explicitely expected/required 🙄

…case

daschuer · 2024-03-15T08:08:23Z

Thank you for taking care.

An alternative solution would be a default of 120 bpm.
That will likely be closer to the usual distance in other tracks. But destroys the easy to explain "1 'beat' = 1 second" assumption.

I have no strong opinion here, I just want to make sure we fix the issue optimal.

What are the use-cases? Do we have one, or is it just to do "something" that the buttons not feel broken?

I can think of vokal tracks, ambient tracks. Currently Mixxx applies a beat grid to all of them l, so the user has artificial removed and locked the bpm for some reason.

ronso0 · 2024-03-15T08:59:33Z

IMO "1 'beat' = 1 second" makes it predictable.

Yes, ambient tracks. Tbh me personally ran into this situation only a few times in live situations when attempting to mix in ambient tracks. The UX is currently like this:

play a mix with beats tracks
load an ambient track (I explicitely clear the beats for those when tagging new tracks)
yeah, I want to loop this!
hit a beatloop button
nothing happens
why?
oh, no beats..
(either just drop the lopping idea because those few crucial seconds passed until I realized what happens, and a loop wouldn't make sense anymore, or)
jump back, set loop manually

I use a encoder, so I usually press (4 beats) and instantly turn it to adjust the loop as desired (visually, to either constrain the loop to beatless regions, or the way around, avoid a beatless tail).
Using seconds right away would be a huge step forwrads in terms of UX.

ronso0 · 2024-03-15T09:06:49Z

FWIW I didn't test with quantize yet.
I can't really judge if the new behaviour somehow conflicts with users' expectations if quantize is still enabled, because "no (visible) beats" would actually mean "no quantize".

In order to clarify that situation and not snap seconds loops to invisible beats, we may consider to also set a m_fakeBeats flag when applying the fake beats and use that to override 'quantize enabled'.
Then cue/hotcue behaviour and looping behaviour would be consistent whit quantize and no beats.

daschuer · 2024-03-15T09:07:59Z

IMO "1 'beat' = 1 second" makes it predictable.

A OK since you have a real use case and this works for you, it should be a good choice.

daschuer · 2024-03-15T09:13:52Z

src/engine/controls/loopingcontrol.cpp

+    } else {
+        m_pBeats = getFake60BpmBeats();
+    }
+    // TODO All "if (m_pBeats)" checks are now obsolete actually...


Only if we also initialize it in the constructor. This would be the first use case of a not-null smart pointer. A fun project out of the original scope here.

daschuer · 2024-03-15T09:16:07Z

m_fakeBeats

This flag sounds reasonable.

ronso0 · 2024-03-15T09:55:27Z

Added the flag, now waiting curiously how the tests like that...

ronso0 · 2024-03-15T10:29:38Z

Cooool, it works and tests pass.

Bugfix or feature?
2.4.1 or main?

I'd argue for bugfix, but I don't mind moving this to 2.5 in order to increase the chance of getting completed translations.

daschuer · 2024-03-15T11:28:30Z

My vote is for 2.4

not just the fake beats we use for looping/jumping by seconds. This will prevent unexpectedly placing loop markers on (invisible) fake beat markers.

JoergAtGithub · 2024-03-16T10:41:49Z

IMHO, this is a feature with a risk of side effects and should go into Main therefore.

daschuer · 2024-03-16T13:55:03Z

What type of side effects do you expect?

Before: buttons have no function
Now: Buttons work

JoergAtGithub · 2024-03-16T14:03:57Z

I'm not aware of any particular problem, but information about loop length is used at many different places of our code (mappings, skins, waveforms, vsynthread), therefore the probability to break something is high.

ronso0 · 2024-03-16T16:23:07Z

I don't see what could be broken:

the fake beats added here are used only in these looping/beatjump classes
the loops created by them are treated like any other beatloop
other classes don't care about the loop/beat relation, no effect on other behaviour
all looping and beatjump tests pass

Note I don't mind to move this to 2.5, but only if there are valid concerns.

JoergAtGithub · 2024-03-16T17:37:35Z

* other classes don't care about the loop/beat relation, no effect on other behaviour

At least clockcontrol takes the beat/loop relation into account too.

ronso0 · 2024-03-16T18:51:12Z

Sorry, my phrasing was unclear.
What I mean is that no other class gets the beats from LoopingControl, the fake beats are entirely internal for that class. Other classes (EngineBuffer) only get the LoopInfo and that does not contain any beat information.
ClockControl uses the track beats and the LoopInfo COs.

acolombier · 2024-03-22T09:33:42Z

An alternative solution would be a default of 120 bpm.

I do agree with this too. Perhaps we could introduce a user preference for this? This way we can default to 60 BPM but also allow user to have other default depending of the BPM range they usually works with?

ronso0 · 2024-03-22T09:47:53Z

Yup, we can certainly do that.
Though I'd like to get this merged first.

ronso0 · 2024-04-05T12:24:44Z

Moved this to 2.5-beta

daschuer · 2024-04-21T18:25:28Z

Do we have a conclusion for the merge target here? @JoergAtGithub?

JoergAtGithub · 2024-04-21T18:39:59Z

This is an enhancement and not fixing a bug. My opinion is still, that this is too risky for a bugfix release!

JoergAtGithub · 2024-04-21T18:41:07Z

src/engine/controls/loopingcontrol.cpp

+        m_pBeats = getFake60BpmBeats();
+        m_trueTrackBeats = false;
+    }
+    // TODO All "if (m_pBeats)" checks are now obsolete actually...


Than these if (m_pBeats) need to be removed from the code

I have the impression that it is used as if (m_pTrack).
I'll take a look.

If refined updateBeats() so we now have a Beats nullptr again if no track is loaded, and all if (m_pBeats) checks can stay.

JoergAtGithub · 2024-04-21T18:43:23Z

src/test/hotcuecontrol_test.cpp

-TEST_F(HotcueControlTest, CueLoopWithoutLoopOrBeats) {
-    createAndLoadFakeTrack();
-
-    EXPECT_DOUBLE_EQ(static_cast<double>(HotcueControl::Status::Empty), m_pHotcue1Enabled->get());
-    EXPECT_FALSE(mixxx::audio::FramePos::fromEngineSamplePosMaybeInvalid(
-            m_pHotcue1Position->get())
-                         .isValid());
-    EXPECT_FALSE(mixxx::audio::FramePos::fromEngineSamplePosMaybeInvalid(
-            m_pHotcue1EndPosition->get())
-                         .isValid());
-
-    m_pHotcue1CueLoop->set(1);
-    m_pHotcue1CueLoop->set(0);
-
-    EXPECT_DOUBLE_EQ(static_cast<double>(HotcueControl::Status::Set), m_pHotcue1Enabled->get());
-    EXPECT_FRAMEPOS_EQ_CONTROL(mixxx::audio::kStartFramePos, m_pHotcue1Position);
-    EXPECT_FALSE(mixxx::audio::FramePos::fromEngineSamplePosMaybeInvalid(
-            m_pHotcue1EndPosition->get())
-                         .isValid());
-    EXPECT_FALSE(m_pLoopEnabled->toBool());
-}
-


Why did you remove these unit tests? Now we have more cases to test and not less.

This tests that there is no (cue) loop created if the track has no beats. This situation will not occur anymore (with a track loaded).

We don't need to test "no beats" cases --> less tests.
What new cases are you referring to?

ronso0 · 2024-04-21T23:51:51Z

This is an enhancement and not fixing a bug. My opinion is still, that this is too risky for a bugfix release!

I already assigned it to the 2.5-beta milestone, just need to change the base branch.

I'm still curious where you see the risk, please elaborate.

…no-beats

ronso0 · 2024-05-28T11:23:17Z

ping @JoergAtGithub @daschuer
See latest changes #12961 (comment) / 761c7d6, CI is all green now.

I moved it to 2.5-beta since. IMO it's still a bugfix for 2.4 but due concerns I shifted it, so let's try to get it in soonish (i don't see the need to delay this until 2.6).

daschuer

LGTM. We have not yet a formal beta so it should be OK to have the string changes included.

JoergAtGithub · 2024-05-28T20:18:23Z

Thank you!

github-actions bot added the engine label Mar 13, 2024

ronso0 added the looping label Mar 13, 2024

github-actions bot added the code quality label Mar 13, 2024

ronso0 force-pushed the loop-jump-seconds-with-no-beats branch 8 times, most recently from aebdb89 to 0e2bed5 Compare March 14, 2024 12:56

ronso0 added 2 commits March 14, 2024 14:11

Looping/Beatjump: use seconds if track has no beats

514b71e

Looping: fix tests after implementing fake beats for tracks with no b…

1cfcc1b

…eats

ronso0 force-pushed the loop-jump-seconds-with-no-beats branch from 0e2bed5 to 1cfcc1b Compare March 14, 2024 13:14

update tooltips and control descriptions for 'no beats, use seconds' …

079cf44

…case

daschuer reviewed Mar 15, 2024

View reviewed changes

Looping: act quantized only if we have true track beats ...

01ed90b

not just the fake beats we use for looping/jumping by seconds. This will prevent unexpectedly placing loop markers on (invisible) fake beat markers.

ronso0 force-pushed the loop-jump-seconds-with-no-beats branch from 3aaccf6 to 01ed90b Compare March 15, 2024 12:10

ronso0 mentioned this pull request Mar 22, 2024

Looping: mention seconds fallback in case track has no beats mixxxdj/manual#630

Open

ronso0 added this to the 2.5-beta milestone Apr 5, 2024

ronso0 modified the milestones: 2.5-beta, 2.4.2 Apr 13, 2024

JoergAtGithub requested changes Apr 21, 2024

View reviewed changes

ronso0 changed the base branch from 2.4 to main April 21, 2024 23:52

ronso0 modified the milestones: 2.4.2, 2.5-beta May 27, 2024

ronso0 changed the base branch from main to 2.5 May 27, 2024 21:55

ronso0 added 3 commits May 28, 2024 00:18

Merge remote-tracking branch 'mixxx/2.5' into loop-jump-seconds-with-…

9e91ead

…no-beats

adjust looping/seconds tooltip and control description

e7ffb5b

Looping: revert to beats nullptr if no track is loaded

761c7d6

daschuer approved these changes May 28, 2024

View reviewed changes

JoergAtGithub merged commit d0f03e4 into mixxxdj:2.5 May 28, 2024
13 checks passed

ronso0 deleted the loop-jump-seconds-with-no-beats branch May 28, 2024 20:47

ronso0 mentioned this pull request Aug 31, 2024

make Beatjump use seconds if no beatgrid is detected #11124

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Looping/Beatjump: use seconds if track has no beats #12961

Looping/Beatjump: use seconds if track has no beats #12961

ronso0 commented Mar 13, 2024 •

edited

Loading

ronso0 commented Mar 13, 2024

ronso0 commented Mar 13, 2024

ronso0 commented Mar 14, 2024 •

edited

Loading

daschuer commented Mar 15, 2024

ronso0 commented Mar 15, 2024

ronso0 commented Mar 15, 2024

daschuer commented Mar 15, 2024

daschuer Mar 15, 2024

daschuer commented Mar 15, 2024

ronso0 commented Mar 15, 2024

ronso0 commented Mar 15, 2024

daschuer commented Mar 15, 2024

JoergAtGithub commented Mar 16, 2024

daschuer commented Mar 16, 2024

JoergAtGithub commented Mar 16, 2024

ronso0 commented Mar 16, 2024

JoergAtGithub commented Mar 16, 2024

ronso0 commented Mar 16, 2024 •

edited

Loading

acolombier commented Mar 22, 2024

ronso0 commented Mar 22, 2024

ronso0 commented Apr 5, 2024

daschuer commented Apr 21, 2024

JoergAtGithub commented Apr 21, 2024

JoergAtGithub Apr 21, 2024

ronso0 Apr 21, 2024

ronso0 May 27, 2024

JoergAtGithub Apr 21, 2024

ronso0 Apr 21, 2024

ronso0 commented Apr 21, 2024

ronso0 commented May 28, 2024

daschuer left a comment

JoergAtGithub commented May 28, 2024

Looping/Beatjump: use seconds if track has no beats #12961

Looping/Beatjump: use seconds if track has no beats #12961

Conversation

ronso0 commented Mar 13, 2024 • edited Loading

TODO

ronso0 commented Mar 13, 2024

ronso0 commented Mar 13, 2024

ronso0 commented Mar 14, 2024 • edited Loading

daschuer commented Mar 15, 2024

ronso0 commented Mar 15, 2024

ronso0 commented Mar 15, 2024

daschuer commented Mar 15, 2024

daschuer Mar 15, 2024

Choose a reason for hiding this comment

daschuer commented Mar 15, 2024

ronso0 commented Mar 15, 2024

ronso0 commented Mar 15, 2024

daschuer commented Mar 15, 2024

JoergAtGithub commented Mar 16, 2024

daschuer commented Mar 16, 2024

JoergAtGithub commented Mar 16, 2024

ronso0 commented Mar 16, 2024

JoergAtGithub commented Mar 16, 2024

ronso0 commented Mar 16, 2024 • edited Loading

acolombier commented Mar 22, 2024

ronso0 commented Mar 22, 2024

ronso0 commented Apr 5, 2024

daschuer commented Apr 21, 2024

JoergAtGithub commented Apr 21, 2024

JoergAtGithub Apr 21, 2024

Choose a reason for hiding this comment

ronso0 Apr 21, 2024

Choose a reason for hiding this comment

ronso0 May 27, 2024

Choose a reason for hiding this comment

JoergAtGithub Apr 21, 2024

Choose a reason for hiding this comment

ronso0 Apr 21, 2024

Choose a reason for hiding this comment

ronso0 commented Apr 21, 2024

ronso0 commented May 28, 2024

daschuer left a comment

Choose a reason for hiding this comment

JoergAtGithub commented May 28, 2024

ronso0 commented Mar 13, 2024 •

edited

Loading

ronso0 commented Mar 14, 2024 •

edited

Loading

ronso0 commented Mar 16, 2024 •

edited

Loading