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 all 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.
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.
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/
.