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

Denon MC7000 mapping #2546

Merged
merged 12 commits into from
Apr 3, 2020
Merged

Denon MC7000 mapping #2546

merged 12 commits into from
Apr 3, 2020

Conversation

toszlanyi
Copy link
Contributor

Initial Denon MC7000 mapping. PAD Modes are working for CUE, ROLL and SAMPLER.

@Holzhaus
Copy link
Member

Hi @toszlanyi, thanks for contributing this mapping and documenting it to great extent on the Wiki!
Usually I'd ask you to rebase this branch on 2.2 but since the Denon MC7000 requires a very recent kernel, I think we can put this into master.

There are some code style issues though. If you have at least Python 3.6, I suggest you install pre-commit and then install our pre-commit hooks from our repository:

$ pre-commit install
$ pre-commit install -t pre-push

Then you can just run:

$ pre-commit run --files res/controllers/Denon-MC7000-scripts.js res/controllers/Denon-MC7000.midi.xml

That will check for code style issues with your files (and them automatically if possible).

@Holzhaus
Copy link
Member

Holzhaus commented Mar 13, 2020

Another thing: 2.3 will feature colored hotcues. If you intend to add support for that, I suggest you wait until #2520 has been merged (which will hopefully happen soon-ish).

@toszlanyi
Copy link
Contributor Author

Thanks a lot for your quick action. As this is my first commit ever then I am unsure how to follow up on that but will try to stay on top of it... As for pre-commit: I am on KDE Neon built upon Ubuntu 18.04 that includes Python 2.7.17 at present. Any chance to check the files in a different way before commit? Maybe you'd know if a plugin for KDevelop exists. On a quick search I didn't find one. Cheers!

@Holzhaus
Copy link
Member

I am on KDE Neon built upon Ubuntu 18.04 that includes Python 2.7.17 at present.

Try sudo apt install python3.

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.

Overall pretty feature complete mapping. However, there is a lot of redundant code for handling the pads which is not ideal.

res/controllers/Denon-MC7000-scripts.js Outdated Show resolved Hide resolved
res/controllers/Denon-MC7000-scripts.js Outdated Show resolved Hide resolved
res/controllers/Denon-MC7000-scripts.js Show resolved Hide resolved
res/controllers/Denon-MC7000-scripts.js Outdated Show resolved Hide resolved
res/controllers/Denon-MC7000-scripts.js Outdated Show resolved Hide resolved
Comment on lines 679 to 721
// Assign Spinback length to STOP TIME knob
MC7000.stopTime = function(channel, control, value, status, group) {
var deckNumber = script.deckFromGroup(group);
// "factor" for engine.brake()
// this formula produces factors between 31 (min STOP TIME for ca 7 sec back in track)
// and 1 (max STOP TIME for ca 18.0 sec back in track)
MC7000.factor[deckNumber] = (1.1 - (value / 127)) * 30 - 2;
};

// Use the CENSOR button as Spinback with STOP TIME adjusted length
MC7000.brake_button = function(channel, control, value, status, group) {
var deckNumber = script.deckFromGroup(group);
var deck = parseInt(group.substring(8,9)); // work out which deck we are using
engine.brake(deck, value > 0, MC7000.factor[deckNumber], - 15); // start at a rate of -15 and decrease by "factor"
};
};
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 explain the purpose of this code? If you just need to reverse or censor the track you can use the reverse and reverseroll controls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially I had CENSOR mapped as you suggested but instead I thought it was more useful to get a back spin assigned to this button as the platter doesn't rotate freely. With the STOP TIME knob you can adjust the length of spin back (defines the factor for midi signals 0-127 when turning the knob from min to max).

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, but when you want to merge a mapping so other users can use it easily, they expect the controller to act according to how it does in other applications. @Be-ing is this statement correct?

Obviously you can still use this code for your own use case, but you can't expect other users to have the same usecase as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... or I can make it optional with a user definable variable to be true or false. Standard would be Censor and if one wants to set it then easily gets the spin back function on that button. Thanks for the hint.

Copy link
Member

Choose a reason for hiding this comment

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

Well yes, that's what I did as well when I started mapping, but doing it is generally discouraged as it just unnecessarily bloats the mapping.
Have you taken a look at the coding guidelines for mappings yet?

Copy link
Contributor Author

@toszlanyi toszlanyi Mar 14, 2020

Choose a reason for hiding this comment

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

