Reworked beat_active control: #3608

JoergAtGithub · 2021-01-31T17:06:26Z

-Made CO beat_active read-only
-Set CO only if there's a state change
-Update from engine::updateIndicators instead of process, which isn't updated enough for a reliable beat indicator
-Added NoTolerance mode in beatgrid.cpp
-Don't calculate if deck is not playing or vinyl/jog is hold standstill (inside +-2.5ms tracktime at normal speed)
-Set beat_active at the beat not +-50ms before/after
-Reverse state
-Period of beat_active on is no longer hard coded 200ms, it's no 20% of the peat period

See also discussion at: https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/beat_active

Holzhaus · 2021-01-31T17:36:10Z

Thanks. Please open a manual PR to document the new behavior. That will make review easier.

-Made CO beat_active read-only -Update from engine::updateIndicators instead of process, which isn't updated enough for a reliable beat indicator -Fixed accuracy issue in beatgrid.cpp (compare to 1/100 of a beat period can be severalsamples off) -Don't calculate if deck is not playing -Set beat_active at the beat not +-50ms before/after -More states -Period of beat_active on is no longer hard coded 200ms, it's no 20% of the peat period

JoergAtGithub · 2021-02-04T21:35:50Z

Thanks. Please open a manual PR to document the new behavior. That will make review easier.

Documentation is in mixxxdj/manual#347

…ithout tolerance epsilon

daschuer · 2021-02-21T22:25:34Z

I am unsure if this is now just right. What is your use case for -1 -0.5 and 0.5?

The simple use case is just a beat LED. It can be controlled like
if (beat_active) {
set_led()
} else {
reset_led()
}

This works perfectly if we play forward and backward. But IMHO not too nice when scratching, because it will not reset after changing direction. My expectation is that I can cue by eye just like I can do by ear. Therefore a change at the beat is required.

if (beat_active >= 1 || beat_active <= -1) {
set_led()
} else {
reset_led()
}

will be a solution.

daschuer · 2021-02-21T22:31:37Z

src/engine/controls/clockcontrol.cpp

+
+            m_blinkIntervalSamples = (m_NextBeatSamples - m_PrevBeatSamples) * kBlinkInterval;
+        }
+        //qDebug() << "dRate:" << dRate << " m_lastPlayDirection:" << m_lastPlayDirection << " m_pCOBeatActive->get(): " << m_pCOBeatActive->get() << " currentSample: " << currentSample << " m_lastEvaluatedSample: " << m_lastEvaluatedSample << " m_PrevBeatSamples: " << m_PrevBeatSamples << " m_NextBeatSamples: " << m_NextBeatSamples << " m_blinkIntervalSamples: " << m_blinkIntervalSamples;


Please break these long lines.

I will remove this line later - it's just for my own debugging.

daschuer

Thank you very much for taking care.
The code looks good. Just a minor code style comment.
do you the precommit hook?
https://github.com/mixxxdj/mixxx/wiki/Using-Git#set-up-automatic-code-checking

JoergAtGithub · 2021-02-21T22:52:59Z

I am unsure if this is now just right. What is your use case for -1 -0.5 and 0.5?

The intention was to give the mapping developer all possibilities:
1)Search the beat by LED (therfore the LED must go of if direction changes)
2)Only show LED in normal playing direction.
3)LED On if position is close to the beat independent of direction

But maybe it's too much. If I remove the +- 0.5 states an use the value 0 instead, a simple
if (beat_active !== 0)
set_led()
} else {
reset_led()
}
would work for case 1.
Only case 3) would be impossible. The reason for case 3 is taht it's similar to the current behavior

JoergAtGithub · 2021-02-21T22:58:46Z

do you the precommit hook?

No, I don't use it. But the pre-commit check in the GitHub actions passed. Isn't this the same check?

JoergAtGithub · 2021-02-21T23:05:33Z

Regarding the state of this code:

Slow searching of a position with vinyl and LED works very well now!
But while playing, the indication is not always at the expected moment. At least no beat seems to be missing anymore.
My laptop is not very powerfull, maybe this is the reason

daschuer · 2021-02-21T23:45:17Z

I won't use 3) do you?
If not, let's remove it to simplify the other two.

daschuer · 2021-02-21T23:47:48Z

do you the precommit hook?

No, I don't use it. But the pre-commit check in the GitHub actions passed. Isn't this the same check?

Yes, that should be the same. I have no Idea why it is not complaining about the long comments.
My point is just to remove the annoyance of vertical scrolling during code reviews.

daschuer · 2021-02-21T23:50:57Z

Did you test using a controller LED or a GUI LED? I can imagine that there is a significant jitter due to the processing of the midi messages. Do you have similar issues with the VU meter?

The CO returns now 0 for the cases of play direction changes while beat indication is on

JoergAtGithub · 2021-02-27T10:23:22Z

I won't use 3) do you?
If not, let's remove it to simplify the other two.

I changed this, the states +-0.5 are no longer send to the CO.

daschuer · 2021-03-01T16:13:54Z

