add support for loading controller scripts as JS modules #2868

Be-ing · 2020-06-14T19:36:57Z

This bypasses the old code path used for the scriptfiles/file XML elements. Instead, the root JS module file is specified by a new XML element:

<controller>
    <module>MyController.mjs</module>
</controller>

MyController.mjs can use the standard JS import keyword to import other modules.

The module must export 3 functions:

handleInput(data, timestamp)
init()
shutdown()

All incoming data is passed to receiveData. Functions defined in the JS module cannot be used in the legacy XML system and this will not be supported.

The standard JS "import" keyword only works if the script is loaded with QJSEngine::importModule. JS modules cannot be used with the legacy system of reading JS files into a QString and calling QJSEngine::evaluate.

This makes Qt 5.12 a strict requirement to build Mixxx.

TODO:

~~cleanup redundancies with legacy code~~ The legacy code is a mess. I will not touch it.
automatic reloading scripts
search for XML files in subdirectories and load JS module from path relative to XML file so controllers can have their own directories
add MidiDispatcher module

Be-ing · 2020-06-14T20:32:16Z

Here is the proof-of-concept JS file I am testing with:

import * as mod from "./test.mjs";

export function receiveData(data, timestamp) {
    print("RECEIVING DATA " + data + " " + timestamp);
    mod.test();
}
export function init() {
    print("LOADING MODULE!!!!!!!!!!!!!!!!!!!!");
}
export function shutdown() {
    print("shutting down........................");
}

Be-ing · 2020-06-15T00:54:58Z

I have stumbled upon an ugly limitation of using the JS import keyword with QJSEngine. There is no way to add a directory to the search path for the import keyword. This exists for QQmlEngine with QQmlEngine::addImportPath, but not QJSEngine. I tried using QDir::setSearchPaths but this does not work. I have reported that bug upstream to Qt. This leaves two options, neither of which are great:

The libraries can be compiled into the application via the Qt Resource System. Developing the libraries would require temporarily switching a script to importing via relative paths; otherwise, Mixxx would need to be recompiled to test every change to the library.
Import modules via relative paths. This would not work well for mappings posted to the forum because users would either have to install them to the system folder or the mapping developer would have to copy the libraries into the archive posted to the forum.

Thoughts on how to proceed?

Swiftb0y · 2020-06-15T09:11:37Z

Is there the possibility of a hybrid approach?
All mappings in the system directory would use relative imports and would thus respond to changes made in their imported modules, while the modules still get compiled into the resource system? Mappings intended to go into the git repo are already developed in the res folder and should use relative imports while mappings developed outside of that (and are thus not intended to get merged) can use the modules from the resource system. Another nice sideeffect would then be, that once we can specify a searchpath, we only have to change the mappings in the repo and don’t break any other mappings already not using relative imports.

Be-ing · 2020-06-15T10:22:49Z

The Qt bug has been marked as priority 2, important. So I hope we can expect it to be solved soonish. I am not sure if that will not help with legacy Qt versions. I have asked in the bug report if a fix would be backported to Qt 5.12, since that is still a supported LTS release.

@Swiftb0y your proposal could work, however, it still requires the mapping developer to make some changes every time they upload to the forum. I am concerned any step like that which requires manual intervention will get skipped and forum users will get confused. Perhaps we could recommend starting development using the qrc:/lib/module.mjs path then switch to a relative path at the very end when opening a pull request.

Swiftb0y · 2020-06-15T10:42:48Z

I didn't take into account when somebody develops a mapping in the repository but then decides to publish it via the forum. We could provide an awk or python script to run on a directory to transform all imports automatically. That would still involve an extra step but make the "export" process easier. Obviously the other way around works as well.

Be-ing · 2020-06-15T10:44:41Z

I think we should tell mapping developers to use the QRC paths until https://bugreports.qt.io/browse/QTBUG-85025 is fixed upstream and widely available in Linux distros. Most mapping developers will not need to edit the libraries shipped with Mixxx, so this requires no extra work and is therefore the least error prone. While we develop the libraries, we can use the relative paths to import in the controller script we are working with to develop the library. When the fix is made upstream, it will be easy to change all the import statements in the Git repository to relative paths.

Be-ing · 2020-06-15T22:10:50Z

