Check validity of builder structs

[Timbals]

The generator generated a bunch of builders for structs that aren't used. I modified the generator to not generate these. Only the builders for the composition layers and haptics are publicly exported. The other builders are only used internally.

Replaced builder structs with high-level enums/structs. Polymorphic structs got replaced with an enum. There's also a union for each enum that combines all the raw structs from the sys crate.

I also marked create_swapchain as unsafe. Similar to create_session, the interaction with the graphics API means the runtime isn't required to check all requirements.

Fixes #163

[Timbals]

I've been playing around with various ideas on how to make the API safe. Here are some things I tried:

Initializing required fields in new. Doesn't allow checking that the correct session is used.

Example

```rust #[derive(Copy, Clone)] #[repr(transparent)] pub struct SwapchainSubImage<'a, G: Graphics> { inner: sys::SwapchainSubImage, _marker: PhantomData<&'a G>, } impl<'a, G: Graphics> SwapchainSubImage<'a, G> { #[inline] pub fn new(swapchain: &'a Swapchain) -> Self { Self { inner: sys::SwapchainSubImage { swapchain: swapchain.as_raw(), image_rect: Rect2Di::default(), image_array_index: 0, }, _marker: PhantomData, } } /// Initialize with the supplied raw values /// /// # Safety /// /// The guarantees normally enforced by this builder (e.g. lifetimes) must be /// preserved. #[inline] pub unsafe fn from_raw(inner: sys::SwapchainSubImage) -> Self { Self { inner, _marker: PhantomData, } } #[inline] pub fn into_raw(self) -> sys::SwapchainSubImage { self.inner } #[inline] pub fn as_raw(&self) -> &sys::SwapchainSubImage { &self.inner } #[inline] pub fn image_rect(mut self, value: Rect2Di) -> Self { self.inner.image_rect = value; self } #[inline] pub fn image_array_index(mut self, value: u32) -> Self { self.inner.image_array_index = value; self } } #[derive(Copy, Clone)] #[repr(transparent)] pub struct CompositionLayerProjectionView<'a, G: Graphics> { inner: sys::CompositionLayerProjectionView, _marker: PhantomData<&'a G>, } impl<'a, G: Graphics> CompositionLayerProjectionView<'a, G> { #[inline] pub fn new(sub_image: SwapchainSubImage<'a, G>) -> Self { Self { inner: sys::CompositionLayerProjectionView { ty: sys::StructureType::COMPOSITION_LAYER_PROJECTION_VIEW, next: null_mut(), pose: Default::default(), fov: Default::default(), sub_image: sub_image.into_raw(), }, _marker: PhantomData, } } /// Initialize with the supplied raw values /// /// # Safety /// /// The guarantees normally enforced by this builder (e.g. lifetimes) must be /// preserved. #[inline] pub unsafe fn from_raw(inner: sys::CompositionLayerProjectionView) -> Self { Self { inner, _marker: PhantomData, } } #[inline] pub fn into_raw(self) -> sys::CompositionLayerProjectionView { self.inner } #[inline] pub fn as_raw(&self) -> &sys::CompositionLayerProjectionView { &self.inner } #[inline] pub fn pose(mut self, value: Posef) -> Self { self.inner.pose = value; self } #[inline] pub fn fov(mut self, value: Fovf) -> Self { self.inner.fov = value; self } } #[repr(transparent)] pub struct CompositionLayerBase<'a, G: Graphics> { pub(crate) _inner: sys::CompositionLayerBaseHeader, _marker: PhantomData<&'a G>, } #[derive(Copy, Clone)] #[repr(transparent)] pub struct CompositionLayerProjection<'a, G: Graphics> { pub(crate) inner: sys::CompositionLayerProjection, _marker: PhantomData<&'a G>, } impl<'a, G: Graphics> CompositionLayerProjection<'a, G> { #[inline] pub fn new(space: &'a Space, views: &'a [CompositionLayerProjectionView<'a, G>]) -> Self { assert!(!views.is_empty()); Self { inner: sys::CompositionLayerProjection { ty: sys::StructureType::COMPOSITION_LAYER_PROJECTION, next: null_mut(), layer_flags: CompositionLayerFlags::EMPTY, space: space.as_raw(), view_count: views.len() as u32, views: views.as_ptr() as *const _ as _, }, _marker: PhantomData, } } /// Initialize with the supplied raw values /// /// # Safety /// /// The guarantees normally enforced by this builder (e.g. lifetimes) must be /// preserved. #[inline] pub unsafe fn from_raw(inner: sys::CompositionLayerProjection) -> Self { Self { inner, _marker: PhantomData, } } #[inline] pub fn into_raw(self) -> sys::CompositionLayerProjection { self.inner } #[inline] pub fn as_raw(&self) -> &sys::CompositionLayerProjection { &self.inner } #[inline] pub fn layer_flags(mut self, value: CompositionLayerFlags) -> Self { self.inner.layer_flags = value; self } } impl<'a, G: Graphics> Deref for CompositionLayerProjection<'a, G> { type Target = CompositionLayerBase<'a, G>; #[inline] fn deref(&self) -> &Self::Target { unsafe { mem::transmute::<&sys::CompositionLayerProjection, &Self::Target>(&self.inner) } } } #[derive(Copy, Clone)] #[repr(transparent)] pub struct CompositionLayerQuad<'a, G: Graphics> { pub(crate) inner: sys::CompositionLayerQuad, _marker: PhantomData<&'a G>, } impl<'a, G: Graphics> CompositionLayerQuad<'a, G> { #[inline] pub fn new(space: &'a Space, sub_image: SwapchainSubImage<'a, G>) -> Self { Self { inner: sys::CompositionLayerQuad { ty: sys::StructureType::COMPOSITION_LAYER_QUAD, next: null_mut(), layer_flags: CompositionLayerFlags::EMPTY, space: space.as_raw(), eye_visibility: EyeVisibility::BOTH, sub_image: sub_image.into_raw(), pose: Posef::IDENTITY, size: Extent2Df::default(), }, _marker: PhantomData, } } /// Initialize with the supplied raw values /// /// # Safety /// /// The guarantees normally enforced by this builder (e.g. lifetimes) must be /// preserved. #[inline] pub unsafe fn from_raw(inner: sys::CompositionLayerQuad) -> Self { Self { inner, _marker: PhantomData, } } #[inline] pub fn into_raw(self) -> sys::CompositionLayerQuad { self.inner } #[inline] pub fn as_raw(&self) -> &sys::CompositionLayerQuad { &self.inner } #[inline] pub fn layer_flags(mut self, value: CompositionLayerFlags) -> Self { self.inner.layer_flags = value; self } #[inline] pub fn eye_visibility(mut self, value: EyeVisibility) -> Self { self.inner.eye_visibility = value; self } #[inline] pub fn pose(mut self, value: Posef) -> Self { self.inner.pose = value; self } #[inline] pub fn size(mut self, value: Extent2Df) -> Self { self.inner.size = value; self } } impl<'a, G: Graphics> Deref for CompositionLayerQuad<'a, G> { type Target = CompositionLayerBase<'a, G>; #[inline] fn deref(&self) -> &Self::Target { unsafe { mem::transmute::<&sys::CompositionLayerQuad, &Self::Target>(&self.inner) } } } #[derive(Copy, Clone)] #[repr(transparent)] pub struct CompositionLayerCylinderKHR<'a, G: Graphics> { inner: sys::CompositionLayerCylinderKHR, _marker: PhantomData<&'a G>, } impl<'a, G: Graphics> CompositionLayerCylinderKHR<'a, G> { #[inline] pub fn new(space: &'a Space, sub_image: SwapchainSubImage<'a, G>) -> Self { Self { inner: sys::CompositionLayerCylinderKHR { ty: sys::StructureType::COMPOSITION_LAYER_CYLINDER_KHR, next: null_mut(), layer_flags: CompositionLayerFlags::EMPTY, space: space.as_raw(), eye_visibility: EyeVisibility::BOTH, sub_image: sub_image.into_raw(), pose: Posef::IDENTITY, radius: 0.0, central_angle: 0.0, aspect_ratio: 0.0, }, _marker: PhantomData, } } /// Initialize with the supplied raw values /// /// # Safety /// /// The guarantees normally enforced by this builder (e.g. lifetimes) must be /// preserved. #[inline] pub unsafe fn from_raw(inner: sys::CompositionLayerCylinderKHR) -> Self { Self { inner, _marker: PhantomData, } } #[inline] pub fn into_raw(self) -> sys::CompositionLayerCylinderKHR { self.inner } #[inline] pub fn as_raw(&self) -> &sys::CompositionLayerCylinderKHR { &self.inner } #[inline] pub fn layer_flags(mut self, value: CompositionLayerFlags) -> Self { self.inner.layer_flags = value; self } #[inline] pub fn eye_visibility(mut self, value: EyeVisibility) -> Self { self.inner.eye_visibility = value; self } #[inline] pub fn pose(mut self, value: Posef) -> Self { self.inner.pose = value; self } #[inline] pub fn radius(mut self, value: f32) -> Self { self.inner.radius = value; self } #[inline] pub fn central_angle(mut self, value: f32) -> Self { self.inner.central_angle = value; self } #[inline] pub fn aspect_ratio(mut self, value: f32) -> Self { self.inner.aspect_ratio = value; self } } impl<'a, G: Graphics> Deref for CompositionLayerCylinderKHR<'a, G> { type Target = CompositionLayerBase<'a, G>; #[inline] fn deref(&self) -> &Self::Target { unsafe { mem::transmute::<&sys::CompositionLayerCylinderKHR, &Self::Target>(&self.inner) } } } // TODO and so on... ```

