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

Traktor Kontrol S2MK3: Use FX select buttons to set quick effect presets #11702

Merged

Conversation

olivier-mauras
Copy link

@olivier-mauras olivier-mauras commented Jul 1, 2023

Rework of #11104 on 2.4 branch

@ronso0
Copy link
Member

ronso0 commented Jul 2, 2023

Thank you!
Did also take a look at the reviews of #11104 ?

Copy link

@echozio echozio left a comment

Choose a reason for hiding this comment

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

I tested this out with the controller, and the 8 effect logic works intuitively, however pressing 1 sets the effect at index 0, which is no effect (not visible when reordering in the settings).
This change should fix that.

const activeFx = {};
// Detect which fx should be enabled
for (const group of targetGroups) {
activeFx[group] = engine.getValue(`[QuickEffectRack1_${group}]`, "loaded_chain_preset");
Copy link

Choose a reason for hiding this comment

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

Suggested change
activeFx[group] = engine.getValue(`[QuickEffectRack1_${group}]`, "loaded_chain_preset");
activeFx[group] = engine.getValue(`[QuickEffectRack1_${group}]`, "loaded_chain_preset") - 1;

TraktorS2MK3.outputHandler(TraktorS2MK3.fxButtonState[fxNumber], field.group, "fxButton" + fxNumber);
// Now apply the new fx value
for (const group of targetGroups) {
engine.setValue(`[QuickEffectRack1_${group}]`, "loaded_chain_preset", fxToApply[group]);
Copy link

@echozio echozio Jul 2, 2023

Choose a reason for hiding this comment

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

Suggested change
engine.setValue(`[QuickEffectRack1_${group}]`, "loaded_chain_preset", fxToApply[group]);
engine.setValue(`[QuickEffectRack1_${group}]`, "loaded_chain_preset", fxToApply[group] + 1);


const activeFx = {};
for (const group of availableGroups) {
activeFx[group] = engine.getValue(`[QuickEffectRack1_${group}]`, "loaded_chain_preset");
Copy link

Choose a reason for hiding this comment

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

Suggested change
activeFx[group] = engine.getValue(`[QuickEffectRack1_${group}]`, "loaded_chain_preset");
activeFx[group] = engine.getValue(`[QuickEffectRack1_${group}]`, "loaded_chain_preset") - 1;

Copy link
Author

Choose a reason for hiding this comment

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

Actually I didn't want to sort it out this way, because I believe there's probably a bug in the quick effects settings page.
in 2.4-beta - at least for me:

  • the list order is never saved upon Mixxx exit and needs to be changed after restart
  • List index does not start with the first effect in the list but with "---" - If you get to the effect settings, hit "Restore Defaults", then the first index becomes the first effect

I believe that working around this behavior in the mapping will thus break next time the effect list behavior is sorted out

Copy link
Member

Choose a reason for hiding this comment

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

the list order is never saved upon Mixxx exit and needs to be changed after restart

Can't confirm. The list is saved correctly to effects.xml when Mixxx exits. Are you sure Mixxx is shutting correctly?

Copy link

@echozio echozio Jul 2, 2023

Choose a reason for hiding this comment

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

List index does not start with the first effect in the list but with "---" - If you get to the effect settings, hit "Restore Defaults", then the first index becomes the first effect

I would imagine this is a bug in the restore logic ("---" is not re-added when restoring)?

Copy link
Author

Choose a reason for hiding this comment

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

Mmmmh I must have badly tested, you're right list is properly saved, but "---" disappear on "Restore Defaults" which would break the "workaround" mapping though.
Which way should we fix it first? :)

Copy link
Member

Choose a reason for hiding this comment

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

I'll take a look.

Copy link
Member

Choose a reason for hiding this comment

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

This was fixed by #11705

@ronso0
Copy link
Member

ronso0 commented Jul 2, 2023

Yes, I added the empty preset in #10859 so the list is 1-based.
Some (all?) new effect controls are not documented , yet, see mixxxdj/manual#501

@olivier-mauras
Copy link
Author

Ok so I tested @echozio fix and added a new commit.
Works like a charm, just don't expect the behavior to be correct right after reloading defaults :)

@ronso0
Copy link
Member

ronso0 commented Jul 2, 2023

Fix for the missing empty preset / list offset is here #11705

@ronso0
Copy link
Member

ronso0 commented Jul 3, 2023

If mappings are created/updated the respective manual PR is a prerequisite for merging.
https://manual.mixxx.org/2.3/en/hardware/controllers/native_instruments_traktor_kontrol_s2_mk3.html
@olivier-mauras are you familiar with the manual repo?

@olivier-mauras
Copy link
Author

@ronso0 mixxxdj/manual#568

Comment on lines +692 to +696
// If no shift button was pressed fall back to target all decks.
if (targetGroups.length === 0) {
targetGroups = availableGroups;
noShift = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make sense to check here that the effect knob is in zero position (or close to it)? Otherwise we could reassign the effect chain preset, while it's audible.

Copy link
Member

Choose a reason for hiding this comment

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

IMO this is up to the user to decide, and the learing curve should be pretty steep ;)
Related: #11198

Copy link

Choose a reason for hiding this comment

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

I'm not sure how this works currently, but when I used the code from my old PR knobs would always reset when switching effect, whether that be to the center position or the zero position and the knob would only control it again when moved back to the initial point.

@JoergAtGithub
Copy link
Member

@ronso0 Than I think we can merge this. But in mixxxdj/manual#568 you asked to have a final look, when the mapping PR is ready.

@ronso0
Copy link
Member

ronso0 commented Dec 10, 2023

Indeed, I'll do so soonish.

@JoergAtGithub
Copy link
Member

LGTM! Thank you!

@JoergAtGithub JoergAtGithub merged commit 3bb94a6 into mixxxdj:2.4 Jan 8, 2024
@JoergAtGithub JoergAtGithub added changelog This PR should be included in the changelog and removed needs review labels Jan 8, 2024
@daschuer
Copy link
Member

Hi @olivier-mauras thank you for your contribution.
It looks like we have missed to ask for your permission to distribute your code before merge. Can you catch that up, sign https://docs.google.com/a/mixxx.org/spreadsheet/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ and comment here when done? I will then put your name to the Mixxx 2.4 contributor list in the Mixxx about box.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog This PR should be included in the changelog controller mappings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants