Updated mappings for the Hercules Inpulse 300 #12503
Conversation
|
Welcome at Mixxx! |
|
The pre-commit check is failing. The best way to fix pre-commit issues is to install pre-commit locally on your system, as described here: https://github.com/mixxxdj/mixxx/wiki/Using-Git#set-up-automatic-code-checking Please note, that this will not fix the typo, that the codespell check reports - this one must be fixed manually. |
|
Alright, I've filled out the agreement, installed pre-commit, and fixed all the typos and style issues. Let me know if there's anything else I need to do. |
|
Target for this PR should be 2.4 not main. Please rebase. |
|
If you need help with Git, have a look here: https://github.com/mixxxdj/mixxx/wiki/Using-Git#targeting-another-base-branch |
|
I'm still figuring my way around git, but assuming I rebased it properly, it should be ready to merge now. I also took the opportunity to add a whole bunch of other stuff, but now the manual PR is no longer up to date. I will be busy in the coming weeks, but hopefully I can get the manual up to speed after that. In the meantime, it would be nice to have some feedback on this mapping (especially from @DJPhatso ;) |
|
Pretty busy right now, but I'll see what I can do. |
|
Some feedback of what I was able to try:
Work as expected.
Been a while since I used this functionality in Traktor, so I might not be the best to judge if the flow is correct, but all functions were assigned as described.
Glad you could make it work as intended :-)
I noticed some unexpected scratching on occasion (i.e. without actually touching the wheels), which did no occur when I switched back to the default mapping (even tried with another unit to rule out some hardware problems), so there might be some adjustments to be done on that side. Looking at the script, there are a few User Options I might try to modify next time.
No problems there |
|
Never mind, I figured out the QT issue. This should be ready for review. The manual is also done, sans the broken link checks. |
|
Never mind, the random scratching is still there. |
|
Scratching issue has been resolved. Ready for review again. |
| DJCi300.connectSlicerFunctions(1); | ||
| DJCi300.connectSlicerFunctions(2); | ||
| // Then disconnect them since we're not in slicer mode (yet) | ||
| DJCi300.disconnectSlicerFunctions(1); | ||
| DJCi300.disconnectSlicerFunctions(2); |
There was a problem hiding this comment.
I think a better model here would be to have the functions always connected, and then use a bool to decide if they are active. So the handler functions would say "if slicing is not active, return". Then you don't need to be making and disconnecting connections all the time.
There was a problem hiding this comment.
That results in lots of function call overhead from C++ into the JS. I think we need to carefully consider how many of these are active vs disabled. (dis-)Connecting is not that expensive IIRC. its just a hashmap lookup with a QObject::connect call.
| DJCi300.connectSlicerFunctions(1); | ||
| DJCi300.connectSlicerFunctions(2); | ||
| // Then disconnect them since we're not in slicer mode (yet) | ||
| DJCi300.disconnectSlicerFunctions(1); | ||
| DJCi300.disconnectSlicerFunctions(2); |
There was a problem hiding this comment.
That results in lots of function call overhead from C++ into the JS. I think we need to carefully consider how many of these are active vs disabled. (dis-)Connecting is not that expensive IIRC. its just a hashmap lookup with a QObject::connect call.
| DJCi300.changeMode = function(channel, control, value, _status, _group) { | ||
| const deck = channel; |
There was a problem hiding this comment.
this would be a good opportunity to incorporate ComponentJS. You can probably copy paste large parts of the code there from the Roland DJ-505 mapping (which I personally consider a gold standard componentJS mapping).
| const sampleRate = engine.getValue(`[Channel${ deck }]`, "track_samplerate"); | ||
| const bpm = engine.getValue(`[Channel${ deck }]`, "local_bpm"); | ||
| // For some reason, multiplying by 60 makes the size 1/2 as large as it's supposed to be | ||
| // Hence, we multiply by 120 instead |
There was a problem hiding this comment.
interesting, this may be a bug (its probably reporting the "true" samplerate so per buffer without taking into account that there are two channels, thus double the samples). The docs don't clarify whether they mean what we internally call samplerate or framerate (which is samples per channel per second) and what you're expecting here.
There was a problem hiding this comment.
almost certainly "samplerate" is indeed the audio card sample rate, so that makes sense
There was a problem hiding this comment.
I don't quite understand. Are you arguing that returning 2x the audio sampling frequency is correct behavior of the CO or that its a bug?
| // Helper function that calculates current position of play indicator in samples | ||
| DJCi300._currentPosition = function(deck) { | ||
| const beatClosest = engine.getValue(`[Channel${ deck }]`, "beat_closest"); | ||
| let beatDistance = engine.getValue(`[Channel${ deck }]`, "beat_distance"); | ||
|
|
||
| // Map beatDistance so that it scales from 0 to .5, then -.5 to 0 | ||
| beatDistance = (beatDistance > .5) ? (beatDistance - 1) : beatDistance; | ||
| // Adjust beatClosest and return | ||
| return (DJCi300._samplesPerBeat(deck) * beatDistance) + beatClosest; | ||
| }; |
There was a problem hiding this comment.
this seems convoluted. Doesn't track_samples * playposition suffice?
There was a problem hiding this comment.
we already have beat_active whichj might help here
I made some improvements to Mixxx's included Inpulse 300 mapping. I tried to stay close to the manufacturer mappings listed in this manual.
Full list of changes:
Added ability to stop samplers (shift + button)
Added toneplay
Added slicer/slicer loop:
Added actual beatmatch guide functionality to the LEDs
Changed the way scratching works
Updated VU meter syntax
Replaced "hotcue_X_enabled" with "hotcue_X_status" in XML file to comply with newer version of Mixxx
Manual is being updated accordingly in another PR: PR#604.