Use an enum and union to represent the polymorphic structs. This would require less validation because the enum can contain references to the high-level structs (e.g. xr::Space instead of sys::Space) and would allow validating the same-session requirement because the high-level structs contain that information. Introduces run-time overhead and is awkward to use with slices (layers in FrameStream::end).

Example

```rust #[non_exhaustive] pub enum CompositionLayer<'a, G: Graphics> { Projection { layer_flags: CompositionLayerFlags, space: &'a Space, views: &'a [CompositionLayerProjectionView<'a, G>], }, Quad { layer_flags: CompositionLayerFlags, space: &'a Space, eye_visibility: EyeVisibility, sub_image: SwapchainSubImage<'a, G>, pose: Posef, size: Extent2Df, }, Cylinder { layer_flags: CompositionLayerFlags, space: &'a Space, eye_visibility: EyeVisibility, sub_image: SwapchainSubImage<'a, G>, pose: Posef, radius: f32, central_angle: f32, aspect_ratio: f32, }, Cube { layer_flags: CompositionLayerFlags, space: &'a Space, eye_visibility: EyeVisibility, swapchain: &'a Swapchain, image_array_index: u32, orientation: Quaternionf, }, Equirect { layer_flags: CompositionLayerFlags, space: &'a Space, eye_visibility: EyeVisibility, sub_image: SwapchainSubImage<'a, G>, pose: Posef, radius: f32, scale: Vector2f, bias: Vector2f, }, Equirect2 { layer_flags: CompositionLayerFlags, space: &'a Space, eye_visibility: EyeVisibility, sub_image: SwapchainSubImage<'a, G>, pose: Posef, radius: f32, central_horizontal_angle: f32, upper_vertical_angle: f32, lower_vertical_angle: f32, }, } impl<'a, G: Graphics> CompositionLayer<'a, G> { pub(crate) fn into_raw(self) -> CompositionLayerRaw { match self { CompositionLayer::Projection { layer_flags, space, views, } => CompositionLayerRaw { projection: sys::CompositionLayerProjection { ty: sys::CompositionLayerProjection::TYPE, next: ptr::null_mut(), layer_flags, space: space.as_raw(), view_count: views.len() as _, views: views.as_ptr() as _, }, }, CompositionLayer::Quad { layer_flags, space, eye_visibility, sub_image, pose, size, } => CompositionLayerRaw { quad: sys::CompositionLayerQuad { ty: sys::CompositionLayerQuad::TYPE, next: ptr::null_mut(), layer_flags, space: space.as_raw(), eye_visibility, sub_image: sub_image.into_raw(), pose, size, }, }, CompositionLayer::Cylinder { layer_flags, space, eye_visibility, sub_image, pose, radius, central_angle, aspect_ratio, } => CompositionLayerRaw { cylinder: sys::CompositionLayerCylinderKHR { ty: sys::CompositionLayerCylinderKHR::TYPE, next: ptr::null_mut(), layer_flags, space: space.as_raw(), eye_visibility, sub_image: sub_image.into_raw(), pose, radius, central_angle, aspect_ratio, }, }, CompositionLayer::Cube { layer_flags, space, eye_visibility, swapchain, image_array_index, orientation, } => CompositionLayerRaw { cube: sys::CompositionLayerCubeKHR { ty: sys::CompositionLayerCubeKHR::TYPE, next: ptr::null_mut(), layer_flags, space: space.as_raw(), eye_visibility, swapchain: swapchain.as_raw(), image_array_index, orientation, }, }, CompositionLayer::Equirect { layer_flags, space, eye_visibility, sub_image, pose, radius, scale, bias, } => CompositionLayerRaw { equirect: sys::CompositionLayerEquirectKHR { ty: sys::CompositionLayerEquirectKHR::TYPE, next: ptr::null_mut(), layer_flags, space: space.as_raw(), eye_visibility, sub_image: sub_image.into_raw(), pose, radius, scale, bias, }, }, CompositionLayer::Equirect2 { layer_flags, space, eye_visibility, sub_image, pose, radius, central_horizontal_angle, upper_vertical_angle, lower_vertical_angle, } => CompositionLayerRaw { equirect2: sys::CompositionLayerEquirect2KHR { ty: sys::CompositionLayerEquirect2KHR::TYPE, next: ptr::null_mut(), layer_flags, space: space.as_raw(), eye_visibility, sub_image: sub_image.into_raw(), pose, radius, central_horizontal_angle, upper_vertical_angle, lower_vertical_angle, }, }, } } } #[repr(C)] pub(crate) union CompositionLayerRaw { projection: sys::CompositionLayerProjection, quad: sys::CompositionLayerQuad, cylinder: sys::CompositionLayerCylinderKHR, cube: sys::CompositionLayerCubeKHR, equirect: sys::CompositionLayerEquirectKHR, equirect2: sys::CompositionLayerEquirect2KHR, } impl CompositionLayerRaw { pub(crate) fn as_base(&self) -> *const sys::CompositionLayerBaseHeader { &self as *const _ as _ } } ```

