Skip to content
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

Support drop guard for session #86

Merged
merged 1 commit into from
Sep 4, 2021
Merged

Conversation

zmerp
Copy link
Contributor

@zmerp zmerp commented Jul 19, 2021

This PR allows to pass a custom object to Instance::create_session() used to force a specific drop order. In particular, in case the graphics device and instance is kept around in a clonable object (with a proper impl Drop) and its time of destruction is unknown, the drop_guard argument should be set to a clone of this object. For context, here is a related PR: gfx-rs/wgpu#1609

This is needed for OpenXR support in bevy. Until now this solution was used:

#[derive(Clone)]
pub struct OpenXrSession {
    inner: Option<xr::Session<xr::AnyGraphics>>,
    _wgpu_device: Arc<wgpu::Device>,
}

impl Deref for OpenXrSession {
    type Target = xr::Session<xr::AnyGraphics>;

    fn deref(&self) -> &Self::Target {
        self.inner.as_ref().unwrap()
    }
}

impl Drop for OpenXrSession {
    fn drop(&mut self) {
        // Drop OpenXR session before wgpu::Device.
        self.inner.take();
    }
}

Instead of exposing openxr::Session to the user directly, this OpenXrSession object is used. But the underlying openxr::Session is still cloned by the bevy plugin implementation (for interop with graphics), and this makes it tricky to ensure safety.

The vulkan example is updated, but it passes None as the drop guard since the drop order is already enforced and the example is linear.

Copy link
Owner

@Ralith Ralith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, after some consideration I agree with the usefulness of this. However, I'm not sure about requiring all users to pass None explicitly. It seems to clutter the interface, as I expect many won't bother with a real one. Maybe add a create_session_with_guard method to Instance?

@zmerp
Copy link
Contributor Author

zmerp commented Sep 4, 2021

Thank you for the suggestion! I updated the PR.

Copy link
Owner

@Ralith Ralith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Ralith Ralith merged commit c06ec05 into Ralith:master Sep 4, 2021
@zmerp zmerp deleted the drop-safety branch September 4, 2021 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants