-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Ability for controller to share data at runtime #12199
Ability for controller to share data at runtime #12199
Conversation
2329ee6
to
0c4a57d
Compare
5d0adaa
to
55eab2a
Compare
55eab2a
to
51444e7
Compare
51444e7
to
2bd1d91
Compare
7dd2b4c
to
e71de08
Compare
7477ab9
to
f04c13a
Compare
f04c13a
to
80b1543
Compare
If you need to share some data at runtime between controllers, that means you are the author of the both (modified) mapping, so you may use some due diligence to make sure you do have clashing data. I'm also in favour of |
I wonder if it wouldn't be better to decalare each shared variable instead of just the namespace. While more text to write, I think it would make it more clear how it works. And since this will remain a rarely used functionality, self explaining code might be more importat, than small code.
|
I have added namespace restriction. Hopefully this is heading toward the right direction.
While this provide more visibility of what data is available in certain namespace, this is likely to make the devx less efficient as the mapping are static file, which you usually don't use actively while developing new controller mappings. |
src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp
Outdated
Show resolved
Hide resolved
src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp
Outdated
Show resolved
Hide resolved
src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp
Outdated
Show resolved
Hide resolved
/** | ||
* Gets the shared runtime data. | ||
* @returns Runtime shared data value | ||
*/ | ||
function getSharedData(): SharedDataValue; |
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.
/** | |
* Gets the shared runtime data. | |
* @returns Runtime shared data value | |
*/ | |
function getSharedData(): SharedDataValue; | |
/** | |
* Gets the shared runtime data. | |
* @returns Runtime shared data value - or undefined if not yet defined | |
*/ | |
function getSharedVariable(variablename:string): SharedDataValue; |
We should not share one single complex blob. This makes it really difficult to understand for developers and we can't print meaningful error messages, when two mappings do no match together.
The later can especially happen, when you edit the variable type at runtime of Mixxx, because only one mapping is reloaded at a time.
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 should not share one single complex blob.
What if, in your namespace foobar
, you want to share just a TypedArray
or a string, like in the tests? I think adding variables don't add any values to the feature and just another level.
we can't print meaningful error messages, when two mappings do no match together.
This isn't possible anyway. I thought we agreed that introducing namespace was a good enough option to prevent conflict.
I also don't see how this would work anyway either since any controllers part of a namespace can get/edit values as well: this means that it is acceptable for MappingA
to do get("MappingBRunning")
(or get().MappingBRunning
currently) and find no values.
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.
Namespacing is good enough to prevent unintended iterference of completly unrelated mappings.
But this does not address compatibility between the related scripts using the same namespace.
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 this request is consistent with my original namespacing proposal above: #12199 (comment)
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.
How your solution will compatibility between the related scripts compare to the current one? Do you mean I should also implement the API change mentioned 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.
This would be one approach. But also without an explicit declaration, it would be possible to print warnings like:
Line 1234 - Shared variable "deckSwitch" changed type in call of engine.setSharedVariable from number to string
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 all javascript, there is no schema / static typing, nor is it really necessary. If there is no technical reason why variables could not change their type I don't see a good reason to introduce one (especially when it would only result in extra complexity).
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.
While I understand you are using type change as a way to detect a potential problem, this lefts out other usecase where a type change wouldn't occur but yet doesn't occur - eg wrong unit.
I think it all boils down to understanding what you are sharing, and making sure producers and consumers expect the same data AND don't conflict with each other.
I think this type of detection is a nice to have, but I cannot really see how this is required. Realistically, only a hand full of official mappings will use this features, so I think it is a matter of making sure this is well document (e.g using shared context class for example).
If you really feel like there should be a key for data set/get, I will add it, but lets agree first this is the last significant change I'm being asked to do.
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 the record, I don't think a schema is necessary. exchanging an untyped/dynamic object is completely fine.
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.
@JoergAtGithub is that a hard requirement in your opinion, or are you happy to go with the untyped dynamic data as it is currently?
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.
full review. we're almost there IMO
src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp
Outdated
Show resolved
Hide resolved
src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp
Outdated
Show resolved
Hide resolved
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.
LGTM from my side tbh. Thank you very much for your patience @acolombier. waiting for @JoergAtGithub.
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 really sorry, but I cannot agree with this technical approach. This is due to a number of concerns, in particular:
This is an extension of the JS mapping API. Once such an API is in in the field, we have to maintain it forever due to backward compatibility issues. Especially because we cannot test later adaptations of existing mappings, for which we have no hardware. Furthermore any duplicated API would mean a future debt for maintenance.
As a consequence, an extension of the mapping API must be thought through much more thoroughly or designed in such a way that it can be extended at a later date.
So far, I am only aware of two use cases for shared data between mappings:
- The synchronization of a shift button status
- The synchronization of a deck-select status
However, these two states can have a very different scope depending on the functional distribution across the devices. With a global namespace which is hardcoded in XML file, this is impossible to achieve.
I have created the following sketches to illustrate this:
My suggestion would be that we create a concept document at https://github.com/mixxxdj/proposals in which we draft an API design that covers all requirements.
Perhaps a subset can be derived from this, which is sufficient for the use case of the S4 Mk3. Everything else could then be extended at a later time.
Thank you @JoergAtGithub for this thorough writeup. I have a couple comments: Case2: This is a very valid usecase and to be honest, not one I had thought about before. The issue here is even more general in that sense that in that scenario, it would even be difficult to match which screen belongs to which controller (at least with PortMidi, since it doesn't expose the required information, right?). Case 3, 4 and 5 are not supposed to be handled by this PR. There should not be (enforced by code review) a global namespace. I fully agree that the current solution does not work for "open-ended" interfaces. Those need to be centrally coordinated.
I also have a another one to add. the Traktor Kontrol S8 has buttons on the sides of the screens which likely communicate via the HID endpoint, but in practice only change the state of the QML screens. Usecase 5 is also interesting because its slightly ambigous in the sense that we should not assume that the user would always want these layers to be in sync. maybe they would like Deck 1/2 to be purely their "Stems deck" controlled by the S8 (having access to all the stems features) and deck 3/4 controlled by the turntables to have their turntable control (for accurate scratching). In that case, whatever mechanism manages the "focused layer" needs to be able to also be ignored. But this discussion should probably be continued in a proposal setting. |
TLDR; we need to address case 2 somehow, case 3,4,5 are and should not be solved in/via this PR. |
This (and the following about maintaining an API) is only true for stable API. In this case, we could release this feature and mark this feature as unstable to unblock S4 Mk3 implementation (which as a reminder is meant to be officially supported by Mixxx). This way you can freely remove those methods once there is a replacement. I assume your point is also valid for #11407 I must say I find it disrespectful to play that card after 2 months of back and forth review processes.
I guess you are taking Traktor/Native Instrument capability as reference. I suggest you look into #12557 or this mod for Traktor. TL;DR: The scope for mapping synchronisation is infinite.
That sounds reasonable, except that https://github.com/mixxxdj/proposals doesn't seem of great interest to the Mixxx community so far (see my stale PR trying to bring that repo to live). So till proven otherwise, I won't spend time draft a thorough proposal to see it being ignored. |
I agree, that it would be okay to mark this feature private. especially since its essentially just a workaround for the fact that we can't run multiple controllers in the same engine anyways. ergo, this should be replaced in the mid-term future anyways.
Let me assure you that @JoergAtGithub didn't mean for his veto to be disrespectful. His criticism is valid (especially considering the complications for case 2 which we should work through).
I'm sorry that the PR has gone stale for so long. It's just easy to loose track of things. @daschuer would you mind continuing your ongoing review in mixxxdj/proposals#1? |
What is the status of this PR? Should I close it or is there something we can do to finish it? |
I think case 2 is valid. Do you have an idea on how to identify two different controllers and match with screen belongs to which controller? I think once we have identified that, attaching that info (invisibly) to the namespace handles is straightforward. But we need some way of ensure that the screen of one device doesn't get matched to the midi ports of another device. |
I'm sorry but no, it does not make sense, and quite frankly I've lost the motivation to try and explain why. For anyone reading this with a S4 Mk3 - note that I will keep my dev branch ( FYI @ywwg please note that it won't be possible to complete #13004 now and will need someone else to take on that feature. |
I regret that this PR has been closed. As a reviewer, I also spent a lot of time on this topic. |
it does sound like a formal proposal is required for this to be resolved. Hopefully the ultimate solution is not so far off from this implementation that we can use this code. @acolombier I appreciate your contributions and understand your frustration. With large, delicate new features such as the ones you are working on, it is often hard to do design in the PR process, which is why we need to make better use of the proposal feature. That way the implementation can be worked out before you have done a bunch of work. In the past, when we have taken a step back from PR discussions and moved to a design discussion we have usually found that I am thinking of the hotcue color PR which was a huge issue back in the day. We ended up mapping out the two designs and almost immediately found a way through. |
I'm sorry that I have previously missed your concerns in regard to the multiple devices issue.
I disagree with your assessment that this was an option. Imposing any grouping criteria later would've limited the API again, resulting in a breaking change. Unless I'm misunderstanding your "grouping criteria" concept.
We can still add the grouping criteria now, at least without breaking the API description. The namespace defined in the XML could just be part of a larger identifier that takes more info, such as the usb serial into account. That same extension would not be hard to retrofit into this PR imo.
I disagree. Please elaborate. |
If we would have the namespace as string in JavaScript like in
we could later add an API like
and could use it together with the unmodified sharing API like
|
I see two fundamental issues with that:
|
"Viele Köche verderben den Brei" Sorry, I was also involved in that here: #12199 (comment) Thank you @JoergAtGithub to bringing all the cases up. My hope is to strip that PR down to the original use case and just make sure we have not created a maintenance burden that will bite us in future as expressed. Let him cook? What do you think. |
I had the same feeling. I appreciate we came to the same conclusion.
I think this makes sense. This way I can keep the namespace scoping I had already implemented following the general consensus on the need to isolate shared data, but keep it to a "default" for now. |
Per discussion, this work is not abandoned but on the back burner while we do a proper design for controller data sharing. |
This is a poor man solution, but it does the job with a minimal amount of effort.
TODO: