Mixxx Macros #2873

xeruf · 2020-06-15T13:18:28Z

For now only to get early feedback whether the file structure & initialization code is correct - no actual functionality.

Holzhaus

I think it makes more sense to have a flip control per deck than a single one. This class boilerplate looks like it was inspired by RecordingManager, but IMHO the recording manager is unrelated and you should rather look at CueControl or LoopingControl for inspiration instead.

uklotzde · 2020-06-15T19:49:55Z

Instead of introducing boilerplate classes that we might never need we should better focus on finalizing the basic concepts.

uklotzde · 2020-06-15T20:00:16Z

Hints that might provide some inspiration:
https://github.com/satchelspencer/repsyps/blob/dd92130672c44749515ff0818e7b8665818b8576/src/render/util/types.ts#L113
https://gitlab.com/uklotzde/aoide-rs/-/blob/development/core/src/track/marker/cue/mod.rs#L47

Holzhaus · 2020-06-15T20:48:00Z

This is the format that Serato uses. It would be cool to use a format that can be made compatible for import/export: https://github.com/Holzhaus/serato-tags/blob/master/docs/serato_markers2.md#subentries-of-flip-entries

Be-ing · 2020-06-15T23:31:49Z

I can't tell if this is an appropriate structure because it doesn't do anything and I don't know what it's planned to do. I'm not sure if this class is needed, and if it would be needed, why it would need access to the UserSettings.

xeruf · 2020-06-16T18:00:12Z

Hints that might provide some inspiration:
https://github.com/satchelspencer/repsyps/blob/dd92130672c44749515ff0818e7b8665818b8576/src/render/util/types.ts#L113
https://gitlab.com/uklotzde/aoide-rs/-/blob/development/core/src/track/marker/cue/mod.rs#L47

@uklotzde I am not quite sure how these types are supposed to inspire me.

xeruf · 2020-06-16T18:03:23Z

If we have central recording controls as serato does, we need some class like this, no?

xeruf · 2020-06-17T09:46:10Z

I don't see a use case for Macro recording controls for each deck - you are not gonna record multiple macros in parallel anyways.

Holzhaus · 2020-06-17T11:04:28Z

I think it makes sense because then it's all bound to a deck. It shouldn't be possible to record a macro and then save it to a slot of a different deck.

ronso0 · 2020-06-17T11:42:17Z

I didn't wrap my head around the macro concept too much, but it make sense to have the respective controls as close as possible to the deck they're bound to.
For the mockups it's okay IMO to just add another row to decks, below transport controls.

xeruf · 2020-06-17T11:44:53Z

I think it makes sense because then it's all bound to a deck. It shouldn't be possible to record a macro and then save it to a slot of a different deck.

This won't be possible - it remembers which deck it recorded on and will save it to that.

Please tell me a use-case where this way would improve it. How are we supposed to map the recording button if there is no single general Macro recording control? Also, it would clutter the UI.

xeruf · 2020-06-17T11:56:21Z

I didn't wrap my head around the macro concept too much, but it make sense to have the respective controls as close as possible to the deck they're bound to.
For the mockups it's okay IMO to just add another row to decks, below transport controls.

The mockup above is almost exactly how Serato does it. Where this Row goes might change, but I'm pretty sure there is no benefit of having multiple such rows.

I don't want to discuss about what's technically architecturally "correct", I want to find a solution that makes sense. And as far as I have seen, an architecture similar to the RecordingManager does make sense for the recording part.

xeruf · 2020-06-17T11:58:26Z

Let's continue this discussion in Zulip and don't forget that I have already written a specification for the controls.

xeruf · 2020-06-22T18:35:11Z

It works!

Here's how to use it for now:

Click on the record button between the audio recording and broadcast buttons to arm recording
Jump to some hotcue on a deck to start recording. It will now only record this deck, and only cue jumps - feel free to try other kinds of jumps, there is a debug message printed when something was recorded
Click the recording button again to stop
The recorded jumps are printed as debug statements

Holzhaus

👍 Didn't have time to test it yet, but here are some first comments.

