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

Mapping for Numark Platinum FX #12872

Open
wants to merge 15 commits into
base: 2.4
Choose a base branch
from
Open

Mapping for Numark Platinum FX #12872

wants to merge 15 commits into from

Conversation

@ronso0 ronso0 changed the title Mapping for Numark Platinum FX for Mixxx 2.4 Mapping for Numark Platinum FX Feb 26, 2024
@ronso0
Copy link
Member

ronso0 commented Feb 26, 2024

Thank you for this PR!

Please sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

@ronso0
Copy link
Member

ronso0 commented Feb 26, 2024

Instead of commiting through the Github web API, I highly recommend you set up git and work locally.
For example this will allow you to run the pre-commit code checks and notice issues beofre pushing.
Here for example, pre-commit complains about indentation.

Deails can be found here https://github.com/mixxxdj/mixxx/blob/main/CONTRIBUTING.md#git-workflow
and the git setup steps are explained here https://github.com/mixxxdj/mixxx/wiki/using-git

Since you're working only on a mapping you don't need to worry about building Mixxx.

@evoixmr
Copy link
Author

evoixmr commented Feb 26, 2024

@ronso0 I just signed the Mixxx Contributor Agreement.

@JoergAtGithub
Copy link
Member

Just to clarify: Please confirm that the uploaded mapping files are exclusively your own work or are based exclusively on files that are already part of Mixxx.

@evoixmr
Copy link
Author

evoixmr commented Feb 26, 2024

@JoergAtGithub the mapping is a mixxx community work. I believe I am the only active member from that forum. All the update from the mapping for the past year were all mine, the original mapping is almost not usable for mixxx 2.4.

All the contributors are listed on the author's area of Numark Mixtrack Platinum FX.midi.xml

The thread for the mapping - #12872 (comment)

@evoixmr
Copy link
Author

evoixmr commented Feb 26, 2024

@ronso0 One more update then I will move locally on weekend., I was trying to work remotely, away from home.

@ronso0
Copy link
Member

ronso0 commented Feb 26, 2024

Great, because I think all these "Update ..." commits may as well be squashed.
FYI as long as a PR has not seen a review you may squash, rebase and force-push as you like. You may set this to Draft in order to signal you're still polishing.
Small fixups (typos, pre-commit fixes) can go to fixup commits and be squashed before merge.

@evoixmr
Copy link
Author

evoixmr commented Feb 27, 2024

@ronso0 I believe this mapping is ready. I did not get any errors or warning.

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.

very shallow first look. This will take some work go get into shape. I'm a bit short on time rn so I kept my comments short, but please ask if they've been too short and/or you need help. Thank you.

res/controllers/Numark-Mixtrack-Platinum-FX-scripts.js Outdated Show resolved Hide resolved
Comment on lines +22 to +24
// whether the corresponding Mixxx option is enabled
// (Settings -> Preferences -> Waveforms -> Synchronize zoom level across all waveforms)
MixtrackPlatinumFX.waveformsSynced = true;
Copy link
Member

Choose a reason for hiding this comment

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

why is that relevant for the mapping?

Copy link
Author

Choose a reason for hiding this comment

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

                            // need to zoom both channels if waveform sync is disabled in Mixxx settings.
                            // and when it's enabled then no need to zoom 2nd channel, as it will cause
                            // the zoom to jump 2 levels at once

Copy link
Member

Choose a reason for hiding this comment

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

I see I see. I think this would be better solved by introducing a CO that always zooms the CO in unison regardless of the preference setting. Can you file a feature request for that?

AUTOLOOP: 0x0D,
FADERCUTS: 0x07,
SAMPLE1: 0x0B,
BEATJUMP: 0x01, // DUMMY not used by controller
Copy link
Member

Choose a reason for hiding this comment

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

can you elaborate? what do you mean with not used by controller?

Copy link
Author

Choose a reason for hiding this comment

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

Beatjump is working on the map. It might just need correction to
BEATJUMP: 0x02

// state variable, don't touch
MixtrackPlatinumFX.shifted = false;

MixtrackPlatinumFX.initComplete=false;
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to remove this. its a rather ugly workaround and fixing the root problem would be better.

Copy link
Author

Choose a reason for hiding this comment

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

I will need more research or help on this.

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 try to find some time to look into it. I'm just saying its a code smell for now.

Comment on lines 157 to 171
// status, extra 04 is just more device id, not sure what the 05 is
//F0 00 20 04 7F 03 01 05 F7

// wake (not sure what the extra 07 is for?
//F0 7E 00 07 06 01 F7

