Optimize OSC Implementation #125
Conversation
…nclude and utilize unmanaged rust lib (https://github.com/benaclejames/SimpleRustOSC). Lotta optimization and performance optimizations.
…ome params would get stuck at 0 for seemingly random reasons.
There was a problem hiding this comment.
Found some things that might be worth looking at. I'll leave it up to you if you wanna make those changes! I haven't look at your Rust OSC lib but these are just addressing the interfacing of it (what you're probably looking for). Everything else looks pretty good from looking at the other changes.
Also have yet to test it but just some observations of the code.
There was a problem hiding this comment.
Looks good! Everything is working as expected and the review comments are just for things we might want to do in the future (if at all).
Also for your Rust lib you could bring back the DependencyManager, I had removed it from the SRanipal rejection from the VRCFaceTracking namespace but would make sense for this (or even embedding the compiled binary into the EXE somehow like how ILMerge or Fody.Costura can be used to achieve that).
| @@ -9,12 +9,13 @@ namespace VRCFaceTracking.OSC | |||
| public class OSCParams | |||
| { | |||
| private const string DefaultPrefix = "/avatar/parameters/"; | |||
| private const string CurrentVersionPrefix = "v2/"; | |||
There was a problem hiding this comment.
Kinda makes me think we should include some sort of versioning within the IParameter side of the parameter eventually, and use OSCParams or another wrapper that enforces it (like with the base parameters getting added to AllParameters for eg.).
I think for now this works pretty good and we can always revisit it ofc!
| { | ||
| _paramName = paramName; | ||
| _negativeParam = new BaseParam<bool>(paramName + "Negative"); | ||
| _getValueFunc = getValueFunc; | ||
| _negativeParam = new BaseParam<bool>(paramName + "Negative", data => getValueFunc.Invoke(data) < 0); |
There was a problem hiding this comment.
Also another thing we could consider doing in another PR in the future: if there isn't a negative parameter parsed then it won't return the negative values on output (there might be some cases where an avatar creator wants to use the positive part of a combined param and exclude the negative bool to save a little space).
There was a problem hiding this comment.
This is already the case! Binary bits check to ensure the negative parameter is relevant before sending. If not and the value would be negative, they just return false
https://github.com/benaclejames/VRCFaceTracking/pull/125/files/5fa1874372286802b9c2d04cf6a0c7f6f3f9eca3..fe8709c89c71dcd946767fcb05435204076c54fe#diff-5ed98eabd76f774c20ea2dc83b3436d6ff97b8b5df8f0ea0d7b07facd30a0d27R115-R117
There was a problem hiding this comment.
Though maybe you mean allowing it to return a value regardless of whether or not the negative parameter is relevant or not (might be misunderstanding you here). Though as you said, can always be fixed in the future :)
This PR utilizes a rust-made .dll to handle the bulk of OSC serialization/deserialization. It helps a little with optimization but also clears up our internal codebase a bit!