-
-
Notifications
You must be signed in to change notification settings - Fork 609
Protocol cleanup (4) #2901
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
Protocol cleanup (4) #2901
Conversation
The-personified-devil
left a 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.
Tried to be as thorough as possible but especially the interaction code is just too much to really do that.
alvr/client_openxr/src/lib.rs
Outdated
|
|
||
| let lobby_body_tracking_config = if platform.is_pico() { | ||
| Some(BodyTrackingSourcesConfig { | ||
| meta: BodyTrackingMetaConfig::default(), |
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'd argue that meta should still take an explicitly disabled config. + I think we should have a comment why this workaround is even needed in the first place.
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.
Still does the default thing and not explicitly disabling meta body tracking, defaults are in-transparent as to what they actually are.
|
I might have to break this PR up. Often for me PRs grow to become monsters, and this one I didn't realize it before pushing |
|
Eh, it's already a PR now, I'm also not the biggest fan of splitting up prs because they so frequently require needless scaffolding code that'll be removed again immediately. |
|
|
||
| #[default] |
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 we really want this to be the default?
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.
Well, we just need to have something as default, just for how I setup the code in client_openxr li.rs
I ended up biting myself way too many time by doing big PRs that try to do too much. While this PR has a theme ("tracking refactoring") it's too broad and I can simplify it, with minimal extra changes (already made and tested a break off branch, I'm going to push it soon). The important thing is having separate commits so the changes can be tested with more granularity before deep diving and dissecting thousands of changed lines, which usually it's only me that can do if the PR is mine |
TrackingData(exTracking)TrackingDataand extract default trackers on the server sideThis PR improves the architecture for potential Monado support, by moving prediction to the server side. Also this PR supersedes #2481. Video frames must still be pushed with the original tracking poll timestamp, otherwise the code to calculate the total latency breaks. The final step of the tracking rewrite is to remove timestamps as IDs altogether and use pose inter/extrapolation searching to determine their timestamps.
This PR is untested. I will do only basic testing, possibly gathering testing from community. I'm pushing it early to gather review feedback