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

Mc7000 23 #4729

Merged
merged 15 commits into from
Feb 15, 2023
Merged

Mc7000 23 #4729

merged 15 commits into from
Feb 15, 2023

Conversation

holopansel
Copy link
Contributor

start and stop with softstart and break function, time configurable with stop time knob,
pitch play,
sampler with up to 32 samplers,
velocity sampler with up to 32 samplers.

@Swiftb0y
Copy link
Member

You seem to have started your work on the main branch, please rebase on the 2.3 branch. https://github.com/mixxxdj/mixxx/wiki/Using%20Git#targeting-another-base-branch

…nob,

pitch play, sampler with up to 32 samplers,
velocity sampler with up to 32 samplers.
@holopansel
Copy link
Contributor Author

thanks, rebase done.

@daschuer
Copy link
Member

Thank you.
Unfortunately the pre-comit hook fails. You van apply the patch found here https://github.com/mixxxdj/mixxx/actions/runs/2196709533 to fix the issue.
In future I recommend to install pre-commit locally as described here:
https://github.com/mixxxdj/mixxx/wiki/Using-Git#set-up-automatic-code-checking

@Swiftb0y
Copy link
Member

@holopansel I'm sorry for forgetting about this PR and creating merge conflicts. Are you able to resolve them, I'll take a look at this PR asap then. Also, the new feature additions have to be added to the manual. Can you open a PR for that too?

@holopansel
Copy link
Contributor Author

@holopansel I'm sorry for forgetting about this PR and creating merge conflicts. Are you able to resolve them, I'll take a look at this PR asap then. Also, the new feature additions have to be added to the manual. Can you open a PR for that too?

@Swiftb0y There are three conflicts, and I see where they came from. No. 1 and No. 2 are don`t cares, where there is a function currently not in use, Either version is ok. No problem to remove those. No. 3 needs to be kept, that are the changes for the new functionality. How is the resolution done?

I`ll take care of PR for the manual.

@Swiftb0y
Copy link
Member

I don't know how proficient you are with git. The usual course of action is to merge 2.3 into your branch locally. Then git will complain that there are merge conflicts. You can then open the file containing the conflicts (the mapping in our case) with a text-editor and git will have modified the file everywhere where conflicts are. It will show both your part and the "incoming" part that conflict with each other. You can then manually keep the parts you want to keep and overwrite the ones you want to reject. Once you brought your mapping into that new conflict-free. You continue the merge via the usual git add and git commit. I hope that made it clear for you, if not just ask or consult one of the plenty online tutorials on resolving merge-conflicts.

@holopansel
Copy link
Contributor Author

@Swiftb0y Thank you, merge done.

@holopansel
Copy link
Contributor Author

Also, the new feature additions have to be added to the manual. Can you open a PR for that too?

mixxxdj/manual#484

@holopansel
Copy link
Contributor Author

@holopansel I'm sorry for forgetting about this PR and creating merge conflicts. Are you able to resolve them, I'll take a look at this PR asap then. Also, the new feature additions have to be added to the manual. Can you open a PR for that too?

@Swiftb0y
is there anything I can help with to make some progress here?

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.

I'm sorry I didn't review this earlier. It took me about 3-4 tries and I'm still not finished.
I'd really appreciate if you could spend some more time on this code to clean it up. I don't know how well versed you are in JS or programming languages in general. I'd be happy give you a few pointers on how you can improve this code. It's just currently in a state that I find very, very hard to review (the reason why I was unable to review this faster), and also not really comfortable merging because of the pointed out issues.
Thank you for understanding.

Comment on lines 160 to 162
color codes:

0x followed by
Copy link
Member

Choose a reason for hiding this comment

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

These seem to be following the simple schema of packing each color channels into two bits. So 0b0RRGGBB + 1. Instead of subjective impressions of how the color looks, it might be better to just document this schema so colors can be translated from RGB to the color intended by the controller.
Since this mapping does not use components JS, the integration is not as trivial as it could be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this is just a comment in the code, I do not want to spend the time now to fully understand how the RGB values will be translated. I think it is currently ok as it is. If you disagree, I propose to remove this comment, I can store it locally in a separate file for me to use.

Copy link
Member

Choose a reason for hiding this comment

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

Ok then I propose you replace the list with this comment:

