-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Improve Hercules inpulse 300 mappings #4246
Improve Hercules inpulse 300 mappings #4246
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome and thank you for deciding to contribute to Mixxx. As a First-time contributor we need to ask you to sign our Contributor Agreement before merging any code.
I cannot really comment on most of the changes and how the mapping should differ between the 200 and 300 model. Ideally the mapping should match closely that of the bundled software to make it easy for users to switch to Mixxx.
Please also check the manual repo. A corresponding PR for updating the manual page for this controller is required. At first glance the page for the 200 seems to be in better state than that of the 300.
You may have noticed that the pre-commit job failed due to whitespace issues.
https://github.com/mixxxdj/mixxx/wiki/Using-Git#set-up-automatic-code-checking
@@ -364,7 +384,7 @@ | |||
<!--Sync--> | |||
<control> | |||
<group>[Channel1]</group> | |||
<key>sync_leader</key> | |||
<key>sync_master</key> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep using sync_leader
. sync_master
is just an alias for backward compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except if this PR would target 2.3 instead of main. Then use sync_master
because the renaming was performed on the main branch only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sync_leader
doesn't seem to be available in 2.3, so we need to stick to sync_master
:
https://manual.mixxx.org/2.3/en/chapters/appendix/mixxx_controls.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just noticed that in 2.3
the mapping is correct so no changes here are needed.
Please also rebase your PR on 2.3 instead of main. This would allow to publish improvements earlier with a 2.3.x release. |
|
I've submitted the form. Thank you for the opportunity ;) |
e163372
to
ac878f9
Compare
7a0be04
to
ac878f9
Compare
Added FX mappings inspired by Inpulse 200 mappings Controlled LED lights on active effects
ac878f9
to
366e681
Compare
Also, I struggle a lot with the browser wheel in my library. Is there an option to add a mapping that will unfold/fold folders in my library? Otherwise, I have to switch to my keyboard every time I browse my library. Edit: |
I've created a PR for the |
Updated Sampler mappings to allow for usage of 16 samples 5746cf5 |
I've rethought and remade the effect pads. Just like you said that they should be as much close to the original mappings in their software I've made the second row to trigger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No objections. Only XML changes and I cannot test the mapping myself.
I have left a comment in the Discourse thread, hoping that someone else will test the new mapping. Reaching out and announcing what you plan to do or did is always recommended.
@Holzhaus Someone with permissions has to approve the next CI workflow run for the manual: mixxxdj/manual#425
@Holzhaus Merge? |
I think the changes sound sensible. However, it would be nice if @DJPhatso could double-check (he contributed the original mapping IIRC). |
I'll do my best to check it out this week if I can find some time, but my goal with the original mapping was always as a starting point for the community to improve upon. So if someone finds something creative to do, I'll all for it. And from the look of it, there is probably a big part of the mapping that could be applied to the Inpulse 500 mapping I've yet to finish and submit... |
Wow! Nice job @michalvankodev. A especially love the new feature on the browser knob :-) I approve these changes and will try to find the time to apply them and finish the Inpulse 500 mapping. |
Thanks for checking back! I consider your comment as the final approval ;) Unfortunately, 2.3.1 has already been branched off. |
Hi, I've bought myself my first DJ Controller.
My friend has an Inpulse 200 and I've noticed that he already has some effects mapped on his smaller controller. So I've done some digging and came up with some improvements for Inpulse 300.
EffectN
SHIFT
mode pads control FX3 Effect units on deck A and FX4 units on deck B.keylock
forquantize
as the controller has aQ
label on that place.SHIFT
+Q
tobeats_translate_curpos
MoveHorizontal
when inSHIFT
modeUpdated manual documentation: mixxxdj/manual#425 (review)
I've been using mixxx just for a month so please let me know if I could do more improvements.