Yes I did look into that coding guidelines and believe that I worked to an extend of it - even not all though. Anyhow as it is called guideline and not rule I thought it was not too strict.
As for the STOP TIME function, actually Be pointed me to use it as factor for engine.brake in the forum mapping thread and to be honest the CENSOR function in my case is useless. You could still manually activate CENSOR by switching SLIP on and then use REVERSE button which is the shifted CENSOR.
One more point: I cannot maintain different versions of the mapping - which means one for "public" and one for myself so we have to find the right solution for everybody's or most users case.

Copy link
Member

Choose a reason for hiding this comment

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

I can understand all of your points. Frankly, I don't have the authority here, so I'd like to ask @Be-ing to decide.

res/controllers/Denon-MC7000-scripts.js Outdated Show resolved Hide resolved
res/controllers/Denon-MC7000-scripts.js Outdated Show resolved Hide resolved
res/controllers/Denon-MC7000-scripts.js Outdated Show resolved Hide resolved
res/controllers/Denon-MC7000-scripts.js Outdated Show resolved Hide resolved
@toszlanyi
Copy link
Contributor Author

@Swiftb0y, thanks a lot for looking into this. I have asked so many times in the forum when starting this mapping but got very few help so I had to guess and steal from other mappings and adjust to match the MC7000. So that coding has got several influences and things are not consistent. One piece maybe written in another way as others. Anyhow I am so glad you looking into that and I thank you for all the hints. Gonna work around the pieces as far as I can but especially would need help with PAD modes. WOuld you mind checking the forum and we continue there? https://www.mixxx.org/forums/viewtopic.php?f=7&t=13126 Again, many many thanks!!!

// lower values make it less, higher value more sensible
MC7000.jogParams = {
jogSensitivity: 30 ,// default: 30
maxJogValue: 3 ,// default: 3
Copy link
Member

Choose a reason for hiding this comment

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

Whats the reason for this maxJogValue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the scratchingExample.js code from the WIKI was not satisfying for me. But as I am still pleased with the MC4000 way how the scratching feels like then I copied the formula from there. Honestly - without really knowing what the parameters tell me. Experimenting with the values showed me different behavior though.

Copy link
Member

Choose a reason for hiding this comment

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

The parameters only change things when moving the wheel while having vinyl mode deactivated. So they shouldn't influence the Jog behavior in any way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for confusion: You are right - this is for controlling how sensitive the the platter is when beat matching - accidentally wrote scratching...

Copy link
Member

Choose a reason for hiding this comment

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

I get the reason for having jogSensitivity but not really for maxJogValue because that just clamps the value given to mixxx which doesn't really make sense. It should make a difference for you at higher speeds. Can you set it to a really high value and then beatmatch a bit? does it get more difficult or can you do it faster (since not clamping the value should allow you to do faster adjustments)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This didn't let me go so I checked in more detail what the author of the MC4000 wanted to happen: The "jog" values generally can be set from -3 to 3. Inside the MC7000.wheelTurn function the "jog" parameter can be narrowed down in the way when it asks for the Math.min(MC7000.jogParams.maxJogValue, jogAbsolute). So even if the jogSensitivity is high, e.g you turn too hard - which influences the maxJogValue - the value for "jog" maybe maximum 3 (or lower if you set MC7000.jogParams.maxJogValue accordingly) if the variable "jogAbsolute" exceeding it. So it works like a limiter on the "jog" value.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. exactly. But I don't see a practical reason for implementing this.

@Swiftb0y
Copy link
Member

I'm not active on the forum at all (I don't think I even have an account on there).
I'll look at the forum, but Id like to keep the discussion here (since we're already here) because it's easier to work in tandem with the code.
About the Pads: I frankly didn't come up with a good solution to it myself yet but I can take a more thorough look at it later again.
Your code is quite good for a beginner. Once you are comfortable enough the JS like this, I'd like to encourage you to take a look at components JS because it makes writing mappings much more efficient (though you have to know OOP in JS).

@toszlanyi
Copy link
Contributor Author

Once you are comfortable enough the JS like this, I'd like to encourage you to take a look at components JS because it makes writing mappings much more efficient (though you have to know OOP in JS).

Oh - this was my first project - never programmed before and I guess I almost know the WIKI pages out of my head now - not really but you get it. The issue is that some points are not clear if you start from zero, like me. Especially the sampler buttons seams so easy on that wiki page but also asked in the forum about how to implement - never got help. Tried different ways and came up with a quite excessive but for me easy to understand code for the PADs. I am sure this can be made much easier. As for the discussion I am happy to keep it here. Thanks again.

@Swiftb0y
Copy link
Member

When I got started with JS, I did have some previous python experience but none in JS so my code looked very similar to yours. In case you would like to dig deeper into JS and programming in general, I can recommend you codecademy. Especially their JS course.