colors are encoded using the following schema: Take the individual components of the color (R, G, B). Then use
the two most significant bits of that color (rr, gg, bb) and pack that into a 7-byte integer using the following schema `0b0rrggbb`. Then add 1 before sending to the controller. 

This essentially describes how you can create any color from its individual components.

Copy link
Contributor Author

@holopansel holopansel Jun 21, 2022

Choose a reason for hiding this comment

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

This is technically the shortest way to describe the colors. But practically it is not of any help when trying to find the correct green color for your application on the controller. Then you have to try every green that is available to see which one suits best.

Anyway comment updated according to your suggestion.

Comment on lines 429 to 536

// change PAD color when switching to Sampler Mode
for (var i = 1; i <= 8; i++) {
if (engine.getValue("[Sampler" + i + "]", "play")) {
if (MC7000.SamplerQty === 16) {
if (deckOffset >= 2) {
samplerOffset = (deckOffset -2) * 8 + i;
} else {
samplerOffset = deckOffset * 8 + i;
}
} else {
samplerOffset = deckOffset * 8 + i;
}
Copy link
Member

Choose a reason for hiding this comment

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

IMO this is faster and easier to understand:

var samplersShouldWrap = MC7000.SamplerQty === 16 && deckOffset >= 2
var samplerUnitOffset = (samplersShouldWrap ? deckOffset % 2 : deckOffset) * 8;
for (var i = 1; i <= 8; i++) {
    var samplerOffset = samplerUnitOffset + i;
    [...]
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not easier for me, but understood and implemented.

Comment on lines 634 to 635
var samplerOffset = 0;
var samplerOffsetJ = 0;
Copy link
Member

Choose a reason for hiding this comment

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

please declare variables as late as possible instead of at the top of the function. i,j have to be declared at the top because they are reused in many different loops further down (which is a code-smell by itself).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

samplerOffset, samplerOffsetJ: done
Maybe we can agree that I spend some time on an improved code for this section in a future update.

return;
}
};
} else if (MC7000.PADModeVelSamp[deckNumber]) { // reset pregain on stop required?
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. see #2043

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently working fine without, due to setting to 1 in sampler mode, line 728.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some lines regarding this topic anyway, to avoid misbehavior when playing velocity samplers with the controller and samplers on mixxx user interface at the same time.

}
}
// ... before the actual sampler to play gets started
engine.setValue("[Sampler" + samplerOffset + "]", "pregain", script.absoluteNonLin(value, 0, 1.0, 4.0));
Copy link
Member

Choose a reason for hiding this comment

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

why the pregain instead of volume?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pregain is used for velocity, otherwise the sampler volume knob which affects all samplers will not work correctly.