Unfortunately using QDir::setPrefixPaths with JS module imports is targeted for Qt 6.2, so we won't be able to use it anytime soon. So I added the MidiDispatcher module to the QRC system and it can be loaded with import {MidiDispatcher} from "qrc:/controllers/lib/mididispatcher.mjs". This PR is ready for review. I am pleasantly surprised how small this PR ended up being. I don't want to add any more features to this branch.

The next step will be rewriting Components as JS modules using this new system. But that's for another PR.

Holzhaus · 2020-06-15T22:47:43Z

res/controllers/lib/mididispatcher.mjs

+        }
+    }
+    receiveData(data, timestamp) {
+        const key = JSON.stringify([data[0], data[1]]);


Hmm this assumes that MIDI data is at least 2 bytes, so it can't be used to map things like FA (sent by my controller when pressing the play button of the built-in drum computer).

This class doesn't need to handle every single possibility for esoteric MIDI messages. The controller module's receiveData function could check for exceptional messages and return before calling MidiDispatcher.receiveData.

According to this list, all messages are 3 bytes, except:

Messages starting with C or D are 2 bytes long where byte 2 is the value

Messages starting with F are 1 byte long or "special messages" like SysEx

I think we only need 2 cases:

First midi nibble is C, D or F: only use first byte as key

Use first 2 bytes in all other cases

I added support for 0xC (program change) and 0xD (aftertouch) messages, but I don't think it would be helpful to add logic for handling 0xF messages which are highly variable. I suggest implementing controller-specific logic for handling those messages in the controller module's receiveData function and returning before calling MidiDispatcher.receiveData.

FWIW there is a single controller mapping in Mixxx (Akai LPD8) that uses 0xC (program change) MIDI messages and zero that use 0xD (aftertouch).

res/controllers/lib/mididispatcher.mjs

Swiftb0y

I have a couple of nitpicks/questions about the mididispatcher, but as far as I can tell with my limited C++ knowledge, the C++ part looks good.

Swiftb0y · 2020-06-16T07:01:00Z

res/controllers/lib/mididispatcher.mjs

@@ -0,0 +1,26 @@
+export class MidiDispatcher {


Can you document the class and it’s members using JSdoc comments?

Please review the documentation: 98a3d82

res/controllers/lib/mididispatcher.mjs

Be-ing · 2020-06-16T10:23:21Z

@Holzhaus do you have any comments about the C++ parts?

res/controllers/lib/mididispatcher.mjs

Holzhaus · 2020-06-16T10:52:07Z

src/controllers/controllerpresetinfoenumerator.cpp

@@ -57,7 +57,7 @@ void PresetInfoEnumerator::loadSupportedPresets() {
    m_bulkPresets.clear();

    for (const QString& dirPath : m_controllerDirPaths) {
-        QDirIterator it(dirPath);
+        QDirIterator it(dirPath, QDirIterator::Subdirectories);


This can is unrelated, right? I think enabling support for controller scripts in subdirectories is a different matter.

It was very simple to implement and I don't think there's any reason to remove it now.

src/controllers/engine/controllerengine.cpp

Swiftb0y · 2020-06-16T11:04:46Z

Just a small comment from a conceptional standpoint, you are probably already aware of:
Modelling a Controller in the form of module essentially makes each method static. This will force mixxx to create a dedicated ControllerEngine for each controller (or more explicitly, for each mapping). This is not an issue right now, but I remember there being plans to invert this relationship iirc which this design could interfere with.

Be-ing · 2020-06-16T23:00:44Z

If we implement support for interacting with multiple controllers through one script, I think the required JS changes would be minimal, if any. But that would require rewriting the whole controller system so I'm in no hurry to do it.

Swiftb0y · 2020-06-17T20:55:43Z

What’s the plan with the other libraries like common-controller-scripts.js? Since the old way we implemented modules by polluting the global namespace is incompatible with ES6 modules (which run in strict mode and thus disallow modification of the global namespace), I think the only possible way would be to rewrite them as well. That shouldn’t be too difficult though as we’re not trying to make breaking changes feature-wise or otherwise improve their design much.

While that’s obviously out-of-scope for this PR, it’s definitely a requirement for creating ES6 mappings IMO.

Holzhaus · 2020-06-17T21:07:30Z

I think we should redesign common controller scripts completely. I feel like a some of the functions are unneeded. I also think we should remove stuff like print and use the console extension instead.

Be-ing · 2020-06-18T00:02:25Z

What’s the plan with the other libraries like common-controller-scripts.js?

No changes. There is no need to rewrite them as modules. They are imported into the global namespace just as they always have been.

I think we should redesign common controller scripts completely. I feel like a some of the functions are unneeded.

Yes, there is some junk in common-controller-scripts.js. But we can't remove it without breaking backwards compatibility.

Holzhaus · 2020-06-18T00:10:43Z

Ideally I'd like to move all of res/controllers to res/controllers/legacy and keep all of that unchanged. The new controller script modules shouldn't have access to these scripts and import the stuff they need from other modules instead.

Be-ing · 2020-06-18T00:30:53Z

Looking at the contents of common-controller-script.js, yeah I think we should deprecate the whole file. I think the only code in that worth recycling are the regexes, script.toggleControl and script.triggerControl. Perhaps we could copy those into a new module called mixxx.

Be-ing · 2020-06-18T00:47:43Z

Ideally I'd like to move all of res/controllers to res/controllers/legacy and keep all of that unchanged.

I think this would require some changes to how script files are located using the legacy <scriptfile> XML element. We could change those to work like the new <module> element which treats the script file name as a path relative to the location of the XML file. But that would not work if an XML file is modified via the GUI mapping editor then copied from the system folder to the user folder... thoughts about how to handle that?

Holzhaus · 2020-07-09T22:21:22Z

But won't that be an additional context switch? C++ -> JS (module.handleInput) -> C++ -> JS callback? Probably doesn't make much of a difference, but just wanted to make sure I understand the proposal.

Swiftb0y · 2020-07-09T23:28:58Z

The idea was to eliminate that context switch by having the logic for registering and calling callbacks for specific midi messages purely in C++. This logic would run before module.handleInput so there is only a context switch as part of executing the callback (or module.handleInput as a fallback). So simply C++ -> callback / module.handleInput.

Be-ing · 2020-07-10T01:44:22Z

I want to keep the C++ API to the bare minimum required for controller scripts to interact with Mixxx. IMO everything else should be in JS to leave us maximum flexibility. We could implement a class with the same API as MidiDispatcher in C++, but then we would be stuck with it in the JS environment indefinitely. We have lots of junk that has built up over time in the legacy scripting environment. With JS modules, scripts only need to load what they use. We are free to implement alternate ways of approaching the same task in different modules. Perhaps we end up not liking this MidiDispatcher API in 2 years. No problem, write a new JS module and leave the old scripts using the old module.

ColorMapper is an exception because it was implemented for 2.3 before we had Map available in JS. I am fine with keeping that in C++ because it is already there.

This is also a foundation for a position scratch controller. This one is will be implemented in close connection to the engine and obviously no js task. Therfore it is reasonable to dispatch the frequent scratching messages directly to the controler and avoid jumping into js and back.
The js task is to configure the scratching controller.

I don't think this is a realistic idea. We need the full flexibility of JavaScript to handle jog wheels because there is no standardization of the signals used for jog wheels. My idea is to expose a C++ class to the JS environment which passes the data to the audio thread via lock free queue. We still need JS to interpret the controller's signals into data Mixxx can process. Maybe we could hack something together in C++ that would work for many MIDI use cases, but then we'd need a separate API for HID. As @Swiftb0y mentioned, one of the design goals is to make the APIs as similar as practical regardless of the wire protocol used by the controller.

But that requires to export each callback from the module, right? And we don't have the single entry point anymore.

exporting each callback wouldn't be necessary as long as the module has access to the MidiHandler object. But yes, we would no longer have a single entry point which would make the API fundamentally different from HID.

Sorry I don't find our discussion anymore.

I grow increasingly annoyed with GitHub on long PRs. I hope we can move to Gitea eventually. Maybe it has a nicer UX.

Be-ing · 2020-07-10T02:32:29Z

Thank you for reviewing the JSDoc @Swiftb0y. Could you take another look at the new updates?

daschuer · 2020-07-10T10:11:36Z

I want to keep the C++ API to the bare minimum required for controller scripts to interact with Mixxx.

This is not reasonable. With the proposed C++ dispatcher we are able to release the JS Engine from a lot of rubber band work.

We could implement a class with the same API as MidiDispatcher in C++, but then we would be stuck with it in the JS environment indefinitely.

There is no difference from the user perspective of compiling JS modul into the mixxx binary or a c++ object.
Due to the optional catch-all event handler that is still available, a mapping writer is anyway not forced to use the internal dispatcher. Full flexibility is maintained.

This is also a foundation for a position scratch controller.

I don't think this is a realistic idea. We need the full flexibility of JavaScript to handle jog wheels because there is no standardization of the signals used for jog wheels.

That is not entirely true, if you look into existing mapping. While many are individual switching states, all have in common a Midi message that give the position (wheelTurn) This is the only one that is updated with a high rate and real-time sensitive and the one that we directly need to pass into the position scratch controller. The rest will be remain in the JS domain to control the position scratch controller or the other jog wheel related COs.

Maybe we could hack something together in C++ that would work for many MIDI use cases, but then we'd need a separate API for HID.

Unless you are planning the re-implement the position scratch controller for every mapping, we will have separate APIs for separate protocols anyway. The mapping writer will barely notice the difference if it is an integrated JS module or a C++ object.

But that requires to export each callback from the module, right? And we don't have the single entry point anymore.

This is still there, If you don't use the build in dispatcher. This is no different if the map of callbacks live in the C++ or JS domain.
Once you use the dispatcher, your single entry point is gone.

Be-ing · 2020-07-10T10:30:11Z

I am still in doubt this proposed optimization to reduce the use of the JS runtime is necessary with JIT compilation and efficient JS (no string usage in JS). Maybe we can revisit this when implementing the new jog wheel API. For now I would like to merge this and move on. I already have #2920 based on this and I'd like to not have to keep rebasing that. Originally I did not intend the discussion on this PR to focus on the MidiDispatcher module anyway. I included it in this PR as a proof of concept to test how a controller module would load another module as a library.

daschuer · 2020-07-10T10:56:39Z

I think we either need a conclusion here ore remove the dispatcher part.

I am still in doubt this proposed optimization to reduce the use of the JS runtime is necessary with JIT compilation and efficient JS

Normally every CPU cycle counts, think to the Raspberry use case and to battery drain. We normally optimize everywhere even in not real time contexts. Here we are in a hot path and real time relevant.
There is IMHO no point that stand against it.

Be-ing · 2020-07-10T10:59:50Z

How about we merge this now with the understanding that we will try your approach later before releasing 2.4? The MidiDispatcher JS module is already useful for developing proof-of-concept JS code with this. This JS module feature is still just a proof of concept with this PR anyway. In #2920 I have already removed the hacks which made JS modules usable with the legacy XML files.

Swiftb0y · 2020-07-10T11:15:35Z

unfortunately, the C++ Mididispatcher API would be slightly different than the JS version so we have to keep that in mind when developing PoCs using it. Otherwise, I'd be ok with merging the PR as is and swapping the MidiDispatcher out later.

daschuer · 2020-07-10T11:28:27Z

Let's get it right here.
Either remove the Dispatcher or move the Map part into C++.

Be-ing · 2020-07-10T11:31:25Z

Okay, I have removed the MidiDispatcher JS module. If anyone wants to use it, here it is for future reference.

Be-ing · 2020-07-12T16:25:39Z

Ready for merge?

daschuer · 2020-07-12T20:00:01Z

LGTM, Thank you. @Holzhaus anything missing? AppVeyor failure is a timeout.

daschuer · 2020-07-12T20:01:13Z

Hey, we have the rule not to merge own PRs. In addition there was a Pending review from @Holzhaus.

Be-ing · 2020-07-12T20:02:54Z

I don't think @Holzhaus had any pending concerns?

Holzhaus · 2020-07-12T20:12:35Z

Not really. I'm still not super-convinced that it's a good idea to allow loading JS files from subdirectories without further planning the layout of the controller script directory, but we can easily disable that before the 2.4 release.

Be-ing · 2020-07-12T20:17:13Z

Considering we'll be making a new metadata format after all, I agree. #2936

This fixes two bugs that were caused by mixxxdj#2868: 1. Mixxx didn't check if the JS module path was really a file. If you pass in a directory, QtJSEngine::importModule makes Mixxx crash (at least on Qt 5.15.0) 2. If a mapping does not have JS module associated with it, then the moduleFileInfo always pointed to the controller directory and hence triggered bug 1. This fixes both of these issues and adds some debug assertions to make sure it stays like that.

Be-ing added this to the 2.4.0 milestone Jun 14, 2020

Be-ing force-pushed the js_modules branch from c59c91e to d34c554 Compare June 14, 2020 20:29

Be-ing force-pushed the js_modules branch 3 times, most recently from e66ae18 to ce22c15 Compare June 15, 2020 01:00

Be-ing requested a review from Holzhaus June 15, 2020 10:45

Be-ing force-pushed the js_modules branch from ce22c15 to 49edf88 Compare June 15, 2020 21:51

Be-ing added the controller mappings label Jun 15, 2020

Holzhaus requested changes Jun 15, 2020

View reviewed changes

Swiftb0y requested changes Jun 16, 2020

View reviewed changes

mixxxdj deleted a comment from Swiftb0y Jun 16, 2020

Holzhaus requested changes Jun 16, 2020

View reviewed changes

MidiDispatcher: more improvements to JSDoc documentation

3ef56de

ControllerEngine: fix build for Qt < 5.12

ecc740d

remove MidiDispatcher JS module

f963cc3

daschuer approved these changes Jul 12, 2020

View reviewed changes

Be-ing merged commit 65a01f4 into mixxxdj:master Jul 12, 2020

Be-ing deleted the js_modules branch July 12, 2020 19:59

Holzhaus mentioned this pull request Jul 16, 2020

controllers: Fix crash if controller mapping has no module #2946

Merged

Be-ing mentioned this pull request Sep 7, 2020

Traktor s3, main branch #3074

Closed

daschuer mentioned this pull request Aug 16, 2021

2.4.0 changelog #4210

Merged

Be-ing mentioned this pull request Apr 2, 2024

Registering MIDI Input Handlers From Javascript #12781

Merged

add support for loading controller scripts as JS modules #2868

add support for loading controller scripts as JS modules #2868

Conversation

Be-ing commented Jun 14, 2020 • edited Loading

Be-ing commented Jun 14, 2020 • edited Loading

Be-ing commented Jun 15, 2020

Swiftb0y commented Jun 15, 2020

Be-ing commented Jun 15, 2020 • edited Loading

Swiftb0y commented Jun 15, 2020 • edited Loading

Be-ing commented Jun 15, 2020 • edited Loading

Be-ing commented Jun 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Jun 16, 2020

Holzhaus Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y commented Jun 16, 2020

Be-ing commented Jun 16, 2020

Swiftb0y commented Jun 17, 2020

Holzhaus commented Jun 17, 2020

Be-ing commented Jun 18, 2020

Holzhaus commented Jun 18, 2020

Be-ing commented Jun 18, 2020 • edited Loading

Be-ing commented Jun 18, 2020

Holzhaus commented Jul 9, 2020 • edited Loading

Swiftb0y commented Jul 9, 2020 • edited Loading

Be-ing commented Jul 10, 2020 • edited Loading

Be-ing commented Jul 10, 2020

daschuer commented Jul 10, 2020 • edited Loading

Be-ing commented Jul 10, 2020 • edited Loading

daschuer commented Jul 10, 2020

Be-ing commented Jul 10, 2020 • edited Loading

Swiftb0y commented Jul 10, 2020

daschuer commented Jul 10, 2020

Be-ing commented Jul 10, 2020

Be-ing commented Jul 12, 2020

daschuer commented Jul 12, 2020

daschuer commented Jul 12, 2020

Be-ing commented Jul 12, 2020

Holzhaus commented Jul 12, 2020

Be-ing commented Jul 12, 2020

Be-ing commented Jun 14, 2020 •

edited

Loading

Be-ing commented Jun 14, 2020 •

edited

Loading

Be-ing commented Jun 15, 2020 •

edited

Loading

Swiftb0y commented Jun 15, 2020 •

edited

Loading

Be-ing commented Jun 15, 2020 •

edited

Loading

Holzhaus Jun 16, 2020 •

edited

Loading

Be-ing commented Jun 18, 2020 •

edited

Loading

Holzhaus commented Jul 9, 2020 •

edited

Loading

Swiftb0y commented Jul 9, 2020 •

edited

Loading

Be-ing commented Jul 10, 2020 •

edited

Loading

daschuer commented Jul 10, 2020 •

edited

Loading

Be-ing commented Jul 10, 2020 •

edited

Loading

Be-ing commented Jul 10, 2020 •

edited

Loading