Use a const generic typestate pattern to ensure all required fields are initialized. This feels over-engineered and doesn't allow checking that the correct session is used. No run-time overhead and mostly keeps the current API.

Example

```rust #[derive(Copy, Clone)] #[repr(transparent)] pub struct SwapchainSubImage<'a, G: Graphics, const SWAPCHAIN_VALID: bool = true> { inner: sys::SwapchainSubImage, _marker: PhantomData<&'a G>, } impl<'a, G: Graphics> SwapchainSubImage<'a, G, false> { #[inline] pub fn new() -> Self { Self { inner: sys::SwapchainSubImage { ..unsafe { mem::zeroed() } }, _marker: PhantomData, } } } impl<'a, G: Graphics, const SWAPCHAIN_VALID: bool> SwapchainSubImage<'a, G, SWAPCHAIN_VALID> { #[doc = r" Initialize with the supplied raw values"] #[doc = r""] #[doc = r" # Safety"] #[doc = r""] #[doc = r" The guarantees normally enforced by this builder (e.g. lifetimes) must be"] #[doc = r" preserved."] #[inline] pub unsafe fn from_raw(inner: sys::SwapchainSubImage) -> Self { Self { inner, _marker: PhantomData, } } #[inline] pub fn into_raw(self) -> sys::SwapchainSubImage { self.inner } #[inline] pub fn as_raw(&self) -> &sys::SwapchainSubImage { &self.inner } #[inline] pub fn swapchain(mut self, value: &'a Swapchain) -> SwapchainSubImage<'a, G, true> { self.inner.swapchain = value.as_raw(); SwapchainSubImage { inner: self.inner, _marker: self._marker, } } #[inline] pub fn image_rect(mut self, value: Rect2Di) -> Self { self.inner.image_rect = value; self } #[inline] pub fn image_array_index(mut self, value: u32) -> Self { self.inner.image_array_index = value; self } } impl<'a, G: Graphics> Default for SwapchainSubImage<'a, G, false> { fn default() -> Self { Self::new() } } // TODO and so on... ```