Comment on lines 790 to 805
if (value === 0x7F && control === 0x14 + i - 1 && engine.getValue(group, "hotcue_" + i + "_enabled") === 0 && MC7000.HotcueSelectedGroup[deckOffset] === 0) { //hotcue select if none available
engine.setValue(group, "hotcue_" + i + "_activate", true); // set hotcue if not set before
MC7000.HotcueSelectedGroup[deckOffset] = i;
for (z = 1; z <= 8; z++) {
midi.sendShortMsg(0x94 + deckOffset, 0x14 + z - 1, MC7000.padColor.pitchoff); // if hotcue is selected switch to pitch off color
}
} else if (value === 0x7F && control === 0x14 + i - 1 && engine.getValue(group, "hotcue_" + i + "_enabled") === 1 && MC7000.HotcueSelectedGroup[deckOffset] === 0) { // hotcue select if available
MC7000.HotcueSelectedGroup[deckOffset] = i; // store which hotcue should be used for pitch
for (z = 1; z <= 8; z++) {
midi.sendShortMsg(0x94 + deckOffset, 0x14 + z - 1, MC7000.padColor.pitchoff); // if hotcue is selected switch to pitch off color
}
} else if (value === 0x7F && control === 0x14 + i - 1 && MC7000.HotcueSelectedGroup[deckOffset] > 0) { // hotcue selected and button pressed // TODO: play if play, stop if cue
engine.setValue(group, "hotcue_" + MC7000.HotcueSelectedGroup[deckOffset] + "_gotoandstop", true); // stop
for (z = 1; z <= 8; z++) {
midi.sendShortMsg(0x94 + deckOffset, 0x14 + z - 1, MC7000.padColor.pitchoff); // switch to pitch off color
}
Copy link
Member

Choose a reason for hiding this comment

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

please refactor parts like these. You can introduce as many intermediate variables as it makes sense. Reviewing dozens of copy-pasted lines is not only tiring to anyone looking at this code but also very error prone.

This is just one instance, there are many places in this code which could be significantly cleaned up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done for this section.

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.

Thank you for continuing your work on this mapping, the code still deserves some more effort though.

Comment on lines 1159 to 1160
for (var i = 1; i <= 8; i++) {
if (deckNumber === 1) {
MC7000.halftoneToPadMap1[i-1] = MC7000.halftoneToPadMap1[i-1] + 8; // pitch up
} else if (deckNumber === 2) {
MC7000.halftoneToPadMap2[i-1] = MC7000.halftoneToPadMap2[i-1] + 8; // pitch up
} else if (deckNumber === 3) {
MC7000.halftoneToPadMap3[i-1] = MC7000.halftoneToPadMap3[i-1] + 8; // pitch up
} else if (deckNumber === 4) {
MC7000.halftoneToPadMap4[i-1] = MC7000.halftoneToPadMap4[i-1] + 8; // pitch up
}
MC7000.halftoneToPadMap[deckNumber-1][i-1] = MC7000.halftoneToPadMap[deckNumber-1][i-1] + 8; // pitch up
Copy link
Member

Choose a reason for hiding this comment

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

instead of starting at 1 and subtracting 1 everwhere, just start at 0 and count until 8:

for (var i = 0; i < 8; i++) {
    MC7000.halftoneToPadMap[deckNumber-1][i] = MC7000.halftoneToPadMap[deckNumber-1][i] + 8; // pitch up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 813 to 745

MC7000.sendColor = function(sendColorDeckOffset, ColorCode) {
for (var z = 1; z <= 8; z++) {
midi.sendShortMsg(0x94 + sendColorDeckOffset, 0x14 + z - 1, ColorCode); // switch 8 buttons to selected color
}
};

Copy link
Member

Choose a reason for hiding this comment

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

sendColor is quite a confusing name. Also: in JS there is a convention to use TitleCase variable names only for constructors. use camelCase otherwise. Comments belong above whatever they are documenting. Here is an example of what the code looks like when this has been applied.

Suggested change
MC7000.sendColor = function(sendColorDeckOffset, ColorCode) {
for (var z = 1; z <= 8; z++) {
midi.sendShortMsg(0x94 + sendColorDeckOffset, 0x14 + z - 1, ColorCode); // switch 8 buttons to selected color
}
};
MC7000.setPadColor = function(deckOffset, colorValue) {
for (var i = 0; i < 8; i++) {
// switch 8 buttons to selected color
midi.sendShortMsg(0x94 + deckOffset, 0x14 + z, colorValue);
}
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 773 to 779
if (!HotcueOn) { //hotcue select if none available
engine.setValue(group, "hotcue_" + i + "_activate", true); // set hotcue if not set before
MC7000.HotcueSelectedGroup[deckOffset] = i;
MC7000.sendColor(deckOffset, MC7000.padColor.pitchoff);
} else { // hotcue select if available
MC7000.HotcueSelectedGroup[deckOffset] = i; // store which hotcue should be used for pitch
MC7000.sendColor(deckOffset, MC7000.padColor.pitchoff);
Copy link
Member

Choose a reason for hiding this comment

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

you can do much more to reduce these logically. For example, you can see that the last two lines are the same, so you can just pull them out.

Suggested change
if (!HotcueOn) { //hotcue select if none available
engine.setValue(group, "hotcue_" + i + "_activate", true); // set hotcue if not set before
MC7000.HotcueSelectedGroup[deckOffset] = i;
MC7000.sendColor(deckOffset, MC7000.padColor.pitchoff);
} else { // hotcue select if available
MC7000.HotcueSelectedGroup[deckOffset] = i; // store which hotcue should be used for pitch
MC7000.sendColor(deckOffset, MC7000.padColor.pitchoff);
if (!HotcueOn) { //hotcue select if none available
engine.setValue(group, "hotcue_" + i + "_activate", true); // set hotcue if not set before
}
MC7000.HotcueSelectedGroup[deckOffset] = i; // store which hotcue should be used for pitch
MC7000.sendColor(deckOffset, MC7000.padColor.pitchoff);

This applies to many more occurences in your code. For example this

if (engine.getValue(group, "hotcue_" + z + "_enabled", true)) {
    midi.sendShortMsg(0x94 + deckOffset, 0x14 + z - 1, MC7000.padColor.hotcueon);
} else {
    midi.sendShortMsg(0x94 + deckOffset, 0x14 + z - 1, MC7000.padColor.hotcueoff);

can be significantly improved using a variable and a simple conditional ternary statement.

var hotcueEnabled = engine.getValue(group, "hotcue_" + z + "_enabled", true);
midi.sendShortMsg(0x94 + deckOffset, 0x14 + z - 1, hotcueEnabled ? MC7000.padColor.hotcueon : MC7000.padColor.hotcueoff);

Comment on lines 769 to 770
var ButtonPressed = (value === 0x7F), ButtonReleased = (value === 0x00), ControlNumber1 = (control === 0x14 + i -1), ControlNumber2 = (control === 0x1C + i -1);
var HotcueOn = engine.getValue(group, "hotcue_" + i + "_enabled"), HotcueSelectedOnDeck = MC7000.HotcueSelectedGroup[deckOffset];
Copy link
Member

Choose a reason for hiding this comment

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

please break these up into their own lines. Also use camelCase instead of TitleCase for variables containing simple values. ControlNumber1/2 are not very meaningful names. Can you try to name them better?
Also as a simple rule of thumb, if a variable contains a boolean (so either true or false) it usually makes sense to prefix the value with is. Reading if (isButtonPressed) is a lot more natural than just if (ButtonPressed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

variables renamed

Copy link
Member

@JoergAtGithub JoergAtGithub left a comment

Choose a reason for hiding this comment

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

Please combine all these PadMode variables to one.

Copy link
Member

@JoergAtGithub JoergAtGithub left a comment

Choose a reason for hiding this comment

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

Code looking good to me now!
Thank you for going the extra mile to make the code readable!
I guess it's the best to let you test the modified code some days, before merging it. Please tell me, when you are ensured by testing, that everything is still working.

@holopansel
Copy link
Contributor Author

Code looking good to me now! Thank you for going the extra mile to make the code readable! I guess it's the best to let you test the modified code some days, before merging it. Please tell me, when you are ensured by testing, that everything is still working.

Thank you for your reviews and your suggestions, I did some quick tests this evening and everything was fine. I will do some longer tests the next days and let you know afterwards.

@holopansel
Copy link
Contributor Author

param buttons had still the old padmodepitch,
now fully tested, all working fine.

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.

can we revert the permission changes?
Screenshot from 2023-02-13 17-01-24

@holopansel
Copy link
Contributor Author

can we revert the permission changes? Screenshot from 2023-02-13 17-01-24

yes, sure.

@JoergAtGithub JoergAtGithub added the changelog This PR should be included in the changelog label Feb 13, 2023
@daschuer daschuer added this to the 2.3.4 milestone Feb 14, 2023
@daschuer daschuer mentioned this pull request Feb 14, 2023
@JoergAtGithub
Copy link
Member

@holopansel We need this fixed now, if this PR should go into the Mixxx 2.3.4 release

@JoergAtGithub
Copy link
Member

Did you signed the Mixxx contributor agreement already?

@holopansel
Copy link
Contributor Author

not sure if I did in the past, I think yes, but did it again a few minutes ago, just to be sure.
image

@daschuer
Copy link
Member

Yes, he has. Thank you.
@holopansel once this is merged, I will add you name to the contributor list in the about box.
Do you wish to be listed with your real name, like most others, or your nickname?

@holopansel
Copy link
Contributor Author

Real name is fine.

@JoergAtGithub JoergAtGithub merged commit 36567fe into mixxxdj:2.3 Feb 15, 2023
@JoergAtGithub
Copy link
Member

Thank you!

MC7000.StarsDown = function(channel, control, value, status, group) {
var deckNumber = script.deckFromGroup(group);
var deckIndex = deckNumber - 1;
if (value >= 0x00) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this check do anything? I think we're only getting integers, so this is trivially true (and likely the reason for #13587)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I think maybe this should be > instead of >=. But I do not have the controller anymore, so I cannot perform any tests. Can you check please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that it was like this in my original code
if (value === 0x00) { //return; // don't respond to note off messages } else {
and has been shortened during review process, but I do not remember any reason

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this seems to be it, I've fixed it in #13588. Funnily, I actually went back to the if (value === 0) style in #13589 to avoid nesting and since the rest of the script already uses that style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine, thank you!

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.

6 participants