TheButlah / ovr_overlay

Rust OpenVR bindings using an up-to-date OpenVR and autocxx
https://docs.rs/ovr_overlay
Other
7 stars 8 forks source link

Consider alternate API for IVROverlay::{set,get}_transform_* #12

Open kitlith opened 1 year ago

kitlith commented 1 year ago

Idea would be to either use a trait/structs to specify what we want to set/get, and we dispatch to the different functions depending on that. This would mean we end up with a single set/get transform function, instead of the 3/4 pairs we currently have.

Idea came up while discussing #10 in discord, but I'm punting for now and documenting the idea here.

@DASPRiD might have an opinion, since they requested SetOverlayTransformTrackedDeviceRelative in #7.

TheButlah commented 1 year ago

Would prefer to use traits instead of enums here. Enums have a minor runtime cost

kitlith commented 1 year ago

sketch of trait based system would look something like:

pub trait TransformType: SealedTransformType {
    fn set(self, mngr: &OverlayManager, overlay: OverlayHandle, transform: &Matrix3x4) -> Result<(), EVROverlayError>;
    fn get(mngr: &OverlayManager, overlay: OverlayHandle) -> Result<(Matrix3x4, Self), EVROverlayError>;
}

pub struct OverlayRelative(/* parent */ OverlayHandle);
impl TransformType for OverlayRelative {
    fn set(self, mngr: &OverlayManager, child: OverlayHandle, transform: &Matrix3x4) -> Result<(), EVROverlayError> {
        let parent_overlay = self.0;
        mngr.SetOverlayTransformOverlayRelative(/*...*/);
    }
    fn get(mngr: &OverlayManager, overlay: OverlayHandle) -> Result<(Matrix3x4, Self), EVROverlayError> {
        let mut parent_overlay = /*...*/;
        let mut transform = /*...*/;
        mngr.GetOverlayTransformOverlayRelative(/*...*/);
        Ok((transform, OverlayRelative(parent_overlay)))
    }
}

/* repeat for others */

// user facing API
impl OverlayManager {
    pub fn set_transform<T: TransformType>(&self, overlay: OverlayHandle, type: T, transform: &Matrix3x4) -> Result<(), EVROverlayError> {
        type.set(self, overlay, transform)
    }
    pub fn get_transform<T: TransformType>(&self, overlay: OverlayHandle) -> Result<(Matrix3x4, T), EVROverlayError> {
        T::get(self, overlay)
    }
}
TheButlah commented 1 year ago

Its unclear to me what the purpose of the self argument in set and the T in the return of get is actually used for. I would have expected that implementors of TransformType are zero sized types and don't have any relevant data during a set and get operation.

My confusion may stem from ambiguity of the self and overlay parameters in OverlayRelative::set. Perhaps documenting those would help. I suspect you actually intended that OverlayRelative holds not the original overlay, but what any overlay would be relative to, and I missed that at the first read.

Can you explain a bit more?

kitlith commented 1 year ago

overlay relative transforms are relative to a parent index tracked device transforms are relative to a tracked device absolute transforms are the only ones that don't have an additional parameter.

when setting, you need to pass the object it's relative to. when getting, openvr also retrieves the object it's relative to. Therefore, we don't use zero sized types for anything except absolute transforms, and return appropriately. this should allow for

let transform: (Matrix3x4, OverlayRelative) = mngr.get_transform(overlay, /*...*/); // we also now know the parent overlay it's relative to

EDIT: annotated sketch to more clearly indicate the overlay in OverlayRelative is the parent overlay.