Macros Backend respin #13172

JoergAtGithub · 2024-04-23T23:11:42Z

This is #4528 with fixed CI and solved merge conflicts.

It implements the following COs without GUI: https://github.com/mixxxdj/mixxx/wiki/Macros

This PR is the first step to implement the Macro feature. The Mixxx Macros feature is designed functional compatible to Serato Flip. If we later add a Serato Flip importer in a Follow-Up PR, Mixxx DJs can use all files prepared with Flips provided by the common DJ pools.
A good (maybe a bit flippy ;-) ) introduction into the use cases of Serato Flip can be found here: https://www.youtube.com/watch?v=sHRWfdxkI7E&t=186s

-Ignoring return value of function declared with 'nodiscard' attribute -Missing include

…ter clear, but clear intentional leaves a user specified label untouched

acolombier

I tried the feature but could only get a single hotcue to work. It could be that I'm not using properly tho. Do you know if there is any wiki or doc on how to use this feature beside the GSoC wiki page?

src/engine/enginebuffer.cpp

JoergAtGithub · 2024-05-06T19:01:30Z

I tried the feature but could only get a single hotcue to work. It could be that I'm not using properly tho. Do you know if there is any wiki or doc on how to use this feature beside the GSoC wiki page?

I think there are only the three GSoC pages in our wiki, and the original PRs of cause. One aim of this feature was to achieve compatibility to Serato Flip, to allow data import from Serato. Therefore I guess, that the Serato Flip feature is similar to use: https://support.serato.com/hc/en-us/articles/203157550-Serato-Flip-User-Guide

acolombier

Thanks for bringing this feature back to life. Appreciate you didn't write this code but I think there quite a few things that don't line up with our coding guidelines.
Haven't had a chance to test it yet!

src/engine/controls/macrocontrol.cpp

src/engine/controls/cuecontrol.cpp

src/engine/controls/macrocontrol.cpp

acolombier · 2024-10-27T12:58:47Z

src/proto/macro.proto

+    optional uint32 type = 1;
+    optional uint64 source_frame = 2;
+    optional uint64 target_frame = 3;


Looking at MacroAction::serialize, should not this be mandatory fields?

I'm not familar with protobuf and can't answer this. But I couldn't find any review remark in the predecessor PRs about it.

Hmm, maybe these should be required. Not sure. My understanding of protobuf is that this does not affect the serialized output, just the deserialization code. So we should probably make these required and "downgrade" them to optional only if the becomes necessary.

@daschuer @Swiftb0y Do you know Protobuf well enough to answer this question with certainty?

Not with certainty, but until now my rule of thump was that everything should be optional just in case. In protobuf 3 the concept of required fields has been removed altogether:
https://stackoverflow.com/questions/31801257/why-required-and-optional-is-removed-in-protocol-buffers-3

src/track/macroaction.h

src/track/track.cpp

src/util/db/dbid.h

…edLabelRegExp

ronso0 · 2024-10-28T22:56:18Z