src/recording/macromanager.h

src/engine/enginebuffer.cpp

Holzhaus · 2020-06-22T19:30:44Z

src/recording/macromanager.h

+#define MAX_MACRO_SIZE 1000
+#define NO_DECK nullptr
+
+struct MacroAction {


Not that important for now, but please use classes for stuff that isn't in the anonymous name space.

Could you add that to the coding guidelines in the wiki? I wanted to know your policy for structs but didn't find any.

Isn't this the exact use case struts where meant for?

In C, yes. In C++, structs are just regular classes with default member visibility public instead of private.

Yeah, but default visibility public is appropriate here, or not? They are almost plain data structures.

Also, please document in https://github.com/mixxxdj/mixxx/wiki/coding_guidelines

src/recording/macromanager.h

src/engine/enginebuffer.cpp

xeruf · 2020-06-22T20:32:10Z

Theoretically you are right about how everything should be atomic, but I can't envision a use case where it would actually make a difference, so can't we simply avoid that overhead?

uklotzde · 2020-06-22T20:48:22Z

Theoretically you are right about how everything should be atomic, but I can't envision a use case where it would actually make a difference, so can't we simply avoid that overhead?

No.

src/recording/macromanager.h

Be-ing · 2020-06-23T00:34:32Z

can't we simply avoid that overhead?

Code as if the worst case scenario is happening all the time, because it will happen sometimes.

Holzhaus · 2020-06-24T13:12:14Z

I'm unable to build this:

/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp: In member function 'void EngineBuffer::slotControlSeekAbs(double)':
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:606:40: error: qualified-id in declaration before '(' token
  606 | void EngineBuffer::slotControlSeekExact(double playPosition) {
      |                                        ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:610:36: error: qualified-id in declaration before '(' token
  610 | void EngineBuffer::doSeekFractional(double fractionalPos, enum SeekRequest seekType) {
      |                                    ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:619:33: error: qualified-id in declaration before '(' token
  619 | void EngineBuffer::doSeekPlayPos(double new_playpos, enum SeekRequest seekType) {
      |                                 ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:631:49: error: qualified-id in declaration before '(' token
  631 | bool EngineBuffer::updateIndicatorsAndModifyPlay(bool newPlay) {
      |                                                 ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:647:30: error: qualified-id in declaration before '(' token
  647 | void EngineBuffer::verifyPlay() {
      |                              ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:655:42: error: qualified-id in declaration before '(' token
  655 | void EngineBuffer::slotControlPlayRequest(double v) {
      |                                          ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:673:36: error: qualified-id in declaration before '(' token
  673 | void EngineBuffer::slotControlStart(double v)
      |                                    ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:680:34: error: qualified-id in declaration before '(' token
  680 | void EngineBuffer::slotControlEnd(double v)
      |                                  ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:687:44: error: qualified-id in declaration before '(' token
  687 | void EngineBuffer::slotControlPlayFromStart(double v)
      |                                            ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:695:49: error: qualified-id in declaration before '(' token
  695 | void EngineBuffer::slotControlJumpToStartAndStop(double v)
      |                                                 ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:703:35: error: qualified-id in declaration before '(' token
  703 | void EngineBuffer::slotControlStop(double v)
      |                                   ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:710:44: error: qualified-id in declaration before '(' token
  710 | void EngineBuffer::slotKeylockEngineChanged(double dIndex) {
      |                                            ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:724:38: error: qualified-id in declaration before '(' token
  724 | void EngineBuffer::processTrackLocked(
      |                                      ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1031:27: error: qualified-id in declaration before '(' token
 1031 | void EngineBuffer::process(CSAMPLE* pOutput, const int iBufferSize) {
      |                           ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1102:31: error: qualified-id in declaration before '(' token
 1102 | void EngineBuffer::processSlip(int iBufferSize) {
      |                               ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1125:39: error: qualified-id in declaration before '(' token
 1125 | void EngineBuffer::processSyncRequests() {
      |                                       ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1151:31: error: qualified-id in declaration before '(' token
 1151 | void EngineBuffer::processSeek(bool paused) {
      |                               ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1226:31: error: qualified-id in declaration before '(' token
 1226 | void EngineBuffer::postProcess(const int iBufferSize) {
      |                               ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1250:36: error: qualified-id in declaration before '(' token
 1250 | void EngineBuffer::updateIndicators(double speed, int iBufferSize) {
      |                                    ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1297:30: error: qualified-id in declaration before '(' token
 1297 | void EngineBuffer::hintReader(const double dRate) {
      |                              ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1321:29: error: qualified-id in declaration before '(' token
 1321 | void EngineBuffer::loadTrack(TrackPointer pTrack, bool play) {
      |                             ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1333:30: error: qualified-id in declaration before '(' token
 1333 | void EngineBuffer::addControl(EngineControl* pControl) {
      |                              ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1339:31: error: qualified-id in declaration before '(' token
 1339 | void EngineBuffer::bindWorkers(EngineWorkerScheduler* pWorkerScheduler) {
      |                               ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1343:33: error: qualified-id in declaration before '(' token
 1343 | bool EngineBuffer::isTrackLoaded() {
      |                                 ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1350:41: error: qualified-id in declaration before '(' token
 1350 | bool EngineBuffer::getQueuedSeekPosition(double* pSeekPosition) {
      |                                         ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1360:34: error: qualified-id in declaration before '(' token
 1360 | void EngineBuffer::slotEjectTrack(double v) {
      |                                  ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1371:37: error: qualified-id in declaration before '(' token
 1371 | double EngineBuffer::getExactPlayPos() {
      |                                     ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1382:38: error: qualified-id in declaration before '(' token
 1382 | double EngineBuffer::getVisualPlayPos() {
      |                                      ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1386:37: error: qualified-id in declaration before '(' token
 1386 | double EngineBuffer::getTrackSamples() {
      |                                     ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1390:36: error: qualified-id in declaration before '(' token
 1390 | void EngineBuffer::setScalerForTest(EngineBufferScale* pScaleVinyl,
      |                                    ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1401:35: error: qualified-id in declaration before '(' token
 1401 | void EngineBuffer::collectFeatures(GroupFeatureState* pGroupFeatures) const {
      |                                   ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1407:34: error: qualified-id in declaration before '(' token
 1407 | double EngineBuffer::getRateRatio() const {
      |                                  ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1414:37: error: qualified-id in declaration before '(' token
 1414 | void EngineBuffer::notifyTrackLoaded(
      |                                     ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1443:41: error: qualified-id in declaration before '(' token
 1443 | void EngineBuffer::slotUpdatedTrackBeats() {
      |                                         ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:1450:1: error: expected '}' at end of input
 1450 | }
      | ^
/home/jan/Projects/mixxx/src/engine/enginebuffer.cpp:593:60: note: to match this '{'
  593 | void EngineBuffer::slotControlSeekAbs(double playPosition) {
      |                                                            ^

Holzhaus · 2020-06-24T13:17:45Z

After adding a closing brace manually, compilation works but I'm getting a bunch of linker errors:

/usr/bin/ld: /home/jan/Projects/mixxx/src/mixxx.cpp:1178: undefined reference to `RecordingManagerBase::isRecordingActive() const'

Holzhaus · 2020-06-24T13:21:48Z

@xerus2000 We don't want broken commits in our history, so please fix them. My workflow for this is committing the fixes as small atomic commits, then rebase and copy each line with a fixing commit directly behind it's corresponding broken commit and replace pick with fixup in the fixing commit line. Then force-push.

xeruf · 2020-06-24T16:23:14Z

Yeah, I usually watch out for this as well. It built fine for me, but I'll check again.

xeruf · 2020-06-24T20:30:18Z

Yeah, messed up the CMakeList again and didn't notice it due to cache. I fixed it in an extra commit - because of the merge rebasing didn't work properly.

Co-Authored-By: Uwe Klotz <uklotz@mixxx.org>

src/macros/macrorecorder.h

src/macros/macrorecorder.cpp

Holzhaus · 2020-07-27T17:22:05Z

src/macros/macrorecorder.cpp

+    auto armed = MacroState::Armed;
+    // TODO(xerus) add concurrency test
+    while (!m_macroRecordingState.compare_exchange_weak(armed, MacroState::Disabled))
+        QThread::yieldCurrentThread();


If we need to reset the recording state to armed to ensure that the macro has been fully written, you should just use SPSCQueue instead.

@xerus2000 ping.

For Library & MacroRecorder the public definition of their config/control group key was removed and replaced by an entry in the anonymous namespace in the cpp file.

src/macros/macro.h

Holzhaus

Looks good so far, thanks!

lib/rigtorp/SPSCQueue/include/rigtorp/SPSCQueue.h

res/schema.xml

src/database/mixxxdb.cpp

Remove all mutability functionality which was previously needed for recording, instead the Macro class is now used for playback.

xeruf · 2020-08-04T10:09:36Z

Woops. Well, we can start with a blank slate then.

Holzhaus · 2020-08-04T11:16:37Z

Huh? Why did you close this?

xeruf · 2020-08-04T11:31:41Z

Accidentally deleted the branch ^^
But it's not that bad, we have basically resolved all comments here and it was getting really tedious to scroll through all the old stuff.

ronso0 · 2020-08-04T13:08:04Z

How to restore a deleted branch

Holzhaus reviewed Jun 15, 2020

View reviewed changes

xeruf force-pushed the macros branch from 9e0f1c5 to 1bb9899 Compare June 19, 2020 22:12

xeruf marked this pull request as ready for review June 22, 2020 18:30

xeruf requested a review from Holzhaus June 22, 2020 18:30

xeruf changed the title ~~Add MacroManager~~ Implement Macro Recording Jun 22, 2020

Holzhaus requested changes Jun 22, 2020

View reviewed changes

uklotzde reviewed Jun 22, 2020

View reviewed changes

src/recording/macromanager.h Outdated Show resolved Hide resolved

uklotzde reviewed Jun 22, 2020

View reviewed changes

src/recording/macromanager.h Outdated Show resolved Hide resolved

xeruf force-pushed the macros branch from 68beee1 to 4dc6820 Compare June 24, 2020 16:31

xeruf and others added 6 commits July 14, 2020 19:37

Macros: Create proto namespace alias

532e5af

Co-Authored-By: Uwe Klotz <uklotz@mixxx.org>

Macros: Update TODO's & Recording State enum

f29c449

Macros: Add stopRecordingAsync test & fix CAS loop

41e4664

Macros: Add MacroAction Type enum & update serialization

8f0f82a

Merge branch 'master' into macros

3133295

Macros: Fix schema revision

e54928b

Holzhaus requested changes Jul 27, 2020

View reviewed changes

Move config group definitions from header to cpp

a5f93be

For Library & MacroRecorder the public definition of their config/control group key was removed and replaced by an entry in the anonymous namespace in the cpp file.

xeruf force-pushed the macros branch from a6e84e9 to a5f93be Compare July 31, 2020 09:54

Holzhaus reviewed Jul 31, 2020

View reviewed changes

src/macros/macro.h Outdated Show resolved Hide resolved

Merge branch 'master' into macros

ad4dc57

xeruf force-pushed the macros branch from ed201d1 to 6befde4 Compare July 31, 2020 17:56

xeruf requested review from Be-ing, Holzhaus and uklotzde July 31, 2020 17:56

Holzhaus reviewed Jul 31, 2020

View reviewed changes

uklotzde suggested changes Jul 31, 2020

View reviewed changes

lib/rigtorp/SPSCQueue/include/rigtorp/SPSCQueue.h Outdated Show resolved Hide resolved

xeruf force-pushed the macros branch from 6befde4 to 27b1d08 Compare August 1, 2020 10:03

Holzhaus reviewed Aug 1, 2020

View reviewed changes

res/schema.xml Show resolved Hide resolved

src/database/mixxxdb.cpp Outdated Show resolved Hide resolved

xeruf added 3 commits August 4, 2020 00:48

Macros: Use SPSCQueue for recording

a451552

Macros: Repurpose Macro class

fe8bdeb

Remove all mutability functionality which was previously needed for recording, instead the Macro class is now used for playback.

Macros: Slightly reorder database columns

be839d8

xeruf force-pushed the macros branch from f42e0ac to be839d8 Compare August 3, 2020 22:49

xeruf closed this Aug 4, 2020

xeruf deleted the macros branch August 4, 2020 09:45

xeruf mentioned this pull request Aug 4, 2020

GSoC 2020: Mixxx Macros [Moved] #2989

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mixxx Macros #2873

Mixxx Macros #2873

xeruf commented Jun 15, 2020

Holzhaus left a comment •

edited

Loading

uklotzde commented Jun 15, 2020

uklotzde commented Jun 15, 2020

Holzhaus commented Jun 15, 2020

Be-ing commented Jun 15, 2020

xeruf commented Jun 16, 2020 •

edited

Loading

xeruf commented Jun 16, 2020

xeruf commented Jun 17, 2020

Holzhaus commented Jun 17, 2020 •

edited

Loading

ronso0 commented Jun 17, 2020

xeruf commented Jun 17, 2020

xeruf commented Jun 17, 2020

xeruf commented Jun 17, 2020

xeruf commented Jun 22, 2020

Holzhaus left a comment

Holzhaus Jun 22, 2020

xeruf Jun 23, 2020

Holzhaus Jun 29, 2020

xeruf Jun 30, 2020 •

edited

Loading

xeruf commented Jun 22, 2020

uklotzde commented Jun 22, 2020

Be-ing commented Jun 23, 2020

Holzhaus commented Jun 24, 2020

Holzhaus commented Jun 24, 2020

Holzhaus commented Jun 24, 2020 •

edited

Loading

xeruf commented Jun 24, 2020

xeruf commented Jun 24, 2020

Holzhaus Jul 27, 2020

Holzhaus left a comment

xeruf commented Aug 4, 2020

Holzhaus commented Aug 4, 2020

xeruf commented Aug 4, 2020

ronso0 commented Aug 4, 2020

Mixxx Macros #2873

Mixxx Macros #2873

Conversation

xeruf commented Jun 15, 2020

Holzhaus left a comment • edited Loading

Choose a reason for hiding this comment

uklotzde commented Jun 15, 2020

uklotzde commented Jun 15, 2020

Holzhaus commented Jun 15, 2020

Be-ing commented Jun 15, 2020

xeruf commented Jun 16, 2020 • edited Loading

xeruf commented Jun 16, 2020

xeruf commented Jun 17, 2020

Holzhaus commented Jun 17, 2020 • edited Loading

ronso0 commented Jun 17, 2020

xeruf commented Jun 17, 2020

xeruf commented Jun 17, 2020

xeruf commented Jun 17, 2020

xeruf commented Jun 22, 2020

Holzhaus left a comment

Choose a reason for hiding this comment

Holzhaus Jun 22, 2020

Choose a reason for hiding this comment

xeruf Jun 23, 2020

Choose a reason for hiding this comment

Holzhaus Jun 29, 2020

Choose a reason for hiding this comment

xeruf Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

xeruf commented Jun 22, 2020

uklotzde commented Jun 22, 2020

Be-ing commented Jun 23, 2020

Holzhaus commented Jun 24, 2020

Holzhaus commented Jun 24, 2020

Holzhaus commented Jun 24, 2020 • edited Loading

xeruf commented Jun 24, 2020

xeruf commented Jun 24, 2020

Holzhaus Jul 27, 2020

Choose a reason for hiding this comment

Holzhaus left a comment

Choose a reason for hiding this comment

xeruf commented Aug 4, 2020

Holzhaus commented Aug 4, 2020

xeruf commented Aug 4, 2020

ronso0 commented Aug 4, 2020

Holzhaus left a comment •

edited

Loading

xeruf commented Jun 16, 2020 •

edited

Loading

Holzhaus commented Jun 17, 2020 •

edited

Loading

xeruf Jun 30, 2020 •

edited

Loading

Holzhaus commented Jun 24, 2020 •

edited

Loading