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

Pioneer DDJ-ERGO: added mappings #13003

Open
wants to merge 1 commit into
base: 2.4
Choose a base branch
from

Conversation

jssotomdz
Copy link

No description provided.

@jssotomdz
Copy link
Author

manual page is here mixxxdj/manual#602

@ronso0
Copy link
Member

ronso0 commented Mar 27, 2024

Good to see you picked this up again.

Just some thougts, I didn't take a closer look, yet:

@ronso0 ronso0 changed the base branch from main to 2.4 March 27, 2024 03:37
@ronso0
Copy link
Member

ronso0 commented Mar 27, 2024

controller mappings usually go to the stable branch (2.4

I changed the base branch here, and indeed your branch is based on main (all commits since branching 2.4 are listed).
Please rebase onto 2.4, then force-push. Should be as easy as
git rebase -i upstream/2.4 (in case you chose a different name Mixxx remote repo, you need to replace "upstream" accordingly)
then
git push -f jssotomdz pioneer-ergo

If you think yo messed up something don't close this PR right away, we can fix it : )

@JoergAtGithub
Copy link
Member

Welcome at Mixxx!
As a first-time contributor we need you to sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

@jssotomdz jssotomdz force-pushed the pioneer-ergo branch 2 times, most recently from 9003744 to 4572dd8 Compare March 27, 2024 19:50
@ronso0
Copy link
Member

ronso0 commented Mar 27, 2024

@jssotomdz already signed for #12456 as Jesús Soto

@jssotomdz
Copy link
Author

ci was broken due to some white spaces, not sure how they got there

@ronso0
Copy link
Member

ronso0 commented Mar 27, 2024

All good now, thanks!

Did you consider the proposed (Mixxx deafult) effects mapping? #13003 (comment)

I know that implementing that would mean you'd throw away of your own implementation (the script would shrink considerably), but it's the default and it guarantees some kind of consistency when using different controllers with Mixxx. (also it makes 4 effect units available for free ;)
If you need an example you may take a look at how it's done in the Reloop TerminalMix mapping, xml and script:effects + script:shiftButtons

@jssotomdz
Copy link
Author

Yes, took a look at it and makes sense, just haven't had the time to test it with the hardware. Also, will that affect the quick effects? ERGO V has two quick on/off toggles for effects. Or could I leave them as is?

Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

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

I skimmed through the script, here are some first remarks.

Btw I'm surprised eslint doesn't complain about the missing spaces around * + - etc.

engine.setParameter(group, "reloop_toggle", 1);
} else {
engine.setParameter(group, "beatloop_activate", 1);
}
Copy link
Member

Choose a reason for hiding this comment

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

beatloop_activate does create/activate and deactivate, see the beatloop button in skins.
I suggest to map Shift+Autoloop to reloop_toggle.

Copy link
Author

Choose a reason for hiding this comment

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

reloop_toggle is mapped to Shift+Autoloop already, but I was unaware that calling beatloop_activate with a 1 would deactivate the loop. I was trying with 0 and didn't work so figured I could do it with reloop. Fixed now, thanks for pointing it out!

Comment on lines 131 to 134
//this is because the controller never sends a 0x40 message and hence never stops scratching
if (Math.abs(value-0x40)===1) {
value = 0x40;
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand: what dou you need 0x40 for?
Values above/below are used for scratch ticks. No move, no ticks.

res/controllers/PIONEER-DDJ-ERGO-scripts.js Outdated Show resolved Hide resolved
}
engine.setValue(group, "rateRange", curr);
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Why not use the ranges available in the preferences?
4, 6, 8, 10, 16, 24, 50, 90
At least I would set the max higher, probably 90%. The pitch sliders show the range so this should be easy to use, regardless how many ranges are provided.

Another example from the TerminalMix, though that applies the range to all decks and doesn't use the 'official' ranges either

