MixtrackPro3 Fix inverted FX on/off control

[NotYourAverageAl]

current mapping has Tap+FX1 to turn effect on/off & FX1 to focus effect.
This PR changes to a more sensible
FX1 to turn on/off & Tap+FX1 to focus

[Be-ing]

The design was implemented as it is on purpose. This needs further discussion.

[Holzhaus]

The design was implemented as it is on purpose. This needs further discussion.

What was the reason for that? What better reflects the labels and the way it was meant to be used by the manufacturer?

[NotYourAverageAl]

Holzhaus is right with the line of questioning. Maybe I should have been more concise in my reasoning for the change; first PR and all.

Per the manufacturer the controller intended to be used as per my PR. This PR also aligns with the controller labels. I don't know of a controller where the FX buttons don't toggle on/off; I genuinely believe the current implementation was an oversight no one picked up on.

Evidence for this PR defense

• Official Numark guide
  

• The controller recommended software is Serato and per the Mixxx guide my implementation is the recommended way
  

• On the controller section of the Mixxx guide for the Mixtrack Pro3, the FX1 was intended to the turn on/off effect in line with this PR
  

on the Mixxx guide some of the controller features are incorrectly listed or the mapping changed but the wiki/manual was never updated (eg pad+FX1 turns on instantFX1). That was my next job along with some mapping changes I have planned to make the controller work as intended by the manufacturer along with some new ideas.

[Be-ing]

#1287 (comment)

[NotYourAverageAl]

@Be-ing I think this PR is more intuitive to use.
I would argue the current implementation goes against the manufacturer intention and doesn't follow controller de facto standards. I would have voted for the implementation in my PR back in 2017. I see that was the original intention in the PR 1287 before your suggestion to invert.

The newer (compared to the Pro3) Mixtrack Platinum's mapping is inline with my PR

What say everyone else?

[Holzhaus]

I think we should usually try to map the controller in a way that matches the labels and the way it works on other DJ software. That makes it both easier for users that switch from another software and allows people to use it without having to read the manual and remember all the controls because it's labeled on the controller.

In cases this isn't possible because the buttons isn't straightforward to map because Mixxx is lacking a feature or works differently or if Mixxx has some feature that other DJ software doesn't have, we can deviate of course. The same goes for cases where we have an established standard (e. g. our Standard effect mapping): in these cases it makes sense to use that.

Generally speaking, I think we shouldn't map stuff differently than labeled just because of personal preferences - that's what user mappings are for.

[uklotzde]

Any non-obvious deviation from the default mapping and the reason why it differs should be documented twice:

• Comments in the code for the next developers that stumble upon this code
• Documentation of the mapping for users

PRs are not suitable for this purpose.

I agree with @Holzhaus that we should aim to follow the default mapping as defined by the labels on the hardware if possible. Customizations could be added as feature flags to the code, available for experienced users.

In this case we could add a flag like inverse shift FX buttons which is false by default and allows to choose the alternative mapping by setting it to true.

[NotYourAverageAl]

I propose we merge this PR to fix the non-standard FX controls in the current mapping?? I can then work on implementing a feature flag.

Documentation cleanup was already on my todo list as the current info is widely inaccurate/out of date.
Let me know if this works for you guys :)

[Be-ing]

Adding an option is fine, but I'm hesitant to change the default behavior of an older mapping that was intentionally designed the way it is without input from people who have the controller. @radusuciu @mimbert @djsteph what do you think?

[Holzhaus]

without input from people who have the controller

I had the impression that @NotYourAverageAl had that controller.

[NotYourAverageAl]

@Holzhaus I do have the controller

[Be-ing]

I didn't mean to suggest that @NotYourAverageAl doesn't have the controller. Nevertheless I'm uncomfortable changing the default behavior without input from people who have been using this mapping for a long time.

[radusuciu]

