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

Update Novation Launchpad controller scripts #2600

Merged
merged 4 commits into from
Jul 7, 2020

Conversation

dszakallas
Copy link
Contributor

@dszakallas dszakallas commented Mar 25, 2020

Release Notes

Builds on top of the version included in #1795 (2.2.6), and the scope of changes is fairly limited:

  • two new features requiring one-one lines of change.
  • style autofixes
  • no new library or import dependencies

See diff
Manual checks:

@Be-ing
Copy link
Contributor

Be-ing commented Mar 25, 2020

This doesn't depend on any new 2.3 features does it? If not, we can merge this to the 2.2 branch. If you want to implement support for hotcue colors for 2.3, that would be cool.

@Be-ing
Copy link
Contributor

Be-ing commented Mar 25, 2020

Oops, looks like the wiki documentation for cue colors is outdated...

@dszakallas
Copy link
Contributor Author

dszakallas commented Mar 25, 2020

No it doesn't. It uses reloop_andstop introduced in 2.1.0, but that's safely old.
About implementing cue colors, that would be cool. Two problems:

  • I would like to spare having a new build target dimension of mixxx versions (I am already targeting an array of controller make versions :) ), so it would be best if I could feature test it and fallback to the existing behavior. I hope there's some support for it.
  • And about targeting an array of controllers, MKI doesn't have LED lights, so I have to feature test that too and fallback to a default setting.

@dszakallas
Copy link
Contributor Author

Also I wanted to add support for sampler grids too, that could use LED colors as well.

@Be-ing
Copy link
Contributor

Be-ing commented Mar 25, 2020

Also I wanted to add support for sampler grids too, that could use LED colors as well.

Samplers don't have a color in Mixxx, although tracks do in 2.3. Track colors are not yet exposed to the scripting environment (see #2541 which hasn't been reviewed yet).

@Be-ing
Copy link
Contributor

Be-ing commented Mar 25, 2020

I updated the documentation for hotcue colors. If the Launchpad supports full RGB colors, you wouldn't need to use the new ColorMapper class.

@dszakallas
Copy link
Contributor Author

dszakallas commented Mar 25, 2020

Oh, this is nice. I could also build a palette of RGB colors to make the rest of the controls a bit more colorful, and use the ColorMapper to translate to values understood by the controllers. Nice stuff.

@Swiftb0y
Copy link
Member

AFAIK You don't need to use the ColorMapper class anywhere. For the other controls, you could use the launchpads integrated palette and for the RGB hotcues, you can set the color components via Sysex with 6-bit precision per channel.

@dszakallas
Copy link
Contributor Author

dszakallas commented Mar 25, 2020

@Swiftb0y, not really. While Mk2's color palette could be directly used, I would still need to translate that to Mk1, which is complety different (and limited). And it's easier to map from RGB to that. Also I could send the RGB values directly to Mk2. It's better to have a common language for colors (RGB) than some arbitrary controller palette that can be changed between versions.

@Swiftb0y
Copy link
Member

I'm not quite sure about the limitations of the mapping code in regards to the MK1, but I recommend to send the Hotcuecolors directly via Sysex, since hotcues in mixxx can also have arbitrary colors. You can of course use the color mapper for everything else.

@Holzhaus
Copy link
Member

Why is the mapping that complicated btw? 70.000 added/deleted lines for a controller script, custom Makefiles, it's own eslint.json, etc? Isn't it possible to implement a mapping that is more like other mappings we have (i.e. one XML + one JS file)?

@Swiftb0y
Copy link
Member

@Holzhaus
The mapping is made using modern javascript and then transpiled into ES3. The architecture is quite complicated, but it allowed the mapping to be abstract enough so it can be compiled for many different launchpads.
This blogpost goes into a little more detail:
https://szakallas.eu/2017/05/20/modern-javascripting-a-midi-controller/

@dszakallas
Copy link
Contributor Author

Yes, @Swiftb0y is correct, that's was the reason. It was also any experiment whether Javascript toolchains are flexible enough to handle this situation. It turned out they are. The only thing I regret in hindsight is adding Flow to the mix. It's not used that much today, the rest is pretty conventional JS stack. Basically i wanted to use JS best practices, like I discussed in that blogpost.

@Holzhaus
Copy link
Member

Does this mean we can throw all of the transpilation stuff away when we switch to QJSEngine and Qt 5.12?

@Swiftb0y
Copy link
Member

There are a bunch of external dependencies that would still have to be included and the XML is also generated automatically. It would be nice if we could implement this until then as well:
https://bugs.launchpad.net/mixxx/+bug/1867984

@dszakallas
Copy link
Contributor Author

@Holzhaus note that we would need an embedding that can handle ES6 imports as well and a lots of other stuff. Currently the source is transpiled and bundled. That makes us able to generate separate bundles for the different controllers without duplicating the source code. I created a workflow in which I am comfortable working in, ie. modular, uses npm, linter, etc. I'm sure there have been already a lot of discussion on how the controller scripts should be reformed to achieve this, but it is not only about using ES6-7-8

@dszakallas
Copy link
Contributor Author

This ColorMap thing hasn't landed in any releases yet, right? I see it was added in November. I think I could replicate the behavior in JS for the time being.

@dszakallas
Copy link
Contributor Author

dszakallas commented Mar 27, 2020

It turns out it is unusable when one of the main components are missing. E.g it will choose black over any color for blues. I'd still want some color with the same brightness

@Holzhaus
Copy link
Member

It turns out it is unusable when one of the main components are missing. E.g it will choose black over any color for blues. I'd still want some color with the same brightness

