Cues & Loops upgrade for Stems (& Widget) #13759
Conversation
|
I haven't looked at the code yet as is doesn't seem to be ready to review looking at the CI result. My 2 cents on the overall look is that using integer for a volume that is a double feels redundant, so I would recommend using the same type between backend and persistency layer. In terms of feature, I am not sure that fitting this usecase on the existing hotcue is a wise idea. Perhaps we could consider another type of cue, eg. Also, please note that to make this feature ready for review, the DB migration will need to be automated. |
The serato-markers give me grey hair :-( I would like another eye on that. :-) |
|
The database should contain full precision, which is double in terms of COs, but float is precise enough for our 32 bit float engine. The representation in the GUI is unrelated. The proposed 0..100 % is OK we may consider to use 0.0 ... 1.00 or a dB scale. |
| //auto lock = lockMutex(&m_mutex); | ||
| // We should get a trackCuesUpdated call anyway, so do nothing. | ||
| // auto lock = lockMutex(&m_mutex); | ||
| // We should get a trackCuesUpdated call anyway, so do nothing. |
There was a problem hiding this comment.
Unrelated change? Befor clang format we had the rule to use "//" without space for commented code and "// " for information.
Make sure your editor doe not auto-format the whole file to avoid noise in your PRs like this.
There was a problem hiding this comment.
this happens automatically with the pre-commit since the update about 10 days ago (was an update of some minutes)
src/engine/controls/cuecontrol.cpp
Outdated
| // EveCue-Loop | ||
| bool TrackStem = false; | ||
| if (ControlObject::exists(ConfigKey(getGroup(), "stem_count"))) { | ||
| PollingControlProxy proxyStem(getGroup(), "stem_count"); |
There was a problem hiding this comment.
we should avoid CO look up during Mixxx runtime, because this is a looking operation. Can you move this into the constructor or somewhere else? Checking for exist already creates the CO half way.
You can instead create the PollingProxy with ControlFlag::AllowMissingOrInvalid flags.
src/engine/controls/cuecontrol.cpp
Outdated
| @@ -883,6 +891,54 @@ void CueControl::hotcueSet(HotcueControl* pControl, double value, HotcueSetMode | |||
| } | |||
| } | |||
|
|
|||
| // EveCue-Loop | |||
| bool TrackStem = false; | |||
There was a problem hiding this comment.
bools should be named for building a "sentences" with if. Like if (hasStems) { or such. And start with a lower case.
| } | ||
|
|
||
| if (TrackStem) { | ||
| const QString groupBaseName = getGroup().remove("[").remove("]"); |
There was a problem hiding this comment.
you remove the [ her and add it below.
There was a problem hiding this comment.
this was my best solution to make [ChannelXStemY] from [ChannelX]
What are the pros an cons? Without dedicated stem cues all hot-cues of stem will have volume values. What are the cons? Is there an alternative solution for a workaround? |
|
Oh I have jumped into detailed comments before sharing my opinion about this approach. I consider this a great feature, than you :-) . It is useful to mark "teaser" parts a track where you want only vocals or another specific layout. It also is helpful to start a stem based transition with a predefined layout, where you for instance bring later the whole track in. Are there more use cases?
What do you mean by that? Can you explain it or share an external reference? |
Other use case: create reMixxxes, narrowing the line between DJ and producer.
|
To let the serato cues match the 'adapted' cues, they need to have the 4 stem volumes, but compiler says they are unused. If I remove them compiler says missing .... |
This means that if you want to use the cue with the current stem layout there is a risk of unwanted volume change. I can also imagine to use cues after moving a single stem to a second deck. |
Do you mean you prefer to have 0.00 to 1.00 in the dbase in stead of 0 - 100 ? |
Another limitation I can think about is the limiting factor of having only stem volume - assuming you want to isolate a vocal sampler, you may also want to add a reverb + 25% dry/run volume.
Yes, I think it would make more sense. |
|
The cue table is defined here Line 79 in 60abf58 and updated here: Line 441 in 60abf58 In other columns we use sbake_case. so I think "stem_1_vol" does fit better. We had a discussion around "Spatial Audio", where we may have n channels and aposition in space. This kind of applies partially as well to instuments stored in a stem. I have not any demand here but I just want to bring the question up if these four colums are the right choice or if we need something more generic like a meta data blob. Something like
What do you think? |
src/track/serato/markers.h
Outdated
| @@ -35,6 +35,10 @@ class SeratoMarkersEntry { | |||
| bool hasEndPosition, | |||
| int endPosition, | |||
| SeratoStoredHotcueColor color, | |||
| int stem1vol, | |||
There was a problem hiding this comment.
They are unused here and can be removed.
Can you propose this change and we con look at the missing warning?
src/track/serato/markers.cpp
Outdated
| @@ -236,6 +236,10 @@ SeratoMarkersEntryPointer SeratoMarkersEntry::parseID3(const QByteArray& data) { | |||
| hasEndPosition, | |||
| endPosition, | |||
| color, | |||
| 100, | |||
There was a problem hiding this comment.
fixed, there was another occurrence that was too much... solved Compiler happy
yes as a "real" value. |
Yes that make sense, use a protobuffer to store the volume information and probably a verison number to be backward compatible. We use it already to store the keys information in the data base: Line 351 in 60abf58 https://github.com/mixxxdj/mixxx/blob/60abf58e20af9e02c6475b0861008d4f074fa6fb/src/proto/keys.proto |
-> this was indeed something I was thinking of as well, but I was in dubio because sometimes I want to use the FX-rack super in stead of the one fx selected on the stem. On the other hand to be consequent these settings should be saved as well.
-> I try to redefine your words in my language (I never adapted to programmers-slang): I just don't understand why you don't need a dbase migration then. excuse me for my ignorance |
I think I asked once (at the beginning of the stems-implementation) to have a pan-knob besides the volume knob. |
|
|
@acolombier |
|
This PR is marked as stale because it has been open 90 days with no activity. |
github-actions
bot
added
the
stale
|
I gave that a second thought and consider how much we want to open padoras box. Hot cues are points in the track where you like to cue it in, (the whole track). We have one track, with one transport. All effects are NOT related naturally. We have recently introduced loop cues, used to recall loops. This are not Hot cues in the original terms, but since they are close related to transport it fits somehow mentally. We have also the use case of jumping around in a track using cues. This can be used to repeat or skip portions of a track or use the cue buttons like a drum pad. Changing the volume has never be part of cues, but I think here it is more for selecting a certain stem layout, right? Somehow a second dimension of cueing. On the other hand we may treat the volume changes of stems like EQing, which is also not a part of cues. You see, opening cues for stem volume may also create demands for EQs and than for the whole effect unit and whole Mixxx. All this are valid use cases. But I see three different feature:
To which category dos the stem volume control belongs to? To be clear: I do not want to stop the project. I just want to discuss a perfect solution, to make sure that everything we introduce will align with it. Later we can decide if dedicated columns or a protbuf blob is the best fit and if it is am implicit feature of hot-cues or a new layer of controls. |
I understand what you mean, but IMO since the introduction of stems the definition of a (whole) track has changed, else the volume of a stem can also be considered as 'not naturally related' because no part of the whole track, and earlier EQ could have that label as well indeed. |
|
That sounds good. So you actually looking for different thing than an advanced hot cue. It is a "performance cue" or such, right? Is it more like a recoded performance, or like something you would normally do in a pre recorded sampler from the sampler button? Do we also need to consider looping? For this new cue type? |
You're on track! 😄 ❤️ |
github-actions
bot
removed
the
stale
upgrade of cue/loop routines for stems
dbase needs to be updated with 4 fields in cues-table -> REAL + snake_case
alter table cues add stem_1_vol REAL DEFAULT 1.00 NOT NULL
alter table cues add stem_2_vol REAL DEFAULT 1.00 NOT NULL
alter table cues add stem_3_vol REAL DEFAULT 1.00 NOT NULL
alter table cues add stem_4_vol REAL DEFAULT 1.00 NOT NULL
when track has stems the mute and volume settings are saved in the hotcue / loop
else the values are all 1 (default)
volume -> 0-100 (= %) in widget, background double 0.0 .... 1.0
mute -> negative volume value
cue widget adapted to be able to change the stemvolumes after the cue is set.
the serato-markers give me grey hair.