In this case, can you rebate it to 2.3?

git rebase --onto upstream/2.3 4719d5e9529e39b64e46e3cf46fe06a6106e0e66~1

daschuer

Works already good. I have left some commit. For the test, I have mapped the kill buttons, and it is really fun.

Can we switch of the indicator if the track is paused on a cue point? Currently is is sometimes on and sometimes off when at a cue point, which is placed on a beat.

Unfortunately the backward direction does not work instantly. Because in Mixxx we have the definition that ON = x > 0. A rocker switch would have the values 0 1 2 ..
Would it be OK to use 2 for backwards, than it is full compatible to a three state GUI button.

Especially with long audio buffers, the jitter is visible. This can be fixed by moving the ClockControl::updateIndicators into the waveform update loop which is synced with the display refresh rate. Via the visual play position you have access to the sample that is currently played, and not the one that have been processed as in the current solution.

Optimizing this for the GUI did not help much with the latency that happens until the signal is at the controller though. So the full blown solution has to consider both paths individual.

This is obviously no longer a bug fix and a mayor refactoring. Maybe you should just add a note about it in ClockControl::updateIndicators for the case we want to pick it up in future.

src/track/beatgrid.h

src/engine/controls/bpmcontrol.cpp

src/engine/controls/clockcontrol.cpp

…olerance/snap behavior

…h the new term

…te loop.

src/engine/controls/clockcontrol.cpp

daschuer · 2021-03-03T01:14:28Z

src/engine/controls/clockcontrol.cpp

+                m_pCOBeatActive->forceSet(0.0);
+            }
+        }
+        m_lastPlayDirection = false; // Reverse
    }


I think I would prefer

Suggested change

}

} else {

m_pCOBeatActive->forceSet(0.0);

}

Because I experience that the LED is sometimes on and sometimes off when paused at a cue point.

I will look how to solve the cue point issue. The proposed else branch will not do this. It will never be reached, because this is captured before, to not switch the indicator off due the jitter of the DVS pitch.

I applied a fix for this, which should not break the use case of searching the beat position using DVS or jog. Please test, if this solves the cue point issue for you too.

…e button press.

…is hold on a beat

JoergAtGithub · 2021-03-16T21:26:10Z

@daschuer Did you tried the latest version of this PR? From my point of view, everything is working now.

daschuer

Works perfectly now.

Thank you very much.

JoergAtGithub · 2021-03-20T20:48:41Z

Merge?

daschuer · 2021-03-20T21:32:19Z

Ups, must have missed to hit merge.

…ented in PR#3608 (#347) See mixxxdj/mixxx#3608 for details. - Updated to new 3 state implementation - Updated state values to match the outcome of the code review - Added note how to implement mappings for slow controllers

JoergAtGithub marked this pull request as draft January 31, 2021 17:06

JoergAtGithub force-pushed the improve_beat_indicator branch from 1196d3f to b6e25ad Compare January 31, 2021 18:20

JoergAtGithub force-pushed the improve_beat_indicator branch from b6e25ad to 4719d5e Compare January 31, 2021 18:40

JoergAtGithub mentioned this pull request Feb 4, 2021

Updated documentation to the modified behavior of beat_active, implemented in PR#3608 mixxxdj/manual#347

Merged

github-actions bot added build code quality labels Feb 21, 2021

Added switch, to select if findPrevNextBeats should compare with or w…

143f255

…ithout tolerance epsilon

JoergAtGithub force-pushed the improve_beat_indicator branch from 565ddb2 to 143f255 Compare February 21, 2021 17:23

JoergAtGithub added 3 commits February 21, 2021 21:18

Added testcases for NoTolerance mode of findPrevNextBeats

1e6d066

Merge remote-tracking branch 'upstream/main' into improve_beat_indicator

f8c60f3

Improved if cases for NoTolerance code in findPrevNextBeats

1a8f2c2

JoergAtGithub force-pushed the improve_beat_indicator branch from ac6d3a7 to 1a8f2c2 Compare February 21, 2021 21:24

daschuer reviewed Feb 21, 2021

View reviewed changes

daschuer requested changes Feb 21, 2021

View reviewed changes

JoergAtGithub added 2 commits February 22, 2021 22:08

Merge remote-tracking branch 'upstream/main' into improve_beat_indicator

d407101

Merge remote-tracking branch 'upstream/main' into improve_beat_indicator

92c48ce

JoergAtGithub force-pushed the improve_beat_indicator branch from 24c759b to eead474 Compare February 27, 2021 10:15

Moved the 5 states into an internal state machine

5cb6696

The CO returns now 0 for the cases of play direction changes while beat indication is on

JoergAtGithub force-pushed the improve_beat_indicator branch from eead474 to 5cb6696 Compare February 27, 2021 10:20

daschuer changed the base branch from main to 2.3 March 1, 2021 16:11

daschuer changed the base branch from 2.3 to main March 1, 2021 16:11

daschuer requested changes Mar 1, 2021

View reviewed changes

