Ralith / openxrs

OpenXR bindings for Rust
Apache License 2.0
282 stars 59 forks source link

Vulkan with `khr_vulkan_enable` instead of `khr_vulkan_enable2` #84

Closed Tenebryo closed 3 years ago

Tenebryo commented 3 years ago

I was trying to get the Vulkan example to work, but the headset I'm using only supports khr_vulkan_enable and not khr_vulkan_enable2. How would I have to change the example to accommodate this? I'm not sure how much the specific device matters, but I'm trying to run the example on the quest 2 over USB via oculus link.

Ralith commented 3 years ago

High-level bindings are not available for the obsolete Vulkan extension; your vendor (Oculus) should implement support for the current one. In the mean time, you could:

0.13.1 isn't too old yet, so that's probably the easiest thing for now, and saves you some work if Oculus catches up with everyone else before too much longer.

Tenebryo commented 3 years ago

Thanks! I'll probably use version 0.13.1 for now, and otherwise use it as a reference if I need to go with the low-level approach later.

zmerp commented 3 years ago

I'm having the same problem running the vulkan example natively on the Quest 2. I think Oculus has no incentive to upgrade to khr_vulkan_enable2 because 1) khr_vulkan_enable it is part of the OpenXR 1.0 standard and more closely matches its OVR Mobile SDK 2) mobile SoCs only have one GPU and the problem that khr_vulkan_enable2 solves is not even there (the application cannot select the wrong adapter). For these reasons I think it is reasonable to re-add khr_vulkan_enable support to the higher-level API.

Ralith commented 3 years ago

I think Oculus has no incentive to upgrade

Oculus's incentive to update to the revised extension is for compatibility with applications, same as their incentive for implementing OpenXR at all. I recommend reaching out to them if you're concerned about whether they have plans to do so.

1) khr_vulkan_enable it is part of the OpenXR 1.0 standard

It's an extension, not part of 1.0 core.

mobile SoCs only have one GPU and the problem that khr_vulkan_enable2 solves is not even there (the application cannot select the wrong adapter)

The problem that the revised extension solves is allowing the runtime to set features and supply extension structures itself; it has nothing to do with selecting between multiple adapters, which was already accounted for in the original extension.

zmerp commented 3 years ago

The problem that the revised extension solves is allowing the runtime to set features and supply extension structures itself; it has nothing to do with selecting between multiple adapters, which was already accounted for in the original extension.

You're right I missed that.

Anyway, removing support for an extension that is needed by the most popular VR platform seems a bit shortsighted. khr_vulkan_enable might be superseded by khr_vulkan_enable2, but I don't see anywhere that it is actually deprecated.

Ralith commented 3 years ago

Unfortunately, the quality of implementations of the old extension tends to be poor, and hence it should be avoided if at all possible. I'm willing to consider a PR restoring high-level support for it alongside the current functionality, so long as it goes to adequate length to communicate this.

zmerp commented 3 years ago

Ok, I'll work on this PR. What naming scheme do you prefer? For example, vulkan_graphics_device() + vulkan_graphics_device_2() or vulkan_graphics_device_legacy() + vulkan_graphics_device()? Or should the extension selection happen internally (when applicable)?

Ralith commented 3 years ago

Thanks!

What naming scheme do you prefer?

I think the "legacy" naming is clearer. I'm not sure there'll actually be any name collisions, but a _legacy suffix will help disambiguate which methods are associated with which extension.

Or should the extension selection happen internally (when applicable)?

I'm not sure to what extent this will be feasible, but where it is, I'm in favor. Certainly the codepaths should converge post-setup, and iirc the graphics requirements data is the same.