TerminalMix.pitchRange = function (channel, control, value, status, group) {

if (newValue < 0) {
engine.setValue(group, "beats_translate_earlier", 1);
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Re loopMove and adjustGrid: what do yo uthink about this set of combos

  • AUTOLOOP turn: resize loop
  • Shift + AUTOLOOP turn: adjust beatjump_size
  • Shift + In/Out: move loop by beatjump_size
  • idea: hold In+Out + AUTOLOOP turn: move beatgrid

It is certainly nice to have beatgrid controls on the controller (I make massive use of it), but using other beatjump sizes than 1 beat is also very common (IIRC other controllers have a pad mode for this).

Copy link
Author

Choose a reason for hiding this comment

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

I mapped this following the virtual dj mapping as close as possible. I'm not sure if this controller supports long press (hold). Will take a look but that's the why of this combo

Copy link
Member

Choose a reason for hiding this comment

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

I mapped this following the virtual dj mapping as close as possible

Understandable, but there's no reason to mimic the (potentially sub-optimal) behaviour another software.
Let's start from zero and try to make it work good with Mixxx and cover as many use cases as possible.

Copy link
Author

Choose a reason for hiding this comment

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

ok so unfortunately ERGO doesn't support long press. Should I proceed with the next implementation then?

AUTOLOOP turn: resize loop
Shift + AUTOLOOP turn: adjust beatjump_size
Shift + In/Out: move loop by beatjump_size

This will mean discarding the move beatgrid

Copy link
Member

Choose a reason for hiding this comment

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

ok so unfortunately ERGO doesn't support long press

Hmm, what signals does the AUTOLOOP send on press?
Only press (no release)?
Or press+release immediately?

Copy link
Member

Choose a reason for hiding this comment

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

This will mean discarding the move beatgrid

Well, beatjump is more important for performance than beatgrid move, so I'd rather drop the latter.

Maybe you can squeeze it in elsewhere?
Shift + wheel turn?
Or press Loop_In + LoopOut + touch + turn? With the hand on the platter (not just a finger on the rim) you can move very precisely.

Comment on lines 266 to 267
const deck = status-0x90;
if (value === 0x7F) {
Copy link
Member

Choose a reason for hiding this comment

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

what about early return? (in general)

if (value !== 0x7F) {
    return
}

then get the deck number

Comment on lines 268 to 272
if (engine.getParameter(group, "play") === 1) {
engine.setParameter(group, "cue_default", 1);
} else {
engine.setParameter(group, "play", 1);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (engine.getParameter(group, "play") === 1) {
engine.setParameter(group, "cue_default", 1);
} else {
engine.setParameter(group, "play", 1);
}
if (engine.getParameter(group, "play") === 1) {
engine.setParameter(group, "cue_gotoandstop", 1);
} else {
engine.setParameter(group, "cue_gotoandplay", 1);
}

cue_gotoandplay only to make sure you play from cue in case you dropped a track from another deck (clone) and the play pos is not at the cue.

res/controllers/PIONEER-DDJ-ERGO-scripts.js Outdated Show resolved Hide resolved
if (selectedSamples[deck] > 7) {
selectedSamples[deck] -= 8;
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Why 8? Don't you want each Sampler Vol knob to control the samples of the respective side (1-4, 5-8)
Might be useful to know which sample is selected, e.g. turn the respective LED off for .5 sec? (off if a sample is loaded, on if not)

Copy link
Author

Choose a reason for hiding this comment

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

I agree that some visual feedback is needed. However with the limitations of the controller I find it a bit complicated to provide clear and understandable feedback for the user. I remember using the FLX4 a few days ago and the effect selector drew a cursor over the selected effect. Is it possible somehow to highlight the selected sample on the screen? That'd be my first choice

Comment on lines 328 to 329
engine.setValue("[Microphone]", "pregain", value);
engine.setValue("[Auxiliary1]", "pregain", value);
Copy link
Member

Choose a reason for hiding this comment

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

Users might use other inputs besides the Ergo, hence this combo might not be desired.
What about
turn: adjust Aux (first label on the controller)
Shift + turn: adjust Mic

Or, detect if only one mic or one aux is configured ([MicrophoneN], input_configured) then pick the correct one.
Else, do Shift/unshift adjust mentioned above.
Might be a bit overkill to iterate over 4 mics and 4 aux for every knob turn, you could install callback for each mic and aux pointing to one function that iterates over the devices and stores the value (1 mic | 1 aux | none/multiple) to be used here.

res/controllers/PIONEER-DDJ-ERGO-scripts.js Outdated Show resolved Hide resolved
res/controllers/PIONEER-DDJ-ERGO-scripts.js Outdated Show resolved Hide resolved
@jssotomdz jssotomdz force-pushed the pioneer-ergo branch 7 times, most recently from 13b6806 to b23ad28 Compare April 10, 2024 00:00
@jssotomdz
Copy link
Author

Did you consider the proposed (Mixxx deafult) effects mapping?

Implemented now

@jssotomdz jssotomdz force-pushed the pioneer-ergo branch 8 times, most recently from e149289 to dafdc41 Compare April 10, 2024 02:19
@ronso0
Copy link
Member

ronso0 commented Apr 10, 2024

FYI review conversations are resolved by reviewers only (and when the contributor picks a code suggestion :| ). Otherwise it's tedious for reviewers to check whether all comments have been addressed.

Comment on lines +329 to +340
//this toggles on/off effect 1 or 2 based on FX [1-2] buttons clicks
DDJERGO.quickEffects = function(channel, control, value, status) {
if (value === 0x7F) {
const deck = control % 2;
const effect = (control - deck - 0x4C)/4;
const toggleState = 1 - engine.getParameter(effectsStrings[deck][effect], "enabled");
engine.setParameter(effectsStrings[deck][effect], "enabled", toggleState);
//LEDs for FX number button and FX [1-2]
midi.sendShortMsg(0x94 + deck, 0x47 + effect, 0x7F*toggleState);
midi.sendShortMsg(status, control, 0x7F*toggleState);
}
};
Copy link
Member

Choose a reason for hiding this comment

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

The components Fx mapping now takes care of 1-2-3 knops and button.
What does this do?
Is it for the Fx 1-2 buttons?
If yes, those should toggle the deck -> Fx unit assignment. https://manual.mixxx.org/2.4/en/chapters/appendix/mixxx_controls.html#control-[EffectRack1_EffectUnitN]-group_[ChannelI]_enable
Left deck:
(1) [EffectRack1_EffectUnit1],group_[Channel1]_enable
(2) [EffectRack1_EffectUnit2],group_[Channel1]_enable
Right deck:
same, just with Channel2

@ronso0 ronso0 added this to the 2.4.2 milestone Apr 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants