-
Notifications
You must be signed in to change notification settings - Fork 67
implement XR_META_environment_depth #172
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
base: master
Are you sure you want to change the base?
Conversation
|
It would be great to have this extension. How can I help? |
| fn enumerate_depth_environment_swapchain_images( | ||
| _swapchain: &EnvironmentDepthSwapchain<Self>, | ||
| ) -> Result<Vec<Self::SwapchainImage>> { | ||
| unreachable!(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this unreachable? Document, or replace with an appropriate error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it happens in enumerate_swapchain_images no swapchains are available in headless mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to create an headless session on a Quest 3, but it does not supports XR_MND_headless.
Futhermore, you need to create an EnvironmentDepthSwapchain to call this method, and this will probably fail in an headless session.
I'm considering adding this comment, what do you think ?
// as of today (March 2025), no runtime supports both XR_MND_headless and XR_META_environment_depth
// if such a runtime exists in the future, you won't (probably) be able to create an EnvironmentDepthSwapchain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me why we should assume these (or a future more general headless extension that might share this code) are mutually exclusive. Consuming information about the environment is independent of displaying graphics. Why not just handle the possibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, I suspect the spec actually guarantees that these are mutually exclusive (since there's no way to communicate an image without one), but we should ensure that the failure is graceful, and document the reasoning with reference to the spec.
| texture: ptr::null_mut(), | ||
| }, | ||
| |capacity, count, buf| unsafe { | ||
| (swapchain.fp().enumerate_environment_depth_swapchain_images)( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if this is called when the extension isn't present, or is present but not enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot call this method without having the extension enabled and already used.
To use this method, you need an EnvironmentDepthSwapchain, which requires an EnvironmentDepthProvider.
If the extension is not present or loaded, Session::create_depth_environment_provider will panic with the message 'XR_META_environment_depth' not loaded.
I have limited ability to review complex changes at the moment. Help with code review would be welcome, as would reports from real-world use of the proposed changes. |
|
Makes sense to me. |
Implements the XR_META_environment_depth extension.