-
-
Notifications
You must be signed in to change notification settings - Fork 588
Tracking settings refactor #3060
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
base: master
Are you sure you want to change the base?
Conversation
alvr/session/src/settings.rs
Outdated
| #[schema(suffix = "m")] | ||
| position_offset: [f32; 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.
This should probably be in mm instead.
| ))] | ||
| pub detached_controllers_steamvr_sink: bool, | ||
| #[derive(SettingsSchema, Serialize, Deserialize, Clone, Copy)] | ||
| pub enum OutputTrackerType { |
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.
Honestly not a fan of this being two different enums, I think we'd be fine just ignoring the tracker if it's set to genericX
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 know but I think it's for the best. It's confusing for the user being able to select invalid configurations. And surely people will still do the wrong thing even with 10 different warnings
| content: vec![ | ||
| tracker_mapping_default( | ||
| InputTrackerTypeDefaultVariant::LeftDetachedController, | ||
| OutputTrackerTypeDefaultVariant::LeftFoot, |
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 interface doesn't protect against assigning two inputs to one output, ig we could always just error on that, but maybe explicitly listing all different outputs and having a selection for each is better? Sure it's more verbose, but idk
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.
Sometimes you may want this. And you see an example right here. Both detached controllers and generic trackers are mapped to left and right feet. This is because different platforms have different inputs, but we want to be able to use all of them with the same server settings configuration
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.
That feels more like something that should be sorted out by a preset system that allows swapping between user configs, than to allow double inputs that need to be resolved at runtime. And when resolving at runtime either we decide once and then commit or we need some way to select prioritization of the active input.
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.
Hell, having an explicit list of outputs that's active at all times would also enable that.
Just have a fixed list of outputs and a vector of inputs for each one and then that would automatically have prioritization by placement in the list.
Ideally this would come with a nice full body graphic where you can just click on the output to modify the inputs in a dedicated sub-menu, but maybe it also looks fine when fitting it in the normal alvr settings scheme (tho for that I think you kinda need a build to actually look at it, shouldn't we be able to build the dash mostly independent of the other crates consuming the settings?).
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 also a matter of whether we want to make an input or output centric system (i.e. mapping fixed inputs to variable outputs vs mapping variable inputs to fixed outputs). I'm aware that the current solution is actually neutral, but I don't think that's a very "human" way to deal with 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.
It should be "output centric", i think, because there are multiple sources that can produce the same kind of trackers, and also output trackers are linked to SteamVR which are harder to deal with because you cannot unregister them at runtime
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 idea of the body graphic is nice. I thought about this before. It would just be quite some work though. And also it would mean that the outputs are not in a list so it would be awkward and verbose on the server-side code. But instead we could just keep the output vector and hide with the body graphics
alvr/session/src/settings.rs
Outdated
| pub enable_vive_tracker_proxy: bool, | ||
|
|
||
| pub face_tracking: Switch<FaceTrackingConfig>, | ||
| pub enabled_trackers: Vec<SteamvrTrackerConfig>, |
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.
So this is essentially another layer for enabling/not enabling the trackers?
This seems like a pretty clunky system and likely needs a preset on the page to reset it back to default to be non-hostile to use.
Apart from that a fixed interface for all the trackers would maybe be more intuitive?
Nevertheless I think this needs some more help strings
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.
Yeah, here you can actually properly enable/disable trackers regardless of the data source. But these can't be changed without a steamvr restart because of SteamVR limitations. And yes there will be a body tracking preset. One glaring limitation is that we cannot have a preset selectively remove an element in the list, so we would have to set all elements at once. So this means that if we have a body tracking preset (Enabled/Disabled), we cannot have a controller enabled/disabled preset also (unless we allow to have always some controller devices that can be ignored by not sending data to them)
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.
Honestly a steamvr tab for ostensibly steamvr only stuff is a good idea, but with things like tracking that are tied in so deeply with the rest of the system I think separating it out is a bad idea.
I think we can handle monado/steamvr stuff with another schema flag that gets interpreted based on which thing is selected to launch. Or we just fully build one dash for steamvr and another one for monado, but I'd avoid that if possible.
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 we can handle monado/steamvr stuff with another schema flag that gets interpreted based on which thing is selected to launch
This was my idea also. A Monado build will not have the SteamVR tab and viceversa. But still, i think it's bad to put the list of enabled trackers in the Input tab, because it contains the SteamVR-specific extra properties field. How should we deal with this? Should we make yet another separate vector in the SteamVR tab just for the openvr configs per-tracker?
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.
But if we go with this idea, should we make the input/source tracker optional? I'm not sure if there is a proper usecase for this
|
Following your suggestions, after the changes I made, I think we don't need any extra presets. (of course it would be nice to have the human body graphic for tracker assignment, but it's too much to ask atm). about head and controller tracking, I decided to make them enabled implicitely regardless of the trackers mapping setting (HMD always enabled, controllers enabled with their own separate setting, basically as before) |
I'm making this PR early for feedback. Please check the settings.rs file. CI failures are normal since I did not bother with any other file
Some notes:
Inputstab