@Holzhaus
Copy link
Member

It makes sense to get pre-commit working before doing further changes to the mapping to make sure the code style guilines are enforced.

@toszlanyi
Copy link
Contributor Author

It makes sense to get pre-commit working before doing further changes to the mapping to make sure the code style guilines are enforced.

Thanks everyone - by now I corrected almost all issues - the unnecessary semicolon were not detected so I do not exactly know which ones to delete.

$ pre-commit run --files res/controllers/Denon-MC7000-scripts.js
Check for byte-order marker..............................................Passed
Check for case conflicts.................................................Passed
Check JSON...........................................(no files to check)Skipped
Check for merge conflicts................................................Passed
Check Xml............................................(no files to check)Skipped
Check Yaml...........................................(no files to check)Skipped
Fix End of Files.........................................................Passed
Mixed line ending........................................................Passed
Trim Trailing Whitespace.................................................Passed
Don't commit to branch...................................................Passed
eslint...................................................................Failed
- hook id: eslint
- exit code: 1


/home/tobias/Entwicklung/MIXXX/mixxx/res/controllers/Denon-MC7000-scripts.js
427:9   warning  Closing curly brace does not appear on the same line as the subsequent block  brace-style
431:9   warning  Closing curly brace does not appear on the same line as the subsequent block  brace-style
435:9   warning  Closing curly brace does not appear on the same line as the subsequent block  brace-style
487:9   warning  Closing curly brace does not appear on the same line as the subsequent block  brace-style
491:9   warning  Closing curly brace does not appear on the same line as the subsequent block  brace-style
495:9   warning  Closing curly brace does not appear on the same line as the subsequent block  brace-style
499:9   warning  Closing curly brace does not appear on the same line as the subsequent block  brace-style
502:22  error    'i' is already defined                                                        no-redeclare
508:21  warning  Closing curly brace does not appear on the same line as the subsequent block  brace-style
513:21  warning  Closing curly brace does not appear on the same line as the subsequent block  brace-style
530:9   warning  Closing curly brace does not appear on the same line as the subsequent block  brace-style
534:9   warning  Closing curly brace does not appear on the same line as the subsequent block  brace-style
652:9   warning  Closing curly brace does not appear on the same line as the subsequent block  brace-style
656:9   warning  Closing curly brace does not appear on the same line as the subsequent block  brace-style
720:13  error    'level' is already defined                                                    no-redeclare
738:13  error    'activeLED' is already defined                                                no-redeclare
778:14  error    'i' is already defined                                                        no-redeclare
789:14  error    'i' is already defined                                                        no-redeclare
796:14  error    'i' is already defined                                                        no-redeclare
803:14  error    'i' is already defined                                                        no-redeclare
815:14  error    'i' is already defined                                                        no-redeclare

✖ 21 problems (8 errors, 13 warnings)


clang-format.............................................................Failed
- hook id: clang-format
- exit code: 1

git: 'clang-format' ist kein Git-Befehl. Siehe 'git --help'.

black................................................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
qsscheck.............................................(no files to check)Skipped

How to proceed further? Thanks a million.

@Swiftb0y
Copy link
Member

Swiftb0y commented Mar 14, 2020

You should get rid of all eslint errors and warnings. All the warnings are formatting/style issues and all the x already defined issues are because you are creating a new variable (using var) even though they have already been created in the enclosing scope. Each line of the eslint report begins with the linenumber so you can locate where you have to fix the issue. I encourage you to try to do it yourself.

You should be able to get rid of the x already defined issues by just deleting the var keyword in the corresponding lines and the warnings can be avoided by following the guidelines in the coding conventions for javascript

@toszlanyi
Copy link
Contributor Author

OK - this is the status now but especially the "Closing curly brace does not appear on the same line as the subsequent block" changes now make the code less readable. Anyhow, the warning disappeared. Thanks a lot for your further guidance.

$ pre-commit run --files res/controllers/Denon-MC7000-scripts.js
Check for byte-order marker..............................................Passed
Check for case conflicts.................................................Passed
Check JSON...........................................(no files to check)Skipped
Check for merge conflicts................................................Passed
Check Xml............................................(no files to check)Skipped
Check Yaml...........................................(no files to check)Skipped
Fix End of Files.........................................................Passed
Mixed line ending........................................................Passed
Trim Trailing Whitespace.................................................Passed
Don't commit to branch...................................................Passed
eslint...................................................................Passed
clang-format.............................................................Failed
- hook id: clang-format
- exit code: 1

