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.

Sign up for GitHub

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

Jump to bottom

Some fixes for S4MK2 controller and features #3331

Merged
merged 9 commits into from
Mar 3, 2021
Merged

Some fixes for S4MK2 controller and features #3331

merged 9 commits into from
Mar 3, 2021

Conversation

fayaaz
Copy link
Contributor

@fayaaz fayaaz commented Nov 17, 2020

Adds auto-slipmode and range for pitch fader as well

@Holzhaus
Copy link
Member

Holzhaus commented Nov 17, 2020 •
edited
Loading

Thanks. I can't review the code here (maybe @ywwg or @Be-ing?).

Can you rebase on 2.3? I don't think controller mapping changes need to go into the main branch.
Also, If this adds new features or changes existing behaviour from the user's perpective, we need to document that on the wiki manual page. Please fork the https://github.com/mixxxdj/manual repository and document the changes in the corresponding file: https://github.com/mixxxdj/manual/blob/2.3/source/hardware/controllers/native_instruments_traktor_kontrol_s2_mk3.rst

@JoergAtGithub
Copy link
Member

Is AutoSlip-mode the same as the Flux-Mode in Traktor?
Did you try to use beatlooproll_activate to implement this behavior?

@Be-ing
Copy link
Contributor

Be-ing commented Nov 17, 2020

Yes please update the documentation. It is hard to review code when I'm not sure what it's intended to do.

@fayaaz
Copy link
Contributor Author

fayaaz commented Nov 17, 2020

@JoergAtGithub not sure what Flux-Mode is. I got the idea from Pioneer CDJs to activate the slip mode whenever something was going on (scratches, loops, beatrolls - although they already do that). Honestly I wrote it such a long time ago I've forgotten what it was meant for, but I use it at times and it kinda works.

@Be-ing ok - I should also mark this as WIP as there's some library selection stuff that seems to be broken with this controller on 2.3 that I'd like to fix.

@Be-ing Be-ing marked this pull request as draft November 17, 2020 23:56
@fayaaz
Some fixes for S4MK2 controller and features
c6c9e5c 
Adds auto-slipmode and range for pitch fader as well
@fayaaz fayaaz changed the base branch from main to 2.3 November 19, 2020 00:17
@fayaaz fayaaz force-pushed the patch-4 branch 3 times, most recently from 3c476ff to da1bdce Compare November 19, 2020 01:37
@Holzhaus
Copy link
Member

Holzhaus commented Nov 19, 2020 •
edited
Loading

Corresponding manual PR: mixxxdj/manual#309

@fayaaz fayaaz force-pushed the patch-4 branch 2 times, most recently from 6a1edc1 to 593a0e9 Compare November 29, 2020 00:44
@fayaaz fayaaz marked this pull request as ready for review November 30, 2020 00:40
Be-ing
Be-ing reviewed Dec 10, 2020
View reviewed changes
res/controllers/Traktor-Kontrol-S4-MK2-hid-scripts.js
TraktorS4MK2.linkDeckOutputs("beatlooproll_1_activate", TraktorS4MK2.outputCallback);
}

engine.connectControl("[Recording]", "status", "TraktorS4MK2.onRecordingChanged");
Copy link
Contributor

@Be-ing Be-ing Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
engine.connectControl("[Recording]", "status", "TraktorS4MK2.onRecordingChanged");
engine.makeConnection("[Recording]", "status", TraktorS4MK2.onRecordingChanged);

engine.connectControl is deprecated

Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you revert this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want me to update connectControl for all of them?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, just the new code. If you want to clean up the rest in another PR after this, go for it. But this PR is already big enough.

@fayaaz fayaaz requested a review from Be-ing January 25, 2021 18:49
@uklotzde
Copy link
Contributor

What's the status of this PR? There are still code style issues. What about the corresponding manual PR mixxxdj/manual#309?

We should try to get the controller mappings finalized and merged for 2.3.

@Holzhaus
Copy link
Member

Holzhaus commented Feb 5, 2021

@fayaaz Can you fix the code style issues? Check the pre-commit check details for the diff.

@Be-ing
Copy link
Contributor

Be-ing commented Feb 14, 2021

I don't care about eslint complaining about legacy code. The diff on this branch is already too big, let's not make it bigger.

@Be-ing
Copy link
Contributor

Be-ing commented Feb 23, 2021

ping @fayaaz there are only a few minor issues left to fix

@Be-ing Be-ing merged commit 9e973e5 into mixxxdj:2.3 Mar 3, 2021
@Be-ing
Copy link
Contributor

Be-ing commented Mar 3, 2021

Thanks for maintaining this mapping @fayaaz.

Holzhaus added a commit to mixxxdj/manual that referenced this pull request Apr 30, 2021
@fayaaz @ronso0
@Be-ing @Holzhaus
Updated S4Mk2 manual with proposed changes from PR (#309)
467c803 
Corresponding Mixxx PR: mixxxdj/mixxx#3331

Co-authored-by: ronso0 <[email protected]>
Co-authored-by: Be <[email protected]>
Co-authored-by: Jan Holthuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Be-ing Be-ing Be-ing left review comments

Assignees
No one assigned
Labels
build controller mappings library ui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fayaaz @Holzhaus @JoergAtGithub @Be-ing @uklotzde