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

implement instant doubles functionality #1892

Merged
merged 38 commits into from Feb 8, 2019

Conversation

@iamcodemaker
Copy link
Contributor

commented Nov 6, 2018

This is a start on implementing instant doubles functionality. As is, copying rate, and loop state works. Copying play state works sometimes. Copying pitch_adjust and playposition don't seem to work at all. I'm open to suggestions.

https://bugs.launchpad.net/mixxx/+bug/408111

@iamcodemaker iamcodemaker force-pushed the iamcodemaker:seeing-double branch from af01be3 to f42d5e8 Nov 6, 2018
Copy link
Contributor Author

left a comment

Some of the elements aren't copied. I'm not sure where to go from here. Any ideas?

src/mixer/basetrackplayer.cpp Outdated Show resolved Hide resolved
src/mixer/basetrackplayer.cpp Outdated Show resolved Hide resolved
src/mixer/basetrackplayer.cpp Show resolved Hide resolved
Copy link
Member

left a comment

Thank you for the work so far. I assume your problems are mainly cases by follow up code reverting your changes. So I think the code need to be moved arround a bit and special case these places losing the initial or the copied values.
I think in general, you shools load a paused track, adjust the COs and then play and seek it with special engine code that achieved playing sample exact like the source track. If this is a case we will get +6 dB volume. If not we get an unwanted phaser effect.

src/mixer/basetrackplayer.cpp Outdated Show resolved Hide resolved
src/mixer/basetrackplayer.cpp Outdated Show resolved Hide resolved
src/mixer/basetrackplayer.cpp Show resolved Hide resolved
src/mixer/basetrackplayer.cpp Outdated Show resolved Hide resolved
src/mixer/basetrackplayer.cpp Outdated Show resolved Hide resolved
@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2018

With the latest revision, this works, but the play position is slightly behind.

@mixxxdj mixxxdj deleted a comment Nov 9, 2018
@mixxxdj mixxxdj deleted a comment Nov 9, 2018
@mixxxdj mixxxdj deleted a comment from daschuer Nov 9, 2018
@daschuer

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

slotControlSeekExact() excepts double, there must be another problem.

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2018

Yes, there is another problem. "playposition" is a fractional value between
0.0 and 1.0 but slotControlSeekExact() does not expect a fractional value. I need to get the non fractional value, but I don't see an interface for this. So when I modify the engine code, I'll need to either add an interface for this or do it in the engine buffer directly. But I need to figure out how to access one engine buffer from another if I go that route.

@daschuer

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

If you move thelogic to EngineBuffer:: process, you can access all data. You just need a static atomic varianle that is used to communicate between the instances if EmgineBuffer. Did you have a look at control valve atomic? IMHO that suites well as a container class for all shared data.

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2018

I'll take a look.

@rryan

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

If you move thelogic to EngineBuffer:: process,

To be specific, please move just the "sync to other deck's playposition atomically" logic, not the whole instant doubles functionality! Let's try to keep as much "business logic" as possible out of the engine, as it's already a rats nest.

@rryan

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

"playposition" is a fractional value between
0.0 and 1.0 but slotControlSeekExact() does not expect a fractional value.

It may be moot if you move this into the engine, but just for reference you can get the "track_samples" CO to get the number of samples in a deck's loaded track -- you can multiply that by playposition to get a sample value for slotControlSeekExact.

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2018

Ahh, I wondering if you could get the non fractional position. Thanks.

@daschuer

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

I have looked into the engine code when working on #1896
I think your task is not too hard.
I think you may tweak BpmControl::getNearestPositionInPhase()
To use the copy source as sync target.
Than you need to handle the case where no beatgrid is available, and add a new seek the calling this function.

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2018

I'll look at that. I have been looking at adding code to EngineMaster that tells one channel to seek to the position of another. I need to figure out the best way to trigger it though.

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2018

I pushed a new rev that performs the copy operation in the engine thread. This works perfectly, the new track is started in the exact right position. I am going to work on cleaning this up. I think I am going to use EngineSync::pickNonSyncSyncTarget() to find the source channel and then tell EngineMaster to clone the position. Or cloning the sync target's position may need to be a special seek request or a special sync request. I think it kinda makes sense as a sync request.

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