I thought macros are per track? (= per deck, not one macro manager.

JoergAtGithub · 2024-10-28T23:02:07Z

The GUI controls are global, you click the record button and it records the deck were the DJ does the next action.

I also thought, that this might be difficult to understand and had the idea of a colored border around the deck - e.g. when armed, all decks get a orange border, and once a deck is selected, only this one get a red border.

ronso0 · 2024-10-28T23:06:52Z

But (as I understood it back then) macros are per track, so I can start a macro on deck1, load a track in deck2 and start its macro#5 to do prepared mashups and stuff.
Thats only possible if we have a per-deck GUI.

I don't want to move this off-topic, just saying...

JoergAtGithub · 2024-10-28T23:14:57Z

Yes, lets discuss the GUI on Zulip. The COs in this PRs should be flexible enough to allow per deck controls as well.

daschuer · 2024-10-29T08:00:11Z

Great you picked this up.

What is the difference between the Status::Playing and Status::Enabled? I have not tested it ...

Am I right that Macros do not necessarily include transport actions? Will the track start playing if pressing macro play? This depends probably on the macro itself?

-Use QStringLiteral: in getConfigKey instead of QString member -Avoiding multiple calls to getConfigKey by using a lambda function

JoergAtGithub · 2024-10-30T19:34:17Z

What is the difference between the Status::Playing and Status::Enabled? I have not tested it ...

See https://github.com/mixxxdj/mixxx/wiki/Macros
This is the unchanged functionality of the GSoC project, which was jointly agreed on. This PR only addresses the open code style and CI issues for which the original GSoC PR was not merged.

daschuer · 2024-10-31T16:55:47Z

Unfortunately I have not yet understand the difference and the underlying use case. Can you explain it?

Holzhaus · 2024-10-31T17:32:04Z

What is the difference between the Status::Playing and Status::Enabled? I have not tested it ...

There is no Status::Enabled.

mixxx/src/engine/controls/macrocontrol.h

Lines 24 to 31 in 3ea8edd

    
           enum class Status : int { 
        
               NoTrack = -1, 
        
               Empty = 0, 
        
               Armed = 1, 
        
               Recording = 2, 
        
               Recorded = 3, 
        
               Playing = 4, 
        
           };

src/engine/controls/enginecontrol.h

src/engine/controls/macrocontrol.cpp

src/engine/controls/macrocontrol.h

daschuer · 2024-11-01T07:46:01Z

There is no Status::Enabled.

Oh I see, I got confused with the StateFlag::Enabled

I have still not fully understand how that interacts.

Is the a description of the state transitions somewhere?
Maybe there is a chance to simplify it a bit.

For my understanding a macro is comparable to Auto DJ, but only for a single track. Is that correct?

It is possible to record a jump between two cue points.
After recording the user has to press "play" on the macro button for the deck to enable the macro. When the deck is then played in terms of transport, for example by Auto DJ, the jump is automatically performed.

Is that correct?

JoergAtGithub · 2024-11-01T07:51:28Z

I don't see a similarty to AutoDJ, but yes, it's per track. I added a video that explains the functionality to the PR description.

And please let not put the feature in question at all. It's all defined and lengthy discussed in GSoC. This is just to fix the final code style and and unit test issues, which prevented merging of the original PR.

src/engine/controls/enginecontrol.h

…ead of INT_MAX

not available for review

Swiftb0y

couple comments to try and help get this along.

Swiftb0y · 2024-11-06T12:32:02Z

src/util/db/dbid.h

+    int value() const {
+        return m_value;
+    }


this breaks the encapsulation which is the sole purpose of this class. If the value is used in queries, I think its supposed to go through the QVariant (you may need to look at other usages of DbId).

Swiftb0y · 2024-11-06T12:35:46Z

src/track/track.cpp

+    for (const MacroPointer& pMacro : std::as_const(m_macros)) {
+        if (pMacro->isDirty()) {
+            return true;
+        }
+    }
+    return false;


std::any_of

Swiftb0y · 2024-11-06T12:36:47Z

src/track/macroaction.h

+/// but that is subject to change.
+class MacroAction {
+  public:
+    enum class Type : uint32_t {


Suggested change

enum class Type : uint32_t {

enum class Type {

Swiftb0y · 2024-11-06T12:37:38Z

src/track/macroaction.h

+///
+/// Note that currently only jumps to a m_target position are available,
+/// but that is subject to change.
+class MacroAction {


trivial constexpr opportunity of the class.

Swiftb0y · 2024-11-06T12:37:48Z

src/track/macroaction.h

+#include "audio/frame.h"
+#include "engine/engine.h"
+#include "proto/macro.pb.h"
+namespace proto = mixxx::track::io;


namespace pollution in header

Suggested change

namespace proto = mixxx::track::io;

The suggested code change breaks the build

right, you'll obviously have to replace all the occurences of proto with the longer name...

src/engine/controls/macrocontrol.cpp

Swiftb0y · 2024-11-06T13:02:30Z

src/engine/controls/macrocontrol.cpp

+          m_COStatus([this]() { return getConfigKey("status"); }()),
+          m_CORecord([this]() { return getConfigKey("record"); }()),
+          m_COPlay([this]() { return getConfigKey("play"); }()),
+          m_COEnable([this]() { return getConfigKey("enable"); }()),
+          m_COLoop([this]() { return getConfigKey("loop"); }()),
+          m_activate([this]() { return getConfigKey("activate"); }()),
+          m_toggle([this]() { return getConfigKey("toggle"); }()),
+          m_clear([this]() { return getConfigKey("clear"); }()) {


this is dubious because it relies on the class already being partially initialized. I think the better option would be to just pass in the group along with getConfigKey and make it static...

Swiftb0y · 2024-11-06T13:02:58Z

src/engine/controls/macrocontrol.cpp

+
+    m_updateRecordingTimer.moveToThread(qApp->thread());


this needs a comment.

Swiftb0y · 2024-11-06T13:03:47Z

src/engine/controls/macrocontrol.cpp

+// FIXME(xeruf) Jumps while paused (e.g. via GotoAndStop) are not properly recorded
+// since this function is not called


do you plan to address these FIXMEs? These certainly sound like bugs.

Swiftb0y · 2024-11-06T13:06:15Z

src/engine/controls/macrocontrol.h

+    /// The unquantized FramePos of a jump that is yet to be processed
+    mixxx::audio::FramePos m_queuedJumpTarget;
+
+    rigtorp::SPSCQueue<MacroAction> m_recordedActions;


since this is designed for multithreading, this suggests that MacroControl is also used from multiple threads. Its not clear to me how though so I'd appreciate if you could document which member functions are expected to be accessed concurrently (and thus need to be threadsafe and which dont).

MacroControl obviously lives in the GUI thread, but IIRC some methods (e.g., slotJumpQueued) are invoked via direct connection from the engine thread.

xeruf and others added 9 commits November 18, 2021 16:27

Macros: Implement Controls and Database

809335d

Macros: Update Schemata and namings

e74bc59

Macros: Add todo comments

16f1f17

Macros: Remove redundant calls

999288a

Macros: Add proper isEmpty check

21ae95a

Macros: Create placeholder Macros in Track for better encapsulation

affdefb

Merge remote-tracking branch 'upstream/main' into macros-backend

87692da

Fixed Linux build errors on Linux:

3cbeb7f

-Ignoring return value of function declared with 'nodiscard' attribute -Missing include

Fixed failing testcase, because isEmpty checked if no label is set af…

454d882

…ter clear, but clear intentional leaves a user specified label untouched

github-actions bot added library engine build code quality labels Apr 23, 2024

JoergAtGithub mentioned this pull request Apr 23, 2024

Saved beatjump #13161

Closed

Merge remote-tracking branch 'upstream/Main' into macros-backend

d61749c

JoergAtGithub force-pushed the macros-backend branch from 9ac8800 to d61749c Compare April 23, 2024 23:16

JoergAtGithub added the needs review label Apr 29, 2024

acolombier reviewed May 6, 2024

View reviewed changes

src/engine/enginebuffer.cpp Show resolved Hide resolved

acolombier previously requested changes Oct 27, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into macros-backend

f9847d9

JoergAtGithub force-pushed the macros-backend branch from f0ebdab to f9847d9 Compare October 27, 2024 21:33

JoergAtGithub added 2 commits October 28, 2024 00:18

Optimized if/else structures

4cc7d2c

Removed #include QDebug

18c6730

JoergAtGithub force-pushed the macros-backend branch from 544ba01 to 18c6730 Compare October 27, 2024 23:40

JoergAtGithub added 3 commits October 28, 2024 00:40

Replaced the inline regular expression with the constant kAutoGenerat…

6fbd4de

…edLabelRegExp

std::as_const instead of qAsConst

1e7e56f

Added prefix m_ to member variables

53e8d61

JoergAtGithub force-pushed the macros-backend branch from b953cf0 to 53e8d61 Compare October 28, 2024 18:57

Simplify DEBUG_ASSERT

a950e68

Improved initialization of ConfigKey members in MacroControl:

3ea8edd

-Use QStringLiteral: in getConfigKey instead of QString member -Avoiding multiple calls to getConfigKey by using a lambda function

Holzhaus reviewed Oct 31, 2024

View reviewed changes

daschuer reviewed Nov 1, 2024

View reviewed changes

src/engine/controls/enginecontrol.h Show resolved Hide resolved

Renamed some symbols according Mixxx conventions

1cf78c4

JoergAtGithub mentioned this pull request Nov 1, 2024

Improved comments in enginecontrol and use of std::size_t for bufferSize accross the codebase #13819

Merged

JoergAtGithub added 2 commits November 1, 2024 11:29

Clarified macro status enum

238b502

Refactor MacroControl to use std::optional for m_nextActionIndex inst…

20e55d0

…ead of INT_MAX

JoergAtGithub force-pushed the macros-backend branch from 3f2c892 to 20e55d0 Compare November 1, 2024 12:30

JoergAtGithub added 2 commits November 1, 2024 13:36

Remove redundant QList m_macroControls

46ab818

Merge remote-tracking branch 'upstream/main' into macros-backend

bc6f687

JoergAtGithub mentioned this pull request Nov 3, 2024

Clarify the term rate used in various parts of the source code #13833

Open

Merge remote-tracking branch 'upstream/main' into macros-backend

faa76d0

JoergAtGithub force-pushed the macros-backend branch from ba4dc9f to faa76d0 Compare November 4, 2024 19:51

Swiftb0y reviewed Nov 6, 2024

View reviewed changes

JoergAtGithub force-pushed the macros-backend branch from 666998f to faa76d0 Compare November 11, 2024 18:30

acolombier mentioned this pull request Feb 18, 2025

Macros Backend #4528

Closed

JoergAtGithub added this to the 2.7-beta milestone Mar 16, 2025

acolombier added this to Releases Mar 25, 2025

github-project-automation bot moved this to In progress in Releases Mar 25, 2025

		// FIXME(xeruf) Jumps while paused (e.g. via GotoAndStop) are not properly recorded
		// since this function is not called

Uh oh!

Macros Backend respin #13172

Are you sure you want to change the base?

Macros Backend respin #13172

Uh oh!

Conversation

JoergAtGithub commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acolombier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoergAtGithub commented May 6, 2024

Uh oh!

acolombier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ronso0 commented Oct 28, 2024

Uh oh!

JoergAtGithub commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronso0 commented Oct 28, 2024

Uh oh!

JoergAtGithub commented Oct 28, 2024

Uh oh!

daschuer commented Oct 29, 2024

Uh oh!

JoergAtGithub commented Oct 30, 2024

Uh oh!

daschuer commented Oct 31, 2024

Uh oh!

Holzhaus commented Oct 31, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daschuer commented Nov 1, 2024

Uh oh!

JoergAtGithub commented Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Swiftb0y left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoergAtGithub commented Apr 23, 2024 •

edited

Loading

JoergAtGithub commented Oct 28, 2024 •

edited

Loading

JoergAtGithub commented Nov 1, 2024 •

edited

Loading