git: 'clang-format' ist kein Git-Befehl. Siehe 'git --help'.

black................................................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
qsscheck.............................................(no files to check)Skipped

@Swiftb0y
Copy link
Member

Readibility is subjective. The purpose of the guidelines is to make the codebase consistent. Can you commit and push your progress?

@Holzhaus
Copy link
Member

Please also run sudo apt install clang-format.

res/controllers/Denon-MC7000-scripts.js Outdated Show resolved Hide resolved
res/controllers/Denon-MC7000-scripts.js Outdated Show resolved Hide resolved
res/controllers/Denon-MC7000-scripts.js Show resolved Hide resolved
@Holzhaus
Copy link
Member

Hey, there are still some code style fails. Check the details of the "CodeFactor" check.

@toszlanyi
Copy link
Contributor Author

Hey, there are still some code style fails. Check the details of the "CodeFactor" check.

I am wondering why though. Those lines have been corrected and even when I am going into my Denon_MC7000_mapping branch and wanna change the file then it is all correct. So the analyzer somehow sees something that is not there.

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.

Not fully satisfied but the mapping still seems mergeable.

@toszlanyi
Copy link
Contributor Author

toszlanyi commented Mar 16, 2020

Not fully satisfied but the mapping still seems mergeable.

I am not satisfied either as it is not complete but I am working (mixing) with this for approx 8 weeks now and it is very usable. I still have some work to do and I would appreciate your help. Thank you very much.

@Holzhaus
Copy link
Member

Holzhaus commented Mar 21, 2020

@toszlanyi The Denon MC7000 features color hotcue pads, right? Mixxx 2.3 adds support for custom hotcue colors. Do you want to add support for that in your mapping?

If so, you first need to pull in the latest master state (git pull upstream master).

You'll also need to get a mapping between colors and midi values from your controller (e.g. "if I send midi value 0x23 the pad LED becomes blue (#FF0000)"). You can then create a ColorMapper object, like this:

DJ505.PadColorMap = new ColorMapper({
"#CC0000": DJ505.PadColor.RED,
"#CC4400": DJ505.PadColor.CORAL,
"#CC8800": DJ505.PadColor.ORANGE,
"#CCCC00": DJ505.PadColor.YELLOW,
"#88CC00": DJ505.PadColor.GREEN,
"#00CC00": DJ505.PadColor.APPLEGREEN,
"#00CC88": DJ505.PadColor.AQUAMARINE,
"#00CCCC": DJ505.PadColor.TURQUOISE,
"#0088CC": DJ505.PadColor.CELESTE,
"#0000CC": DJ505.PadColor.BLUE,
"#4400CC": DJ505.PadColor.AZURE,
"#8800CC": DJ505.PadColor.PURPLE,
"#CC00CC": DJ505.PadColor.MAGENTA,
"#CC0044": DJ505.PadColor.RED,
"#FFCCCC": DJ505.PadColor.APRICOT,
"#FFFFFF": DJ505.PadColor.WHITE,
});

You can then get the appropriate MIDI value for a hotcue like this:

var value = colorMapper.getValueForNearestColor(engine.getValue("Channel1"], "hotcue_1_color"));

If you don't want to deal with color changes yourself, I suggest you switch to components-based mapping for Hotcue Buttons. Then you can just do this and everything should work:

var colorMapper = new ColorMapper({ 
     "#CC0000": 0x01, 
     "#CC8800": 0x02, 
     "#00CC00": 0x03, 
     "#0000CC": 0x05, 
    // ...
});
var hotcues = [];
for (var i = 1; i <= 8; i++) {
    hotcues[i] = new components.HotcueButton({
        number: i,
        group: "[Channel1]",
        midi: [0x91, 0x26 + i],
        colorMapper: colorMapper;
    });
}

@toszlanyi
Copy link
Contributor Author