src/track/beatgrid.h Outdated Show resolved Hide resolved

src/engine/controls/bpmcontrol.cpp Show resolved Hide resolved

src/engine/controls/clockcontrol.cpp Outdated Show resolved Hide resolved

src/engine/controls/clockcontrol.cpp Show resolved Hide resolved

JoergAtGithub added 4 commits March 1, 2021 19:08

Removed getBeatContextNoLookup() by using findPrevNextBeats without t…

6adb30c

…olerance/snap behavior

Renamed NoTolerance to snapToNearBeats and inverted the bool, to matc…

8b88974

…h the new term

const -> constexpr

93feabe

Added ToDo comment about moving the updateIndicators to waveform upda…

fd64411

…te loop.

daschuer reviewed Mar 3, 2021

View reviewed changes

src/engine/controls/clockcontrol.cpp Outdated Show resolved Hide resolved

daschuer reviewed Mar 3, 2021

View reviewed changes

JoergAtGithub marked this pull request as draft March 3, 2021 19:22

JoergAtGithub and others added 4 commits March 5, 2021 20:17

Handled the cue point case. Don't show beat indication after (hot) cu…

c3493ea

…e button press.

Changed reverse beat indication state from -1 to +2

d7ab0d0

Merge branch 'main' into improve_beat_indicator

9a00a1e

pre-commit

f04c35a

JoergAtGithub marked this pull request as ready for review March 5, 2021 20:23

JoergAtGithub requested a review from daschuer March 5, 2021 20:24

JoergAtGithub force-pushed the improve_beat_indicator branch 2 times, most recently from eacfdd6 to 02bc663 Compare March 6, 2021 11:48

Improved responsiveness when pressing play/cue button at while track …

277ed26

…is hold on a beat

JoergAtGithub force-pushed the improve_beat_indicator branch from 02bc663 to 277ed26 Compare March 6, 2021 12:19

daschuer mentioned this pull request Mar 13, 2021

Sync Lock: minor refactoring. #3695

Merged

daschuer approved these changes Mar 17, 2021

View reviewed changes

daschuer merged commit 4ffba2a into mixxxdj:main Mar 20, 2021

Swiftb0y mentioned this pull request Mar 22, 2021

MC7000 add Fixed Loop + improvements + experimental beatcount #3738

Merged

daschuer mentioned this pull request Oct 17, 2023

Option to make the BPM display flash on the beat (i.e. at the rate of the BPM) when track is playing? #12128

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reworked beat_active control: #3608

Reworked beat_active control: #3608

JoergAtGithub commented Jan 31, 2021 •

edited

Loading

Holzhaus commented Jan 31, 2021

JoergAtGithub commented Feb 4, 2021

daschuer commented Feb 21, 2021

daschuer Feb 21, 2021

JoergAtGithub Feb 21, 2021

daschuer left a comment

JoergAtGithub commented Feb 21, 2021

JoergAtGithub commented Feb 21, 2021 •

edited

Loading

JoergAtGithub commented Feb 21, 2021

daschuer commented Feb 21, 2021

daschuer commented Feb 21, 2021

daschuer commented Feb 21, 2021

JoergAtGithub commented Feb 27, 2021

daschuer commented Mar 1, 2021 •

edited

Loading

daschuer left a comment

daschuer Mar 3, 2021

JoergAtGithub Mar 3, 2021

JoergAtGithub Mar 5, 2021

JoergAtGithub commented Mar 16, 2021

daschuer left a comment

JoergAtGithub commented Mar 20, 2021

daschuer commented Mar 20, 2021

Reworked beat_active control: #3608

Reworked beat_active control: #3608

Conversation

JoergAtGithub commented Jan 31, 2021 • edited Loading

Holzhaus commented Jan 31, 2021

JoergAtGithub commented Feb 4, 2021

daschuer commented Feb 21, 2021

daschuer Feb 21, 2021

Choose a reason for hiding this comment

JoergAtGithub Feb 21, 2021

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

JoergAtGithub commented Feb 21, 2021

JoergAtGithub commented Feb 21, 2021 • edited Loading

JoergAtGithub commented Feb 21, 2021

daschuer commented Feb 21, 2021

daschuer commented Feb 21, 2021

daschuer commented Feb 21, 2021

JoergAtGithub commented Feb 27, 2021

daschuer commented Mar 1, 2021 • edited Loading

daschuer left a comment

Choose a reason for hiding this comment

daschuer Mar 3, 2021

Choose a reason for hiding this comment

JoergAtGithub Mar 3, 2021

Choose a reason for hiding this comment

JoergAtGithub Mar 5, 2021

Choose a reason for hiding this comment

JoergAtGithub commented Mar 16, 2021

daschuer left a comment

Choose a reason for hiding this comment

JoergAtGithub commented Mar 20, 2021

daschuer commented Mar 20, 2021

JoergAtGithub commented Jan 31, 2021 •

edited

Loading

JoergAtGithub commented Feb 21, 2021 •

edited

Loading

daschuer commented Mar 1, 2021 •

edited

Loading