Fuzzy beats fix. #4366

daschuer · 2021-10-07T17:28:27Z

I have reviewed all edge case for the beat snapping algorithm and adjusted it as it makes most sense.
Normally the functions should not snap to a beat. In most cases this creates exceptional code paths which are hard to maintain, because snapping of an inner function is not expected when reading the code.

We have two exceptions where snapping is still applied:

The "beat_next" "beat_prev" COs used for caching the result of findPrevNextBeats(), along with some grown code build on it. I don't want to touch it to limit the risk of more regressions
The looping code. Here we have some odd situations related to the quantitation done by one audio buffer cycle. In case of beatloops, we need to be sure that the loop is still considered between two beats even if it overshoots slightly due to an audio buffer.

The additional double rounding issue of the BeatGrid code should be fixed now.

I am unsure about the BeatGridIterator it is implemented by repeated m_currentPosition += m_beatLengthFrames. I am unsure if it is exactly the same if we use n * m_beatLengthFrames. I am afraid not. Does anyone know?
Luckily it is only used for visual beats.

This should make #4334 obsolete and fix https://bugs.launchpad.net/mixxx/+bug/1945238

…eatGrid variant

…use it interferes with its own snapping logic.

…cause the snapped beat is the closes anyway.

This fixes a regressions from 8ef9dd3

daschuer · 2021-10-07T17:34:58Z

The CI error is unrelated:

An unexpected error has occurred: CalledProcessError: command: ('/github/home/.cache/pre-commit/repowbjfifn_/node_env-default/bin/node', '/github/home/.cache/pre-commit/repowbjfifn_/node_env-default/bin/npm', 'install', '-g', '/github/home/.cache/pre-commit/repowbjfifn_/dummy_package-0.0.0.tgz', 'eslint@7.32.0')

Holzhaus · 2021-10-08T09:26:46Z

I restarted CI.

Holzhaus

Some first comments

Holzhaus · 2021-10-08T09:28:29Z

src/track/beatgrid.cpp

-    const double nextBeat = ceil(beatFraction);
-
-    audio::FramePos closestBeatPosition;
+    double startBeat = (position - firstBeatPosition()) / m_beatLengthFrames;


I think the naming is a bit confusing. Maybe something like this?

Suggested change

double startBeat = (position - firstBeatPosition()) / m_beatLengthFrames;

double beatIndex = (position - firstBeatPosition()) / m_beatLengthFrames;

Holzhaus · 2021-10-08T09:31:41Z

src/track/beats.cpp

@@ -49,7 +49,22 @@ BeatsPointer Beats::fromBeatPositions(
    return BeatMap::makeBeatMap(sampleRate, subVersion, beatPositions);
 }

