-
-
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
[POC] OSCQuery #14229
base: main
Are you sure you want to change the base?
[POC] OSCQuery #14229
Conversation
Mixxx has currenty 118899 COs tendency rising. |
Someone had been working a lot to, that makes me happy because I've been working on new MixxxInTouchOSCCovCop version all day (and will be working on it tomorrow to) Add: can you include a rar-ed JSon file? |
addAddress("/cov/" + address, "f", "3", ""); | ||
addAddress("/get/cov/" + address, "f", "3", ""); | ||
} | ||
|
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.
what is the "f", "3", "" for?
f = float?
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.
All is described here, but we should make the code readable on its own.
https://github.com/Vidvox/OSCQueryProposal
"f" = float
"3" = rw = Read and write.
@@ -21,6 +21,7 @@ class DlgDeveloperTools : public QDialog, public Ui::DlgDeveloperTools { | |||
void slotControlSearch(const QString& search); | |||
void slotLogSearch(); | |||
void slotControlDump(); | |||
void slotOscQuerryDump(); |
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.
typo is repeated multiple times, please grep
for the other occurrences
void slotOscQuerryDump(); | |
void slotOscQueryDump(); |
continue; | ||
} | ||
fullPath += "/" + pathPart; | ||
if (objects.size()) { |
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.
implicit narrowing cast. what does this imply? this code is only executed on first iteration?
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.
Yes.
void addAddress( | ||
const QString& address, | ||
const QString& type, | ||
const QString& access, | ||
const QString& description); | ||
|
||
void removeAddress(const QString& address); |
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 implementation of both of these is really hard to understand IMO... Can you try to simplify 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.
Not yet, it is still premature. Or do you see a low hanging fruit?
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 suspect the algorithm would be easier to implement if addAddress and removeAddress would not mutate the json object inplace. Instead it should save it in its own internal datastructure (that is easier to understand than QJsonObject) and only saveToFile serializes that datastructure to a QJsonDocument in one go. I think doing it this way may result in simpler code. wdyt?
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.
Yes, that is probably true. I have started with that but than I realized that the main issue remains. You need to start from n-leaves to built the tree. That's why I started with that here.
We finally need auch a tree to allow wildcard addressing and storing our CO objects.
It could be interesting how this performed compared to QHash using the full address and not support wild card addressing..
@Eve00000 has implemented a code to restore the original ini name of our COs and use the established QHash.
We neet to put all in a pot, steer and have a look.
What do you think?
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.
HI, just releasing a balloon:
can we, instead of making a hash of the complete collection of CO's implement a sort of 'favourites', I can imagine the user won't use all CO's in /tree/branch/leaf form but only a 'selection as needed'.
If we create a favourites table, we do a lookup of the converted CO once and store the translation in an intermediate hashtable, this will speed up the interaction between /t/b/l to CO.
In this case we do need to implement a regular checkup to control if the translation of the favourites is stil correct.
If this idea is complete stupid, please ignore it but don't shoot me 😄
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 we, instead of making a hash of the complete collection of CO's implement a sort of 'favourites', I can imagine the user won't use all CO's in /tree/branch/leaf form but only a 'selection as needed'.
Not sure its worth the complexity. Certainly won't improve performance IIU your proposal correctly.
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.
you need to start from n-leaves to built the tree.
I don't understand, can you elaborate?
Qt holds a the string like QJsonValue in memory. The QJsonObject is only an editor interface.
If we want a root QJsonValue with {"a":{"b":{"leaf":{}}}} you need to create the leaf first, create an empty b, insert the leaf create an empty a insert be and insert a to the root.
If you want to add something into the leaf you cannot do via the root, because the leaf is read only. You can copy the leaf, alter it and put it back to b. however b is also read only and you need to copy it ... and so forth.
The Root QJsonObject is only one dimensional array [1] with only a, a QJsonValue with all the rest as a string. You need to create QJsonObject for a, then get b, create the QJsonObject for leaf.
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.
Sure... but thats orthogonal to the design I'm questioning. Why does the class manage the json tree directly instead of keeping its own (easier to manipulate) datastructure and then only build the json tree when actually writing the file? I feel like that would result in a simpler implementation than the current one (whose clarity is limited by the clunky Qt API). Or rather: the limiting factor in terms of code quality seems to the API of the Qt classes, so to improve the code, we limit the amount of code that has to interact with that API. If that code was constrained to the saveToFile
member function, the amount of clunky code would be minimized. 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.
I had have this also in mind. We will need our own data structure anyway when we decide to allow wild card lockups. However, we always need a similar single branch up ad down crawling when writing the Json File. Since the structure is constant, it is reasonable to keep it in memory and not do it for all 118883 leaves again on every OSCQuery call. Writing the file is only a debug feature for the developers. If we have that code anyway, id does not matter in which function it is.
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.
Ah I see, I misunderstood then. Yes in case this is called often, it does make sense to do it in place. I thought writing the file was the primary purpose of the class.
This PR implements the a bit of the OSCQuery protocol into Mixxx. It finally allows to subscribe CO updates form remote. Somehow a mapping mechanism. See https://github.com/Vidvox/OSCQueryProposal
@Eve00000 and I have discussed this and came to the conclusion that we need two root namespaces:
cop = Returns the parameter represenatation of a ControlObject (Controller Domain)
cov = Returns the value presnetation of a ControlObject (Engine Domain)
Both are duplicated as /get/cop and /get/cov to query the values from clients which do not have implemented the
OSCQuery protocol
This results for a compelex example to this OSC path:
"/cop/EffectRack1/EffectUnit1/group/(Channel1Stem1)/enable"
This results in a nice tree of COs, however we put OSCQuery to the edge, because the resulting formatted JSON file is 233 MB.
I have created a button in the developer menu to store the file in the Mixxx config folder as oscquerry.json.
You can Open it with Firefox (after copy it to your download folder) to nicely collapse all and navigate.
This is a good preview how it will work in client tools when selecting a Mixxx control.