I no longer have this controller, but @NotYourAverageAl's reasoning makes sense. I can imagine workflows where one might prefer the current mapping, but it's definitely more intuitive to have the buttons toggle the effects directly, without modifiers.

I don't recall why that might've been the way it was coded, and I'm assuming that the behavior predated my refactor, but I'm not sure.

[Holzhaus]

I no longer have this controller, but @NotYourAverageAl's reasoning makes sense. I can imagine workflows where one might prefer the current mapping, but it's definitely more intuitive to have the buttons toggle the effects directly, without modifiers.

Alright, then I suppose we can merge this?

[NotYourAverageAl]

ping @Be-ing

[ronso0]

The design was implemented as it is on purpose. This needs further discussion.

This was now discussed sufficiently. The original implementation contrary to our own convention was made on @Be-ing#s request even if he doesn't own the controller.
Now a user owning and using the controller reverted that to that to match our own mapping and fx layout convention.
Sure the change may cause irritation for those already used to the current behaviour. I guess either they'll figure out the change themselves or it'll be covered by forum support.
Ready to be merged IMO.

Interestingly the manual is already describing the intended behaviour:
https://manual.mixxx.org/2.3/en/hardware/controllers/numark_mixtrack_pro_3.html#fx-1-on-off
Btw, how does focusing an effect play with the fx parameter knob mapping? @NotYourAverageAl
https://manual.mixxx.org/2.3/en/hardware/controllers/numark_mixtrack_pro_3.html#high-eq-knobs

[ronso0]

pre-commit is failing https://github.com/mixxxdj/mixxx/pull/3758/checks?check_run_id=2245647800#step:6:616 due to wrong indentation I guess (unrelated to the chnages, though)

[Holzhaus]

pre-commit is failing https://github.com/mixxxdj/mixxx/pull/3758/checks?check_run_id=2245647800#step:6:616 due to wrong indentation I guess (unrelated to the chnages, though)

Yes, it would be nice if @NotYourAverageAl could apply the patch from pre-commit to fix the formatting, but even then pre-commit would fail due to undeclared variables, etc. If he wants to fix that too, it's nice, but not a requirement for merge IMHO.

[Be-ing]

I think there has been sufficient time for anyone who has this controller to voice opposition and nobody has, so I guess we can merge this.

For context, I suggested this design based on my experience mapping the Pioneer DDJ-SB2. I only had a day to work with that controller and found it more useful to make it fast to switch effect focus quickly than toggle enable switches quickly. However, I didn't have time to really play with that mapping much, let alone play a gig with it.

[ronso0]

okido
so @NotYourAverageAl would you mind clarifying if the mapping is already correctly documented in the manual?
then we can merge this.

[NotYourAverageAl]

okido
so @NotYourAverageAl would you mind clarifying if the mapping is already correctly documented in the manual?
then we can merge this.

To answer your question the manual defo needs some cleanup. I've started with PR mixxxdj/manual#371. I'm still deciphering the mapping and verifying against the manual. I'll do more cleanups as I go along

I've tried cleaning up the formatting in this PR as best as I could. local Eslint is not seeing anymore issues; maybe I configured it incorrectly. For the undeclared variables, I'll look at a later PR once I'm familiar with the mapping.

[Holzhaus]

I've tried cleaning up the formatting in this PR as best as I could. local Eslint is not seeing anymore issues; maybe I configured it incorrectly.

Set up pre-commit and then run:

$ pre-commit run --files res/controllers/Numark-Mixtrack-3-scripts.js

This will run eslint locally with the same settings as on CI.

[Holzhaus]

Thanks, I'm going to merge this now. If you want to clean up the code later, that can happen in another PR.

[Holzhaus]

Ah, before we merge, can you please sign https://docs.google.com/a/mixxx.org/spreadsheet/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ
and comment here when done?

[NotYourAverageAl]

@Holzhaus I've signed the agreement. Thanks everyone for the support. I look forward to contributing more