Change the relevant functions to unsafe. I'm currently leaning towards this being the best option. None of the "safe" variants seem to be worth the extra complexity/maintenance[^1]. Especially because the OpenXR API seems to allow a lot of local reasoning about validity (at least when compared to Vulkan)[^2]. But that's a question about the general direction of this crate. Should it be more like ash and mark more things unsafe, or should it be more like vulkano and start tracking a bunch of state?

@Ralith any thoughts?

[^1]: E.g. XR_META_recommended_layer_resolution actually changes the validity requirement of composition layers because it allows NULL swapchains to be passed to xrGetRecommendedLayerResolutionMETA. I don't know how this could be integrated ergonomically. [^2]: E.g. create_swapchain requires face_count to be 1 or 6 but we currently allow any u32, additionally it requires that the corresponding graphic APIs limits are respected which would require implementing getting these limits for all graphics APIs

[Ralith]

Thank you for this detailed investigation!

Initializing required fields in new.

I remain skeptical of adding arguments to new. It seems difficult for users to predict which fields would be affected, and might lead to large argument lists. Inability to check some invariants is also unfortunate.

Use an enum and union to represent the polymorphic structs.

This is very interesting. I think this might provide the best ergonomics. Many openxrs APIs already have similar marshaling to what this would require (e.g. anything involving strings). If anything, I think the existing "zero-cost" pattern is over-engineered and inconsistent with OpenXR and openxrs conventions at large, so I am not concerned about overhead. It's only once a frame, after all.

