-
Notifications
You must be signed in to change notification settings - Fork 59
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
Check validity of builder structs #166
base: master
Are you sure you want to change the base?
Conversation
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 Example#[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<G>) -> 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 Example#[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<G>,
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. Example#[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<G>) -> 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 @Ralith any thoughts? Footnotes
|
Thank you for this detailed investigation!
I remain skeptical of adding arguments to
This is very interesting. I think this might provide the best ergonomics. Many As a minor nit, I'd also consider making each variant a unit tuple ( 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).
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
This is strict improvement and I'd be happy to merge it.
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 |
Notes mostly for myself:
|
Extension structs could be represented with |
The removed builders aren't publicly exported and aren't used internally
The only thing that isn't checked is that the `space` and `swapchain` handles come from the same session. The spec only mentions this for the cube layer, but it is probably required for all layers.
Polymorphic structs get replaced with an enum. There's also a union for each enum that combines all the raw structs from the `sys` crate.
Similar to `create_session`, the interaction with the graphics API means the runtime isn't required to check all requirements.
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 tocreate_session
, the interaction with the graphics API means the runtime isn't required to check all requirements.Fixes #163