// I think these are the dial updates
//F0 00 20 04 7F 02 02 04 08 00 00 04 00 00 00 05 F7
//F0 00 20 04 7F 04 01 04 00 00 00 04 00 00 00 05 F7
//F0 00 20 04 7F 02 04 04 08 00 00 04 00 00 00 07 00 00 F7
//F0 00 20 04 7F 03 02 04 08 00 00 04 00 00 00 05 F7
//F0 00 20 04 7F 03 04 04 08 00 00 04 00 00 00 07 00 00 F7
//F0 00 20 04 7F 01 04 04 08 00 00 04 00 00 00 07 00 00 F7
//F0 00 20 04 7F 04 04 04 08 00 00 04 00 00 00 07 00 00 F7

Copy link
Member

Choose a reason for hiding this comment

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

not sure how relevant this is for you, but I've reverse-engineered this all already. https://github.com/mixxxdj/mixxx/pull/11075/files#diff-39ced933348af25b914ee7fcd0d44355bb001d6151fb8781d7aca1471e304d52R3-R42

Copy link
Author

Choose a reason for hiding this comment

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

I will need more research or help on this.

Copy link
Member

Choose a reason for hiding this comment

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

The comment now just posts some messages, the link I shared has some extra info that explains them in better detail. I think some of this code could be better rewritten by salvaging code from that mapping, such as NS6II.numberToSysex and NS6II.Display.

res/controllers/Numark-Mixtrack-Platinum-FX-scripts.js Outdated Show resolved Hide resolved
Comment on lines 94 to 100
MixtrackPlatinumFX.bpms = [];
MixtrackPlatinumFX.trackBPM = function(value, group, _control) {
// file_bpm always seems to be 0?
// this doesn't work if we have to scan for bpm as it will be zero initially
// so we hook into the bpm change as well, and if we have 0 then set it to the first value seen (in bpm output)
MixtrackPlatinumFX.bpms[script.deckFromGroup(group) - 1] = engine.getValue(group,"bpm");
}
Copy link
Member

Choose a reason for hiding this comment

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

this should be removed. Either store the BPM along with the component or query it directly with engine.getValue when you need it.

I can not reproduce the file_bpm issue mentioned here.

Also, avoid script.deckFromGroup(group) if possible. its slow. Also indexing into some array like here MixtrackPlatinumFX.bpms[script.deckFromGroup(group) - 1] is usually a sign that some feature could've been implemented much better directly using components. So keep an eye out for that.

Copy link
Author

Choose a reason for hiding this comment

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

Will check this..