I cleaned this up some. Now it uses EngineSync::pickNonSyncSyncTarget() to determine which deck to copy from. Also the play position copy is triggered from a special sync request I added to EngineBuffer. I plan to add code to do a copy instead of a normal load if the same track is loaded twice withing a certain period of time. I am also going to look into adding some tests.

There are still some oddities around looping. It looks like the loop in and out points aren't copied to duplicate track. I'll need to set them manually.

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

updated the loop copying code

Copy link
Member

left a comment

Nice progress. Thanks

I wonder if this should be the default if you drag a track from one deck to an other.

src/engine/enginebuffer.cpp Outdated Show resolved Hide resolved
src/engine/enginebuffer.cpp Outdated Show resolved Hide resolved
@@ -1137,6 +1147,12 @@ void EngineBuffer::processSyncRequests() {
m_pEngineSync->requestEnableSync(m_pSyncControl, true);
m_pEngineSync->requestEnableSync(m_pSyncControl, false);
break;
case SYNC_REQUEST_POSITION:
pChannel = m_pEngineSync->pickNonSyncSyncTarget(m_pSyncControl->getChannel());

This comment has been minimized.

Copy link
@daschuer

daschuer Nov 15, 2018

Member

This picks a kind of random other channel, but we want the source deck.
I would use a QAtomicPointer and store the source pChannel this can be checked in processSyncRequests() and if set, bypass the other flags and setNewPlaypos with the pOtherEngineBuffer->getVisualPlayPos().

This comment has been minimized.

Copy link
@iamcodemaker

iamcodemaker Nov 18, 2018

Author Contributor

A pickNonSyncSyncTarget() call is still in there but, everything has been reworked to copy from a channel passed in. I'll need some guidance on how to determine which channel to copy from though.

src/mixer/basetrackplayer.cpp Outdated Show resolved Hide resolved
@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2018

Added a way to trigger the copy on double tap of the load buttons. I'll address the feedback shortly. I tried writing some tests for this, but didn't get very far. I ran into issues when the clone code tries to load the track playing on the other deck.

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2018

Ok, I think I have addressed the comments thus far. There are still some tweaks that need to be made though.

mixxx::Duration elapsed = m_cloneTimer.restart();
if (elapsed < mixxx::Duration::fromSeconds(1)) {
// load pressed twice quickly, clone instead of loading
EngineChannel* pChannel = m_pEngineMaster->getEngineSync()->pickNonSyncSyncTarget(m_pChannel);

This comment has been minimized.

Copy link
@iamcodemaker

iamcodemaker Nov 18, 2018

Author Contributor

this needs to be replaced with something more specific and predictable, but I don't know what that is. I'll need some guidance here.


// copy play state
ControlObject::set(ConfigKey(getGroup(), "play"),
ControlObject::get(ConfigKey(m_cloneFromChannel->getGroup(), "play")));

This comment has been minimized.

Copy link
@iamcodemaker

iamcodemaker Nov 18, 2018

Author Contributor

previously I was triggering a play on load, but this caused the track to play before it was seeked and this was audible sometimes. Triggering the play here reduces the gap between play and seek, but it might make sense to move the play operation as part of the requestSyncPosition() operation though.

This comment has been minimized.

Copy link
@iamcodemaker

iamcodemaker Nov 30, 2018

Author Contributor

I attempted to play from the engine thread as part of the sync operation, but it didn't work quite right.

@Be-ing

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

I wonder if this should be the default if you drag a track from one deck to an other.

Yes, I think that would be the most straightforward way to expose the functionality in the GUI.

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

I'll look into that.

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2018

Updated with drag and drop functionality. Dragging a track from one deck to another clones the deck.

@esbrandt

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

@iamcodemaker
Thanks for adding that feature.

Trying to incorporate this into my setup, and some questions arise about how this feature is supposed to work.

  • Cloning works by dragging a loaded track from one deck to another, while dragging a track from the library to the deck does not invoke cloning, correct?
  • Using the CloneFromDeck control, should it be possible to clone a deck by midi/keyboard shortcut? I added a shortcut for cloning Deck 1, and it does not work as intended.
Debug [Main]: keyboard press:  "Ctrl+Shift+Left"
Critical [Main]: DEBUG ASSERT: "pBehavior" in function void ControlDoublePrivate::setValueFromMidi(MidiOpCode, double) at src/control/control.cpp:244
Warning [Main]: Cannot set "[Channel1]" , "CloneFromDeck" by Midi

Any hints? Thanks

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2019

Dragging from the library should not trigger a clone. I didn't test this with a keyboard shortcut, it worked from my midi mapping. I'll test with a keyboard shortcut. The other way to clone is to press load twice quickly (shift+left or shift+right)

@daschuer

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

"[Channel1]" , "CloneFromDeck" is just an ControlObject and expects the 1 based source deck number as value. I think you cannot map it via keyboard.
For this we need a new ControlPushButton.
What should happen if a user "Ctrl+Shift+Left"?
I think we need "[Channel1]" , "CloneFromDeck1" for this. Since we will have "Ctrl+Shift+Left" and "Ctrl+Shift+Right", how can we make it more flexible and predicting what the user wants in a four deck layout?
Ideas?

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2019

So right now that's the idea behind the double load functionality. It clones the first playing deck. That doesn't work if you want to have two separate groups of doubling going on though or if you have three or 4 decks playing, but for most uses, just cloning the other playing deck will be sufficient.

We could also just hard code cloning between decks 1+2 and 3+4, such that if the user activates clone on one of those decks, the target is always the other deck in the group. Not sure if that's better or not. Personally I like it the way it is now, I think that's more flexible.

@ronso0

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2019

One of my Load buttons is faulty (sends multiple 'pressed' signals sometimes). The connected long-press function (set star rating) fails and I'd try to press again, but currently the hard-coded clone functionality gives me a deck clone and I'd have to pick and load the previous track again manually.

Can I somehow opt out of that 'double-press to Load' behaviour?

@ronso0

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2019

Maybe it doesn't need an option but just an improvement of the double-press detection.

as described, by accident this press pattern happens with my broken button:
[looong press] [short release] [short press]
This unsual click gesture that's not used anywhere else AFAICT and is kinda hard to execute on purpose so I think it only occurs with faulty hardware and can safely be excluded to trigger cloning.

Only trigger cloning when a simple double-tap is detected. This would be an improvement for some, while noone would suffer.
[short press] [short release] [short press]

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2019

I don't think it makes sense to compensate for broken hardware in the code like that. This could have unforseen consequences on non-broken hardware. IMO the best solution would be to detect this in your mapping script and supress the second press. That or patch your local copy.

@ronso0

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

Okay, I'll find a way around, probably with complex timer filters in my script (as I have a long-press function already).
Though I really think other affected users won't find a way. A (opt-out) Preferences option would still be my preferred choice.

After some quick investigation I think the issue is that the time between button releases is measured, rather in between button presses, or better: duration of both presses.

Out of curiosity (and to not just cut off the disccussion):

This could have unforseen consequences on non-broken hardware

I suggest to filter out [long press] [short press] which is a gesture not supposed to do anything, [short press] [short press] would work as before.
What could be potential impairments in your opinion?

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

The control object doesn't know anything about the duration of button presses or the difference between press and release. It just knows a load operation was requested. It's checking the timing between load operations. Thinking about this further, the mapping script is the only place you could implement your solution.

Regarding potential unforseen issues. I don't know, but that's the point. If we are making assumptions about what the user or hardware is telling us (beyond the known expected behavior), there is the potential for our assumptions to be wrong and lead to unexpected behavior. Think of when auto correct fails on your smartphone keyboard.

I think other users with broken hardware will just replace or repair their broken hardware. If every one of these devices was faulty in the same exact way, then it might make sense to distribute this work around in the mapping script.

@daschuer

This comment has been minimized.

Copy link
Member

commented Feb 24, 2019

In general, I think the mapping is a actually the right place to fix it. I consider a missing denouncing in the controller firmware more likely than a hardware issue.

However, something else might be wrong.
Do you use the "LoadSelectedTrack" control? It is sensitive only for rising slopes.
[looong press] [short release] [short press]
has two rising slopes. For the clone feature they have to be withing 500 ms. If we assume [looong press] is longer than 500 ms the issue must not happen and we have a bug somewhere else.

Or do we have actually [looong press] [short release] [short press] [short release] [short press]?

@ronso0

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

Or do we have actually [looong press] [short release] [short press] [short release] [short press]?

After further testing I think this is the case unfortuantely. It happened quite often earlier when I tried [looong press] [short press], now I can't reproduce anymore. Maybe because of this extensive button massage :)
sorry for the noise.

Anyway, the double-tap happens accidentially too often, and filtering this in the script is a lot of work, so I'd prefer this to be opt-out.

@daschuer

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Ok, this makes sense in this case.
Would you mind to file a bug for reference?

@ronso0

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

I won't forget as I already work on the peferences option in https://github.com/ronso0/mixxx/tree/clone-deck-opt-out but it'not working yet.

@ronso0

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

anyone's currently working on the direct-clone COs like [Channel2],cloneToChannel3?

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

No, we decided to go the CloneFromDeck route.

@ronso0

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

I don't mind to or from if in the end we have 12 different COs to specify the clone route deckX->deckY.
so it's WIP?

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

The CloneFromDeck CO was implemented as part of this PR. You specify the deck number you want to clone from as the parameter.

@ronso0

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

could you give me a working example for a script? I have a hard time scrolling through the code and expected to find something in controllers/controllerpickermenu.cpp

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

The CloneFromDeck CO is documented on the wiki. I don't have a working example to share. Basically in a controller mapping, it would work like this. The function names and argument order may be wrong, but this is the idea.

engine.set("[Channel1]", "CloneFromDeck", 2)

That would clone from deck 2 to deck 1.

@ronso0

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2019

this works:
engine.setValue("[Channel1]", "CloneFromDeck", 2)

used in a script:
engine.setValue( group, "CloneFromDeck", targetDeck);

@esbrandt

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

Double pressing Shift+Left and Shift+Right invoke 2 operations: loading a track to the respective deck, then replacing the just loaded track with the clone.

Issues

  • Unlike other standard keyboard shortcuts like Dor L , the hardcoded cloning shortcuts do only work, if the library table has focus.
  • If the library table has focus, and the highlighted track in a library table is inaccessible, a modal window appears (error message), and cloning stops.

If the shortcuts for loading and cloning could be decoupled, it would make this feature even more useful. And it makes #2042 obsolete, introducing another preference option just to disable this feature is not necessary.

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2019

The problem with only having a separate way to trigger this is that you then have to duplicate that everywhere you might trigger a load or clone.

We need a way to say "load button was pressed, then pressed again, cancel the first load". That would solve your second issue and result in the dialog briefly appearing then disappearing.

For first issue, I don't know Mixxx or Qt well enough to really address that. In theory it should be possible to capture any shift+left/right for the whole window and then determine the correct action.

@ronso0

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

an idea I had while working on #2042 :
can we start a timer on first Load press that waits for 500ms (current double-tap period)?

  • clone if there's a 2nd Load press within that wait period
  • regular load with no 2nd Load press

Wouldn't that solve both issues?

@ronso0

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

#2042 would still be very helpful for controller use case IMO

@ronso0

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

cloning comes in very handy when I play with 3 decks and want to get the running tracks back to decks 1 & 2.
👍

@iamcodemaker

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

can we start a timer on first Load press that waits for 500ms (current double-tap period)?

I considered this approach. This would work, but we would need to take special care to skip the delay in the "LoadAndPlay" case. Also, a delay would make loading tracks feel laggy.

@ronso0

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

what about this:
clone a deck by default if it is already loaded & playing in another deck?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.