Repository navigation
feat: Add support for Pico headsets face tracking - #2666
Conversation
|
Will test a bit later (on p4pro with 5.9 pui), only one thought for now - can you use eye/face tracking separately? |
|
Added to review @The-personified-devil considering you have been in the og issue (maybe interested) |
I don't really see any option to disable eye tracking data from |
|
The reason we were enabling eye gaze interaction on pico unconditionally is because we didn't have access to any other api allowing to create the tracker objects separate from the action bindings which don't allow disabling either. If we are able to use the new XR_PICO_eye_tracking, we can skip XR_EXT_eye_gaze_interaction. |
|
Pico's private eye-tracking functions also require |
|
Just wanted to rename branch, didn't know that it will close PR, sorry. |
| fb_face_expression: None, | ||
| htc_eye_expression: None, | ||
| htc_lip_expression: None, | ||
| pico_face_expression: None, |
There was a problem hiding this comment.
As discussed on Discord, this is a breaking change and so we should reuse the fb_face_expression field for pico.
There was a problem hiding this comment.
Shall I rename it into face_expression?
There was a problem hiding this comment.
While it wouldn't break the protocol (because it's binary and packed) i think we should keep the current name for now. We will add a proper separate field on v21.
|
|
||
| impl FaceTrackerPico { | ||
| pub fn new<G>(session: xr::Session<G>, visual: bool, audio: bool) -> xr::Result<Self> { | ||
| let mut tracking_flags = 0u64; |
There was a problem hiding this comment.
There should be no need to specify the type in the literal (u64) but if you do, put a underscore in between: 0_u64
| let get_face_tracking_data = unsafe { | ||
| let mut get_face_tracking_data = None; | ||
| let _ = (session.instance().fp().get_instance_proc_addr)( | ||
| session.instance().as_raw(), | ||
| c"xrGetFaceTrackingDataPICO".as_ptr(), | ||
| &mut get_face_tracking_data, | ||
| ); | ||
|
|
||
| get_face_tracking_data | ||
| .map(|pfn| mem::transmute::<VoidFunction, GetFaceTrackingDataPICO>(pfn)) | ||
| } | ||
| .ok_or(sys::Result::ERROR_EXTENSION_NOT_PRESENT)?; |
There was a problem hiding this comment.
Before this code, you might want to check that the extension exists, using the extra_extensions list (you need to add the parameter in the new()). Check the multimodal extension for a reference.
| super::xr_res(start_eye_tracking(session.as_raw()))?; | ||
| super::xr_res(set_tracking_mode(session.as_raw(), tracking_flags))? |
There was a problem hiding this comment.
Like multimodal, this API uses global start/stop functions. so i'd advise to expose those functions from this structure and let the caller site manage when to call those functions. Of course you also would need to rev-eng the stop/pause function signature
| }) | ||
| } | ||
|
|
||
| pub fn get_face_expression_weights(&self, time: xr::Time) -> xr::Result<Option<Vec<f32>>> { |
There was a problem hiding this comment.
The name of this function should reflect more the name of the inner function you are calling. I'd like for the APIs we are exposing in the modules in extra_extensions to be as transparent as possible.
| self.append_packet_vrcft(b"LipHtc\0\0", &arr); | ||
| } | ||
|
|
||
| if let Some(arr) = face_data.pico_face_expression { |
There was a problem hiding this comment.
Here, instead of the pico_face_expression, you should match fb or pico prefix based on the length of fb_face_expression
zmerp
left a comment
zmerp
left a comment
There was a problem hiding this comment.
Looks alright. there is mainly just a couple of lints to fix. Also, you haven't exposed set_tracking_mode separately but I think it's better this way
* feat: Add support for Pico headsets face tracking * style: Code formatting * refactor: Address requested changes * fix: Fix small code mistakes * fix: Resolve lint issues
* feat: Add support for Pico headsets face tracking * style: Code formatting * refactor: Address requested changes * fix: Fix small code mistakes * fix: Resolve lint issues
Adds face tracking support for Pico headsets. Tested on Pico 4 Pro with Pico OS 5.12.0S.
Resolves #2558, alvr-org/VRCFT-ALVR#3.
See related VRCFT-ALVR pull request: alvr-org/VRCFT-ALVR#4.