Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a preference option for the order that tracks will be loaded in 4-deck skins #6

Closed
wants to merge 25 commits into from

Conversation

ywwg
Copy link
Member

@ywwg ywwg commented May 21, 2013

No description provided.

@daschuer
Copy link
Member

Gihub complains:
"This diff is too big to show! We're showing status information only."

Any idea to solve it?

@ywwg
Copy link
Member Author

ywwg commented May 21, 2013

I'll remove my new skins, that should help

@ywwg
Copy link
Member Author

ywwg commented May 21, 2013

OK This should be readable now.

@@ -123,6 +147,8 @@ class PlayerManager : public QObject {
// Used to protect access to PlayerManager state across threads.
mutable QMutex m_mutex;

QList<QList<int> > ms_deck_orderings;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you name this with camelCase

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, what's ms_?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh it was originally a static member, so I was like, ms_!

@rryan
Copy link
Member

rryan commented May 21, 2013

I added some particular comments on the implementation, but in general I think we should do something more general than make a hard-coded fix for 4-decks.

@@ -34,17 +34,24 @@
m_pAnalyserQueue(NULL),
m_pCONumDecks(new ControlObject(ConfigKey("[Master]", "num_decks"), true, true)),
m_pCONumSamplers(new ControlObject(ConfigKey("[Master]", "num_samplers"), true, true)),
m_pCONumPreviewDecks(new ControlObject(ConfigKey("[Master]", "num_preview_decks"), true, true)) {
m_pCONumPreviewDecks(new ControlObject(ConfigKey("[Master]", "num_preview_decks"), true, true)),
m_pCOSkinNumDecks(new ControlObject(ConfigKey("[Skin]", "num_decks"), true, true)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to create [Skin],num_samplers and [Skin],num_preview_decks as well.

@rryan
Copy link
Member

rryan commented May 21, 2013

The [Skin],num_decks thing is kind of unsettling the more I think about it.

If you can't load a track into a deck the skin didn't declare (e.g. decks the MIDI controller declares) then what's the point of letting a MIDI controller declare decks? This change means that in order to use N-decks your skin must support the N.

What issues are solved by the [Skin],num_decks change? I wonder if there's a better fix.

@ywwg
Copy link
Member Author

ywwg commented May 21, 2013

I strongly disagree with allowing decks to exist that aren't visible in the skin. It's a recipe for user confusion -- it's even confused me when I have a third track that I can't see loaded in a 2-deck skin. The problem skin-decks solves is when I switch between 2-deck skins and 4-deck skins. I want to be able to load as many tracks as the skin has, period. Is there an example of a midi controller we support that has 4 decks that would be usable without a 4 deck skin? It would have to have waveform overview displays, text displays, something like a CDJ.

This change reveals the need for more 4-deck skins, but that's the point. We don't support 4 decks if you can't see them in the interface.

As for hard-coding 4-deck solutions, I think 4-decks is a special case. With more decks, like 8 or 16, the user is doing loop-based work and the decks should just be loaded in order as I've coded. With 2, it's obvious that they are left and right. 4 decks is where you see this CABD stuff, and when I've spun at middlesex they definitely had the "outside/inside" method of pairing decks. It's odd, but the way 4 decks are actually used justifies the code.

@ywwg
Copy link
Member Author

ywwg commented May 21, 2013

Pushed changes. Untested, but it builds.

@rryan
Copy link
Member

rryan commented May 21, 2013

On May 21, 2013 9:29 AM, "Owen Williams" [email protected] wrote:

I strongly disagree with allowing decks to exist that aren't visible in
the skin. It's a recipe for user confusion -- it's even confused me when I
have a third track that I can't see loaded in a 2-deck skin.

Hmm you're right. I agree we shouldn't show decks that don't have a ui
representation.

But maybe we should instead fix the issue where mixxx can't remove a deck
that has already been created. That would fix the switching skins issue.

It wouldn't fix midi controllers that will just break when they have been
written assuming a fixed number of decks. We could make mappings and script
that touches non existent controls just silently fail instead of throwing a
hissy fit. Also loading a preset that is for 4 decks should give the user a
warning if that many decks do not exist.

@ywwg
Copy link
Member Author

ywwg commented May 21, 2013

I agree that we should be able to remove decks. In fact I tried to do that at first and ran into race conditions if I tried to delete a deck while it was playing. Maybe decks should be able to receive a "die()" signal so they can safely eject tracks and shut themselves down? I'm not sure what the proper multithreaded OO way of doing this is.

@rryan
Copy link
Member

rryan commented May 21, 2013

I think deck ordering is a good idea but I think the implementation should
be more general. What about 3 decks? Or 5? We'll surely get requests for it
even if it is not the main use case and if we went with the current
implementation it will box us in to adding a similar special case for every
number we want to support.
On May 21, 2013 9:29 AM, "Owen Williams" [email protected] wrote:

I strongly disagree with allowing decks to exist that aren't visible in
the skin. It's a recipe for user confusion -- it's even confused me when I
have a third track that I can't see loaded in a 2-deck skin. The problem
skin-decks solves is when I switch between 2-deck skins and 4-deck skins. I
want to be able to load as many tracks as the skin has, period. Is there an
example of a midi controller we support that has 4 decks that would be
usable without a 4 deck skin? It would have to have waveform overview
displays, text displays, something like a CDJ.

This change reveals the need for more 4-deck skins, but that's the point.
We don't support 4 decks if you can't see them in the interface.

As for hard-coding 4-deck solutions, I think 4-decks is a special case.
With more decks, like 8 or 16, the user is doing loop-based work and the
decks should just be loaded in order as I've coded. With 2, it's obvious
that they are left and right. 4 decks is where you see this CABD stuff, and
when I've spun at middlesex they definitely had the "outside/inside" method
of pairing decks. It's odd, but the way 4 decks are actually used justifies
the code.


Reply to this email directly or view it on GitHubhttps://github.com//pull/6#issuecomment-18220170
.

@ywwg
Copy link
Member Author

ywwg commented May 21, 2013

true, I have a three-deck mixer actually, though the use-case is more for two-decks and an mp3 player. I don't know how to programatically generate sane orderings. e.g., no one is going to want DACB, right?

I guess all I can think of is a drag-reorderable list of decks. Which seems like overkill. But now that I think about use-cases, what if someone wants to use Mixxx as a kind of mixing board for a whole band? Maybe they have two turntables, and a mic, and a guitar, and a synth. Who knows where the turntables are.

@rryan
Copy link
Member

rryan commented May 21, 2013

Yea - The UI could be hard to make general. The preferences could be 4deck
hard coded for 1.12 but the underlying implementation should be general.
See what I mean? That makes future 3 deck support a UI issue alone.
On May 21, 2013 11:23 AM, "Owen Williams" [email protected] wrote:

true, I have a three-deck mixer actually, though the use-case is more for
two-decks and an mp3 player. I don't know how to programatically generate
sane orderings. e.g., no one is going to want DACB, right?

I guess all I can think of is a drag-reorderable list of decks. Which
seems like overkill. But now that I think about use-cases, what if someone
wants to use Mixxx as a kind of mixing board for a whole band. Maybe they
have two turntables, and a mic, and a guitar, and a synth. Who knows where
the turntables are.


Reply to this email directly or view it on GitHubhttps://github.com//pull/6#issuecomment-18227087
.

@ywwg
Copy link
Member Author

ywwg commented May 21, 2013

How about making deckorderings a hash of int to lists of strings? QHash < QList < QString > > >. Or maybe a small class that has that structure internally, but externally just has convenience functions like .begin and .end for easy enumeration. (That would also solve the translation issue between the visual ordering, eg CABD, and the load ordering, eg 1,2,0,3.

the generic code would be:

if number_of_decks in deckorderings.keys:
return list at that key of possible orderings, where each ordering is a string "ABCDEFGH..." etc
else:
return numeric order only.

so for now we'd only prepopulate hashvalue of 4. later if we want to add something for 3 deck, we just push a string into the list for hash 3. There could even be a UI that would push those values in.

// Make sure the number of internal decks is in sync with the number of decks in the skin.
connect(m_pCOSkinNumDecks, SIGNAL(valueChanged(double)),
this, SLOT(slotNumDecksControlChanged(double)),
Qt::DirectConnection);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Qt::DirectConnection should be removed.
After our atomic changes this is always a Qt::DirectConnection because the signal is resend by the ControlObject itselve.
Direct connections are somehow not supported now because if the signal is from an other thread it is transfered into the current thread by the ControlObject before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, GitHub did a really bad job formatting my reply-via-email. Re-posting:

That's not true -- an AutoConnection decides between direct invocation and queueing based on the emitting thread and the receiver object's thread property alone (if they match, direct. if not, queue). Nothing we did in atomic-co changes this -- a non main thread change made to this control (by looking it up with getControl or in the case of our many shared state multiple thread classes) would queue, not directly invoke. Also queueing is not acceptable here because other parts of Mixxx want this processing to be done immediately (eg so skin processing doesn't have to block while waiting for PlayerManager to get the signal) and this class is designed to be thread safe (locks) because of this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2013/5/22 RJ Ryan [email protected]

In mixxx/src/playermanager.cpp:

@@ -46,6 +50,17 @@
this, SLOT(slotNumPreviewDecksControlChanged(double)),
Qt::DirectConnection);

  • // Make sure the number of internal decks is in sync with the number of decks in the skin.
  • connect(m_pCOSkinNumDecks, SIGNAL(valueChanged(double)),
  •        this, SLOT(slotNumDecksControlChanged(double)),
    
  •        Qt::DirectConnection);
    

Oops, GitHub did a really bad job formatting my reply-via-email.
Re-posting:

That's not true -- an AutoConnection decides between direct invocation and
queueing based on the emitting thread and the receiver object's thread
property alone (if they match, direct. if not, queue). Nothing we did in
atomic-co changes this -- a non main thread change made to this control (by
looking it up with getControl or in the case of our many shared state
multiple thread classes) would queue, not directly invoke. Also queueing is
not acceptable here because other parts of Mixxx want this processing to be
done immediately (eg so skin processing doesn't have to block while waiting
for PlayerManager to get the signal) and this class is designed to be
thread safe (locks) because of this.ect

Hi RJ,

in general, you are right, but in this case we have a double signal, one
with (QObject* sender), which is a queued connection in case of a different
sender and a direct connection else. The final signal without sender is a
direct connection in any case. But isn't the PlayerManager and the
LegacySkin parser in the same thread anyway?

Reply to this email directly or view it on GitHubhttps://github.com//pull/6/files#r4343623
.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean by double signal? If you are referring to the chain of signals from ControlObject::set() -> ControlDoublePrivate::set() -> ControlDoublePrivate::valueChanged() -> ControlObject::privateValueChanged() -> ControlObject::valueChanged/valueChangedFromEngine then I think you are missing that the ControlNumericPrivate / ControlObject connection is also a direct connection (and must be -- otherwise Qt will take unintended roundtrips through the event queue if the current thread when set() is called does not match the thread property of the object attached to the signal).

Qt signals and slots do not care about the emitter of the signal at all, all they care about is the current thread when emit() is called. If the current thread when emit is called does not match the receiving objects 'thread' property then AutoConnection is queued, otherwise it is a DirectConnection.

isn't the PlayerManager and the LegacySkin parser in the same thread anyway

PlayerManager is thread safe and its methods are invoked by multiple threads in addition to the main thread. LegacySkinParser is totally in the main thread.

For example, MIDI controllers can (and do) set the number of decks. Then the valueChanged signal would come from the controller thread.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, Yes, I have overlooked that ControlObject::initialize() sets up a
direct connection.

2013/5/23 RJ Ryan [email protected]

In mixxx/src/playermanager.cpp:

@@ -46,6 +50,17 @@
this, SLOT(slotNumPreviewDecksControlChanged(double)),
Qt::DirectConnection);

  • // Make sure the number of internal decks is in sync with the number of decks in the skin.
  • connect(m_pCOSkinNumDecks, SIGNAL(valueChanged(double)),
  •        this, SLOT(slotNumDecksControlChanged(double)),
    
  •        Qt::DirectConnection);
    

Not sure what you mean by double signal? If you are referring to the chain
of signals from ControlObject::set() -> ControlDoublePrivate::set() ->
ControlDoublePrivate::valueChanged() ->
ControlObject::privateValueChanged() ->
ControlObject::valueChanged/valueChangedFromEngine then I think you are
missing that the ControlNumericPrivate / ControlObject connection is also a
direct connection (and must be -- otherwise Qt will take unintended
roundtrips through the event queue if the current thread when set() is
called does not match the thread property of the object attached to the
signal).

Qt signals and slots do not care about the emitter of the signal at all,
all they care about is the current thread when emit() is called. If the
current thread when emit is called does not match the receiving objects
'thread' property then AutoConnection is queued, otherwise it is a
DirectConnection.

isn't the PlayerManager and the LegacySkin parser in the same thread anyway

PlayerManager is thread safe and its methods are invoked by multiple
threads in addition to the main thread. LegacySkinParser is totally in the
main thread.

For example, MIDI controllers can (and do) set the number of decks. Then
the valueChanged signal would come from the controller thread.


Reply to this email directly or view it on GitHubhttps://github.com//pull/6/files#r4351458
.

@daschuer
Copy link
Member

I did not get the point why we need [Master] and [Skin] deck_num.
We have also m_preview_decks.size() for a internal count of decks.

I would like to stay with [Master] deck_num, Because there is IMHO no need for renaming it.

I like the Idea to let the skin control the Deck count. Is there a way to avoid others to control the deck count?
Should we implement a ro control object for this purpose?


Will the deck lable be shown in the Skin?
I wonder why the Mixer orientation is no issue here?

@ywwg
Copy link
Member Author

ywwg commented May 22, 2013

The reason I added [Skin] deck_num is because [Master] deck_num can never be reduced. So if I go from a skin with 4 decks to a skin with 2 decks, [Master]deck_num still says 4. So I wanted a CO that would keep track of how many decks are actually usable.

A better solution to this would be to support the removing of decks so that [Master]deck_num is accurate in that use-case.

daschuer referenced this pull request in daschuer/mixxx Feb 26, 2014
Reimplement reverb effect with newly-ported CAPS Reverb effect.
@daschuer daschuer mentioned this pull request Dec 28, 2014
ferranpujolcamins referenced this pull request in ferranpujolcamins/mixxx Jul 22, 2015
ywwg pushed a commit that referenced this pull request Oct 28, 2015
Fix compile on WIN, use GetSystemTimePreciseAsFileTime() on Windows 8
@daschuer daschuer mentioned this pull request Aug 13, 2017
19 tasks
@daschuer daschuer mentioned this pull request Jun 3, 2018
@daschuer daschuer mentioned this pull request Jun 11, 2018
11 tasks
daschuer pushed a commit that referenced this pull request Mar 4, 2020
use QEvent::WindowDeactivate to reset the press state
Holzhaus referenced this pull request in Holzhaus/mixxx Apr 9, 2020
Holzhaus pushed a commit that referenced this pull request Apr 27, 2020
Holzhaus pushed a commit that referenced this pull request Jun 12, 2020
Hercules DJControl Inpulse 200: Fix some code stye issues
jusko pushed a commit to jusko/mixxx that referenced this pull request Dec 19, 2020
jusko pushed a commit to jusko/mixxx that referenced this pull request Dec 22, 2020
jusko pushed a commit to jusko/mixxx that referenced this pull request Jan 30, 2021
ywwg pushed a commit that referenced this pull request Mar 7, 2022
…h sync

When loading a track that is not yet present in the library (and thus
doesn't have any BPM because it hasn't been analyzed yet) while another
deck is playing and both decks have sync enabled, a debug assertion is
triggered:

    DEBUG ASSERT: "isValid()" in function double mixxx::Bpm::value() const at src/track/bpm.h:53
    Aborted (core dumped)

The backtrace looks as follows:

    #0  0x00007f175c87234c in __pthread_kill_implementation () at /usr/lib/libc.so.6
    #1  0x00007f175c8254b8 in raise () at /usr/lib/libc.so.6
    #2  0x00007f175c80f534 in abort () at /usr/lib/libc.so.6
    #3  0x00007f175cf05ee4 in qt_assert(char const*, char const*, int) () at /usr/lib/libQt5Core.so.5
    #4  0x000055deb2e67e1c in mixxx::(anonymous namespace)::handleMessage(QtMsgType, QMessageLogContext const&, QString const&) (type=<optimized out>, context=<optimized out>, input=<optimized out>) at src/util/logging.cpp:355
    #5  0x00007f175cf47128 in  () at /usr/lib/libQt5Core.so.5
    #6  0x00007f175cf3fd8a in  () at /usr/lib/libQt5Core.so.5
    #7  0x00007f175cf06526 in QMessageLogger::critical(char const*, ...) const () at /usr/lib/libQt5Core.so.5
    #8  0x000055deb2e5c720 in mixxx_debug_assert(char const*, char const*, int, char const*) (assertion=assertion@entry=0x55deb39bd0db "isValid()", file=file@entry=0x55deb39bbf30 "src/track/bpm.h", line=line@entry=53, function=function@entry=0x55deb39bbf08 "double mixxx::Bpm::value() const") at gsrc/util/assert.h:9
    #9  0x000055deb2ee7e7e in mixxx_debug_assert_return_true(char const*, char const*, int, char const*) (function=0x55deb39bbf08 "double mixxx::Bpm::value() const", line=53, file=0x55deb39bbf30 "src/track/bpm.h", assertion=0x55deb39bd0db "isValid()") at gsrc/util/assert.h:18
    #10 mixxx::Bpm::value() const (this=<synthetic pointer>) at src/track/bpm.h:53
    #11 mixxx::operator*(mixxx::Bpm, double) (multiple=1, bpm=...) at src/track/bpm.h:160
    #12 SyncControl::setLocalBpm(mixxx::Bpm) (this=<optimized out>, localBpm=...) at src/engine/sync/synccontrol.cpp:567
    #13 0x000055deb34c7ba3 in EngineBuffer::postProcess(int) (this=0x55deb56eb060, iBufferSize=2048) at src/engine/enginebuffer.cpp:1318
    #14 0x000055deb3139023 in EngineMaster::processChannels(int) (this=0x55deb5449440, iBufferSize=<optimized out>) at src/engine/enginemaster.cpp:383
    #15 0x000055deb31394f7 in EngineMaster::process(int) (this=0x55deb5449440, iBufferSize=iBufferSize@entry=2048) at src/engine/enginemaster.cpp:410
    #16 0x000055deb2f91d0b in SoundManager::onDeviceOutputCallback(long) (this=<optimized out>, iFramesPerBuffer=iFramesPerBuffer@entry=1024) at src/soundio/soundmanager.cpp:596
    #17 0x000055deb32dd794 in SoundDevicePortAudio::callbackProcessClkRef(long, float*, float const*, PaStreamCallbackTimeInfo const*, unsigned long) (this=0x55deb553e6b0, framesPerBuffer=1024, out=<optimized out>, in=<optimized out>, timeInfo=<optimized out>, statusFlags=<optimized out>) at src/soundio/sounddeviceportaudio.cpp:965

This happens because `newLocalBpm` is invalid when `localBpm` is
invalid. Trying to do sync decks while no tempo information is available
does not make sense, so we only synchronize decks if the local BPM is
available.
daschuer pushed a commit that referenced this pull request Jun 3, 2023
Silence some float conversion warnings by making conversion explicit
daschuer pushed a commit that referenced this pull request Jan 28, 2024
VersionStore: Remove Apple fallback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants