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

controllers: fix Numark DJ2GO2 Touch sliders and knobs #4835

Merged
merged 2 commits into from
Jul 3, 2022

Conversation

fjordstrom
Copy link
Contributor

Fix input for sliders and knobs to use single byte value, since all
are on single CC status message.

Fix direction of tempo sliders.

Resolves: lp1948596

Fix input for sliders and knobs to use single byte value, since all
are on single CC status message.

Fix direction of tempo sliders.

Resolves: lp1948596
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Hey there, thank you for taking the time to address a bug within mixxx.

res/controllers/Numark_DJ2GO2_Touch_scripts.js Outdated Show resolved Hide resolved
remove line hardcoding tempo slider direction that affects user
configurable settings
@fjordstrom fjordstrom requested a review from Swiftb0y July 1, 2022 13:10
@Swiftb0y
Copy link
Member

Swiftb0y commented Jul 3, 2022

Great. So to confirm, irregardless of the actual slider direction in the mixxx UI, when you move the slider to the + region on the controller, the slider within also goes towards + (and vice versa)?

@Swiftb0y
Copy link
Member

Swiftb0y commented Jul 3, 2022

In order to merge your PR, we must kindly ask you to sign our contributor agreement. Thanks.
https://docs.google.com/a/mixxx.org/spreadsheet/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ


@fjordstrom
Copy link
Contributor Author

Great. So to confirm, irregardless of the actual slider direction in the mixxx UI, when you move the slider to the + region on the controller, the slider within also goes towards + (and vice versa)?

Yes, this is the intended effect. I re-checked the current branch head with default settings and it works.

@fjordstrom
Copy link
Contributor Author

In order to merge your PR, we must kindly ask you to sign our contributor agreement. Thanks.
https://docs.google.com/a/mixxx.org/spreadsheet/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ


Done, thank you

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you very much.

@Swiftb0y Swiftb0y merged commit 1d185ca into mixxxdj:2.3 Jul 3, 2022
@Swiftb0y Swiftb0y added the changelog This PR should be included in the changelog label Jul 3, 2022
@Swiftb0y
Copy link
Member

Swiftb0y commented Jul 3, 2022

Also quick question @fjordstrom:
In the bug report on launchpad, the reporter mentioned that their tempo faders don't center on 0%. This is somewhat common on individual units of cheap hardware. Does the incorrect center issue occur on your hardware as well?

@fjordstrom
Copy link
Contributor Author

@Swiftb0y the incorrect center occurs on mine as well, as is caused by the same issue with parsing two midi messages as one single value. This pull request fixes this too (tempo sliders center properly at 0% )

@fjordstrom
Copy link
Contributor Author

(for reference, I have tested the controller on release 2.3 with default settings and the controller scripts and mapping present in the release, as well as with the fix, and checked midi messages with midisnoop to verify consistency)

@Swiftb0y
Copy link
Member

Swiftb0y commented Jul 5, 2022

Ah, yes. Upon closer inspection of the XML I actually see the problem. My fault this slipped through the initial review. This makes sense. Thank you for clarifying and submitting this patch.

@ronso0 ronso0 added this to the 2.3.4 milestone Jul 11, 2022
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.

3 participants