+audio::FramePos Beats::snapPosToNearBeat(audio::FramePos position) const {


This method is very similar to Beats::findClosestBeat. Can we unify this? https://github.com/mixxxdj/mixxx/blob/main/src/track/beats.cpp#L73

that code is a little dangerous for beatmap tracks which have no beats before the beginning. Loops will always snap to the first beat even if the loop start is before the beginning (i.e., no prev beat). (I have tracks where that's necessary because the track starts on an off-beat)

I think we could either

introduce a maxDelta argument to prevent this, or

make this method call findClosestBeat and then return the closest beat position if abs(position - closestBeatPosition) > kEpsilon and position otherwise.

I think for now it's acceptable to have loops snap to the first available beat. Note that with the current code, loops initiated before the first beat don't get created at all, so we should at least fix that. Reusing this existing function might fix that.

This method is very similar to Beats::findClosestBeat.

Beats::snapPosToNearBeat snaps only if a beat is within the snapping range. Beats::findClosestBeat finds the beat in any range. I think the description in the header file is already explaining this difference.

that code is a little dangerous for beatmap tracks which have no beats before the beginning. Loops will always snap to the first beat even if the loop start is before the beginning (i.e., no prev beat). (I have tracks where that's necessary because the track starts on an off-beat)

You can't set beat loops in beatmap tracks before the first beat. However loops are snapped to the next beat when you are in the snapping region. Extrapolating into the preroll would be a new feature. I think We should postpone it to a follow up PR.

ywwg · 2021-10-08T16:45:05Z

src/engine/controls/loopingcontrol.cpp

@@ -1266,20 +1266,21 @@ void LoopingControl::slotBeatLoop(double beats, bool keepStartPoint, bool enable
        } else {
            newloopInfo.startPosition = currentPosition;
        }
-    } else {
+    } else if (m_pQuantizeEnabled->toBool()) {


I can see that you moved this here to avoid an unnecessary beats lookup. This nesting is getting deep and confusing, is there a way we can rearrange the logic to simplify it? I think the whole block could be pulled into a new small function ("findLoopStart"?) that takes the current start position and loop length and returns a new one. That would allow us to return as soon as we find the new start position, which I think is the only output of this block:

ex:

if (!m_pQuantizeEnabled->toBool()) { return currentPosition; }

ywwg · 2021-10-08T16:56:19Z

This code works correctly with my most recent failure case. For beatmap tracks, trying to create a loop before the first beat results in nothing -- clicking "make a 4 beat loop" here has no effect:

…beat maps

…first beat by using the beat length of the fist beat.

daschuer · 2021-10-11T00:09:05Z

Review comments are integrated. A bug when playing reverse was fixed and we can now set a beat loop one beat before the first beat.

ywwg · 2021-10-21T02:48:19Z

This still doesn't create any loop if the playhead is before the first beat in a beatmap track. Can we make it round up to the first beat in that situation? (see my previous comment)

daschuer · 2021-10-21T06:46:18Z

The feature is limited in the region of one beat length before the first beat.
This should be sufficient to fix the original rounding bug.

This PR does not fix the issue when you are way before the first beat.

A solution is more complicated, because normally the first beats is not really significant for a suitable tempo in an initial loop.

We need a more sophisticated solution that consideres all other beats in the loop.
I have something in mind that we add the beat length of the next bear after loop out, as if you have shifted the loop one beat back snapping the loop out position.

A topic for another PR.

ywwg

Yeah I would still like something where we propagate the first beat length backward in time to work around these issues.

OK I see how this still works when the playhead is slightly before the first beat. That is good enough for now. thanks! LGTM

Holzhaus · 2021-10-22T06:40:00Z

Yeah I would still like something where we propagate the first beat length backward in time to work around these issues.

Please check out #4255 which does exactly that :D

daschuer added 7 commits October 7, 2021 17:12

Reimplement the findNBeatsFromPosition for BeatGrid without any rounding

6d103e7

Remove snapping from BeatMap::findNBeatsFromPosition() to match the B…

6749023

…eatGrid variant

Don't use findPrevNextBeats() in snapping mode in slotBeatLoop() beca…

a839493

…use it interferes with its own snapping logic.

Don't use findPrevNextBeats() in snapping mode in findClosesBeat() be…

736c7cb

…cause the snapped beat is the closes anyway.

Don't lookup beats if the next and pervious COs are valid.

a1f2c2a

This fixes a regressions from 8ef9dd3

Add Beats::snapPosToNearBeat() and use it in numBeatsInRange()

35cc59f

Simplify BeatGrid::findNthBeat()

1f2e3f5

daschuer mentioned this pull request Oct 7, 2021

Revert "BeatGrid: Remove fuzzy behavior from findNthBeat" #4334

Closed

Holzhaus reviewed Oct 8, 2021

View reviewed changes

ywwg reviewed Oct 8, 2021

View reviewed changes

daschuer added 7 commits October 10, 2021 07:29

Rename startBeat to fromBeatIndex

7f4d22a

Rearrange the control flow in slotBeatLoop() for better readability.

e63181e

split out findQuantizedBeatloopStart()

99e8ec6

Fix beat loops for the reverse case when not quantized and non const …

13fb27d

…beat maps

Avoid recalculation of beat positions if track is paused.

bbc8c8f

Simplify BeatMap::findPrevNextBeats and consider the area before the …

d45fc05

…first beat by using the beat length of the fist beat.

use p prefix for pointer parameters in findPrevNextBeats()

939abd9

remove unused variable

f9387d4

Be-ing requested a review from ywwg October 12, 2021 18:32

ywwg approved these changes Oct 21, 2021

View reviewed changes

ywwg merged commit c3f913a into mixxxdj:main Oct 23, 2021

daschuer deleted the fuzzy_beats branch April 14, 2022 21:20

mixxxbot mentioned this pull request Aug 23, 2022

beatjump by 1 beat sometimes beatjumps by 2 beats #10510

Closed

mixxxbot mentioned this pull request Aug 23, 2022

wrong sized loops #10541

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fuzzy beats fix. #4366

Fuzzy beats fix. #4366

daschuer commented Oct 7, 2021

daschuer commented Oct 7, 2021

Holzhaus commented Oct 8, 2021

Holzhaus left a comment

Holzhaus Oct 8, 2021

Holzhaus Oct 8, 2021

ywwg Oct 8, 2021

Holzhaus Oct 9, 2021 •

edited

Loading

ywwg Oct 9, 2021

daschuer Oct 10, 2021

daschuer Oct 10, 2021

ywwg Oct 8, 2021

ywwg commented Oct 8, 2021

daschuer commented Oct 11, 2021

ywwg commented Oct 21, 2021

daschuer commented Oct 21, 2021

ywwg left a comment

Holzhaus commented Oct 22, 2021

	double startBeat = (position - firstBeatPosition()) / m_beatLengthFrames;
	double beatIndex = (position - firstBeatPosition()) / m_beatLengthFrames;

Fuzzy beats fix. #4366

Fuzzy beats fix. #4366

Conversation

daschuer commented Oct 7, 2021

daschuer commented Oct 7, 2021

Holzhaus commented Oct 8, 2021

Holzhaus left a comment

Choose a reason for hiding this comment

Holzhaus Oct 8, 2021

Choose a reason for hiding this comment

Holzhaus Oct 8, 2021

Choose a reason for hiding this comment

ywwg Oct 8, 2021

Choose a reason for hiding this comment

Holzhaus Oct 9, 2021 • edited Loading

Choose a reason for hiding this comment

ywwg Oct 9, 2021

Choose a reason for hiding this comment

daschuer Oct 10, 2021

Choose a reason for hiding this comment

daschuer Oct 10, 2021

Choose a reason for hiding this comment

ywwg Oct 8, 2021

Choose a reason for hiding this comment

ywwg commented Oct 8, 2021

daschuer commented Oct 11, 2021

ywwg commented Oct 21, 2021

daschuer commented Oct 21, 2021

ywwg left a comment

Choose a reason for hiding this comment

Holzhaus commented Oct 22, 2021

Holzhaus Oct 9, 2021 •

edited

Loading