Add Derived Velocity Tracking Support for Natural Locomotion#1750
Add Derived Velocity Tracking Support for Natural Locomotion#1750M1DNYT3 wants to merge 13 commits intoSlimeVR:mainfrom
Conversation
…finements for possible config values.
…ity streaming to IMU Widget element
|
What the fuck... First, we don't accept AI-generated code. I'm not even reading all that, this should have been a couple hundred lines feature, not +10k -10k... Second, why is it so unnecessarily "comprehensive"? Who needs all of these settings, policies and other stuff. You either send real speed or you don't, maybe have a on/off setting. |
|
There was a prolonged discussion with @ButterscotchV and @kitlith on Discord. 10k lines are because Protobuf was regenerated. The entirety of the code is tested and I spent like two months developing and testing it. You spent like maybe 5 minutes looking at the PR and started bitching. Job well done I suppose. |
|
We have rules on AI-generated code and this PR breaks them:
In addition, maybe I have spent less than 5 minutes reviewing code, because I don't want to get brain damage, but maintainers including @ButterscotchV and @loucass003 have reviewed it and had things to say... I'm just taking a bullet here enforcing the rules. |
|
I agree that frontend part is not the best, as it's not one of my strongest suits. However, I may assure that the backend is pretty clean. I would've really accepted any request to eliminate bad practices in the UI or rewrite entire blocks of code on certain files accordingly. There's also a suggestion mentioned on Discord to put Protobuf update into a separate PR so it won't look like 10k lines got changed as part of my actual code. Some of the UI code is AI assisted, but not vibe coded. |
|
I'd agree with @Eirenliel that the change is too big, but most of the code actually comes from:
It's not up to me if SlimeVR wants to add full NaLo support, but if all we are doing is sending velocity to the driver, I believe that we can create a MVP which is just a few hundred lines of code. I'd suggest:
|
|
We chatted further on Discord. It sounds like just sending velocity to OpenVR isn't good enough for NaLo to behave well. The reason this PR has a lot of config is so that the user can customize the NaLo behavior. I'm worried that adding these settings to SlimeVR means that SlimeVR will be supporting NaLo as a feature. The separation would be better if SlimeVR published raw velocities, and some program or driver outside of SlimeVR would apply the NaLo adjustments. Unfortunately, NaLo itself doesn't seem to be maintained anymore. |
|
since I have been mentioned here, I feel the need to clarify my position. I am a 3rd party contributor. I happen to be the main person approving PRs to SlimeVR-Feeder-App, as I was the person who originally developed it. I also (somehow) have some amount of weight when it comes to reviewing PRs on the OpenVR driver, but do not have merge permissions there. I have mostly avoided working on the server due to both a dislike of Java and UI code, as well as unfamiliarity with the math involved in FK, IK, and motion prediction. This is to say, my opinions usually only extend to interaction with openvr. I am supportive of piping velocity and/or accel through the whole stack. TBH, ideally, this would've happened before slimevr sprung up its own prediction mechanisms. Too late for that now, and it may take a while to fix that as a result. I have not reviewed the code or the approach taken in this PR. The conversation I participated in was focused on "would having real velocities from the driver/feeder for the HMD/Trackers be useful?" with butterscotch answering in the affirmative, for their own needs, unrelated to this PR. I could be thinking of the wrong conversation, though. Regarding the AI summary: As a reviewer, I would prefer to see the prompt you gave the AI over the nicely formatted, but fluffy/noisy AI generated output. The PR description doesn't have to be long in most cases. It needs to hold what you think is the most important information we need to know, since it is the first thing we will read, and will be in the back of our heads as we review the code. Preferably, the description focuses on the "why", or your reasoning behind the choices you made, particularly on choices that may look weird at first glance, and not the "what", since we will be able to see the "what" when we take a look at the commits themselves. Taking an old PR of mine as an example: SlimeVR/SlimeVR-OpenVR-Driver#24 (comment) This first comment contains information that should've been in the PR description. Instead it was clarified over discord (before/during opening the PR? I don't remember) and then I backfilled it to Github for posterity. (Good thing, otherwise I would never be able to find it again.) The weird choice that needs justification in my PR is the use of a non-standard, invented, tracking universe. Then caveats are covered both in that comment and as review comments, and in another comment I wrote up a sketch of what testing should look like. For this PR, the weird choice would have been all the extra configuration options, and caveats could have included that you were struggling with writing the UI code. My biggest question here is how this is interacting with the prediction mechanism built into slimevr server, and what the correct way to combine slimevr's prediction with steamvr's prediction (which this will effectively enable). My gut feeling says that the initial implementation, before the addition of velocity scaling, felt wrong because the interaction between slimevr prediction and steamvr prediction was wrong, somehow. Unfortunately, I don't have a background in this type of math, and my gut feelings in this regard have been wrong before. So, I agree with @jabberrock. I think the best way forward is to create an MVP version (without any AI use), of this so that people who are more familiar with the math can weigh in. If you are struggling with UI code, then you should omit the UI code entirely instead of using AI. That way, someone else who is more familiar with the UI can write it. Aside, it can sometimes be useful to throw up a PR even before you think it is finished/polished/ready so that you have a place to receive early feedback & ask questions. I have a PR to monado that I'm going to get up soon-ish, after a prereq gets merged, even though it doesn't perfectly solve the problem, because I want to try and get feedback from people more familiar with the codebase and the other systems involved on how to fix it. This can help avoid cases where you spend a long time working on a solution to a problem, only to find out later that your solution was going in the wrong direction. |
|
I'll make another PR with an MVP backend-only version. I'll try to make a UI toggle (and only toggle), separately, after I make the backend MVP PR, and I don't expect any issues with making a toggle because, for the component itself, I can reuse the existing logic, and only hook it to the right config setting. And I'll refrain from using AI to help me compose a PR description in the future because I figured it gives off a feeling that I used the same approach in writing a code. I think the protobuf in Main branch of the server still doesn't have velocity definitions in it, so I'll make a separate PR to update protobuf in case that's actually required, so it won't confuse anyone into thinking that I added/removed 10k lines of code. Regarding the future of the feature. If it ever gets merged, that is.
I am currently hooked to the final resolved position of a virtual tracker anchored to the skeleton. The reason for that was actually because I consider the final resolved position from SlimeVR similar to that of Vive when they resolve theirs on the hardware + driver level (Lighthouse handles corrections, so they also apply filtering of sorts before resolving and sending to SteamVR). I drew the parallels between how Vive does this and how SlimeVR does this and came to this design choice. Oh, and it never occurred to me that we can turn off certain filtering features in the server, which, in theory, may improve the SteamVR behavior when Velocity is supplied. |
Add Derived Velocity Tracking Support for Natural Locomotion
Overview
This PR implements comprehensive velocity tracking functionality for SlimeVR trackers, enabling support for Natural Locomotion and similar locomotion systems that rely on velocity data. The implementation includes server-side velocity calculation, configurable scaling presets, role-based policies, and a complete GUI for configuration and real-time monitoring.
Key Features
Server-Side Implementation
VelocityRolePolicyto control which tracker roles can send velocity data, with implicit blacklisting of non-movement trackers (HMD, controllers, shoulders, neck, lower arms)VelocityConfigwith persistent settings for enable/disable state, role presets (ALL, HYBRID, CUSTOM), and scaling presets (UNSCALED, HYBRID, CUSTOM_UNIFIED, CUSTOM_PER_AXIS)Protocol Integration
GUI Features
Dynamic Behavior
Technical Details
Modified Components
Core Server (
server/core/):Tracker.kt: Added velocity calculation, scaling logic, and policy checksVelocityRolePolicy.kt: Role-to-group mappings and allowance logicVelocityConfig.kt: Configuration data classes and defaultsHumanSkeleton.kt: Integration hook for velocity configurationProtobufBridge.kt: Velocity data transmission to OpenVR DriverRPCSettingsBuilder.kt&RPCSettingsHandler.kt: RPC protocol handlers for settingsDataFeedBuilder.kt: Tracker data feed with velocity informationGUI (
gui/src/):GeneralSettings.tsx: Main velocity settings UI with dynamic renderingIMUVisualizerWidget.tsx: Real-time velocity display in tracker cardsVelocityInstructionsModal.tsx: Setup instructions and troubleshooting guidetracker.ts&velocity-settings.ts: React hooks for data managementdatafeed-config.ts: Configuration typesProtocol (
solarxr-protocol): Updated to include velocity schema and regenerated all language bindingsConfiguration File Changes
The feature adds the following to
vrconfig.yml:Use Cases
Primary: Natural Locomotion with FBT
Secondary: Natural Locomotion without FBT
Warning: Not Recommended for FBT-Only Setups
Testing Recommendations
Breaking Changes
None - feature is disabled by default and opt-in.
Dependencies
solarxr-protocolsubmodule reference to include velocity supportRelated Issues
Implements support for Natural Locomotion integration while maintaining FBT compatibility: #25