As a minor nit, I'd also consider making each variant a unit tuple (enum CompositionLayer<'a, G> { Projection(CompositionLayerProjection<'a, G>), ... }) and implementing From. This might be a bit more convenient for downstream code to manipulate, e.g. perhaps when building up a layer through multiple functions.

Either way, generating this might take a bit of effort, but I think it's in the running for best ergonomics, which is my overriding concern (after soundness, obviously).

is awkward to use with slices

Sorry, why is that awkward? There's nothing wrong with a slice of an enum. Sure, we'd have to marshal into a separate allocation, but so what?

There's one other option to consider: a builder pattern. It might look something like:

impl<G> FrameStream {
    fn end(&mut self) -> FrameBuilder<'_, G> { /* ... */ }
}

pub struct FrameBuilder<'a, G> {
    session: &'a Session<G>,
    layers: Vec<CompositionLayerRaw>,
}

impl<'a, G> FrameBuilder<'a, G> {
    pub fn push<L: CompositionLayer>(&mut self, layer: L) -> Result<&mut Self> {
        layer.validate(self.session)?;
        self.layers.push(layer.into_raw());
        Ok(self)
    }

    /// Calls `xrEndFrame` with the supplied composition layers
    pub fn present(self) { /* ... */ }
}

/// Safety: `validate` must return `Ok` if and only if the result of `into_raw` can be safely passed to `xrEndFrame` for the supplied session
pub unsafe trait CompositionLayer {
    /// Check whether `self` can be safely presented on `session`
    fn validate<G>(&self, session: Session<G>) -> Result<()>;
    fn into_raw(self) -> CompositionLayerRaw;
}

pub union CompositionLayerRaw { /* ... */ }

This is ergonomically similar to the enum case, but exposes a few more implementation details. One upside is that users can supply their own composition layers, but that's already possible by calling the raw xrEndFrame function pointer yourself, so I don't know if it's much of an advantage. Probably not easier to generate either, so on balance I lean towards the enum approach, which reduces the API surface.

Change the relevant functions to unsafe.

This is strict improvement and I'd be happy to merge it.

But that's a question about the general direction of this crate. Should it be more like ash and mark more things unsafe, or should it be more like vulkano and start tracking a bunch of state?

The intent of this crate is to offer safe bindings, and it's largely successful in that so far. This is motivated by OpenXR's decision to prioritize safety itself, with most invariants being enforced dynamically by the implementation already, a sharp contrast to Vulkan where checks are almost never guaranteed. I'm willing to expose unsafe escape hatches when convenient, and to consider unsafe APIs if enforcing safety seems disproportionately expensive, but as you note, safety in OpenXR is generally easy to reason about, so I don't think it's likely to get out of hand the way vulkano does.

[Timbals]

Notes mostly for myself:

• initializing requires all fields, so ergonomics might be worse (although everything but the LayerFlags for the composition layers should be explicitly initialized anyway)
• how to push_next? (XrCompositionLayerAlphaBlendFB, XrCompositionLayerColorScaleBiasKHR, XrCompositionLayerDepthTestFB, XrCompositionLayerImageLayoutFB, XrCompositionLayerSecureContentFB, XrCompositionLayerSettingsFB)
  • other high-level bindings for extensions use dedicated functions to configure the next, but the layers could reasonably want multiple things at the same time -> maybe implement the FrameBuilder
• can't implement Copy/Clone because the enum can contain mutable references
• There can't be a Haptic enum because it clashes with the Haptic marker struct, so the enum is named HapticData
• the samples_consumed: &mut u32 causes problems:
  • enum can't be Copy/Clone because mutable references can't be copied
  • as_raw can't be implemented because &self gives a &&mut u32 which can't be de-referenced
  • possible alternative: implement into_raw consuming self. Works but requires always passing owned versions.
• from_raw not possible to implement because going from just the handle to high-level struct is not possible

[Ralith]

how to push_next?

Extension structs could be represented with Option fields in the enum scheme, perhaps?