Don't pass #000000 as a color to ColorMapper. On/Off states should be handles separately. Doesn't the Launchpad support all RGB colors? In that case there is no need to ColorMapper at all, just implement sendRGB.

@Swiftb0y
Copy link
Member

Doesn't the Launchpad support all RGB colors?

Depends on the kind of launchpad. The project generates mappings for launchpads that do have RGB Leds and for launchpads that don't. So we'd still use ColorMapper for launchpads that don't support arbitrary colors.

@Holzhaus
Copy link
Member

There are merge conflicts now.

@Holzhaus Holzhaus marked this pull request as draft June 21, 2020 21:58
@daschuer
Copy link
Member

Is this ready for merge now?
This PR fixes also some GitHub warnings about security issues.

@Holzhaus
Copy link
Member

I can't review this, because it's transpiled code and clashes with our mapping system IMHO. If @dszakallas says it works, I'm okay with merging this.

@Be-ing
Copy link
Contributor

Be-ing commented Jun 24, 2020

Does this still work with master since #2682 was merged?

@Holzhaus
Copy link
Member

Shouldn't this be retargeted to 2.3 anyway? Do we even know if the current Launchpad mapping works with Master?

@daschuer
Copy link
Member

daschuer commented Jun 25, 2020

The original version was merged as a part of #2682
This was never part of 2.3. The version number in the branch name refers the upstream version of the mapping not of Mixxx.

See: 1df12b1

@Holzhaus
Copy link
Member

Okay.

@daschuer
Copy link
Member

@dszakallas: I really like to get rid of the security warnings. If this is not ready yet, can you prepare a commit, that just updates the packages?

@dszakallas
Copy link
Contributor Author

dszakallas commented Jun 26, 2020

This is (was?) ready. I specifically tested this version against #1795. Don't know how #2682 affects this, but my guess is if the previous version worked, this will work too (changes are kind of trivial).

The only dependencies of the app are

"eventemitter3": "~3.1.0",
"json-stringify-safe": "~5.0.1",
"lodash-es": "~4.17.14"

which are relatively safe, however a new one is detected every other week in the build dependencies of the project.
Kind of unmanageable at this frequency. I tried to integrate dependabot, but that seems to require that I publish the package on npm.

If you get security vuln warnings in this project because of my package.json files, we can get rid of them once and for all by deleting the controller sources.
I never really understood the benefit of having to include the source files in this project. The mapping file already contains a link to the repo, which the interested person can clone and edit. It's only one extra command. I don't think that knowing git and having access to GitHub is an obstacle for a user who is about to build a project that requires npm packages, babel and bloody complicated build process. And it's even better if we take into account that contributing back is easier this way, as the user already took the step to clone the repo.

@daschuer
Copy link
Member

Ok, what we need to do for it?

@dszakallas
Copy link
Contributor Author

dszakallas commented Jun 26, 2020

Let's see.

  • If everyone agrees I delete the sources under res/controllers/novation-launchpad
  • I update the version to 2.3.1, which contains an additional enhancement
  • test that it works on master

Sounds good?

@uklotzde
Copy link
Contributor

IMO only the final, ready-to-use .xml/.js mapping file belong into the mixxxdj/mixxx repo, but nothing else.

I don't see a reason why to maintain a custom generator framework for a single controller model? No one of us is able to take care of it or verify if it still works with the current version of Mixxx. This code should be managed and versioned in a separate repo. This could be hosted within the mixxxdj organization if desired.

@dszakallas
Copy link
Contributor Author

Ready with the changes.

@Be-ing Be-ing marked this pull request as ready for review July 7, 2020 20:39
@Be-ing Be-ing merged commit 4ade374 into mixxxdj:master Jul 7, 2020
@Be-ing
Copy link
Contributor

Be-ing commented Jul 7, 2020

Thank you for the update. Can update the wiki page too? We recently moved the wiki to GitHub.

This was merged to the master branch. Should it be backported to 2.2 and/or 2.3 too?

We are working on a new controller scripting system using JS modules without the annoying XML file now that we have ES6 support. If you'd like to help us design that, we've been discussing it on Zulip:
https://mixxx.zulipchat.com/#narrow/stream/113295-controller-mapping/topic/ControlObjects.20as.20JS.20objects
https://mixxx.zulipchat.com/#narrow/stream/113295-controller-mapping/topic/New.20Metadata.20format

@Swiftb0y
Copy link
Member

Swiftb0y commented Jul 7, 2020

If you eventually get rid of flow and external dependencies, we could even integrate your original JS code without the transpilation step.

@dszakallas
Copy link
Contributor Author

@Be-ing what is the preferred way of updating the documentation?
Is it ok, if I just paste a link to https://github.com/dszakallas/mixxx-launchpad/blob/v2.3.1/README.MD ?

I don't think the script needs to be backported, as the control usages are compatible with >=2.1.0, and I don't use any new features on the JS side (Components or ColorMapper).

@Holzhaus
Copy link
Member

Holzhaus commented Jul 7, 2020

You can just copy and paste the markdown code (we're using GitHub wiki now).

@dszakallas
Copy link
Contributor Author

I'd be glad to help designing a new scripting environment, but I've been steering away from JS for quite a long time now, and I'm not sure if I was much of a help.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 7, 2020

Is it ok, if I just paste a link to https://github.com/dszakallas/mixxx-launchpad/blob/v2.3.1/README.MD ?

Sure.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 7, 2020

I don't think the script needs to be backported, as the control usages are compatible with >=2.1.0, and I don't use any new features on the JS side (Components or ColorMapper).

I just meant updating the other branches with these changes, not changing the mapping.

@dszakallas
Copy link
Contributor Author

dszakallas commented Jul 7, 2020 via email

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