> ... I suggest you [switch to components-based mapping for Hotcue Buttons](https://mixxx.org/wiki/doku.php/components_js#hotcuebutton). Then you can just do this and everything should work:

@Holzhaus, I would really love to switch to components based mapping as mine is far too complicated but I do not know how. In the forum I have asked for different versions that all did not work but unfortunately nobody could help. I always get error message that variable "components" cannot be found. Any possibility you could support with the syntax directly in my script for hotcues? Surely I would be able to adopt to the other modes then when I have got an example for the first one. Cheers!

I would like to get the PAD modes somehow better coded until I switch to coloured hotcues but then yes - would be great!

Thanks a lot

@Swiftb0y
Copy link
Member

Swiftb0y commented Mar 21, 2020

You most likely forgot to include the components library in your environment.
I highly suggest you take a look at the componentsJS wiki page.

Components could also aid you in developing a better solution for the way you handle the pads.

@Holzhaus
Copy link
Member

Holzhaus commented Mar 21, 2020

OT: We really should add a step-by-step tutorial how to write a good controller mapping to the manual.

@toszlanyi
Copy link
Contributor Author

OT: We really should add a step-by-step tutorial how to write a good controller mapping to the manual.

Maybe I can make a proposal then... I gonna try to rewrite the PADs using components and if successful then I could go and make a proposal to the WIKI site. Maybe I create a draft site combining all my experience as a beginner in coding and what I have learned through different WIKI sites, Forum, different mappings and your support via GitHub get my mapping done. Step by step in one page as summary but still linking to the different existing sites... Cheers!

@Holzhaus
Copy link
Member

Anyhow, should we then just finish the mapping as I have got it and push it to 2.2.4? For 2.3 I would create that new one.

Sounds good to me. Can you remove the confusing Deck stuff I mentioned above and rebase on the 2.2 branch? https://www.mixxx.org/wiki/doku.php/using_git#targeting_another_base_branch

@toszlanyi
Copy link
Contributor Author

Sounds good to me. Can you remove the confusing Deck stuff I mentioned above and rebase on the 2.2 branch?

already removed and all working fine - tomorrow I gonna check if I can somehow simplify the Roll and Slicer section so it passes the CodeFactor check.

@toszlanyi toszlanyi changed the base branch from master to 2.2 March 24, 2020 09:34
@toszlanyi toszlanyi changed the base branch from 2.2 to master March 24, 2020 09:35
@toszlanyi toszlanyi requested a review from Holzhaus March 24, 2020 12:31
@toszlanyi toszlanyi changed the base branch from master to 2.2 March 25, 2020 17:24
@Holzhaus
Copy link
Member

I was planning to write a tutorial how to write a controller mapping from scratch. We can combine these two projects: I start writing and you can give me feedback if it's easy enough to follow and if the code samples are useful. @Swiftb0y are you interested as well?

Started a discussion about this here: https://mixxx.zulipchat.com/#narrow/stream/113295-controller-mapping/topic/How.20to.20write.20a.20controller.20mapping.20from.20scratch

@toszlanyi
Copy link
Contributor Author

Hello @Holzhaus & @Swiftb0y - hope you are doing good these days.
I think now I am done to improve the mapping in the way I could. Is there anything more you would like to address? Thanks a lot for your feedback.

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Thank you very much. Please change the mapping name to just "Denon MC7000" and add a changelog entry, then I'll merge.

res/controllers/Denon-MC7000.midi.xml Outdated Show resolved Hide resolved
@Holzhaus Holzhaus added this to the 2.2.4 milestone Apr 3, 2020
Co-Authored-By: Jan Holthuis <holthuis.jan@googlemail.com>
@toszlanyi
Copy link
Contributor Author

Thank you very much. Please change the mapping name to just "Denon MC7000" and add a changelog entry, then I'll merge.

Thank you very much! It's changed accordingly.

@Holzhaus
Copy link
Member

Holzhaus commented Apr 3, 2020

@toszlanyi Please don't forget to add the changelog entry ;-)

@toszlanyi
Copy link
Contributor Author

@toszlanyi Please don't forget to add the changelog entry ;-)

I did at the web page and pressed "Propose file change" ... Guess one of you guys need to accept as I do not have write access

@Holzhaus
Copy link
Member

Holzhaus commented Apr 3, 2020

@toszlanyi Please don't forget to add the changelog entry ;-)

I did at the web page and pressed "Propose file change" ... Guess one of you guys need to accept as I do not have write access

You should edit the CHANGELOG file on your computer, then commit the change and add it to this PR. The link was just meant to help you find the correct location in the file.

@toszlanyi
Copy link
Contributor Author

@toszlanyi Please don't forget to add the changelog entry ;-)

I did at the web page and pressed "Propose file change" ... Guess one of you guys need to accept as I do not have write access

You should edit the CHANGELOG file on your computer, then commit the change and add it to this PR. The link was just meant to help you find the correct location in the file.

Okay, done... Thank you so much for your patience and your nice guidance. Cheers!

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Thanks you very much for your contribution! LGTM.

@Holzhaus Holzhaus merged commit 5e5d66f into mixxxdj:2.2 Apr 3, 2020
@toszlanyi toszlanyi deleted the Denon_MC7000_mapping branch April 3, 2020 20:38
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.

4 participants