Comment on lines 329 to 335
// TODO in 2.3 it is not possible to "properly" map the FX selection buttons.
// this should be done with load_preset and QuickEffects instead (when effect
// chain preset saving/loading is available in Mixxx)
MixtrackPlatinumFX.EffectUnit = function(deckNumber) {
this.effects = [false, false, false];
this.isSwitchHolded = false;

Copy link
Member

Choose a reason for hiding this comment

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

this todo and implementation is now obsolete since you're targetting mixxx 2.4 and loaded_chain_preset is available (just not yet documented, PR is in progress though, see the preview below).

https://deploy-preview-612--mixxx-manual.netlify.app/chapters/appendix/mixxx_controls#control-[QuickEffectRack1_[ChannelI]]-loaded_chain_preset

Copy link
Author

Choose a reason for hiding this comment

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

WIll check on this..

@Swiftb0y
Copy link
Member

Also please take care of the eslint issues. Do you need help with setting that up? Also, you should probably create a new branch for this mapping, otherwise you will get problems down the line (the "don't commit to branch" pre-commit hook should've prevented you from this, but it seems you don't have pre-commit set up).

@Swiftb0y
Copy link
Member

Here's my template response that contains some links that should get you started with pre-commit:

Please understand that in order for us to be able to merge this, your contribution need to follow our style guide. Most of the required changes can be made automatically by use of pre-commit. Please set up pre-commit and make sure to manually apply the required changes using pre-commit run --from-ref 2.4 --to-ref HEAD. Thank you very much.

@evoixmr evoixmr marked this pull request as draft February 28, 2024 14:47
@evoixmr
Copy link
Author

evoixmr commented Feb 28, 2024

@Swiftb0y The mapping and manual started as a community work, I am the only one remaining that is still working on both. I'm actually working remotely and can't really setup to work on this locally. My time is also limited but this mapping is the community has been past due. I actually need all the help I can get. I really appreciate your feedback.

@Swiftb0y
Copy link
Member

I'm actually working remotely and can't really setup to work on this locally.

Thats fine. May I ask for some more details on your setup so I can determine whats the best way to proceed then?

I actually need all the help I can get.

Sure sure, no problem. I suggest you start by trying to address my previous comments and if you have trouble implementing them, reply in the relevant thread and I can go into more detail if necessary.

@evoixmr
Copy link
Author

evoixmr commented Feb 28, 2024

Thats fine. May I ask for some more details on your setup so I can determine whats the best way to proceed then?

On weekdays, I'm mostly on android device.
On weekends, Arch Linux LTS desktop or Debian Trixie laptop

Sure sure, no problem. I suggest you start by trying to address my previous comments and if you have trouble implementing them, reply in the relevant thread and I can go into more detail if necessary.

Updated.

@Swiftb0y
Copy link
Member

On weekdays, I'm mostly on android device.
On weekends, Arch Linux LTS desktop or Debian Trixie laptop

What kind of work can you do from your android device and what can you from your arch/debian setups? Interacting with git? cloning the repo? compiling code?

@evoixmr
Copy link
Author

evoixmr commented Feb 29, 2024

@Swiftb0y I was able to set up a branch. I will clean this up on the branch before moving them to my main, I changed the PR to draft.

What kind of work can you do from your android device: Just github directly.

Work on the manual and work on github dirrectly.

and what can you from your arch/debian setups?

Test the mapping.

Interacting with git? cloning the repo?
Yesy

compiling code?

Yes

@evoixmr evoixmr marked this pull request as ready for review March 2, 2024 17:42
@evoixmr
Copy link
Author

evoixmr commented Mar 2, 2024

@Swiftb0y I tested this on my branch, . all checks passed.. it was also tested by forum members, all functions work with no issue.

@ronso0
Copy link
Member

ronso0 commented Mar 11, 2024

Re Tap button:
I see you have some logic in place to guess which deck the user wants to address.
Wouldn't it simplify things a lot, i.e. make it prediactbale/reliable for users, to use the left/right deck's Shift for this, like you do for the Browse knob?

BPM reset could be mapped to Tap longpress then (required for default 'file BPM' tap, for tempo reset there's the rate slider).

@evoixmr
Copy link
Author

evoixmr commented Mar 11, 2024

@ronso0 selecting a deck for bpm is already tied to the pfl. I guess I just need to be clear on the manual that the user can select a deck for the bpm by selecting only one pfl.

The shift+bpm is already assign to reset the deck's bpm.

@ronso0
Copy link
Member

ronso0 commented Mar 11, 2024

I understand that (and I understand you don't want to drop your current implementation unless strictly necessary ; ) but I think it would be much simpler: Browse and Tap are close and it would be consistent to use the same logic for both.

(note that I don't insist on this, just want to share my thoughts for other reviewers to consider)

@evoixmr
Copy link
Author

evoixmr commented Mar 11, 2024

If I use shift+BPM to select the deck, I will need to use BPM to reset both decks, no way to of selecting which deck to reset.

I updated the manual on the BPM's instruction (item# 23).

@ronso0
Copy link
Member

ronso0 commented Mar 11, 2024

If I use shift+BPM to select the deck, I will need to use BPM to reset both decks, no way to of selecting which deck to reset.

I thought of Shift (hold) + Tap longpress to do the reset. Wouldn't that also read left/right Shift?
Though no need to complicate things as long as the instructions are clear and the workflow is intuitive. I don't have this controller so I can't test the mapping.

@evoixmr
Copy link
Author

evoixmr commented Mar 11, 2024

Left/Right - Shift (hold) + Tap longpress to do the reset. - I will work on this on my next update, Hopefully target mixxx target 2.4.2.

I actually have an updated mapping that can switch the time elapse/remaining (using shift+play), This is being tested on the community now.

I will include both on my next update. Targeting one of the future releases.

@ronso0 ronso0 added this to the 2.4.2 milestone Apr 13, 2024
Fix for pad mode buttons staying blinking when using multiple shift+pad mode.
Long press on pad mode did not work on 2.4, this is working now.
@daschuer
Copy link
Member

@ronso0 What is the review status here?

@ronso0
Copy link
Member

ronso0 commented Oct 21, 2024

I only tried to make some sugestions to improve the workflow. I think @Swiftb0y was reviewing the code.

@ywwg
Copy link
Member

ywwg commented Nov 27, 2024

@Swiftb0y any updates on this? I know it's a huge PR

@Swiftb0y
Copy link
Member

well, most of the comments I made previously have stalled. I can try to take another swing at this if @evoixmr is available.

@daschuer daschuer modified the milestones: 2.4.2, 2.5.0 Nov 27, 2024
@evoixmr
Copy link
Author

evoixmr commented Nov 28, 2024

well, most of the comments I made previously have stalled. I can try to take another swing at this if @evoixmr is available.

@Swiftb0y I'm still here. I was hoping for milestone 2.4.2.

@Swiftb0y
Copy link
Member

Great. I'll try to take another look in the next couple days. Remind me in case I forget.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants