Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
New Controller Engine #7
base: main
Are you sure you want to change the base?
New Controller Engine #7
Changes from 2 commits
d4505eb
8afaba4
3af26ac
816bfdc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can even allow to share mappings via an unpacked zip file with a fancy extension that Mixxx is able to unpack.
I just remember discussions that mapping writers want to work only in a single file. So we need to be clear about our goals here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally find it a little silly to zip files and then use a custom extension (looks like a bad attempt at hiding something) but from a technical perspective, I don't see anything wrong with that. Its just an extra step required while exporting. Importing is probably suboptimal if we would unpack the file during each scan to read the manifest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's less about hiding something and more about saying "this is a mixxx-mapping" so that we could add an "Import Mapping' button to the controller preferences that allows picking a file from the downloads directory and automatically copies it to the right location without listing every single non-mixxx-related zipfile in the directory.
I suppose the performance impact is neglible. As an alternative, mixxx could provide an "export button" that zips the file w/o the manifest and includes the manifest text in the zip comment if we're worried about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, all pretty fair suggestions. We'll just need need a good library for it I guess because Qt doesn't have any facilities afaict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have zlib, zstd and liblzma already in the dependencies and may use also 7zip
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could do something similar than for NI stem format and add a prefix to the extension (e.g
.mixxx.zip
or.mixxx.tar.gz
). This would also us to support a wide range of compression format.Probably getting too much into implementation details, but we could inspire from tools that uses a similar mechanism. Thinking of tools like Terraform or Helm that also have many "third party" extension compressed in an archive, you can expand them into a "cache" folder on import and maintain a checksum comparison on the source archive. As long as that checksum doesn't change, you can safely use the manifest file from the filesystem. This also greatly simplify modding as users can change the expanded module on the fly, without having to use a different mechanism (some kind of "draft" folder) or need to repack an archive.
TL;DR; I don't think this is a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would really be a non-goal for me. Then we need to support all these compression formats forever, and I think there is no good reason for it. Just agree on a single format (or rather two: packed for easy exchange and unpacked for easy development).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many file formats (like OpenDocument for Office application) use JAR files as container (just with a different file extension). The JAR file container defines the zipping and the manifest file format. And also handle topics like fast reading the manifest without unziping the whole files.
But I don't think that JAR files are good for the developer experience of a mapping developer. I suggest to use just sub-directories per mapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid this flexibility will introduce lot of boilerplate for no reason. I would prefer to straight forward code a new engine without adding facilities for a not yet known future engine. YAGNI
That does not mean to not allow such future engine, it is always a balance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean boilerplate in the C++ domain or just in the manifest? For the manifest I could imagine adding some shortcuts for the common usecase sure, but I'd like to keep the decoupled nature on the C++ side. Otherwise we'll be right back where we started with the current engine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more a general concern. This flexibility and abstraction to allow different engines, comes with the cost that things are harder to implement and cannot be optimized because we have only the common ground of all engines.
So I would rather focus on solving the issues with the current engine and not to be full generic and future proof.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should I specify the direction for HID or BULK report? This information can be read from the device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate?
These sections should allow the mapping to declare things like "I need an HID endpoint and I intend to read from it". Then if no hardware with that capability is found, the mapping can not be loaded in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every USB device has "Endpoint Descriptors" ( https://beyondlogic.org/usbnutshell/usb5.shtml ), which contain this information per enpoint. If I know the supported device type, why should I store a copy of this information in the mapping files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. Primarily so the mapping can declare stable identifiers to address them. What do you propose instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to assign the device type to mapping by "Manufacuter ID" and "Product ID" and read out all information of the device, which we can gather via userspace APIs. Only what is not accessible on all supported systems, shall be stored in the mapping files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a risk of doing a 1-to-1 relationship here. Having the IO driver respecting the USB descriptor is definitely a must have, but it would make sense to also explicately define the direction the mapping is interested with. This would help us planning for vendor which may be using not so compliant descriptor (BULK being probably the most at risk here, since it already involve a custom API made by the vendor), meaning that the port could be used for more than just receiving/sending mapping relevant data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not convinced how this direction property should be clearly seperated, e.g.:
For what is this information needed for?
Why do we need to enforce this by the top level architecture. I fear that this will restrict us in the future, as we will always have to force all USB protocols into this arbitrary structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your concerns are valid. So the consequence is that we only apply the source/sink abstraction to midi. So no abstraction over these protocols. That may be fine, but then we would have to reimplement the PnC logic for each protocol. Do you have some other abstraction in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need an entity to assign to the execution engine here, but this must be an abstract definition to be future proof. It should cover cases like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comprehensive collection, but it doesn't answer my question. What do you imagine the common API between these to look like? We need some abstraction unless we want to reimplement common features such as the PnC editor for each protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have failed to implement that in other protocols than midi.
To allow it for BULK and HID, we need a "driver" layer that converts the proprietary telegrams into something usable for a reactive implementation. This will probably end up in two script stages. 1. The driver 2. The mapping.
Did you consider this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, The dispatcher. I know @acolombier made some experiments regarding that. I'm sure a dispatcher functionality could work for protocols other than midi (at least as long the data format is simple enough) but I consider them out-of-scope for this PR. It wouldn't be super hard to add on later though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Than lets at least consider it right form the start. It would be an immediate user benefit to have all back-ends GUI learnable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have build a working PoC which allows to learn action by implementing a naive diff on HID report. Certain venodr (like NI) also appears to be providing a fairly complete HID descriptor. This means that we could be able to implement a HID layer for it. I don't think it would be possible with BULK tho, but I believe that controller using BULK for input are becoming rare nowadays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my understanding it is only a difference how generic such a driver will be. I can Imagine to build a driver for a bulk controller with a weird protocol without any self documentation.
At this point, I just want to make sure our data flow allows it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fairly confident it will if we adapt the sources and sinks model...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would disagree with this statement. Yes, it is correct for JavaScript, especially as we have very different styles of JS across on the different mapping.
However, code generate in QML should be possible and provide fairly good result. Since JS is QML, there would also be no value loss in using QML. This would mean that it would be possible to have fully hybrid mapping, where non-technical user can easily tweak the behaviour of button, share the same mapping and have technical user implementing more complicated logic in JS, on the same source, without having some kind of "override" logic hidden and abstracted in the manifest file.
I guess we could still consider this approach for JS mapping so we provide feature parity, but could "default" to QML when creating mapping on the wizard.
I would also add that, as per the PoC I did on point-and-click, using QML could help us to optionally capture valuable metadata to help with the representation of the controller for interactivity purpose, customisation and educational purpose.
Kooha-2024-05-28-21-24-49.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The simple standard mappings must be stored in a semantic language, which allows the mapping wizard to read the already mapped controls. This must work for MIDI and HID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two issues with code generation:
I didn't really intend there to be override logic. At least not directly. Each protocol can opt into using a dispatcher module (instead of just getting all its traffic dumped to some handler directly). Then the manifest can specify a CO to be controlled and the execution engine can specify some code to executed based on receiving some message. If both the manifest and the execution specify something for the same message, the action declared in the manifest takes precedence.
Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A code generator is not what is needed, we need an editor! Which allows to read a mapping into the GUI and allows to re-map the controls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, this is why, due to its declarative nature, QML could be a good pick.
Not sure why this wouldn't? This proposal suggestd a new engine agnostic to the IO backend, but also stands as an independent implementation, that doesn't have to live with the legacy engine, but rather be an alternative. At least I believe that was the the ambition when we discussed about it with @Swiftb0y
That's exactly what Qt Creator does. And all the relevant source is under compatible license AFAIK. QML is a declarative language, and custom QML component can allow to keep mappable property in the "code" using well constrained property
qtdeclarative and qt-creator(particularly
src/qmldom
) can provide that. They are also maintain by Qt itself, so they provide premium support of the QML standard going forward.I think this statement is not always true. QML boilerplate can be extremely simple if we design the engine in a sensible manner. My previous attempt as part of #11407 shouldn't be taken as reference as it was attempting to align as much as possible with the legacy JS engine it would live in. Now assuming we make separate agnostic engine (or even QML only), this could be greatly simplified, and potentially contain even less boilerplate than bare JS
It does make sense, and was what I had in mind already. But this is what I meant by the override logic: instead of having a single source of truth, which follow the same API, you end up with two APIs which aren't compatible. This means that someone with a limited knowledge cannot use the point-and-click to establish a basic behaviour and then override certain hooks with simple logic, especially when trying to implement new controller, not yet supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds scary to be honest. Remember that we need to catch users with school level programming skills.
Every high level implementation may scare them away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on how that's scary?
All I'm saying is that we would export the libraries loaded from outside the mapping folder and copy those into mapping. Then when we load the mapping we prefer the versions bundled with the mapping over those built into mixxx. This avoids issues where users would complain when the mapping they download from the forum suddenly breaks / works differently when they run it on a different mixxx version. That allows us to have less strict stability requirements for our libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably sound scary for me because I did not understand it.
My general concern is that we should keep the required programming knowledge at a minimum. With a stable API and a complete documentation. The idea presented here sounds like a receipt for the opposite, but I could be wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. All I'm proposing here is how we can ensure we bundled and load the correct shared (across mappings; eg common-controller-scripts.js) dependencies when the mappings are being shared on the forum. This is essentially the same we already do when overlaying
.mixxx/controllers/
overres/controllers/
.